fix(cache): root-escape guard in mirror walker purgeOrphans
Sub-threshold finding from a focused security review of the CI URL work — defense-in-depth even though it sits inside the documented "trust upstream" boundary. The mirror walker's purgeOrphans deletes local files that aren't in the upstream's listing. It walked a dirPath built recursively from upstream-supplied entry names and called os.Remove on the resolved local path with no containment check. A hostile or compromised upstream returning ".." in a directory listing could steer the walker out of cache.root and into the parent — deleting whatever matches the upstream's "expected to be there" filter in the wrong directory. A healthy master never produces such entries (listing.FromDirEntries filters dot-prefix names), so the bug only fires under an actively malicious or MITM'd upstream — confidence stayed below the report threshold. But the fix is small and the cost of being wrong is real deletion of files outside the cache, so it's worth doing. Two layers: 1. walker.go walkDir filters upstream listing entries with name == "" / "." / ".." or containing "/" / "\" before recursing. Logs a WARN with the dropped name so an operator can see if their upstream is misbehaving. 2. purgeOrphans verifies the resolved localDir is contained under s.cache.root (HasPrefix(root + sep) || == root) before ReadDir+Remove. Logs a WARN and bails on mismatch. Either layer alone would fix the original vector; both together match the defense-in-depth pattern cachePathFor already follows for single-file writes (line 506). New TestWalker_HostileUpstreamCannotEscapeCacheRoot constructs a fake upstream that returns a "../" entry in its listing, places a sentinel file in the parent of cache.root, runs a mirror walk, and asserts the sentinel survives. Both filter and containment guard fire; the sentinel stays put. Existing mirror tests unchanged — the filter only drops names that shouldn't appear in healthy listings. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
85521b98de
commit
2ce5336289
2 changed files with 82 additions and 0 deletions
25
zddc/internal/cache/walker.go
vendored
25
zddc/internal/cache/walker.go
vendored
|
|
@ -206,6 +206,20 @@ func (s *MirrorScheduler) walkSubtree(ctx context.Context, subtree string) (walk
|
||||||
return ctx.Err()
|
return ctx.Err()
|
||||||
}
|
}
|
||||||
name := strings.TrimSuffix(e.Name, "/")
|
name := strings.TrimSuffix(e.Name, "/")
|
||||||
|
// Defense-in-depth against a hostile or compromised
|
||||||
|
// upstream listing: drop any entry name that could
|
||||||
|
// steer the walker — and the orphan-purge pass below —
|
||||||
|
// outside the cache root. A healthy master's listing
|
||||||
|
// pipeline (internal/listing/listing.go) already filters
|
||||||
|
// these, so this only fires under MITM or upstream
|
||||||
|
// compromise. An empty / "."/".." / slash-bearing name
|
||||||
|
// has no legitimate use as a child entry.
|
||||||
|
if name == "" || name == "." || name == ".." ||
|
||||||
|
strings.ContainsAny(name, "/\\") {
|
||||||
|
slog.Warn("walker: dropping unsafe upstream listing entry",
|
||||||
|
"dir", dirPath, "name", e.Name)
|
||||||
|
continue
|
||||||
|
}
|
||||||
childURL := dirURL + url.PathEscape(name)
|
childURL := dirURL + url.PathEscape(name)
|
||||||
childPath := dirPath + name
|
childPath := dirPath + name
|
||||||
if e.IsDir {
|
if e.IsDir {
|
||||||
|
|
@ -248,6 +262,17 @@ func (s *MirrorScheduler) walkSubtree(ctx context.Context, subtree string) (walk
|
||||||
func (s *MirrorScheduler) purgeOrphans(dirPath string, upstreamNames map[string]bool, addPurged func()) {
|
func (s *MirrorScheduler) purgeOrphans(dirPath string, upstreamNames map[string]bool, addPurged func()) {
|
||||||
rel := filepath.FromSlash(strings.Trim(dirPath, "/"))
|
rel := filepath.FromSlash(strings.Trim(dirPath, "/"))
|
||||||
localDir := filepath.Join(s.cache.root, rel)
|
localDir := filepath.Join(s.cache.root, rel)
|
||||||
|
// Belt-and-suspenders containment check: even though walkDir
|
||||||
|
// filters unsafe names before recursing, refuse to operate on
|
||||||
|
// any localDir that filepath.Join resolved outside the cache
|
||||||
|
// root. Without this, an upstream listing with a `..` entry
|
||||||
|
// that slipped past the walker's filter could steer os.Remove
|
||||||
|
// at a parent of cache.root.
|
||||||
|
if localDir != s.cache.root && !strings.HasPrefix(localDir, s.cache.root+string(filepath.Separator)) {
|
||||||
|
slog.Warn("walker: refusing purge outside cache root",
|
||||||
|
"dirPath", dirPath, "resolved", localDir)
|
||||||
|
return
|
||||||
|
}
|
||||||
entries, err := os.ReadDir(localDir)
|
entries, err := os.ReadDir(localDir)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return
|
return
|
||||||
|
|
|
||||||
57
zddc/internal/cache/walker_test.go
vendored
57
zddc/internal/cache/walker_test.go
vendored
|
|
@ -436,3 +436,60 @@ func TestLoadMirrorState_CorruptReturnsEmpty(t *testing.T) {
|
||||||
t.Errorf("corrupt should yield empty map, got %v", got)
|
t.Errorf("corrupt should yield empty map, got %v", got)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// TestWalker_HostileUpstreamCannotEscapeCacheRoot: a malicious upstream
|
||||||
|
// listing that includes ".." or slash-bearing entry names must not steer
|
||||||
|
// the walker's purge or fetch outside the cache root. The walker drops
|
||||||
|
// such entries early; purgeOrphans's containment check is a second line
|
||||||
|
// of defense if any slip through.
|
||||||
|
func TestWalker_HostileUpstreamCannotEscapeCacheRoot(t *testing.T) {
|
||||||
|
// Set up a parent directory we'd like NOT to lose files from.
|
||||||
|
parent := t.TempDir()
|
||||||
|
sentinel := filepath.Join(parent, "sentinel.txt")
|
||||||
|
if err := os.WriteFile(sentinel, []byte("must-not-be-deleted"), 0o644); err != nil {
|
||||||
|
t.Fatal(err)
|
||||||
|
}
|
||||||
|
|
||||||
|
cacheRoot := filepath.Join(parent, "cache")
|
||||||
|
if err := os.MkdirAll(cacheRoot, 0o755); err != nil {
|
||||||
|
t.Fatal(err)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Hostile upstream: returns a listing for /Vendors/Acme/ that
|
||||||
|
// includes a ".." entry. A naive walker would recurse into
|
||||||
|
// /Vendors/Acme/../ and then purgeOrphans would operate on the
|
||||||
|
// PARENT of cacheRoot — deleting the sentinel.
|
||||||
|
upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||||
|
switch r.URL.Path {
|
||||||
|
case "/Vendors/Acme/":
|
||||||
|
w.Header().Set("Content-Type", "application/json")
|
||||||
|
_, _ = w.Write([]byte(`[
|
||||||
|
{"name":"a.txt","is_dir":false},
|
||||||
|
{"name":"../","is_dir":true},
|
||||||
|
{"name":"sub/with/slashes","is_dir":false}
|
||||||
|
]`))
|
||||||
|
case "/Vendors/Acme/a.txt":
|
||||||
|
_, _ = w.Write([]byte("alpha"))
|
||||||
|
default:
|
||||||
|
http.NotFound(w, r)
|
||||||
|
}
|
||||||
|
}))
|
||||||
|
defer upstream.Close()
|
||||||
|
|
||||||
|
c, err := New(config.Config{Root: cacheRoot, Upstream: upstream.URL, Mode: "mirror"})
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("New: %v", err)
|
||||||
|
}
|
||||||
|
sched, err := NewMirrorScheduler(c, []string{"/Vendors/Acme"}, 5*time.Millisecond, 0)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("NewMirrorScheduler: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
sched.Trigger("/Vendors/Acme/a.txt")
|
||||||
|
waitForFile(t, filepath.Join(cacheRoot, "Vendors", "Acme", "a.txt"), 2*time.Second)
|
||||||
|
|
||||||
|
// The sentinel outside the cache root must still be there.
|
||||||
|
if _, err := os.Stat(sentinel); err != nil {
|
||||||
|
t.Fatalf("sentinel was removed — walker escaped cache root: %v", err)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue