Fixes GHSA-rc9g-fmpg-c6pp and GHSA-966r-mw4j-rv64 (#223)

Author: patrickhener

httpserver/security_bypass_test.go

@@ -134,3 +134,108 @@ func TestUpload_ValidFilename_Succeeds(t *testing.T) {
 	require.NoError(t, err)
 	require.Equal(t, "hello", string(got))
 }
+
+// GHSA-966r-mw4j-rv64: HTTP PUT opens the target with O_TRUNC, so overwriting an
+// existing file destroys its contents. Under --no-delete that must be blocked and
+// the file left intact.
+func TestPut_NoDelete_BlocksOverwrite(t *testing.T) {
+	webroot := t.TempDir()
+	fs, cleanup := newTestFileServer(t, webroot)
+	defer cleanup()
+	fs.NoDelete = true
+	require.NoError(t, os.WriteFile(filepath.Join(webroot, "victim.txt"), []byte("VICTIM"), 0644))
+
+	r := httptest.NewRequest(http.MethodPut, "/victim.txt", bytes.NewBufferString("CLOBBERED"))
+	r.Header.Set("X-CSRF-Token", "test-csrf")
+	w := httptest.NewRecorder()
+
+	fs.put(w, r)
+
+	require.Equal(t, http.StatusForbidden, w.Code)
+	got, err := os.ReadFile(filepath.Join(webroot, "victim.txt"))
+	require.NoError(t, err)
+	require.Equal(t, "VICTIM", string(got), "existing file must not be truncated/overwritten")
+}
+
+// --upload-only likewise forbids destroying existing content via PUT overwrite.
+func TestPut_UploadOnly_BlocksOverwrite(t *testing.T) {
+	webroot := t.TempDir()
+	fs, cleanup := newTestFileServer(t, webroot)
+	defer cleanup()
+	fs.UploadOnly = true
+	require.NoError(t, os.WriteFile(filepath.Join(webroot, "victim.txt"), []byte("VICTIM"), 0644))
+
+	r := httptest.NewRequest(http.MethodPut, "/victim.txt", bytes.NewBufferString("CLOBBERED"))
+	r.Header.Set("X-CSRF-Token", "test-csrf")
+	w := httptest.NewRecorder()
+
+	fs.put(w, r)
+
+	require.Equal(t, http.StatusForbidden, w.Code)
+	got, err := os.ReadFile(filepath.Join(webroot, "victim.txt"))
+	require.NoError(t, err)
+	require.Equal(t, "VICTIM", string(got))
+}
+
+// Creating a new file via PUT destroys nothing and must stay allowed under
+// --no-delete. Confirms the overwrite guard does not over-block fresh writes.
+func TestPut_NoDelete_AllowsNewFile(t *testing.T) {
+	webroot := t.TempDir()
+	fs, cleanup := newTestFileServer(t, webroot)
+	defer cleanup()
+	fs.NoDelete = true
+
+	r := httptest.NewRequest(http.MethodPut, "/fresh.txt", bytes.NewBufferString("NEW"))
+	r.Header.Set("X-CSRF-Token", "test-csrf")
+	w := httptest.NewRecorder()
+
+	fs.put(w, r)
+
+	got, err := os.ReadFile(filepath.Join(webroot, "fresh.txt"))
+	require.NoError(t, err)
+	require.Equal(t, "NEW", string(got))
+}
+
+// The multipart upload path renames the temp file over any existing same-named
+// file, clobbering it. Under --no-delete that overwrite must be skipped and the
+// existing file left intact.
+func TestUpload_NoDelete_BlocksOverwrite(t *testing.T) {
+	webroot := t.TempDir()
+	fs, cleanup := newTestFileServer(t, webroot)
+	defer cleanup()
+	fs.NoDelete = true
+	require.NoError(t, os.WriteFile(filepath.Join(webroot, "victim.txt"), []byte("VICTIM"), 0644))
+
+	body, ctype := multipartUpload(t, "victim.txt", "CLOBBERED")
+	r := httptest.NewRequest(http.MethodPost, "/upload", body)
+	r.Header.Set("Content-Type", ctype)
+	r.Header.Set("X-CSRF-Token", "test-csrf")
+	w := httptest.NewRecorder()
+
+	fs.upload(w, r)
+
+	got, err := os.ReadFile(filepath.Join(webroot, "victim.txt"))
+	require.NoError(t, err)
+	require.Equal(t, "VICTIM", string(got), "existing file must not be clobbered by upload")
+}
+
+// A new-named upload must still succeed under --no-delete.
+func TestUpload_NoDelete_AllowsNewFile(t *testing.T) {
+	webroot := t.TempDir()
+	fs, cleanup := newTestFileServer(t, webroot)
+	defer cleanup()
+	fs.NoDelete = true
+
+	body, ctype := multipartUpload(t, "fresh.txt", "NEW")
+	r := httptest.NewRequest(http.MethodPost, "/upload", body)
+	r.Header.Set("Content-Type", ctype)
+	r.Header.Set("X-CSRF-Token", "test-csrf")
+	w := httptest.NewRecorder()
+
+	fs.upload(w, r)
+
+	require.Equal(t, http.StatusSeeOther, w.Code)
+	got, err := os.ReadFile(filepath.Join(webroot, "fresh.txt"))
+	require.NoError(t, err)
+	require.Equal(t, "NEW", string(got))
+}

httpserver/updown.go

@@ -52,6 +52,16 @@ func (fs *FileServer) put(w http.ResponseWriter, req *http.Request) {
 		return
 	}
 
+	// Overwriting an existing file with O_TRUNC destroys its previous contents,
+	// which is a deletion. Block it under --no-delete / --upload-only, mirroring
+	// the WebDAV PUT/COPY overwrite guard. Creating a new file stays allowed.
+	if fs.UploadOnly || fs.NoDelete {
+		if info, statErr := os.Stat(savepath); statErr == nil && !info.IsDir() {
+			fs.handleError(w, req, fmt.Errorf("%s", "Overwriting existing files is not allowed due to 'no delete' / 'upload only' option"), http.StatusForbidden)
+			return
+		}
+	}
+
 	// Enforce .goshs ACL and upload size limit
 	if !fs.prepareWrite(w, req, filepath.Dir(savepath)) {
 		return
@@ -153,6 +163,16 @@ func (fs *FileServer) upload(w http.ResponseWriter, req *http.Request) {
 		finalPath := filepath.Join(targetDir, filenameClean)
 		tempPath := finalPath + "~"
 
+		// The final os.Rename clobbers any existing same-named file, destroying
+		// its contents. Skip such overwrites under --no-delete / --upload-only,
+		// matching the WebDAV PUT/COPY overwrite guard; new files still upload.
+		if fs.UploadOnly || fs.NoDelete {
+			if info, statErr := os.Stat(finalPath); statErr == nil && !info.IsDir() {
+				logger.Warnf("blocked overwrite of existing file %q due to 'no delete' / 'upload only' option", finalPath)
+				continue
+			}
+		}
+
 		// Defence in depth: ensure the resolved destination stays inside targetDir
 		// even if filenameClean ever slips a separator or traversal past the checks
 		// above.

httpserver/webdav_acl.go

@@ -48,6 +48,20 @@ func (fs *FileServer) webdavGuard(next http.Handler) http.HandlerFunc {
 				http.Error(w, "read-only", http.StatusForbidden)
 				return
 			}
+			// A PUT onto an existing file truncates/replaces its contents, which
+			// is a destruction of the previous data — gate it exactly like the
+			// overwriting COPY case. MKCOL only ever creates, so it is unaffected.
+			if r.Method == http.MethodPut && (fs.UploadOnly || fs.NoDelete) && fs.webdavTargetExists(r) {
+				http.Error(w, "overwrite disabled", http.StatusForbidden)
+				return
+			}
+		case http.MethodPost:
+			// golang.org/x/net/webdav routes POST to the same read handler as GET,
+			// so an unguarded POST returns the full file body — bypassing the
+			// GET/HEAD upload-only block below. WebDAV POST has no legitimate use
+			// in this server, so reject it outright regardless of mode.
+			http.Error(w, "method not allowed", http.StatusMethodNotAllowed)
+			return
 		case "COPY":
 			if fs.ReadOnly {
 				http.Error(w, "read-only", http.StatusForbidden)
@@ -84,6 +98,19 @@ func (fs *FileServer) webdavGuard(next http.Handler) http.HandlerFunc {
 	}
 }
 
+// webdavTargetExists reports whether the request URL itself already points at an
+// existing (non-directory) resource inside the webroot. Used to distinguish an
+// overwriting PUT (which truncates/replaces existing content) from a PUT that
+// merely creates a new file.
+func (fs *FileServer) webdavTargetExists(r *http.Request) bool {
+	abs, err := sanitizePath(fs.Webroot, r.URL.Path)
+	if err != nil {
+		return false
+	}
+	info, err := os.Stat(abs)
+	return err == nil && !info.IsDir()
+}
+
 // webdavDestExists reports whether the Destination header of a COPY/MOVE
 // request already points at an existing resource inside the webroot. It is used
 // to distinguish an overwriting COPY (which deletes the destination) from a

httpserver/webdav_modeflags_test.go

@@ -147,3 +147,75 @@ func TestWebdav_Default_AllowsMove(t *testing.T) {
 	require.True(t, fileMissing(t, dir, "secret.txt"), "MOVE should remove the source")
 	require.Equal(t, "TOP-SECRET", fileContent(t, dir, "gone.txt"))
 }
+
+// GHSA-rc9g-fmpg-c6pp: golang.org/x/net/webdav routes POST to the same read
+// handler as GET, so an unguarded POST returned the full file body, bypassing
+// the GET/HEAD --upload-only block. POST must be rejected outright and never
+// leak file contents — here under --upload-only.
+func TestWebdav_UploadOnly_PostDoesNotLeak(t *testing.T) {
+	dir := webdavModeTree(t)
+	fs, cleanup := newTestFileServer(t, dir)
+	defer cleanup()
+	fs.UploadOnly = true
+	h := newWebdavTestHandler(fs)
+
+	w := davReq(t, h, "POST", "/secret.txt", "", "")
+	require.Equal(t, http.StatusMethodNotAllowed, w.Code)
+	require.NotContains(t, w.Body.String(), "TOP-SECRET", "POST must not leak file contents")
+}
+
+// POST must be rejected even with no mode flags — it has no legitimate WebDAV
+// use here and would otherwise alias GET.
+func TestWebdav_Default_PostRejected(t *testing.T) {
+	dir := webdavModeTree(t)
+	fs, cleanup := newTestFileServer(t, dir)
+	defer cleanup()
+	h := newWebdavTestHandler(fs)
+
+	w := davReq(t, h, "POST", "/secret.txt", "", "")
+	require.Equal(t, http.StatusMethodNotAllowed, w.Code)
+	require.NotContains(t, w.Body.String(), "TOP-SECRET")
+}
+
+// GHSA-966r-mw4j-rv64: a PUT onto an existing file truncates/replaces its
+// contents. Under --no-delete that overwrite must be blocked and the existing
+// file left intact.
+func TestWebdav_NoDelete_BlocksPutOverwrite(t *testing.T) {
+	dir := webdavModeTree(t)
+	fs, cleanup := newTestFileServer(t, dir)
+	defer cleanup()
+	fs.NoDelete = true
+	h := newWebdavTestHandler(fs)
+
+	w := davReq(t, h, "PUT", "/victim.txt", "", "")
+	require.Equal(t, http.StatusForbidden, w.Code)
+	require.Equal(t, "VICTIM", fileContent(t, dir, "victim.txt"), "existing file must survive a blocked PUT")
+}
+
+// --upload-only likewise forbids destroying existing content via PUT overwrite.
+func TestWebdav_UploadOnly_BlocksPutOverwrite(t *testing.T) {
+	dir := webdavModeTree(t)
+	fs, cleanup := newTestFileServer(t, dir)
+	defer cleanup()
+	fs.UploadOnly = true
+	h := newWebdavTestHandler(fs)
+
+	w := davReq(t, h, "PUT", "/victim.txt", "", "")
+	require.Equal(t, http.StatusForbidden, w.Code)
+	require.Equal(t, "VICTIM", fileContent(t, dir, "victim.txt"))
+}
+
+// A PUT that creates a brand-new file destroys nothing and must stay allowed
+// under --no-delete (upload-only permits uploads by definition). Confirms the
+// overwrite guard does not over-block fresh writes.
+func TestWebdav_NoDelete_AllowsPutNewFile(t *testing.T) {
+	dir := webdavModeTree(t)
+	fs, cleanup := newTestFileServer(t, dir)
+	defer cleanup()
+	fs.NoDelete = true
+	h := newWebdavTestHandler(fs)
+
+	w := davReq(t, h, "PUT", "/fresh.txt", "", "")
+	require.Less(t, w.Code, http.StatusBadRequest, "creating a new file must be allowed")
+	require.False(t, fileMissing(t, dir, "fresh.txt"), "new file should have been created")
+}