mirror of
https://github.com/norandom/OpenUltraSAST.git
synced 2026-10-17 12:09:27 +00:00
fix(model-grounded-detection): the absence arbiter was silencing its own family
Two bugs, together making 4.1 report "no bug" on a textbook IDOR. Both found by running it against a real corpus pair rather than a fixture. 1. constraint_params were treated as dischargers. A field named `user_id` is what an IDOR is MADE OF -- `filter_by(id=user_id)` with user_id off the request is the bug, not the fix. The deleted checker only counted an identity constraint whose value's PROVENANCE was the authenticated context, so the parameter name alone evidences nothing. This is the mirror of group 2's sanitizer bug: a thing that must never count as a discharge, counted. 2. Guard tokens matched as substrings. With `user` in the token list, the string "SELECT * FROM users WHERE id = ?" contains `user`, so the query reported every operation in the handler as guarded. Matching is word-boundary now. On threatbyte-api-v1-delete the arbiter previously found three operations, all "guarded" by evidence like '"SELECT * FROM users WHERE id = ?"' and '(user_id,)', and returned no verdict at all. After the fix, on the real pairs: threatbyte-api-v1-delete vuln=model_corroborated fixed=none vampi-users-update-password vuln=model_corroborated fixed=none Both distinguished, deterministically, with no model call. Corroborated rather than entailed is right for these: they are single-handler excerpts with no guarded sibling to be inconsistent with, which is the asymmetry the rung distinction exists to express. identity_sources still discharge; request_sources and permissive_values remain excluded because they describe the vulnerable shape rather than the fix. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01T7XD7x5KonMdmGRkp63EQG
This commit is contained in:
co-authored by
Claude Opus 5
parent
4f8f590b93
commit
eebca3f663
@@ -30,7 +30,11 @@
|
|||||||
val opNames = split(operations)
|
val opNames = split(operations)
|
||||||
val guardTokens = split(dischargers)
|
val guardTokens = split(dischargers)
|
||||||
|
|
||||||
def mentionsGuard(code: String): Boolean = guardTokens.exists(g => code.contains(g))
|
// Word-boundary matching, never substring. A discharger token like `user` matched as a substring hits
|
||||||
|
// `users` inside "SELECT * FROM users WHERE id = ?", marking a textbook IDOR as guarded -- a silent false
|
||||||
|
// negative on the one family this arbiter exists for.
|
||||||
|
def mentionsGuard(code: String): Boolean =
|
||||||
|
guardTokens.exists(g => java.util.regex.Pattern.compile("(?<![A-Za-z0-9_])" + java.util.regex.Pattern.quote(g) + "(?![A-Za-z0-9_])").matcher(code).find())
|
||||||
|
|
||||||
// Match by CALL NAME, never by code containment: the frontend desugars `Note.query.filter_by(x).first()`
|
// Match by CALL NAME, never by code containment: the frontend desugars `Note.query.filter_by(x).first()`
|
||||||
// into a chain of temporaries, and matching on code yields four or five rows for one operation -- some of
|
// into a chain of temporaries, and matching on code yields four or five rows for one operation -- some of
|
||||||
|
|||||||
@@ -109,11 +109,16 @@ class DominanceSpec:
|
|||||||
|
|
||||||
calls, decorators an explicit guard: ``login_required``, ``check_owner``
|
calls, decorators an explicit guard: ``login_required``, ``check_owner``
|
||||||
identity_sources the authenticated context: ``current_user``, ``request.user``
|
identity_sources the authenticated context: ``current_user``, ``request.user``
|
||||||
constraint_params a field that constrains by identity: ``owner_id``, ``user_id``
|
|
||||||
|
|
||||||
``request_sources`` and ``permissive_values`` are excluded because they describe the NEGATIVE case -- a
|
Three fields are deliberately excluded, and each for a different reason:
|
||||||
value that comes from the request discharges nothing, and a permissive literal is the bug rather than the
|
|
||||||
guard. Including them would let the absence arbiter read a vulnerability as its own fix.
|
constraint_params NOT a discharge on its own. A field named ``user_id`` is what an IDOR is
|
||||||
|
*made of* -- ``filter_by(id=user_id)`` with ``user_id`` off the request is
|
||||||
|
the bug, not the fix. The original checker only counted a constraint whose
|
||||||
|
value's provenance was the authenticated context, so the param name alone
|
||||||
|
evidences nothing and treating it as a guard silences the whole family.
|
||||||
|
request_sources a value from the request discharges nothing, by definition.
|
||||||
|
permissive_values a permissive literal is the bug, not the guard.
|
||||||
"""
|
"""
|
||||||
|
|
||||||
family: str
|
family: str
|
||||||
@@ -205,7 +210,6 @@ def dominance_specs(*, language: str, facts: ObligationFacts | None = None) -> M
|
|||||||
{call for fact in scoped.dischargers for call in fact.calls}
|
{call for fact in scoped.dischargers for call in fact.calls}
|
||||||
| {decorator for fact in scoped.dischargers for decorator in fact.decorators}
|
| {decorator for fact in scoped.dischargers for decorator in fact.decorators}
|
||||||
| {source for fact in scoped.dischargers for source in fact.identity_sources}
|
| {source for fact in scoped.dischargers for source in fact.identity_sources}
|
||||||
| {param for fact in scoped.dischargers for param in fact.constraint_params}
|
|
||||||
)
|
)
|
||||||
)
|
)
|
||||||
return {"access_control": DominanceSpec(family="access_control", language=language, operations=operations, dischargers=dischargers)}
|
return {"access_control": DominanceSpec(family="access_control", language=language, operations=operations, dischargers=dischargers)}
|
||||||
|
|||||||
@@ -128,19 +128,20 @@ def test_safe_shape_sinks_are_kept_for_the_shape_test() -> None:
|
|||||||
assert c_memory is not None and "printf" in c_memory.safe_shape_sinks
|
assert c_memory is not None and "printf" in c_memory.safe_shape_sinks
|
||||||
|
|
||||||
|
|
||||||
def test_dischargers_carry_identity_and_constraint_tokens_but_not_the_negative_fields() -> None:
|
def test_dischargers_carry_identity_tokens_but_not_the_negative_fields() -> None:
|
||||||
"""A discharger fact spreads across six fields and two of them mean the opposite.
|
"""A discharger fact spreads across six fields and only three of them evidence a discharge.
|
||||||
|
|
||||||
`identity_sources` (current_user, request.user) and `constraint_params` (owner_id, user_id) are how an
|
`identity_sources` (current_user, request.user) is how an identity constraint is actually written, and
|
||||||
identity constraint is actually written, and omitting them left the absence arbiter reporting "no guard"
|
omitting it left the absence arbiter reporting "no guard" on handlers that plainly had one.
|
||||||
on a handler that plainly had one. `request_sources` and `permissive_values` are excluded because they
|
`request_sources` and `permissive_values` describe the vulnerable shape rather than the fix -- including
|
||||||
describe the vulnerable shape, not the fix -- including them would let the arbiter read a bug as its fix.
|
them would let the arbiter read a bug as its own fix. (`constraint_params` is excluded too, for a
|
||||||
|
different reason, covered by its own test.)
|
||||||
"""
|
"""
|
||||||
from openultrasast.model.specs import dominance_specs
|
from openultrasast.model.specs import dominance_specs
|
||||||
from openultrasast.semantic.obligations import load_obligation_facts
|
from openultrasast.semantic.obligations import load_obligation_facts
|
||||||
|
|
||||||
spec = dominance_specs(language="python")["access_control"]
|
spec = dominance_specs(language="python")["access_control"]
|
||||||
assert "current_user" in spec.dischargers and "owner_id" in spec.dischargers
|
assert "current_user" in spec.dischargers
|
||||||
assert "login_required" in spec.dischargers # decorators still present
|
assert "login_required" in spec.dischargers # decorators still present
|
||||||
|
|
||||||
facts = load_obligation_facts().for_language("python")
|
facts = load_obligation_facts().for_language("python")
|
||||||
@@ -148,3 +149,22 @@ def test_dischargers_carry_identity_and_constraint_tokens_but_not_the_negative_f
|
|||||||
v for fact in facts.dischargers for v in fact.permissive_values
|
v for fact in facts.dischargers for v in fact.permissive_values
|
||||||
}
|
}
|
||||||
assert not (negative & set(spec.dischargers)), "the negative fields must never read as a discharge"
|
assert not (negative & set(spec.dischargers)), "the negative fields must never read as a discharge"
|
||||||
|
|
||||||
|
|
||||||
|
def test_constraint_params_are_not_dischargers_on_their_own() -> None:
|
||||||
|
"""A field named `user_id` is what an IDOR is MADE OF, not what fixes one.
|
||||||
|
|
||||||
|
`filter_by(id=user_id)` with `user_id` taken off the request is the bug. The original checker only
|
||||||
|
counted an identity constraint whose value's provenance was the authenticated context, so the parameter
|
||||||
|
name alone evidences nothing. Treating it as a guard silenced the entire access_control family: on
|
||||||
|
threatbyte-api-v1-delete every operation reported as guarded, and the arbiter found no bug at all.
|
||||||
|
"""
|
||||||
|
from openultrasast.model.specs import dominance_specs
|
||||||
|
from openultrasast.semantic.obligations import load_obligation_facts
|
||||||
|
|
||||||
|
spec = dominance_specs(language="python")["access_control"]
|
||||||
|
facts = load_obligation_facts().for_language("python")
|
||||||
|
params = {p for fact in facts.dischargers for p in fact.constraint_params}
|
||||||
|
assert params, "the fixture would be vacuous if the facts carried no constraint params"
|
||||||
|
assert not (params & set(spec.dischargers)), f"constraint params must not discharge alone: {sorted(params & set(spec.dischargers))}"
|
||||||
|
assert "current_user" in spec.dischargers, "identity sources still do discharge"
|
||||||
|
|||||||
Reference in New Issue
Block a user