From cd8c6863394d02b470d1f5b2a4d36a6510b40dfa Mon Sep 17 00:00:00 2001 From: Pascal Fischer <32096965+pascal-fischer@users.noreply.github.com> Date: Wed, 1 Apr 2026 14:23:24 +0200 Subject: [PATCH] [misc] add path traversal and file size protections (#5755) --- upload-server/server/local.go | 49 +++++++++++++---- upload-server/server/local_test.go | 87 ++++++++++++++++++++++++++++++ 2 files changed, 127 insertions(+), 9 deletions(-) diff --git a/upload-server/server/local.go b/upload-server/server/local.go index f12c472d2..f7ca50011 100644 --- a/upload-server/server/local.go +++ b/upload-server/server/local.go @@ -7,6 +7,7 @@ import ( "net/url" "os" "path/filepath" + "strings" log "github.com/sirupsen/logrus" @@ -82,15 +83,18 @@ func (l *local) getUploadURL(objectKey string) (string, error) { return newURL.String(), nil } +const maxUploadSize = 150 << 20 + func (l *local) handlePutRequest(w http.ResponseWriter, r *http.Request) { if r.Method != http.MethodPut { http.Error(w, "method not allowed", http.StatusMethodNotAllowed) return } + r.Body = http.MaxBytesReader(w, r.Body, maxUploadSize) body, err := io.ReadAll(r.Body) if err != nil { - http.Error(w, fmt.Sprintf("failed to read body: %v", err), http.StatusInternalServerError) + http.Error(w, "request body too large or failed to read", http.StatusRequestEntityTooLarge) return } @@ -105,20 +109,47 @@ func (l *local) handlePutRequest(w http.ResponseWriter, r *http.Request) { return } - dirPath := filepath.Join(l.dir, uploadDir) - err = os.MkdirAll(dirPath, 0750) - if err != nil { + cleanBase := filepath.Clean(l.dir) + string(filepath.Separator) + + dirPath := filepath.Clean(filepath.Join(l.dir, uploadDir)) + if !strings.HasPrefix(dirPath, cleanBase) { + http.Error(w, "invalid path", http.StatusBadRequest) + log.Warnf("Path traversal attempt blocked (dir): %s", dirPath) + return + } + + filePath := filepath.Clean(filepath.Join(dirPath, uploadFile)) + if !strings.HasPrefix(filePath, cleanBase) { + http.Error(w, "invalid path", http.StatusBadRequest) + log.Warnf("Path traversal attempt blocked (file): %s", filePath) + return + } + + if err = os.MkdirAll(dirPath, 0750); err != nil { http.Error(w, "failed to create upload dir", http.StatusInternalServerError) log.Errorf("Failed to create upload dir: %v", err) return } - file := filepath.Join(dirPath, uploadFile) - if err := os.WriteFile(file, body, 0600); err != nil { - http.Error(w, "failed to write file", http.StatusInternalServerError) - log.Errorf("Failed to write file %s: %v", file, err) + flags := os.O_WRONLY | os.O_CREATE | os.O_EXCL + f, err := os.OpenFile(filePath, flags, 0600) + if err != nil { + if os.IsExist(err) { + http.Error(w, "file already exists", http.StatusConflict) + return + } + http.Error(w, "failed to create file", http.StatusInternalServerError) + log.Errorf("Failed to create file %s: %v", filePath, err) return } - log.Infof("Uploading file %s", file) + defer func() { _ = f.Close() }() + + if _, err = f.Write(body); err != nil { + http.Error(w, "failed to write file", http.StatusInternalServerError) + log.Errorf("Failed to write file %s: %v", filePath, err) + return + } + + log.Infof("Uploaded file %s", filePath) w.WriteHeader(http.StatusOK) } diff --git a/upload-server/server/local_test.go b/upload-server/server/local_test.go index bd8a87809..64b8fd228 100644 --- a/upload-server/server/local_test.go +++ b/upload-server/server/local_test.go @@ -63,3 +63,90 @@ func Test_LocalHandlePutRequest(t *testing.T) { require.NoError(t, err) require.Equal(t, fileContent, createdFileContent) } + +func Test_LocalHandlePutRequest_PathTraversal(t *testing.T) { + mockDir := t.TempDir() + mockURL := "http://localhost:8080" + t.Setenv("SERVER_URL", mockURL) + t.Setenv("STORE_DIR", mockDir) + + mux := http.NewServeMux() + err := configureLocalHandlers(mux) + require.NoError(t, err) + + fileContent := []byte("malicious content") + req := httptest.NewRequest(http.MethodPut, putURLPath+"/uploads/%2e%2e%2f%2e%2e%2fetc%2fpasswd", bytes.NewReader(fileContent)) + + rec := httptest.NewRecorder() + mux.ServeHTTP(rec, req) + + require.Equal(t, http.StatusBadRequest, rec.Code) + + _, err = os.Stat(filepath.Join(mockDir, "..", "..", "etc", "passwd")) + require.True(t, os.IsNotExist(err), "traversal file should not exist") +} + +func Test_LocalHandlePutRequest_DirTraversal(t *testing.T) { + mockDir := t.TempDir() + t.Setenv("SERVER_URL", "http://localhost:8080") + t.Setenv("STORE_DIR", mockDir) + + l := &local{url: "http://localhost:8080", dir: mockDir} + + body := bytes.NewReader([]byte("bad")) + req := httptest.NewRequest(http.MethodPut, putURLPath+"/x/evil.txt", body) + req.SetPathValue("dir", "../../../tmp") + req.SetPathValue("file", "evil.txt") + + rec := httptest.NewRecorder() + l.handlePutRequest(rec, req) + + require.Equal(t, http.StatusBadRequest, rec.Code) + + _, err := os.Stat(filepath.Join("/tmp", "evil.txt")) + require.True(t, os.IsNotExist(err), "traversal file should not exist outside store dir") +} + +func Test_LocalHandlePutRequest_DuplicateFile(t *testing.T) { + mockDir := t.TempDir() + t.Setenv("SERVER_URL", "http://localhost:8080") + t.Setenv("STORE_DIR", mockDir) + + mux := http.NewServeMux() + err := configureLocalHandlers(mux) + require.NoError(t, err) + + req := httptest.NewRequest(http.MethodPut, putURLPath+"/dir/dup.txt", bytes.NewReader([]byte("first"))) + rec := httptest.NewRecorder() + mux.ServeHTTP(rec, req) + require.Equal(t, http.StatusOK, rec.Code) + + req = httptest.NewRequest(http.MethodPut, putURLPath+"/dir/dup.txt", bytes.NewReader([]byte("second"))) + rec = httptest.NewRecorder() + mux.ServeHTTP(rec, req) + require.Equal(t, http.StatusConflict, rec.Code) + + content, err := os.ReadFile(filepath.Join(mockDir, "dir", "dup.txt")) + require.NoError(t, err) + require.Equal(t, []byte("first"), content) +} + +func Test_LocalHandlePutRequest_BodyTooLarge(t *testing.T) { + mockDir := t.TempDir() + t.Setenv("SERVER_URL", "http://localhost:8080") + t.Setenv("STORE_DIR", mockDir) + + mux := http.NewServeMux() + err := configureLocalHandlers(mux) + require.NoError(t, err) + + largeBody := make([]byte, maxUploadSize+1) + req := httptest.NewRequest(http.MethodPut, putURLPath+"/dir/big.txt", bytes.NewReader(largeBody)) + rec := httptest.NewRecorder() + mux.ServeHTTP(rec, req) + + require.Equal(t, http.StatusRequestEntityTooLarge, rec.Code) + + _, err = os.Stat(filepath.Join(mockDir, "dir", "big.txt")) + require.True(t, os.IsNotExist(err)) +}