Make the two taint engines agree - #115
Merged
b-macker merged 1 commit intoAug 2, 2026
Merged
Conversation
Taint tracking is implemented twice — the tree-walker walks the AST in
expressionContainsTaint(), the VM carries it on taint_stack_ — and nothing
checked the two agree. Tracing the tracker cold and running 12 scenarios through
both engines found two divergences, in opposite directions, each hiding the
other.
The declaration after a taint source inherited its taint. checkRhsTainted()
clears lastReturnWasTainted on entry and then SETS it again when it identifies a
direct source, so VarDeclStmt can read lastTaintSource() for lineage. Nothing
cleared it afterwards, so the next declaration's step-3 check picked it up:
let t = env.get("HOME") // legitimately tainted
let c = "a clean literal" // silently tainted by the stale flag
That is a false positive on clean data, which makes every taint count
untrustworthy — including the L25-03 baseline. It presented intermittently:
with more statements in between, the stale flag lands on a throwaway variable
that never reaches a sink and nothing is visibly wrong. The identical hazard was
already fixed for ExprStmt (V13-S7); the declaration path was missed.
try as an expression laundered taint. expressionContainsTaint() did not handle
TryCatchExpr, so `let x = try { tainted } catch (e) { "" }` produced an
untainted x. The walk returns false for any node type it does not handle, so
this is fail-open by construction; YieldExpr was missing for the same reason.
The two cancelled. The stale flag was tainting the try-expression anyway, so the
missing TryCatchExpr case was invisible — an early test of it passed because of
a bug. Fixing the leak exposed it as a fresh divergence. The vacuity check shows
the interaction directly: with both reverted try_expr PASSES, and it only fails
once the stale-flag fix is in.
Neither could have been caught by what exists. tests/differential/ is there
precisely to catch VM/tree-walker divergence, but diff_runner.py runs
--no-governance, so taint is switched off in that corpus by construction. The
property suite's test_taint_monotonicity.naab runs on a single engine, so a
divergence is invisible to it too. test_taint_engine_parity.sh closes the gap
and is registered in run-all-tests.sh; its pass condition is agreement PLUS a
per-engine expectation, because two engines that both miss a taint agree
perfectly, plus a control requiring five positive detections so the clean cases
cannot pass vacuously.
One finding retracted. "The tree-walker misses inline subscripts at sinks" was a
measurement artifact: violations print in two formats — checkTaintedSink() names
the variable, checkExpressionTaintedSink()'s expression path does not — and the
detector matched only one, so a working engine looked broken. Recorded because
it is the fourth time in this campaign that a detector carried the defect it was
hunting.
Also corrects the CLAUDE.md path for governance_taint.cpp, which is in
src/interpreter/, not src/runtime/.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ELUfjXZvx8kzXo1UJjrAhC
NAAb Governance Report
All governance checks passed! Generated by NAAb Governance Engine v4.0 |
b-macker
marked this pull request as ready for review
August 2, 2026 00:37
4 tasks
b-macker
added a commit
that referenced
this pull request
Aug 2, 2026
govern.json sandbox settings reached only the VM: syncGovernanceToSandbox() and the sandbox_level rebuild sat inside `if (use_vm)` in main.cpp, so under --tree-walk security.sandbox_level, capabilities.shell/network, and the enforce-mode fail-closed upgrade were all inert. A config that read as locked down enforced nothing on a supported engine. CLI --sandbox-level worked on both engines, which is why the gap stayed hidden. Separately, `async fn` ran unsandboxed on BOTH engines: current_sandbox is thread_local and every capability check fails open on a null one, and neither engine's async lambda established one — though the tree-walker's already propagated taint, counters and governance config. The VM keeps its inline block deliberately; swapping the default engine's working code carries more risk than the duplication does. The guard is a test, not a refactor: test_sandbox_engine_parity.sh, 6 cases, two of them controls proving the probes can distinguish. Duplication is not what let the taint engines diverge in #115 — nothing testing that the two paths agreed is. Also: skip that suite on Windows (absolute-path probes are not meaningful for a native binary under MSYS2), and bound the two Windows CLI test steps with timeout-minutes so the next runner stall produces logs instead of a 404. 441 tests / 0 unexpected, 874 leak checks / 0 failures.
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
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.
Summary
Taint tracking is implemented twice — the tree-walker walks the AST in
expressionContainsTaint(), the VM carries taint ontaint_stack_. CLAUDE.md says changes "must be made in both paths", and nothing checked that they were.Running 12 scenarios through both engines found two divergences, in opposite directions, each hiding the other.
Defect 1 — the declaration after a taint source inherited its taint
checkRhsTainted()clearslastReturnWasTaintedon entry, then sets it again when it identifies a direct source, soVarDeclStmtcan readlastTaintSource()for lineage. Nothing cleared it afterwards:A false positive on clean data, which makes every taint count untrustworthy — including
L25-03's baseline. It presented intermittently: with more statements in between, the stale flag lands on a throwaway variable that never reaches a sink and nothing looks wrong.The identical hazard was already fixed for
ExprStmt(V13-S7,interpreter.cpp); the declaration path was missed.Defect 2 —
tryas an expression laundered taintexpressionContainsTaint()did not handleTryCatchExpr, solet x = try { tainted } catch (e) { "" }produced an untaintedx. The walk returns false for any node type it does not handle, so this is fail-open by construction.YieldExprwas missing for the same reason.They cancelled each other
Defect 1 was tainting the try-expression anyway, so defect 2 was invisible — an early test of the try case passed because of a bug. Fixing the leak exposed it as a fresh divergence.
The vacuity check shows the interaction directly:
clean_after_source(VM 0, tree-walk 2) — andtry_exprpassesTryCatchExprfix revertedtry_expr(VM 2, tree-walk 0)Neither would have been found by fixing them in the other order, or by testing either in isolation.
Why nothing caught them
Both blind spots are structural rather than oversights:
tests/differential/exists precisely to catch VM/tree-walker divergence, butdiff_runner.pyruns--no-governance— taint is switched off in that corpus by construction.tests/property/test_taint_monotonicity.naabruns on a single engine, so a divergence is invisible to it.New
tests/governance_v4/test_taint_engine_parity.sh(12 scenarios × 2 engines), registered inrun-all-tests.sh. Its pass condition is agreement plus a per-engine expectation — two engines that both miss a taint agree perfectly — with a control requiring ≥5 positive detections so the clean cases cannot pass vacuously.Changes
src/interpreter/interpreter.cpp—VarDeclStmtconsumes the stale flag after the lineage lookup, mirroring theExprStmtfixsrc/interpreter/governance_taint.cpp—TryCatchExpr(both arms) andYieldExprhandled;ThrowExprdocumented as N/Atests/governance_v4/test_taint_engine_parity.sh+ registrationCLAUDE.md— corrects thegovernance_taint.cpppath (src/interpreter/, notsrc/runtime/), and records the fail-open walk, the dual implementation, and the two violation message formatsTest Plan
bash run-all-tests.shwith no new failures — 441 tests, 0 unexpected failurestest_taint_engine_parity.sh, 13/13bash tests/security/test_error_msg_leaks.sh— 874 checks, 0 failuresOne finding retracted
A third divergence — "the tree-walker misses inline subscripts at sinks" — was wrong, and was a measurement artifact. Violations print in two formats:
checkTaintedSink()names the variable (taint_tracking.sink_violation), whilecheckExpressionTaintedSink()'s expression path emitsTaint tracking violation: expression contains untrusted data.... The detector matched only the first, so a working engine looked broken.Recorded in the findings doc rather than dropped, because it is the fourth time in this campaign that a detector carried the defect it was hunting.
Related Issues
Follow-up to #113, which found the same class in the assertion set. This is the engine-side counterpart.
Generated by Claude Code