fix(server): MOVE must require config-edit authority for .zddc/.zddc.zip
serveFileMove authorized config files with content verbs — the destination as ActionCreate, a .zddc source as ActionWrite — so a caller holding only create/write authority could plant or relocate an attacker-controlled .zddc / .zddc.zip cascade (admins:/acl:) that PUT and DELETE both gate behind ActionAdmin (VerbA / IsConfigEditor). The MOVE destination rides in the X-ZDDC-Destination header, which no dispatch gate inspects, so the bar must be enforced at the handler on the resolved target path. Centralize the escalation in configWriteAction() (.zddc / .zddc.zip → ActionAdmin, case-insensitive) and apply it to BOTH sides of serveFileMove; replace the inlined `.zddc` checks in serveFilePut/serveFileDelete with the same helper (also escalating whole-file .zddc.zip writes at the handler layer, where previously only the dispatch visibility gate covered them). Found via an authz-subsystem audit; the existing suite did not pin this path. Adds TestFileAPI_MoveOntoConfigRequiresConfigEdit (non-editor MOVE onto/away-from config → 403; config-editor → 200). Full Go suite + vet green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
01b01f8f7a
commit
42f520e087
3 changed files with 136 additions and 10 deletions
37
zddc/internal/handler/configpath.go
Normal file
37
zddc/internal/handler/configpath.go
Normal file
|
|
@ -0,0 +1,37 @@
|
|||
package handler
|
||||
|
||||
import (
|
||||
"path/filepath"
|
||||
"strings"
|
||||
|
||||
"codeberg.org/VARASYS/ZDDC/zddc/internal/apps"
|
||||
"codeberg.org/VARASYS/ZDDC/zddc/internal/policy"
|
||||
)
|
||||
|
||||
// configWriteAction returns the action a write to absPath must be authorized
|
||||
// as. The .zddc cascade file and the .zddc.zip bundle are policy, not content:
|
||||
// mutating either is a VerbA operation requiring standing config-edit authority
|
||||
// (IsConfigEditor — a subtree admin or `a`-verb holder, no elevation), which
|
||||
// the decider enforces when the action is tagged ActionAdmin. For any other
|
||||
// path the supplied default action is returned unchanged.
|
||||
//
|
||||
// This is the single predicate behind the per-verb escalation that previously
|
||||
// lived inlined in serveFilePut/serveFileDelete (.zddc only) and was MISSING
|
||||
// from serveFileMove — letting a MOVE plant or relocate a policy file with mere
|
||||
// create/write authority. PUT/DELETE on a URL-visible .zddc.zip are also
|
||||
// existence-gated to config-editors at dispatch (the bundle visibility gate in
|
||||
// cmd/zddc-server), but a MOVE destination rides in the X-ZDDC-Destination
|
||||
// header and never reaches that gate — so the authority bar must be enforced
|
||||
// here, on the resolved target path, for every write verb.
|
||||
//
|
||||
// Matching is case-insensitive to align with HasReservedSidecar: ZDDC_ROOT may
|
||||
// sit on a case-insensitive filesystem (SMB/CIFS/Azure Files) where `.ZDDC` /
|
||||
// `.ZDDC.ZIP` resolve to the same files, and a case-varied target must not slip
|
||||
// past the gate.
|
||||
func configWriteAction(absPath, def string) string {
|
||||
base := filepath.Base(absPath)
|
||||
if strings.EqualFold(base, ".zddc") || strings.EqualFold(base, apps.BundleName) {
|
||||
return policy.ActionAdmin
|
||||
}
|
||||
return def
|
||||
}
|
||||
|
|
@ -377,10 +377,9 @@ func serveFilePut(cfg config.Config, w http.ResponseWriter, r *http.Request) {
|
|||
if existed {
|
||||
action = policy.ActionWrite
|
||||
}
|
||||
// .zddc writes always require `a` (admin) regardless of create/overwrite.
|
||||
if filepath.Base(abs) == ".zddc" {
|
||||
action = policy.ActionAdmin
|
||||
}
|
||||
// Config files (.zddc / .zddc.zip) always require `a` (admin/config-edit)
|
||||
// regardless of create/overwrite — see configWriteAction.
|
||||
action = configWriteAction(abs, action)
|
||||
|
||||
if !authorizeAction(cfg, w, r, abs, cleanURL, action) {
|
||||
return
|
||||
|
|
@ -545,10 +544,9 @@ func serveFileDelete(cfg config.Config, w http.ResponseWriter, r *http.Request)
|
|||
return
|
||||
}
|
||||
|
||||
action := policy.ActionDelete
|
||||
if filepath.Base(abs) == ".zddc" {
|
||||
action = policy.ActionAdmin
|
||||
}
|
||||
// Config files (.zddc / .zddc.zip) require `a` (admin/config-edit) to
|
||||
// delete — see configWriteAction.
|
||||
action := configWriteAction(abs, policy.ActionDelete)
|
||||
if !authorizeAction(cfg, w, r, abs, cleanURL, action) {
|
||||
return
|
||||
}
|
||||
|
|
@ -676,10 +674,18 @@ func serveFileMove(cfg config.Config, w http.ResponseWriter, r *http.Request) {
|
|||
// ACL: source side requires `w` (rename mutates the source); dest
|
||||
// side requires `c` (creates a new path). Cross-folder moves run
|
||||
// both gates against potentially different chains.
|
||||
if !authorizeAction(cfg, w, r, srcAbs, srcURL, policy.ActionWrite) {
|
||||
//
|
||||
// Config files (.zddc / .zddc.zip) are policy, not content: relocating
|
||||
// one mutates policy at BOTH ends (removing it from the source dir,
|
||||
// installing it at the dest), so each side escalates to ActionAdmin —
|
||||
// the same VerbA/config-edit bar PUT and DELETE enforce. Without this a
|
||||
// caller holding only `w`/`c` could plant an attacker-controlled cascade
|
||||
// (admins:/acl:) via the header-borne destination, which no dispatch
|
||||
// gate inspects. See configWriteAction.
|
||||
if !authorizeAction(cfg, w, r, srcAbs, srcURL, configWriteAction(srcAbs, policy.ActionWrite)) {
|
||||
return
|
||||
}
|
||||
if !authorizeAction(cfg, w, r, dstAbs, dstURL, policy.ActionCreate) {
|
||||
if !authorizeAction(cfg, w, r, dstAbs, dstURL, configWriteAction(dstAbs, policy.ActionCreate)) {
|
||||
return
|
||||
}
|
||||
// If-Match concurrency applies to the source bytes — only meaningful for
|
||||
|
|
|
|||
|
|
@ -949,3 +949,86 @@ func TestFileAPI_PreservesCase(t *testing.T) {
|
|||
t.Errorf("PUT case NOT preserved (%v)", names)
|
||||
}
|
||||
}
|
||||
|
||||
// TestFileAPI_MoveOntoConfigRequiresConfigEdit pins the authorization parity
|
||||
// between MOVE and PUT/DELETE for config files. alice@example.com holds rwcd
|
||||
// (create+write) via the default *@example.com grant and is elevated — but is
|
||||
// named in no admins: and holds no `a` verb, so she is NOT a config-editor.
|
||||
// Moving a file ONTO a .zddc/.zddc.zip, or moving a .zddc AWAY, mutates policy
|
||||
// and must require config-edit (VerbA), not mere create/write. Pre-fix, MOVE
|
||||
// authorized the destination as ActionCreate and the source as ActionWrite, so
|
||||
// each of these succeeded — letting a non-admin plant an attacker-controlled
|
||||
// cascade. See configWriteAction / serveFileMove.
|
||||
func TestFileAPI_MoveOntoConfigRequiresConfigEdit(t *testing.T) {
|
||||
t.Run("move onto .zddc is forbidden", func(t *testing.T) {
|
||||
_, do, root := fileAPITestSetup(t, []string{"Incoming"}, map[string]string{
|
||||
"Incoming/payload.txt": "x",
|
||||
})
|
||||
rec := do(http.MethodPost, "/Incoming/payload.txt", "alice@example.com", nil, map[string]string{
|
||||
headerOp: opMove,
|
||||
headerDestination: "/Incoming/.zddc",
|
||||
})
|
||||
if rec.Code != http.StatusForbidden {
|
||||
t.Fatalf("want 403, got %d: %s", rec.Code, rec.Body.String())
|
||||
}
|
||||
if _, err := os.Stat(filepath.Join(root, "Incoming/.zddc")); err == nil {
|
||||
t.Fatal("a .zddc was planted despite the 403")
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("move onto .zddc.zip is forbidden", func(t *testing.T) {
|
||||
_, do, root := fileAPITestSetup(t, []string{"Incoming"}, map[string]string{
|
||||
"Incoming/payload.txt": "x",
|
||||
})
|
||||
rec := do(http.MethodPost, "/Incoming/payload.txt", "alice@example.com", nil, map[string]string{
|
||||
headerOp: opMove,
|
||||
headerDestination: "/Incoming/.zddc.zip",
|
||||
})
|
||||
if rec.Code != http.StatusForbidden {
|
||||
t.Fatalf("want 403, got %d: %s", rec.Code, rec.Body.String())
|
||||
}
|
||||
if _, err := os.Stat(filepath.Join(root, "Incoming/.zddc.zip")); err == nil {
|
||||
t.Fatal("a .zddc.zip bundle was planted despite the 403")
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("moving a .zddc away is forbidden", func(t *testing.T) {
|
||||
_, do, root := fileAPITestSetup(t, []string{"Incoming", "Other"}, map[string]string{
|
||||
"Incoming/.zddc": "acl:\n permissions:\n \"*@example.com\": rwcd\n",
|
||||
})
|
||||
rec := do(http.MethodPost, "/Incoming/.zddc", "alice@example.com", nil, map[string]string{
|
||||
headerOp: opMove,
|
||||
headerDestination: "/Other/.zddc",
|
||||
})
|
||||
if rec.Code != http.StatusForbidden {
|
||||
t.Fatalf("want 403, got %d: %s", rec.Code, rec.Body.String())
|
||||
}
|
||||
if _, err := os.Stat(filepath.Join(root, "Incoming/.zddc")); err != nil {
|
||||
t.Fatalf("source .zddc was removed despite the 403: %v", err)
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("config-editor may move config (positive control)", func(t *testing.T) {
|
||||
_, do, root := fileAPITestSetup(t, []string{"Incoming"}, map[string]string{
|
||||
"Incoming/payload.txt": "x",
|
||||
})
|
||||
// Grant admin@example.com the `a` verb (standing config-edit) on top
|
||||
// of rwcd. The same MOVE that alice is denied must still succeed for a
|
||||
// config-editor — proving the fix gates on authority, not on the verb.
|
||||
if err := os.WriteFile(filepath.Join(root, ".zddc"),
|
||||
[]byte("acl:\n permissions:\n \"admin@example.com\": rwcda\n \"*@example.com\": rwcd\n"), 0o644); err != nil {
|
||||
t.Fatalf("rewrite root .zddc: %v", err)
|
||||
}
|
||||
zddc.InvalidateCache(root)
|
||||
rec := do(http.MethodPost, "/Incoming/payload.txt", "admin@example.com", nil, map[string]string{
|
||||
headerOp: opMove,
|
||||
headerDestination: "/Incoming/.zddc",
|
||||
})
|
||||
if rec.Code != http.StatusOK {
|
||||
t.Fatalf("config-editor move: want 200, got %d: %s", rec.Code, rec.Body.String())
|
||||
}
|
||||
if _, err := os.Stat(filepath.Join(root, "Incoming/.zddc")); err != nil {
|
||||
t.Fatalf("authorized editor's move did not land the file: %v", err)
|
||||
}
|
||||
})
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue