Skip to content

Commit c09db4d

Browse files
Juanpacolclaude
andcommitted
feat(symbolic): T6 -- SQLi/race-condition pattern-matching prototype, real RuleEngine bug fix
Fase 5 of the research roadmap: SQL Injection Prevention and No Check-Then-Act Race have existed as KG rules since the project's earliest seed data, each with a PRE/POST formal_spec, but neither was ever wired to anything that extracts those preconditions from real code -- prompt guidance only, never independently checked. symbolic/security_facts.py adds two narrow, well-documented AST fact extractors (SQL query built via concatenation/f-string/%-format/ .format() into an execute-like call, with one-hop variable resolution and recognition of the safe parameterized-query idiom; unguarded check-then-act on a shared container, tracking real with-block containment for the lock guard). Z3 cannot help here regardless -- microsoft/z3guide's own Strings page (fetched as real evidence in the prior evidence-pipeline work) states its string solver is "an incomplete heuristic solver" and the combined theory "is not decidable anyway." Wiring the race-condition extractor's output into the pre-existing RuleEngine.apply_rule_to_code surfaced a real design gap, not by inspection but by running it: that method is a positive-derivation checker (precondition met -> derive postcondition -> PASS) with no code path that ever returns FAIL, for any input. Fed a rule whose PRE names a dangerous pattern, meeting that PRE derives the POST and reports PASS on genuinely vulnerable code. Fixed via a new, additive check_for_violation method (apply_rule_to_code itself untouched -- nothing else calls it) with the inverse framing: FAIL if the trigger is present and unmitigated, PASS if mitigated, UNKNOWN if the trigger doesn't apply (never an affirmative "proven safe"). Also corrected SQL Injection Prevention's formal_spec from prose to the same fact-string syntax No Check-Then-Act Race already happened to use. Verified end-to-end against hand-written vulnerable/safe fixtures for both patterns (neither existing security benchmark covers SQLi or race conditions) and against the two closest existing security-benchmark shapes, with zero false positives. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent dc19aae commit c09db4d

7 files changed

Lines changed: 764 additions & 11 deletions

File tree

docs/CASE_STUDY.md

Lines changed: 56 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -173,16 +173,62 @@ the cross-day noise floor — meaning the KG-context effect, unlike the
173173
retry-loop trade-off, is not just noise. Both the retraction and the
174174
survival came from applying the identical check; neither was assumed.
175175

176+
## Finding 6 — a rule engine that had been sitting there, quietly unable to fail
177+
178+
Two KG rules — `SQL Injection Prevention` and `No Check-Then-Act Race`
179+
existed in the seed data since the project's earliest commits, each with
180+
a `PRE`/`POST` formal spec describing exactly the pattern it should
181+
catch. Neither had ever been wired to anything that reads real code:
182+
they were retrieved into the LLM's prompt as guidance and never checked
183+
independently. Building that check (Fase 5 of the research roadmap, T6)
184+
meant writing two small AST-based fact extractors and feeding their
185+
output into `RuleEngine.apply_rule_to_code` — the method that already
186+
existed for exactly this purpose.
187+
188+
It returned `PASS` on the vulnerable sample. Not a crash, not an
189+
exception — a clean, confident, wrong answer on code containing the
190+
exact unguarded check-then-act race the rule's own precondition names.
191+
192+
The cause wasn't the new fact extractors; they were behaving correctly.
193+
`apply_rule_to_code` is a forward-chaining *derivation* checker (the IBM
194+
NSTK pattern this module cites in its own module docstring): precondition
195+
met → derive the postcondition → report `PASS`. That's the right
196+
behavior for deriving new facts from established ones. It is exactly the
197+
wrong behavior for "does this code violate a rule," because the method
198+
has no branch that returns `FAIL` — not a missing case that happens not
199+
to trigger here, but structurally absent from the function for any
200+
input. Feed it a rule whose precondition *is* the dangerous pattern, and
201+
meeting that precondition is treated as license to derive the "safe"
202+
postcondition and call it done. The second rule didn't even reach that
203+
bug: its formal spec was prose (`PRE: user_input is untrusted`) rather
204+
than a fact string, so it silently never fired at all — `UNKNOWN`,
205+
matching nothing, for a different reason than intended but at least not
206+
a false `PASS`.
207+
208+
The fix was small and additive on purpose: a new `check_for_violation`
209+
method with the inverse framing (precondition present + postcondition
210+
absent → `FAIL`; postcondition present → mitigated `PASS`; precondition
211+
absent → `UNKNOWN`, never an affirmative "proven safe"), plus correcting
212+
the SQL rule's formal spec to real fact-string syntax. `apply_rule_to_code`
213+
itself was left untouched — nothing else in the codebase calls it, so
214+
there was no reason to risk its behavior for a legitimate forward-chaining
215+
caller that might exist later. The bug had been shippable-looking code
216+
for as long as those two rules existed; it just had never been asked a
217+
question it could get wrong until real facts from real code were run
218+
through it.
219+
176220
## The pattern
177221

