From 737036a6db5feab8e2a7d5ad8be70565cb2bc7b1 Mon Sep 17 00:00:00 2001 From: Michael Fornaro <20387402+xUnholy@users.noreply.github.com> Date: Wed, 1 Jul 2026 15:51:36 +1000 Subject: [PATCH] fix(audit): scan success defensively in VerifyChain so a poisoned value reports a break MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit VerifyChain scanned the success column into a bare Go bool. Legitimate rows always store 0/1 (schema DEFAULT 1; RecordCtx writes 0/1), but success and entry_hash are independent columns an attacker with direct SQLite write can set separately. Setting success to NULL or an out-of-range integer (e.g. 2) makes rows.Scan fail — "sql: Scan error on column 'success': couldn't convert 2 into type bool" — so VerifyChain returns (nil, error) and ABORTS the entire walk, instead of the designed "chain broken at row N" verdict. Impact: this is fail-loud, not fail-open — it can never read a tampered log as Valid=true (callers surface the error, not "valid"). But it degrades the tamper-evidence guarantee: an operator running audit_verify on a poisoned row gets an opaque type-conversion error instead of a pinpointed break, and the verifier can't report which/how many rows are broken because it aborts at the first poisoned row. A single UPDATE ... SET success=NULL turns the forensic integrity check into a self-inflicted DoS. Fix: scan success as sql.NullInt64 (matching the sibling duration_ms scan) and feed success.Int64 == 1 into chainHash. Recorded rows are always 0/1 so this is behaviour-preserving for legitimate logs; any NULL/out-of-range value now flows into the hash recompute, fails the comparison, and is correctly reported as a chain break (Valid=false, FirstBrokenID=id) rather than aborting. Found via the internal audit sweep of the audit query DSL + hash chain; the rest verified sound (chain pre-image injectivity via length-prefixed fields, verify recomputes rather than trusting the stored hash, reorder/deletion/truncation + CheckpointAnchor detection, fully-parameterized WHERE builder, capped LIMIT). Test: TestVerifyChain_PoisonedSuccessReportsBreak (success=2 and success=NULL) asserts VerifyChain reports a break at the poisoned row without erroring; proven to fail pre-fix (both cases aborted with a bool scan error). Verified: task ci green (lint 0 / vet / build / test:full -race 0 fail / govulncheck clean); task eval 17/17. Follow-up (LOW, banked): the same bare-bool scan exists in Query/QueryFiltered, where a NULL-success row scan-errors and is silently dropped from results; barely reachable (needs attacker DB write) so tracked separately. --- internal/audit/chain.go | 15 ++++++++++++--- internal/audit/chain_test.go | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 3 deletions(-) diff --git a/internal/audit/chain.go b/internal/audit/chain.go index faf98119..3dd1cc13 100644 --- a/internal/audit/chain.go +++ b/internal/audit/chain.go @@ -132,8 +132,17 @@ func (l *Log) VerifyChainAgainst(anchor *CheckpointAnchor) (*VerifyResult, error timestamp, tool, level string input, output, risk, sessionID sql.NullString durationMs sql.NullInt64 - success bool - storedHash sql.NullString + // success is scanned as NullInt64, not a bare bool: legitimate rows + // always store 0/1, but success and entry_hash are independent + // columns an attacker with direct DB write can set separately. A + // bare-bool scan of a poisoned NULL or out-of-range value (e.g. 2) + // fails the scan and aborts the entire verification walk with an + // opaque type error — turning a tamper into a DoS on the integrity + // check rather than the designed "chain broken at row N" verdict. + // Scanning defensively lets any non-1 value flow into the hash + // recompute below, where it correctly fails the comparison. + success sql.NullInt64 + storedHash sql.NullString ) if err := rows.Scan(&id, ×tamp, &tool, &input, &output, &risk, &level, &sessionID, &durationMs, &success, &storedHash); err != nil { @@ -151,7 +160,7 @@ func (l *Log) VerifyChainAgainst(anchor *CheckpointAnchor) (*VerifyResult, error res.HashedRows++ want := chainHash(prevHash, timestamp, tool, input.String, output.String, - risk.String, level, sessionID.String, durationMs.Int64, success) + risk.String, level, sessionID.String, durationMs.Int64, success.Int64 == 1) if want != storedHash.String { if res.Valid { // record only the first break res.Valid = false diff --git a/internal/audit/chain_test.go b/internal/audit/chain_test.go index faa60aef..629b4830 100644 --- a/internal/audit/chain_test.go +++ b/internal/audit/chain_test.go @@ -199,3 +199,39 @@ func TestVerifyChain_ContinuesAcrossReopen(t *testing.T) { t.Errorf("chain should span the reopen: %+v", res) } } + +// TestVerifyChain_PoisonedSuccessReportsBreak pins that a row whose success +// column is poisoned out-of-range (2) or to NULL by an attacker with direct DB +// write is reported as a chain break — not surfaced as an opaque scan error +// that aborts the entire verification walk. success and entry_hash are +// independent columns; scanning success as a bare bool turned a tamper into a +// DoS on the forensic integrity check. +func TestVerifyChain_PoisonedSuccessReportsBreak(t *testing.T) { + for _, tc := range []struct { + name string + poke string + }{ + {"out_of_range", `UPDATE audit_log SET success=2 WHERE id=2`}, + {"null", `UPDATE audit_log SET success=NULL WHERE id=2`}, + } { + t.Run(tc.name, func(t *testing.T) { + l := openTestLog(t) + for i := 0; i < 4; i++ { + l.Record("nfc", map[string]int{"i": i}, "ok", "low", LevelAction, 0, true) + } + if _, err := l.db.Exec(tc.poke); err != nil { + t.Fatalf("poison: %v", err) + } + res, err := l.VerifyChain() + if err != nil { + t.Fatalf("VerifyChain errored on a poisoned success value (must report a break, not abort the walk): %v", err) + } + if res.Valid { + t.Fatal("poisoned success value not detected as a chain break") + } + if res.FirstBrokenID != 2 { + t.Errorf("FirstBrokenID = %d, want 2", res.FirstBrokenID) + } + }) + } +}