From cf209010df633d4b65c13990b11381873faab255 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Sun, 12 Jul 2026 10:47:25 +0100 Subject: [PATCH 1/2] fix: rules-loader guarded read (O_NOFOLLOW+O_NONBLOCK) + private state dir (iss-66) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Trust-boundary hardening for the rules loader, consolidated into one guarded reader used by all three file readers: - C19: Load re-resolved .abcd/rules.json by name (Lstat then ReadFile), a symlink-swap TOCTOU. It now reads via readGuarded: open once with O_NOFOLLOW and validate the SAME fd (regular file, size cap) before reading. - F1 (fixes a regression the first cut introduced): O_NOFOLLOW alone let a FIFO leaf block open(O_RDONLY) forever, wedging the non-blocking-by-contract prompt-router hook. readGuarded adds O_NONBLOCK so a FIFO/device leaf returns immediately and the regular-file check rejects it. Mkfifo regression test. - P14: prompt-router session-state moved off the world-writable shared temp dir to the per-user cache dir (ABCD_RULES_STATE_DIR still overrides). The degraded uid-qualified temp fallback is documented as best-effort, not a guarantee. - LoadState and LoadBackstop now share readGuarded too (was the same Lstat-then- read TOCTOU); the size cap is enforced post-read as well as via fstat. P15 (a per-repo rules.json can set a default domain dormant / flip the kill switch) is document-accepted, not changed — rules are an opt-in, committed, opinionated-but-overridable layer; real enforcement is harness-level. The protected-guardrail-domain alternative is surfaced in DECISIONS.md. Assisted-by: Claude:claude-opus-4-8 --- .abcd/work/DECISIONS.md | 15 ++++++ CHANGELOG.md | 7 +++ internal/core/rules/inject.go | 28 ++++++----- internal/core/rules/inject_test.go | 19 ++++++++ internal/core/rules/rules.go | 77 ++++++++++++++++++++++-------- internal/core/rules/rules_test.go | 52 ++++++++++++++++++++ 6 files changed, 164 insertions(+), 34 deletions(-) diff --git a/.abcd/work/DECISIONS.md b/.abcd/work/DECISIONS.md index ff3eb279..cf3b7e07 100644 --- a/.abcd/work/DECISIONS.md +++ b/.abcd/work/DECISIONS.md @@ -228,3 +228,18 @@ parallel-agent merge contention bites. sign-off before landing (a STOP-adjacent design surface). Ledger triage committed to `main` as a `chore:` record commit (matches prior record-to-main practice; keeps the fix branches clean); each code fix lands on its own `auto/*` branch + PR. +- 2026-07-12 — iss-66 rules-loader trust boundary. Fixed the two mechanical items: + the Load Lstat→ReadFile TOCTOU on `.abcd/rules.json` (now open-once O_NOFOLLOW + + fstat, C19) and the session-state dir moved off the world-writable shared /tmp to + the per-user cache dir (P14). **P15 document-accepted, NOT changed:** a per-repo + `.abcd/rules.json` can set a default domain dormant and flip the global kill + switch (Merge is intentionally per-field + sticky-kill-switch). Rationale: rules + are an *opt-in, opinionated-but-overridable* config layer; `.abcd/rules.json` is a + committed file (editing it needs repo write access, like any committed guardrail), + and the real enforcement of dangerous actions is harness-level (git-guardrails + hooks, the iss-62 identity gate, pre-commit), not the injected advisory prose. + Silencing a domain removes prose, not a hard gate. **Deferred design alternative + (surfaced, not taken):** introduce a protected "guardrail" domain class that a + per-repo override cannot set dormant and that the kill switch cannot silence — + this adds a new protected-domain concept to the rules contract, a maintainer + decision, not an autonomous change. diff --git a/CHANGELOG.md b/CHANGELOG.md index 4985c7dd..0e81218c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -152,6 +152,13 @@ called out in a **Breaking** section. misclassifies a valid UTF-8 file as binary when a multibyte rune straddles the 8 KB boundary (which would have skipped scanning it), and a bundle file that cannot be read is now surfaced in `unscanned` rather than silently dropped. +- **Rules-loader trust hardening** (iss-66). The per-repo `.abcd/rules.json` is now + opened once with `O_NOFOLLOW` and validated on that file descriptor, closing a + Lstat-then-read window where the file could be swapped for a symlink. The + prompt-router's per-session dedup state moved off the world-writable shared temp + dir to the per-user cache dir (`ABCD_RULES_STATE_DIR` still overrides), so a local + co-tenant can no longer pre-create the predictable state path to suppress rule + injection. ## [v0.1.0] - 2026-07-07 diff --git a/internal/core/rules/inject.go b/internal/core/rules/inject.go index 7420864f..02758e4d 100644 --- a/internal/core/rules/inject.go +++ b/internal/core/rules/inject.go @@ -4,6 +4,7 @@ import ( "crypto/sha256" "encoding/hex" "encoding/json" + "fmt" "os" "path/filepath" "sort" @@ -91,12 +92,22 @@ func Inject(rs RuleSet, prompt string, prev SessionState, backstop int) InjectRe // stateDir is the machine-local directory holding per-session ledgers. It is // overridable via ABCD_RULES_STATE_DIR (used by tests and by operators who want -// the state off the default temp dir). +// the state elsewhere). The default is the per-user cache dir, NOT the +// world-writable shared temp dir: a predictable path under a shared /tmp lets a +// local co-tenant pre-create or poison the session-state file and suppress rule +// injection fail-open. The uid-qualified temp fallback (only if the user cache +// dir is unavailable — a degraded env with no HOME/XDG_CACHE_HOME) still lives +// under the shared temp dir and is best-effort only: it avoids a cross-user +// collision, not co-tenant pre-creation. State is fail-open advisory dedup, so a +// poisoned fallback at worst re-injects; it never blocks or corrupts. func stateDir() string { if d := os.Getenv("ABCD_RULES_STATE_DIR"); d != "" { return d } - return filepath.Join(os.TempDir(), "abcd-rules-state") + if cache, err := os.UserCacheDir(); err == nil { + return filepath.Join(cache, "abcd-rules-state") + } + return filepath.Join(os.TempDir(), fmt.Sprintf("abcd-rules-state-%d", os.Getuid())) } // sessionFile maps a session id to a state file. The id is hashed, so an @@ -110,12 +121,7 @@ func sessionFile(session string) string { // file yields the zero state (a fresh session), never an error — dedup is a // best-effort optimisation, not a correctness gate. func LoadState(session string) SessionState { - path := sessionFile(session) - fi, err := os.Lstat(path) - if err != nil || !fi.Mode().IsRegular() || fi.Size() > maxStateFileBytes { - return SessionState{} - } - data, err := os.ReadFile(path) + data, err := readGuarded(sessionFile(session), maxStateFileBytes) if err != nil { return SessionState{} } @@ -167,11 +173,7 @@ func SaveState(session string, st SessionState) error { // non-positive value falls back to DefaultRefreshBackstop. func LoadBackstop(repoRoot string) int { path := filepath.Join(repoRoot, ".abcd", "config.json") - fi, err := os.Lstat(path) - if err != nil || !fi.Mode().IsRegular() || fi.Size() > maxRulesFileBytes { - return DefaultRefreshBackstop - } - data, err := os.ReadFile(path) + data, err := readGuarded(path, maxRulesFileBytes) if err != nil { return DefaultRefreshBackstop } diff --git a/internal/core/rules/inject_test.go b/internal/core/rules/inject_test.go index bc96f74b..c7233de1 100644 --- a/internal/core/rules/inject_test.go +++ b/internal/core/rules/inject_test.go @@ -8,6 +8,25 @@ import ( "time" ) +// TestStateDirNotSharedTemp (iss-66 P14) proves the default session-state dir is +// the per-user cache dir, not the world-writable shared temp dir where a local +// co-tenant could pre-create the predictable session-state path and suppress +// rule injection fail-open. +func TestStateDirNotSharedTemp(t *testing.T) { + t.Setenv("ABCD_RULES_STATE_DIR", "") // exercise the default + cache, err := os.UserCacheDir() + if err != nil { + t.Skip("user cache dir unavailable; only the degraded uid-qualified temp fallback applies") + } + dir := stateDir() + if !strings.HasPrefix(dir, cache) { + t.Fatalf("default state dir should live under the user cache dir %s, got %s", cache, dir) + } + if strings.HasPrefix(dir, os.TempDir()) { + t.Fatalf("default state dir must not live under the shared temp dir, got %s", dir) + } +} + func TestInjectFirstTurnRenders(t *testing.T) { rs := Defaults() res := Inject(rs, "commit and push", SessionState{}, 0) diff --git a/internal/core/rules/rules.go b/internal/core/rules/rules.go index 3deaef74..aa5142e9 100644 --- a/internal/core/rules/rules.go +++ b/internal/core/rules/rules.go @@ -20,13 +20,16 @@ package rules import ( _ "embed" "encoding/json" + "errors" "fmt" "hash/fnv" + "io" "os" "path/filepath" "regexp" "sort" "strings" + "syscall" ) // RepoRelPath is the per-repo override file, relative to the repo worktree. @@ -100,6 +103,46 @@ func mustParseDefaults() RuleSet { // the caller to mutate. func Defaults() RuleSet { return cloneRuleSet(defaultRuleSet) } +var ( + errNotRegular = errors.New("not a regular file") + errTooBig = errors.New("exceeds size cap") +) + +// readGuarded opens path once, read-only, with O_NOFOLLOW (refuse a symlinked +// leaf) and O_NONBLOCK (a FIFO/device leaf returns immediately instead of +// blocking the open forever), then validates on the SAME file descriptor that it +// is a regular file within limit bytes before reading through a LimitReader — so +// no symlink swap between stat and read, no non-regular leaf, and no size overrun +// can reach the caller. The raw open error is returned so callers can test +// os.IsNotExist / syscall.ELOOP; a non-regular or oversize file returns the +// errNotRegular / errTooBig sentinel. +func readGuarded(path string, limit int64) ([]byte, error) { + f, err := os.OpenFile(path, os.O_RDONLY|syscall.O_NOFOLLOW|syscall.O_NONBLOCK, 0) + if err != nil { + return nil, err + } + defer f.Close() + fi, err := f.Stat() + if err != nil { + return nil, err + } + if !fi.Mode().IsRegular() { + return nil, errNotRegular + } + if fi.Size() > limit { + return nil, errTooBig + } + data, err := io.ReadAll(io.LimitReader(f, limit+1)) + if err != nil { + return nil, err + } + if int64(len(data)) > limit { + // The file grew past the cap between fstat and read (a size TOCTOU). + return nil, errTooBig + } + return data, nil +} + // Load returns the defaults merged with /.abcd/rules.json when that // file exists. An absent file yields the defaults unchanged; a present file // that cannot be parsed or fails validation is a fail-closed error. @@ -110,28 +153,20 @@ func Load(repoRoot string) (RuleSet, error) { return RuleSet{}, fmt.Errorf("rules: .abcd is a symlink (refusing to follow)") } path := filepath.Join(repoRoot, ".abcd", "rules.json") - fi, err := os.Lstat(path) - if os.IsNotExist(err) { - return Defaults(), nil - } + data, err := readGuarded(path, maxRulesFileBytes) if err != nil { - return RuleSet{}, fmt.Errorf("rules: stat %s: %w", RepoRelPath, err) - } - // Trust-boundary guards (mirror the ahoy hook-manifest verifier): refuse a - // symlinked leaf and cap the file size so a hostile rules.json cannot force a - // symlink-follow or a memory blow-up. - if fi.Mode()&os.ModeSymlink != 0 { - return RuleSet{}, fmt.Errorf("rules: %s is a symlink (refusing to follow)", RepoRelPath) - } - if !fi.Mode().IsRegular() { - return RuleSet{}, fmt.Errorf("rules: %s is not a regular file", RepoRelPath) - } - if fi.Size() > maxRulesFileBytes { - return RuleSet{}, fmt.Errorf("rules: %s exceeds the %d-byte cap", RepoRelPath, maxRulesFileBytes) - } - data, err := os.ReadFile(path) - if err != nil { - return RuleSet{}, fmt.Errorf("rules: reading %s: %w", RepoRelPath, err) + switch { + case os.IsNotExist(err): + return Defaults(), nil + case errors.Is(err, syscall.ELOOP): + return RuleSet{}, fmt.Errorf("rules: %s is a symlink (refusing to follow)", RepoRelPath) + case errors.Is(err, errNotRegular): + return RuleSet{}, fmt.Errorf("rules: %s is not a regular file", RepoRelPath) + case errors.Is(err, errTooBig): + return RuleSet{}, fmt.Errorf("rules: %s exceeds the %d-byte cap", RepoRelPath, maxRulesFileBytes) + default: + return RuleSet{}, fmt.Errorf("rules: reading %s: %w", RepoRelPath, err) + } } var over RuleSet if err := json.Unmarshal(data, &over); err != nil { diff --git a/internal/core/rules/rules_test.go b/internal/core/rules/rules_test.go index 91bca54c..e1950369 100644 --- a/internal/core/rules/rules_test.go +++ b/internal/core/rules/rules_test.go @@ -4,9 +4,61 @@ import ( "os" "path/filepath" "strings" + "syscall" "testing" + "time" ) +// TestLoadRefusesSymlinkedRulesFile (iss-66 C19) proves a symlinked rules.json +// LEAF is refused via O_NOFOLLOW. The directory-symlink test covers only the +// ancestor guard; reverting O_NOFOLLOW makes Load follow the leaf link and read +// an arbitrary target, so this test fails on that revert. +func TestLoadRefusesSymlinkedRulesFile(t *testing.T) { + dir := t.TempDir() + abcd := filepath.Join(dir, ".abcd") + if err := os.MkdirAll(abcd, 0o755); err != nil { + t.Fatal(err) + } + target := filepath.Join(dir, "real-rules.json") + if err := os.WriteFile(target, []byte(`{"schema_version":1}`), 0o644); err != nil { + t.Fatal(err) + } + if err := os.Symlink(target, filepath.Join(abcd, "rules.json")); err != nil { + t.Fatal(err) + } + if _, err := Load(dir); err == nil { + t.Fatal("a symlinked rules.json leaf must be refused, not followed") + } +} + +// TestLoadRefusesFifoRulesFile (iss-66 F1) proves a FIFO leaf is rejected +// promptly rather than hung on. O_NONBLOCK lets the open return so the +// regular-file check fails closed; without it, open(FIFO, O_RDONLY) blocks +// forever and wedges the (non-blocking-by-contract) prompt-router hook. +func TestLoadRefusesFifoRulesFile(t *testing.T) { + dir := t.TempDir() + abcd := filepath.Join(dir, ".abcd") + if err := os.MkdirAll(abcd, 0o755); err != nil { + t.Fatal(err) + } + if err := syscall.Mkfifo(filepath.Join(abcd, "rules.json"), 0o644); err != nil { + t.Skipf("mkfifo unsupported: %v", err) + } + done := make(chan error, 1) + go func() { + _, err := Load(dir) + done <- err + }() + select { + case err := <-done: + if err == nil { + t.Fatal("a FIFO rules.json must be refused, not read") + } + case <-time.After(3 * time.Second): + t.Fatal("Load hung on a FIFO rules.json leaf (must never block the prompt-router hook)") + } +} + func names(ds []ResolvedDomain) []string { out := make([]string, len(ds)) for i, d := range ds { From 30743c75852bf51c91bc31fdce0068d7ca93a420 Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Sun, 12 Jul 2026 10:58:04 +0100 Subject: [PATCH 2/2] chore: resolve iss-66 in the ledger Assisted-by: Claude:claude-opus-4-8 --- .../{open => resolved}/iss-66-rules-loader-trust-boundary.md | 1 + 1 file changed, 1 insertion(+) rename .abcd/work/issues/{open => resolved}/iss-66-rules-loader-trust-boundary.md (63%) diff --git a/.abcd/work/issues/open/iss-66-rules-loader-trust-boundary.md b/.abcd/work/issues/resolved/iss-66-rules-loader-trust-boundary.md similarity index 63% rename from .abcd/work/issues/open/iss-66-rules-loader-trust-boundary.md rename to .abcd/work/issues/resolved/iss-66-rules-loader-trust-boundary.md index e7987206..f4c64d24 100644 --- a/.abcd/work/issues/open/iss-66-rules-loader-trust-boundary.md +++ b/.abcd/work/issues/resolved/iss-66-rules-loader-trust-boundary.md @@ -7,6 +7,7 @@ category: "bug" source: "agent-finding" found_during: "clean-slate-sweep" found_at: "internal/core/rules/rules.go" +resolution: "Rules loader hardened: guarded read (open-once O_NOFOLLOW+O_NONBLOCK+fstat, shared by Load/LoadState/LoadBackstop) closes the rules.json symlink-swap TOCTOU (C19) and a FIFO-leaf hang the first cut introduced (F1, caught by security review); session-state moved off shared /tmp to the per-user cache dir (P14). P15 (override can disable guardrail domains/kill switch) document-accepted with rationale + surfaced protected-domain alternative in DECISIONS. ruthless SHIP + security PASS (2 rounds; security found + re-verified the FIFO BLOCK)." --- rules loader trust-boundary (itd-3): an untrusted per-repo .abcd/rules.json can silence default guardrail domains — Merge() ORs the global kill switch (rules.go:156) and mergeDomain lets an override set any default domain dormant, so a cloned repo suppresses all rule injection (P15); session-state lives in a predictable shared /tmp path (os.TempDir/abcd-rules-state, sha256(session).json, session defaults to constant default) letting a local co-tenant suppress injection fail-open (inject.go:99, P14); Load() Lstats rules.json then os.ReadFile re-resolves by name, a symlink-swap TOCTOU (rules.go:132, C19). Detector: override cannot disable a default guardrail/kill-switch; state under UserCacheDir+ownership check; open-once O_NOFOLLOW+fstat. Corpus: P15, P14, C19. \ No newline at end of file