security: harden recursive file operations
This commit is contained in:
@@ -326,6 +326,7 @@ func (s *Server) remove(w http.ResponseWriter, r *http.Request) {
|
|||||||
if len(items) == 0 {
|
if len(items) == 0 {
|
||||||
items = r.Form["path"]
|
items = r.Form["path"]
|
||||||
}
|
}
|
||||||
|
paths := make([]string, 0, len(items))
|
||||||
for _, raw := range items {
|
for _, raw := range items {
|
||||||
if unsafePath(raw) {
|
if unsafePath(raw) {
|
||||||
writeJSONError(w, http.StatusBadRequest, "invalid parameter")
|
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")
|
writeJSONError(w, http.StatusForbidden, "has no privilege")
|
||||||
return
|
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 {
|
if e := s.removePath(p); e != nil {
|
||||||
http.Error(w, e.Error(), 500)
|
http.Error(w, e.Error(), 500)
|
||||||
return
|
return
|
||||||
@@ -881,6 +887,12 @@ func davSafePath(name string) (string, error) {
|
|||||||
return p, nil
|
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) {
|
func (f *webdavStorageFS) info(name string) (*webdav.FileInfo, error) {
|
||||||
p, err := davSafePath(name)
|
p, err := davSafePath(name)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
@@ -1023,6 +1035,9 @@ func (f *webdavStorageFS) Copy(ctx context.Context, name, dest string, opts *web
|
|||||||
if err != nil {
|
if err != nil {
|
||||||
return false, err
|
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 opts != nil && opts.NoOverwrite {
|
||||||
if _, err := f.store.Stat(dst); err == nil {
|
if _, err := f.store.Stat(dst); err == nil {
|
||||||
return false, webdav.NewHTTPError(http.StatusPreconditionFailed, fmt.Errorf("destination exists"))
|
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 {
|
if err != nil {
|
||||||
return false, err
|
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 opts == nil || opts.NoOverwrite {
|
||||||
if _, err := f.store.Stat(dst); err == nil {
|
if _, err := f.store.Stat(dst); err == nil {
|
||||||
return false, webdav.NewHTTPError(http.StatusPreconditionFailed, fmt.Errorf("destination exists"))
|
return false, webdav.NewHTTPError(http.StatusPreconditionFailed, fmt.Errorf("destination exists"))
|
||||||
|
|||||||
@@ -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) {
|
func TestWebDAVStorageFSCopyAndMove(t *testing.T) {
|
||||||
root1 := t.TempDir()
|
root1 := t.TempDir()
|
||||||
root2 := t.TempDir()
|
root2 := t.TempDir()
|
||||||
|
|||||||
@@ -97,10 +97,36 @@ func (s Local) Create(path string) (*os.File, error) {
|
|||||||
}
|
}
|
||||||
func (s Local) Remove(path string) error {
|
func (s Local) Remove(path string) error {
|
||||||
p, e := s.real(path)
|
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 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 {
|
func (s Local) Trash(path string) error {
|
||||||
|
|||||||
@@ -7,6 +7,29 @@ import (
|
|||||||
"time"
|
"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) {
|
func TestLocalRejectsSymlinkEscapeAndAllowsNestedCreate(t *testing.T) {
|
||||||
root := t.TempDir()
|
root := t.TempDir()
|
||||||
outside := t.TempDir()
|
outside := t.TempDir()
|
||||||
|
|||||||
Reference in New Issue
Block a user