fix(audit): scan success defensively in VerifyChain so a poisoned value reports a break#179
Merged
Merged
Conversation
…ue reports a break 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
VerifyChain scanned the success column into a bare Go bool. 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 (2) makes rows.Scan fail, so VerifyChain returns an error and ABORTS the entire walk instead of the designed 'chain broken at row N' verdict — a single UPDATE turns the forensic integrity check into a self-inflicted DoS. Fail-loud (never reads Valid=true), but degrades tamper-evidence. Fix: scan success as sql.NullInt64 (like duration_ms), hash success.Int64==1 — behaviour-preserving for legit 0/1 rows; any poisoned value now correctly reports a chain break. Found via the internal audit sweep (rest verified sound). Test proven to fail pre-fix. task ci green; eval 17/17.