From 03ab121b25aa3659c110b2ba3d9b7237da774ae4 Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Sat, 17 Oct 2026 23:22:54 +0200 Subject: [PATCH] v0.329.0 (review): an app named like another app's appdata folder gets no root; USERDATA_PATH is always derived, never the app's env Security review of 2fe6341 (same night, before any release). Both red-proofed. --- CHANGELOG.md | 7 +++++++ controller/internal/privpolicy/privpolicy.go | 12 ++++++++++++ controller/internal/privpolicy/privpolicy_test.go | 15 +++++++++++++++ controller/internal/stacks/allowlist.go | 4 ++++ controller/internal/stacks/allowlist_test.go | 11 +++++++++++ 5 files changed, 49 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c630fae..d669913 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,6 +38,13 @@ template (the socket has no exception any more); an older controller accepts the the multiarch leg; `--print-tags` prints it and stops. `scripts/test_build_push_once.sh` (16 cases): rc/dev/`0.999` carry no `:latest`, `0.999.0` does; the test now runs a COPY of `build.sh` (in place, its workspace was inside the source it copies — the „local build" case failed on HEAD too). Red-proof: every build adding `:latest` → 4 cases fail. +- **Security review of the pushed commit (2026-10-18, same night), two holes closed before any release:** (1) an app + NAMED like a folder the map gives to another app (a new template called `paperless`) got that folder — now it gets no + `${HDD_PATH}` root, and one folder given to two apps panics at load; (2) with no drive, `policyEnv` kept the app's + OWN `USERDATA_PATH` (a deploy field a catalog push can add, filled by `InjectMissingFields`) as a data root, which + could name another app's `appdata/` — it is now always the controller's derivation (no template declares one). Tests + in `TestAllowList_AppdataFolderMustBeOneName` and `TestAllowList_PolicyEnvDerivesTheRootsLikeComposeExec`; both + red-proofed (`audits/night-2026-10-18/P1/redproof-userdata-env.txt`). - i18n: three keys born as keys (`scripts/i18n_go_keys.json`); every page fixture byte-identical. ## v0.328.0 — the catalog allow-list: the box refuses container rights a template asks for and the box does not give (R-971, operator decision D2 = A) (2026-10-17) diff --git a/controller/internal/privpolicy/privpolicy.go b/controller/internal/privpolicy/privpolicy.go index db7b7f2..1da5afe 100644 --- a/controller/internal/privpolicy/privpolicy.go +++ b/controller/internal/privpolicy/privpolicy.go @@ -90,10 +90,15 @@ func mustLoad(b []byte) (map[string]Entry, map[string]string) { } m[key(e.App, e.Service, e.Setting)] = e } + owner := map[string]string{} for app, f := range l.AppdataFolders { if !oneSegment(f) { panic("privpolicy: allowlist.json appdata folder of " + app + " is not one folder name: " + f) } + if o, dup := owner[f]; dup { + panic("privpolicy: allowlist.json gives the appdata folder " + f + " to both " + o + " and " + app) + } + owner[f] = app } return m, l.AppdataFolders } @@ -112,6 +117,13 @@ func AppdataFolder(app string) string { if !oneSegment(app) { return "" } + // A folder the map gives to ANOTHER app is not this app's, even when this app is named like it: a catalog push + // adding a template called "paperless" must not get paperless-ngx's files (security review 2026-10-18). + for _, f := range appdataFolders { + if f == app { + return "" + } + } return app } diff --git a/controller/internal/privpolicy/privpolicy_test.go b/controller/internal/privpolicy/privpolicy_test.go index 7d0ff5f..f7c0322 100644 --- a/controller/internal/privpolicy/privpolicy_test.go +++ b/controller/internal/privpolicy/privpolicy_test.go @@ -527,6 +527,21 @@ func TestAllowList_AppdataFolderMustBeOneName(t *testing.T) { mustLoad([]byte(`{"version":2,"entries":[],"appdata_folders":{"app":"` + bad + `"}}`)) }() } + // Security review 2026-10-18: an app NAMED like a folder the map gives to another app gets no root at all. + if got := AppdataFolder("paperless"); got != "" { + t.Errorf("AppdataFolder(paperless) = %q — a template named like paperless-ngx's folder must not own it", got) + } + if err := admit(t, "paperless", "services:\n app:\n image: x/y:1\n volumes:\n - ${HDD_PATH}/appdata/paperless/media:/m\n", env()); err == nil { + t.Error("a template named paperless was admitted to paperless-ngx's folder") + } + func() { + defer func() { + if recover() == nil { + t.Error("one folder given to two apps was loaded") + } + }() + mustLoad([]byte(`{"version":2,"entries":[],"appdata_folders":{"a":"x","b":"x"}}`)) + }() if got := AppdataFolder("paperless-ngx"); got != "paperless" { t.Errorf("AppdataFolder(paperless-ngx) = %q", got) } diff --git a/controller/internal/stacks/allowlist.go b/controller/internal/stacks/allowlist.go index 8fe6483..3732a1f 100644 --- a/controller/internal/stacks/allowlist.go +++ b/controller/internal/stacks/allowlist.go @@ -34,6 +34,10 @@ func (m *Manager) policyEnv(env map[string]string) map[string]string { out[k] = v } out["DOMAIN"] = m.cfg.Customer.Domain + // Security review 2026-10-18: USERDATA_PATH is ALWAYS the controller's own derivation, never the app's env. A + // catalog push can add a deploy field (InjectMissingFields fills it on the next sync), so an app-env value would + // let a template name any folder — another app's appdata/ included — as its root. No template declares one. + delete(out, "USERDATA_PATH") if hdd := env["HDD_PATH"]; hdd != "" { out["USERDATA_PATH"] = appbackup.UserdataDir(appbackup.NamespaceRootFor(hdd, m.sysDataPath)) } diff --git a/controller/internal/stacks/allowlist_test.go b/controller/internal/stacks/allowlist_test.go index ca4ab22..3f10742 100644 --- a/controller/internal/stacks/allowlist_test.go +++ b/controller/internal/stacks/allowlist_test.go @@ -89,6 +89,17 @@ func TestAllowList_PolicyEnvDerivesTheRootsLikeComposeExec(t *testing.T) { if e["USERDATA_PATH"] == "/" || !strings.HasPrefix(e["USERDATA_PATH"], "/mnt/felhom-drives/a/") { t.Fatalf("USERDATA_PATH = %q — the app's own value must not become a data root", e["USERDATA_PATH"]) } + // Security review 2026-10-18: with NO drive, an app-env USERDATA_PATH (a deploy field a catalog push can add) is + // no root either — it could name another app's appdata/. Consequence: such a bind is refused. + // RED-PROOF: without the delete in policyEnv, the bind into nextcloud's folder is admitted. + noHDD := map[string]string{"USERDATA_PATH": "/mnt/felhom-drives/a/felhom-data/appdata"} + if v, ok := m.policyEnv(noHDD)["USERDATA_PATH"]; ok { + t.Fatalf("an app with no drive kept its own USERDATA_PATH %q as a root", v) + } + c := "services:\n web:\n image: x/y:1\n volumes:\n - ${USERDATA_PATH}/nextcloud:/x\n" + if err := m.admitCompose("web", []byte(c), noHDD); err == nil { + t.Fatal("a bind into another app's folder through an app-env USERDATA_PATH was admitted") + } } // The install / settings / restore refusal is a bundle key: the reader gets it in their language, with the app and