178-
None of these five findings came from writing more tests against the
222+
None of these six findings came from writing more tests against the
179223
existing fakes — they came from running the actual thing (or, in Finding
180-
5's case, re-checking a real run's own numbers harder) and dealing
181-
honestly with what happened, including retracting a prior claim, instead
182-
of defending it. That's the habit this case study is meant to name: a
183-
comprehensive offline test suite is a floor, not a ceiling. It proves the
184-
system does what the scripts told it to expect. It cannot prove the
185-
system does the right thing when something *unscripted* happens — and
186-
something unscripted is exactly what a real model, a real database, or a
187-
real user (or a second run of the same experiment) will always eventually
188-
do.
224+
5's case, re-checking a real run's own numbers harder, and in Finding 6's,
225+
finally connecting two pieces that had each looked fine in isolation) and
226+
dealing honestly with what happened, including retracting a prior claim,
227+
instead of defending it. That's the habit this case study is meant to
228+
name: a comprehensive offline test suite is a floor, not a ceiling. It
229+
proves the system does what the scripts told it to expect. It cannot
230+
prove the system does the right thing when something *unscripted*
231+
happens — and something unscripted is exactly what a real model, a real
232+
database, or a real user (or a second run of the same experiment, or
233+
finally wiring up a rule that had sat dormant since the beginning) will
234+
always eventually do.

docs/PHASE_3_METHODOLOGY.md

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -501,6 +501,104 @@ CLAUDE.md anticipates may behave differently — this ablation only covers
501501
10-48 rules, not the order-of-magnitude jump a real production corpus
502502
would represent.
503503

