POST override error-rollback recursively deletes a directory, bypassing the Perm.Delete gate

Status

Repository / Component

Plain-English Description

A user who is allowed to create and modify files but not to delete them can still wipe out an entire folder. Uploading over an existing directory triggers an error-cleanup step that recursively deletes that directory, doing exactly what the missing delete permission was supposed to prevent.

Description of the Underlying Issue

resourcePostHandler's gate (http/resource.go:128) checks only !Perm.Create and Check(path), never Perm.Delete. For an existing target with ?override=true it requires only Perm.Modify (lines 146-160) and proceeds. writeFile opens the destination with O_RDWR|O_CREATE|O_TRUNC (resource.go:297-307), which returns EISDIR when the target is an existing directory. On that error the handler unconditionally runs `_ = d.user.Fs.RemoveAll(r.URL.Path)` (lines 162-175). Because NewFileInfo with Expand=false stats an existing directory and returns err==nil (files/file.go:77-85,109-134), the override branch is reachable for a directory target. RemoveAll delegates to os.RemoveAll (users/users.go:96), recursively deleting the subtree — achieving a deletion that resourceDeleteHandler (resource.go:87) explicitly requires Perm.Delete to perform.

Potential Attack

A user with Perm.Create+Perm.Modify but not Perm.Delete creates/owns a directory /dir containing files, then sends POST /api/resources/dir?override=true (path without a trailing slash) with any body. NewFileInfo stats /dir (err==nil), the override+Modify checks pass, writeFile's OpenFile on the directory fails with EISDIR, and resource.go:174 executes RemoveAll(/dir), recursively deleting the subtree.

Outcomes of Potential Attack

The attacker deletes an entire directory subtree within their scope despite lacking Perm.Delete, defeating the delete-authorization control and enabling data loss / destruction they were not permitted to cause.

Affected Scope

resourcePostHandler override-rollback (http/resource.go:126-178); any user with Perm.Create+Perm.Modify but without Perm.Delete

Suggested Fix (plain english)

Refuse an upload whose existing target is a directory, and only delete files the request actually created — never recursively remove a pre-existing target during error cleanup.

Suggested Fix (detailed)

Before writing, detect that the existing target is a directory and reject the POST (409/400). Scope the error-rollback to files actually created by this request rather than RemoveAll on a pre-existing target, and/or require Perm.Delete before any RemoveAll. This is a distinct sink from DOK-100045 (recursive-PATCH deny-rule bypass). Verify: a Create+Modify-without-Delete user posting ?override=true at an existing directory receives an error and the directory subtree is preserved. Before writing, reject a POST whose existing target is a directory (return 409/400). Scope the rollback to files actually created by this request (do not RemoveAll a pre-existing target), and/or require Perm.Delete before any RemoveAll. Distinct sink from DOK-100045 (recursive-PATCH deny-rule bypass).

Validation

Independently verified end-to-end and corroborated by peer review: a Create+Modify-without-Delete user can force a recursive RemoveAll of a directory subtree, bypassing the Perm.Delete authorization gate. The vulnerable path is complete in source; fully_valid.

first_opinion

Full Evidence

http/resource.go:128 - gate checks !d.user.Perm.Create || !d.Check(path); never checks Perm.Delete

http/resource.go:146-160 - for an existing target with ?override=true, requires only Perm.Modify (:152), then proceeds

http/resource.go:162-175 - RunHook(writeFile...) then 'if err != nil { _ = d.user.Fs.RemoveAll(r.URL.Path) }' — unconditional recursive rollback on any hook/write error

http/resource.go:297-307 - writeFile OpenFile(dst, os.O_RDWR|os.O_CREATE|os.O_TRUNC, ...) returns EISDIR when dst is an existing directory

files/file.go:77-85,109-134 - NewFileInfo with Expand=false stats an existing directory and returns err==nil, so the override branch (resource.go:146) is reachable for a directory target

http/resource.go:87 - resourceDeleteHandler requires !d.user.Perm.Delete -> Forbidden, establishing that deletion is meant to require Perm.Delete

users/users.go:96 - d.user.Fs = afero.NewBasePathFs(afero.NewOsFs(), scope); RemoveAll delegates to os.RemoveAll (recursive)

Proven fact: A POST to an existing directory path (no trailing slash) with ?override=true is reachable for a user holding Perm.Create+Perm.Modify without Perm.Delete.

Proven fact: writeFile's O_RDWR|O_CREATE|O_TRUNC open of a directory fails (EISDIR), which drives the unconditional d.user.Fs.RemoveAll(r.URL.Path) rollback.

Proven fact: RemoveAll recursively deletes the directory subtree within the user's scope, achieving a deletion the Perm.Delete gate (resource.go:87) is designed to prevent.

Unvalidated fact: None material. The EISDIR-on-directory behavior of an O_RDWR|O_TRUNC open is standard POSIX/Windows semantics, verifiable by reading; no runtime/deployment unknown gates the path.

http/resource.go:128 — gate checks !d.user.Perm.Create || !d.Check(path); never checks Perm.Delete

http/resource.go:146-160 — existing target with ?override=true requires only Perm.Modify (:152), then proceeds

http/resource.go:162-175 — RunHook(writeFile...) then `if err != nil { _ = d.user.Fs.RemoveAll(r.URL.Path) }` — unconditional recursive rollback on any write error

http/resource.go:297-307 — writeFile OpenFile(dst, O_RDWR|O_CREATE|O_TRUNC) returns EISDIR when dst is an existing directory

files/file.go:77-85,109-134 — NewFileInfo (Expand=false) stats an existing directory and returns err==nil, so the override branch is reachable for a directory target

http/resource.go:87 — resourceDeleteHandler requires !d.user.Perm.Delete -> Forbidden, establishing deletion is meant to require Perm.Delete

users/users.go:96 — d.user.Fs = afero.NewBasePathFs(afero.NewOsFs(), scope); RemoveAll delegates to os.RemoveAll (recursive)

http/resource.go:126-179 resourcePostHandler — checks Perm.Create (:128) and Perm.Modify for override (:152), never Perm.Delete

http/resource.go:173-175 — `if err != nil { _ = d.user.Fs.RemoveAll(r.URL.Path) }` unconditional recursive rollback on any RunHook error

http/resource.go:297-308 writeFile — OpenFile(dst, os.O_RDWR|os.O_CREATE|os.O_TRUNC, fileMode) at :304 fails (EISDIR) when dst is an existing directory

files/file.go:77-107 NewFileInfo + :82 stat — returns err==nil for an existing directory (Expand=false), so the override/RunHook path at resource.go:146 is reachable for a directory target

http/resource.go:87 resourceDeleteHandler — `if r.URL.Path == "/" || !d.user.Perm.Delete { return http.StatusForbidden }` establishes that deletion is meant to require Perm.Delete