diff --git a/chfs-ng/internal/api/server.go b/chfs-ng/internal/api/server.go index a49ff2c..ed136af 100644 --- a/chfs-ng/internal/api/server.go +++ b/chfs-ng/internal/api/server.go @@ -326,6 +326,7 @@ func (s *Server) remove(w http.ResponseWriter, r *http.Request) { if len(items) == 0 { items = r.Form["path"] } + paths := make([]string, 0, len(items)) for _, raw := range items { if unsafePath(raw) { writeJSONError(w, http.StatusBadRequest, "invalid parameter") @@ -340,6 +341,11 @@ func (s *Server) remove(w http.ResponseWriter, r *http.Request) { writeJSONError(w, http.StatusForbidden, "has no privilege") return } + paths = append(paths, p) + } + // Preflight every target before mutating anything, so a multi-file delete + // cannot partially succeed when a later item is forbidden. + for _, p := range paths { if e := s.removePath(p); e != nil { http.Error(w, e.Error(), 500) return @@ -881,6 +887,12 @@ func davSafePath(name string) (string, error) { return p, nil } +func sameOrDescendantPath(src, dst string) bool { + src = strings.Trim(path.Clean("/"+src), "/") + dst = strings.Trim(path.Clean("/"+dst), "/") + return src == dst || (src != "" && strings.HasPrefix(dst, src+"/")) +} + func (f *webdavStorageFS) info(name string) (*webdav.FileInfo, error) { p, err := davSafePath(name) if err != nil { @@ -1023,6 +1035,9 @@ func (f *webdavStorageFS) Copy(ctx context.Context, name, dest string, opts *web if err != nil { return false, err } + if sameOrDescendantPath(src, dst) { + return false, webdav.NewHTTPError(http.StatusConflict, fmt.Errorf("destination is inside source")) + } if opts != nil && opts.NoOverwrite { if _, err := f.store.Stat(dst); err == nil { return false, webdav.NewHTTPError(http.StatusPreconditionFailed, fmt.Errorf("destination exists")) @@ -1088,6 +1103,9 @@ func (f *webdavStorageFS) Move(ctx context.Context, name, dest string, opts *web if err != nil { return false, err } + if sameOrDescendantPath(src, dst) { + return false, webdav.NewHTTPError(http.StatusConflict, fmt.Errorf("destination is inside source")) + } if opts == nil || opts.NoOverwrite { if _, err := f.store.Stat(dst); err == nil { return false, webdav.NewHTTPError(http.StatusPreconditionFailed, fmt.Errorf("destination exists")) diff --git a/chfs-ng/internal/api/webdav_test.go b/chfs-ng/internal/api/webdav_test.go index 72e0188..2c936c4 100644 --- a/chfs-ng/internal/api/webdav_test.go +++ b/chfs-ng/internal/api/webdav_test.go @@ -74,6 +74,26 @@ func TestSameWebDAVDestinationHost(t *testing.T) { } } +func TestWebDAVStorageFSRejectsDestinationInsideSource(t *testing.T) { + root := t.TempDir() + if err := os.MkdirAll(filepath.Join(root, "src", "child"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(root, "src", "file.txt"), []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + fs := &webdavStorageFS{store: storage.Local{Root: root}} + if _, err := fs.Copy(context.Background(), "/src", "/src/child/copy", &webdav.CopyOptions{}); err == nil { + t.Fatal("copying a directory into its descendant should fail") + } + if _, err := fs.Move(context.Background(), "/src", "/src/child/moved", &webdav.MoveOptions{}); err == nil { + t.Fatal("moving a directory into its descendant should fail") + } + if _, err := fs.Copy(context.Background(), "/src", "/src", &webdav.CopyOptions{}); err == nil { + t.Fatal("copying a resource onto itself should fail") + } +} + func TestWebDAVStorageFSCopyAndMove(t *testing.T) { root1 := t.TempDir() root2 := t.TempDir() diff --git a/chfs-ng/internal/storage/storage.go b/chfs-ng/internal/storage/storage.go index 1ab6608..2a845a2 100644 --- a/chfs-ng/internal/storage/storage.go +++ b/chfs-ng/internal/storage/storage.go @@ -97,10 +97,36 @@ func (s Local) Create(path string) (*os.File, error) { } func (s Local) Remove(path string) error { p, e := s.real(path) - if e != nil { + if e == nil { + return os.RemoveAll(p) + } + // A final symlink that points outside the root is still safe to delete: + // remove the link itself, but never follow it to its target. Validate its + // parent through real() so nested symlink escapes remain rejected. + clean := strings.TrimPrefix(filepath.Clean("/"+path), string(filepath.Separator)) + if clean == "" { return e } - return os.RemoveAll(p) + parent := filepath.ToSlash(filepath.Dir(clean)) + if parent == "." { + parent = "" + } + if _, parentErr := s.real(parent); parentErr != nil { + return e + } + root, rootErr := filepath.Abs(s.Root) + if rootErr != nil { + return e + } + root, rootErr = filepath.EvalSymlinks(root) + if rootErr != nil { + return e + } + candidate := filepath.Join(root, filepath.FromSlash(clean)) + if info, lstatErr := os.Lstat(candidate); lstatErr == nil && info.Mode()&os.ModeSymlink != 0 { + return os.Remove(candidate) + } + return e } func (s Local) Trash(path string) error { diff --git a/chfs-ng/internal/storage/storage_test.go b/chfs-ng/internal/storage/storage_test.go index 9e85e7c..6f351c7 100644 --- a/chfs-ng/internal/storage/storage_test.go +++ b/chfs-ng/internal/storage/storage_test.go @@ -7,6 +7,29 @@ import ( "time" ) +func TestLocalRemoveSymlinkDoesNotDeleteTarget(t *testing.T) { + root := t.TempDir() + outside := t.TempDir() + target := filepath.Join(outside, "secret.txt") + if err := os.WriteFile(target, []byte("keep"), 0o644); err != nil { + t.Fatal(err) + } + link := filepath.Join(root, "link") + if err := os.Symlink(target, link); err != nil { + t.Skipf("symlink unavailable: %v", err) + } + store := Local{Root: root} + if err := store.Remove("link"); err != nil { + t.Fatalf("remove symlink: %v", err) + } + if _, err := os.Stat(target); err != nil { + t.Fatalf("symlink removal must not delete target: %v", err) + } + if _, err := os.Lstat(link); !os.IsNotExist(err) { + t.Fatalf("symlink should be removed, got %v", err) + } +} + func TestLocalRejectsSymlinkEscapeAndAllowsNestedCreate(t *testing.T) { root := t.TempDir() outside := t.TempDir()