504+
## T6 — can pattern-matching close the SQLi/race-condition gap? (2026-07-15)
505+
506+
Fase 5 of the research roadmap: `docs/CASE_STUDY.md` and this document
507+
already note SQL injection and race-condition detection as explicitly out
508+
of Z3's scope (arbitrary string/heap reasoning isn't something an SMT
509+
solver decides). The KG seed data
510+
(`kg/seed_data/security_rules.json`) has carried a `SQL Injection
511+
Prevention` rule and a `No Check-Then-Act Race` rule since the project's
512+
earliest seed data was written -- both with `PRE`/`POST` formal specs --
513+
but neither had ever been wired to anything that extracts those
514+
preconditions from real code. They were prompt guidance only: injected
515+
into the LLM's context via retrieval, never independently checked.
516+
517+
**What was built**: `symbolic/security_facts.py`, two narrow AST-based
518+
fact extractors -- `extract_sql_injection_facts` (flags a query string
519+
built via concatenation/f-string/%-format/`.format()` and passed to an
520+
`execute`/`executemany`/`executescript`-named call, one hop through a
521+
local variable; recognizes the parameterized-query safe idiom) and
522+
`extract_race_condition_facts` (flags an `if key in container:` /
523+
`container.get(key)` check followed by a subscript assignment to the same
524+
container, anywhere in a function, with no enclosing lock-named `with`
525+
block). Both are deliberately narrow and say so in their own docstrings
526+
-- e.g. no cross-function data-flow, no ORM query builders, direct
527+
self-recursion-style syntactic matching only. Z3 genuinely cannot help
528+
here: the real evidence fetched in the evidence pipeline
529+
(`docs/evidence/z3_docs/z3_docs_d192712d2ab1.json`, microsoft/z3guide's
530+
own Strings theory page) states its string solver is "an incomplete
531+
heuristic solver" and the combined theory "is not decidable anyway" --
532+
this isn't a workaround for a temporary VerityAI limitation, it's the
533+
actual state of the art.
534+
535+
**A real design gap found while wiring this up, not by inspection**:
536+
feeding the vulnerable-race sample's own extracted fact
537+
(`check_then_act_on_shared_resource`, which IS the KG rule's own PRE
538+
condition, verbatim) into the pre-existing `RuleEngine.apply_rule_to_code`
539+
returned `PASS`. Tracing why: that method is a positive-derivation
540+
checker built for the IBM NSTK forward-chaining pattern ("precondition
541+
met -> derive postcondition -> report PASS") -- it has no code path that
542+
returns `FAIL`, for any input, ever. Fed a rule whose PRE names a
543+
*dangerous* pattern, meeting that PRE derives the POST and reports PASS
544+
-- an actively misleading verdict on genuinely vulnerable code, not
545+
merely a missing feature. Separately, the SQL Injection Prevention rule's
546+
`formal_spec` (`PRE: user_input is untrusted; POST: query uses
547+
parameterized statement`) was prose, not fact-string syntax, so it didn't
548+
even reach that bug -- it just silently never fired (`UNKNOWN`,
549+
precondition text matching nothing).
550+
551+
**Fixed, minimally, in this session**: added `RuleEngine.check_for_violation`
552+
as a new, additive method (nothing else in the codebase calls
553+
`apply_rule_to_code`, so its existing behavior for legitimate forward-
554+
chaining callers was left untouched) with the inverse framing -- PRE is
555+
the trigger condition, POST is the required mitigating fact: `FAIL` if
556+
the precondition holds and the postcondition fact is absent, `PASS` if
557+
the postcondition fact is present (mitigated), `UNKNOWN` if the
558+
precondition doesn't apply at all. Also corrected the SQL Injection
559+
Prevention rule's `formal_spec` to real fact-string syntax (`PRE:
560+
sql_query_built_dynamically; POST: uses_parameterized_query`), matching
561+
the vocabulary `extract_sql_injection_facts` actually produces --
562+
`No Check-Then-Act Race`'s spec already happened to be written that way.
563+
564+
**Verified end-to-end against hand-written fixtures** (neither
565+
`correctness_benchmarks.json` nor `security_benchmarks.json` contains a
566+
real SQLi or race-condition sample -- confirmed by inspection; the
567+
existing security tasks are divide-by-zero, bounds-checks, and a
568+
lock-flag proxy, all Z3-reducible):
569+
570+
| Sample | Extracted facts | `check_for_violation` |
571+
|---|---|---|
572+
| Vulnerable SQL (string concat into `execute`) | `sql_query_built_dynamically`, `sql_query_built_via_concatenation` | **FAIL** — violated, unmitigated |
573+
| Safe SQL (`?` placeholder + params tuple) | `uses_parameterized_query` | UNKNOWN — trigger absent (never a false PASS) |
574+
| Vulnerable race (unguarded check-then-act) | `check_then_act_on_shared_resource` | **FAIL** — violated, unmitigated |
575+
| Safe race (same pattern inside `with lock:`) | `check_and_act_combined_atomically` | UNKNOWN — trigger absent |
576+
577+
No false positives on the two existing security-benchmark shapes tested
578+
(`security_003`'s lock-flag proxy, `security_005`'s bounds-check) --
579+
neither extractor fires on int-only assertion code, as expected.
580+
581+
**Honest scope of the answer**: pattern-matching genuinely can close part
582+
of this gap -- for the textbook shape of each vulnerability, with the
583+
caveats documented in each extractor's own docstring (no cross-function
584+
tracking, no ORM builders, ~one-hop variable resolution for SQL). It
585+
cannot certify absence of a vulnerability (`check_for_violation` never
586+
returns an affirmative "no injection risk exists" -- only "found a
587+
violation" or "found nothing to check"), and race conditions in
588+
particular remain much shakier ground than SQL injection: the check-then-
589+
act shape covers exactly one classic pattern, not the much larger space
590+
of real concurrency bugs (cross-thread interleavings, async races,
591+
missing memory barriers). **Recommendation**: worth extending
592+
`security_facts.py` with a handful more OWASP-shaped extractors
593+
(path traversal, command injection via string-built shell commands --
594+
`security_scan.py`'s blocklist already covers the `os.system`/`subprocess`
595+
call surface, so this would be extending pattern coverage of *arguments*
596+
to already-flagged calls, not new call surface) before wiring any of this
597+
into the live orchestrator's retry loop as an actual verification gate;
598+
today it's a prototype demonstrating the fact-extraction approach works
599+
and the existing `RuleEngine` had a real, fixable gap, not a
600+
production-ready scanner.
601+
504602
## Target threshold (confirmed before a real run, per the plan's hardened
505603
acceptance criterion)
506604

src/verityai/kg/seed_data/security_rules.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@
3333
"description": "Verify that SQL queries use parameterized queries, not string concatenation",
3434
"category": "security",
3535
"severity": "critical",
36-
"formal_spec": "PRE: user_input is untrusted; POST: query uses parameterized statement",
36+
"formal_spec": "PRE: sql_query_built_dynamically; POST: uses_parameterized_query",
3737
"applies_to": [
3838
"python",
3939
"java",

src/verityai/symbolic/rule_engine.py

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -173,6 +173,58 @@ def apply_rule_to_code(
173173

174174
return VerificationStatus.UNKNOWN, f"No consequence derived from {rule.name}"
175175

176+
def check_for_violation(
177+
self, rule: Rule, code_facts: dict[str, Any]
178+
) -> tuple[VerificationStatus, Optional[str]]:
179+
"""Checks whether `code_facts` indicates a VIOLATION of `rule`, not
180+
whether the rule's derivation fires -- the opposite framing from
181+
`apply_rule_to_code`.
182+
183+
`apply_rule_to_code` is a positive-derivation checker: "precondition
184+
met -> derive postcondition -> PASS." It can structurally only ever
185+
return PASS or UNKNOWN -- there is no code path in it that returns
186+
FAIL, for any input. That's correct for the forward-chaining
187+
derivation this class was built for (IBM NSTK pattern), but it means
188+
it cannot express "this code violates a security rule" at all: fed
189+
a rule whose PRE names a *dangerous* pattern (e.g. `No Check-Then-
190+
Act Race`'s `PRE: check_then_act_on_shared_resource`), a vulnerable
191+
snippet's own extracted facts satisfy that PRE, `apply_rule_to_code`
192+
derives the POST and reports PASS -- an inverted, actively
193+
misleading verdict on genuinely vulnerable code. Found via T6's
194+
real prototype (`symbolic/security_facts.py`), not by inspection.
195+
196+
This method treats PRE as "the trigger condition to check for" and
197+
POST as "the required mitigating fact" instead: FAIL if the
198+
precondition holds and the postcondition fact is absent from
199+
`code_facts`, PASS if the postcondition fact IS present (the danger
200+
was mitigated), UNKNOWN if the precondition doesn't apply here at
201+
all. Added as a new method rather than changing
202+
`apply_rule_to_code`'s behavior, since nothing else in this
203+
codebase calls it and there was no need to risk its existing
204+
semantics for callers that legitimately want plain derivation.
205+
"""
206+
if not rule.formal_spec or "PRE:" not in rule.formal_spec:
207+
return VerificationStatus.UNKNOWN, f"Rule {rule.name} has no PRE/POST formal_spec"
208+
209+
fact_strings = set(code_facts.keys())
210+
self.reset()
211+
self.facts = fact_strings
212+
213+
if not self._preconditions_met(rule):
214+
return VerificationStatus.UNKNOWN, f"Rule {rule.name} precondition not present"
215+
216+
consequence = self._derive_consequence(rule)
217+
if consequence and consequence in self.facts:
218+
return (
219+
VerificationStatus.PASS,
220+
f"Rule {rule.name}: mitigated ('{consequence}' present)",
221+
)
222+
223+
return (
224+
VerificationStatus.FAIL,
225+
f"Rule {rule.name} violated: precondition present, '{consequence}' not confirmed",
226+
)
227+
176228
def get_applicable_rules(
177229
self, code_facts: dict[str, Any], language: str = "python"
178230
) -> list[Rule]:

0 commit comments

Comments
 (0)