[misc] add path traversal and file size protections (#5755)

Author: pascal-fischer

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 {
+	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
+	}
+	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", file, err)
+		log.Errorf("Failed to write file %s: %v", filePath, err)
 		return
 	}
-	log.Infof("Uploading file %s", file)
+
+	log.Infof("Uploaded file %s", filePath)
 	w.WriteHeader(http.StatusOK)
 }

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))
+}