From 42f520e087bdd6a7fb3fb255b0cae404b13304f4 Mon Sep 17 00:00:00 2001 From: ZDDC Date: Tue, 9 Jun 2026 18:06:28 -0500 Subject: [PATCH] fix(server): MOVE must require config-edit authority for .zddc/.zddc.zip MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- zddc/internal/handler/configpath.go | 37 ++++++++++++ zddc/internal/handler/fileapi.go | 26 +++++---- zddc/internal/handler/fileapi_test.go | 83 +++++++++++++++++++++++++++ 3 files changed, 136 insertions(+), 10 deletions(-) create mode 100644 zddc/internal/handler/configpath.go diff --git a/zddc/internal/handler/configpath.go b/zddc/internal/handler/configpath.go new file mode 100644 index 0000000..61960c4 --- /dev/null +++ b/zddc/internal/handler/configpath.go @@ -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 +} diff --git a/zddc/internal/handler/fileapi.go b/zddc/internal/handler/fileapi.go index bca4abe..ef67253 100644 --- a/zddc/internal/handler/fileapi.go +++ b/zddc/internal/handler/fileapi.go @@ -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 diff --git a/zddc/internal/handler/fileapi_test.go b/zddc/internal/handler/fileapi_test.go index 7e333b3..d19d2a5 100644 --- a/zddc/internal/handler/fileapi_test.go +++ b/zddc/internal/handler/fileapi_test.go @@ -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) + } + }) +}