Skip to content

Commit 1a55b59

Browse files
authored
backup: post-open IsRegular re-check in readRootFile (claude review #831) (#834)
## Summary **Post-open `IsRegular` re-check in `readRootFile`** — a security hardening claude flagged during the PR #831 review that merged without the fix. `readRootFile` (the Redis blob `*.bin` reader) is reached via `walkBlobDir` → `handleBlobEntry`, which pre-filters entries by the **ReadDir** entry type (`ent.Type().IsRegular()`). That type is **stale**: a FIFO/device swapped in for a `*.bin` between `ReadDir` and `root.Open` passes the stale check and reaches `io.ReadAll` — blocking on a writer-attached FIFO, or ingesting attacker-controlled bytes. This adds the authoritative **post-open fstat** `IsRegular` check (the read-side analogue of the post-open guard the JSON-collection walk paths already use), ordered before `refuseHardLink` so a non-regular target is classified as not-regular rather than hard-linked. ## Risk Hardening only; tightens an untrusted-dump-input boundary in the offline encode tool. No change to the happy path (regular `.bin` files read identically). ## Self-review (5 lenses) - **Data loss**: none — read-only offline tool; rejects bad input rather than dropping good data. - **Concurrency/distributed**: closes a TOCTOU window (stale ReadDir type vs. authoritative post-open fstat) in a single-goroutine offline encoder. - **Performance**: one extra `Mode()` comparison on an already-`Stat`'d fd; no new syscall. - **Consistency**: matches the post-open `IsRegular` pattern already used by the JSON-collection walk and the DDB schema reader. - **Test coverage**: new co-located regression test calls `readRootFile` on a directory (cross-platform non-regular stand-in) and asserts `ErrRedisEncodeNotRegular`; confirmed failing without the guard (returned the hard-link error) and passing with it. ## Test plan - [x] `go test ./internal/backup/` (full package) - [x] `golangci-lint run internal/backup/` (0 issues) - [x] Regression test fails without the guard, passes with it (verified by temporarily reverting the guard)
2 parents ef6a58e + 1d0e40d commit 1a55b59

2 files changed

Lines changed: 32 additions & 0 deletions

File tree

internal/backup/encode_redis.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -363,6 +363,14 @@ func readRootFile(root *os.Root, name string) ([]byte, error) {
363363
if err != nil {
364364
return nil, errors.WithStack(err)
365365
}
366+
// Re-check regularity on the OPEN fd: the ReadDir entry type is
367+
// stale, so a FIFO/device swapped in between ReadDir and Open
368+
// would otherwise reach io.ReadAll (blocking, or reading
369+
// attacker-controlled bytes from a writer-attached FIFO). The
370+
// post-open fstat is authoritative; the ReadDir type is not.
371+
if !info.Mode().IsRegular() {
372+
return nil, errors.Wrapf(ErrRedisEncodeNotRegular, "%s (mode=%s)", name, info.Mode())
373+
}
366374
if err := refuseHardLink(info, name); err != nil {
367375
return nil, err
368376
}

internal/backup/encode_redis_test.go

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -289,6 +289,30 @@ func TestRedisEncodeRejectsNonRegularTTLSidecar(t *testing.T) {
289289
}
290290
}
291291

292+
// TestReadRootFileRejectsNonRegularPostOpen pins the post-open fstat
293+
// guard in readRootFile, which is distinct from the walk's pre-open
294+
// ReadDir-type filter: the ReadDir entry type is stale, so a FIFO/device
295+
// swapped in between ReadDir and Open would otherwise reach io.ReadAll
296+
// (blocking on a writer-attached FIFO, or ingesting attacker-controlled
297+
// bytes). A directory is the cross-platform stand-in — it opens cleanly
298+
// but fstat reports non-regular, so readRootFile must fail closed with
299+
// ErrRedisEncodeNotRegular rather than reading it.
300+
func TestReadRootFileRejectsNonRegularPostOpen(t *testing.T) {
301+
t.Parallel()
302+
dir := t.TempDir()
303+
if err := os.Mkdir(filepath.Join(dir, "notafile.bin"), 0o755); err != nil {
304+
t.Fatalf("mkdir: %v", err)
305+
}
306+
root, err := os.OpenRoot(dir)
307+
if err != nil {
308+
t.Fatalf("OpenRoot: %v", err)
309+
}
310+
defer func() { _ = root.Close() }()
311+
if _, err := readRootFile(root, "notafile.bin"); !errors.Is(err, ErrRedisEncodeNotRegular) {
312+
t.Fatalf("readRootFile err = %v, want ErrRedisEncodeNotRegular", err)
313+
}
314+
}
315+
292316
// writeKeymap writes a single-entry KEYMAP.jsonl mapping the encoded
293317
// segment back to its original bytes (base64url, the KeymapRecord
294318
// schema), under <root>/redis/db_0/.

0 commit comments

Comments
 (0)