Repository navigation
feat(behavioral): canonical dangerous-callable resolver + shared evasion corpus (implements #181) - #182
feat(behavioral): canonical dangerous-callable resolver + shared evasion corpus (implements #181)#182zied-jlassi wants to merge 3 commits into
Conversation
| return None | ||
|
|
||
|
|
||
| def _strip_builtins_prefix(name: str) -> str: |
There was a problem hiding this comment.
[Automated SkillSpector Review]
This helper (and the other common.py changes plus the duplicated test_behavioral_ast.py / test_behavioral_taint_tracking.py additions in this PR) come from #180, which this branch is stacked on. Once #180 merges, please rebase onto main so this PR's diff contains only the new canonical-sink layer. No change needed to the code itself.
rng1995
left a comment
rng1995
left a comment
There was a problem hiding this comment.
[Automated SkillSpector Review]
Requesting changes — scope/rebase, not the implementation.
To be clear up front: the canonical-sink layer itself is excellent and I have no correctness or security objection to canonical_sink.py. The single-chokepoint design (resolve_to_canonical_sink covering bare/alias/from/as, builtins.*, dynamic import, reflective getattr, subscript __builtins__["exec"]/os.__dict__[...], and importlib/runpy/code sibling machinery with receiver-gated instance methods), the AST9-parity for reflective subprocess, the _covered_by_ast9 anti-double-fire guard, and the shared evasion-corpus fitness gate are all well-reasoned and thoroughly tested.
The blocker is scope/mergeability: this PR is stacked on #180 and its diff currently includes #180's commit 8bdc3bc (the common.py helpers and the test_behavioral_ast.py / test_behavioral_taint_tracking.py additions are duplicated from #180). As written, this can't be merged independently without pulling in / conflicting with #180.
Requested change (matches your own stated plan): land #180 first, then rebase this onto main so the diff contains only the canonical-sink layer (canonical_sink.py, the behavioral_ast/behavioral_taint_tracking wiring, and tests/evasion_corpus/). I'll re-review and approve once the diff is isolated to that.
ae6c9f1 to
4163d57
Compare
|
Hi @rng1995 — thanks for merging #180 into As requested, I've rebased this branch onto Re-validated in a clean |
rng1995
left a comment
rng1995
left a comment
There was a problem hiding this comment.
The previous stacked-PR scope issue is resolved; this is now a clean, focused diff. Two resolver blockers remain: arbitrary .loader.exec_module(...) calls are falsely classified as dynamic imports, while aliased code.InteractiveInterpreter calls evade detection. Please require importlib receiver provenance, normalize aliases for code-interpreter receivers, and add production-wiring plus corpus coverage for both cases.
…alize code interpreter Addresses the NVIDIA#182 re-review: two resolver precision bugs. exec_module false positive: the `<x>.loader` branch matched ANY object exposing a `.loader` attribute, so an unrelated `self.widget.loader.exec_module(...)` was wrongly classified as a dynamic import. The base must now have importlib provenance -- a spec from importlib.util.module_from_spec / spec_from_loader carried through the type map, or an importlib-rooted dotted name -- via the new `_expr_is_importlib_rooted` helper. The legitimate `spec.loader.exec_module(mod)` chain still fires. code-interpreter false negative: `_receiver_is_code_interpreter` never alias-normalized the inferred constructor type, so `import code as c; c.InteractiveInterpreter().runsource(...)` evaded detection. The inferred name now runs through `apply_import_aliases`, which also covers `from code import InteractiveInterpreter`. Production wiring: `behavioral_ast` now threads import aliases into `canonical_sibling_method` (the taint analyzer already did via `resolve_to_canonical_sink`), so both detection paths apply the gate. Corpus: +2 positive rows (aliased code interpreter, aliased importlib spec loader) and +1 false-positive neighbour (arbitrary `.loader.exec_module`), all exercised end-to-end through the real analyzers as a red->green oracle. Signed-off-by: Zied Jlassi <6190550+zied-jlassi@users.noreply.github.com>
|
Thanks @rng1995 — both resolver blockers are addressed in 1. 2. Production wiring. Corpus coverage (run end-to-end through the real analyzers, not just the resolver):
Verified red→green: on the pre-fix code those three rows fail (the FP neighbour produces the AST3 finding you flagged; the aliased interpreter is missed); on Validation (clean |
rng1995
left a comment
rng1995
left a comment
There was a problem hiding this comment.
[Automated SkillSpector Review]
Re-review: approved. The earlier stacked-scope blocker remains resolved, and both resolver issues from the last review are fixed: exec_module now requires importlib-rooted receiver provenance, while aliased code.InteractiveInterpreter constructors are normalized and wired into the production AST path. The corpus includes positive alias cases and the arbitrary-.loader negative case.
|
@zied-jlassi - Please address review comments and resolve merge conflicts. |
|
@zied-jlassi - Please resolve merge conflicts and CI failures to merge the PR. Thanks for your contribution! |
|
Powered by Codex: PR council review result. This is a triage signal, not a maintainer approval.
|
rng1995
left a comment
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Changes are required at c1c4141. The previously reviewed resolver behavior remains sound, but every current Python CI job fails the repository formatting gate (ruff format --check would reformat src/skillspector/nodes/analyzers/mcp_least_privilege.py), and the branch conflicts with main. Rebase onto current main, preserve the canonical-sink regressions during conflict resolution, format the resulting tree, and rerun all supported Python jobs to green.
rng1995
left a comment
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
@zied-jlassi thank you for the canonical-sink resolver and its evasion corpus, and sorry for the long wait since the July approval.
Value and readiness
The resolver still covers spellings main misses: bare __builtins__["exec"], getattr(subprocess, "Popen") and other names outside AST9's allowlist, the importlib/runpy/code sibling machinery, and taint parity for all of them. Main now covers part of the same surface: #517 grades <module>.__dict__[...] and vars(<module>)[...] as AST9, and #544 resolves joined getattr names. The PR is not ready, because the branch still conflicts with main and the rebase has to account for #517.
Material findings
Prior findings:
- Ruff format failure: resolved. The failing file in the July 14 run,
mcp_least_privilege.py, is not part of this PR, so main had drifted. CI needs a fresh run after the rebase. - Blocker: conflict with
main. Still open, inbehavioral_ast.pyandbehavioral_taint_tracking.py.
New:
3. Blocker, behavioral_ast.py:232. After the rebase, the canonical fallback will report spellings main already reports a second time. Take import os; os.__dict__["system"](x): main's subscript arm emits AST9 on the subscript, and this fallback resolves the call to os.system and emits AST5. vars(builtins)["exec"](x) and builtins.__dict__["exec"](x) get AST9 plus AST1. test_spelling_reaches_ast_sink_rule only asserts that at least one sink finding exists, so corpus rows os_system_dunder_dict, exec_subscript_vars and exec_builtins_dunder_dict would still pass. Please extend _covered_by_ast9, or an equivalent guard, to main's _reflective_module_dict_container cases, and assert exactly one sink finding per spelling.
4. Mechanical rebase points:
- Keep main's
urllib.request.build_opener(...).opencase (#721) ahead ofresolve_to_canonical_sinkin_resolve_sink_name. - Adapt to
_analyze_python(ParsedPythonFile, ...)and the new_emit(node_index, rule, node, msg)signature.
PIC tradeoffs
None identified.
Verification and gaps
- Ran
git merge-treeagainstmainat8b993d32: 2 conflicting files. - Read main's
_analyze_python,_reflective_module_dict_containerandbuild_import_aliases(import osmaps toos), and the PR's_canonical_from_subscriptand_namespace_module. - The failed-job log reads
Would reformat: src/skillspector/nodes/analyzers/mcp_least_privilege.py. - Static review only. I did not run the corpus; the double-reporting finding comes from tracing both code paths.
- The outdated stacked-scope thread at
common.py:107is resolved in substance and can be closed.
| # 'os.system', so it re-enters the os./subprocess. sink ladders below. | ||
| call_name = resolve_dynamic_import_call(ast_node, aliases) | ||
| reflective = False | ||
| if call_name is None and not _covered_by_ast9(ast_node): |
There was a problem hiding this comment.
[SkillSpector Review] Blocker for the rebase. Since #517, main emits AST9 on the subscript in os.__dict__["system"](x), vars(builtins)["exec"](x) and builtins.__dict__["exec"](x) (see _reflective_module_dict_container). _covered_by_ast9 only recognises the literal getattr form, so this fallback would also resolve these calls and emit AST5 or AST1: two findings for one expression. Please extend the guard to main's module-dict cases, or drop those cases from the canonical layer, and have the corpus assert exactly one sink finding per spelling.
…ness gate
Introduce resolve_to_canonical_sink, a single chokepoint reducing every spelling
of a dangerous callable -- bare/alias/from/as, builtins.*,
importlib.import_module, reflective getattr("lit"), subscript __builtins__["exec"]
and <mod>.__dict__["lit"], plus importlib/runpy/code sibling machinery -- to one
canonical sink id. Wire it into behavioral_ast and behavioral_taint_tracking so
the per-idiom ladders stay simple, with a shared parametrized evasion corpus
asserting the invariant as a fitness gate: a future missed spelling fails a
shared test instead of shipping as a silent blind spot.
Reflective subprocess invocations grade AST9-HIGH for parity with reflective
os.system; direct subprocess.* keeps baseline AST4-MEDIUM; AST9 and the canonical
fallback stay complementary (the _covered_by_ast9 guard prevents double-firing).
Benign neighbours never canonicalize to a sink.
Stacked on the builtins/importlib sink-evasion fix (reuses
resolve_dynamic_import_call); to be rebased onto main once that lands.
108 evasion-corpus tests pass; full suite +108 with zero regressions
(ruff 0.15.2 + mypy clean).
Signed-off-by: Zied Jlassi <6190550+zied-jlassi@users.noreply.github.com>
…alize code interpreter Addresses the NVIDIA#182 re-review: two resolver precision bugs. exec_module false positive: the `<x>.loader` branch matched ANY object exposing a `.loader` attribute, so an unrelated `self.widget.loader.exec_module(...)` was wrongly classified as a dynamic import. The base must now have importlib provenance -- a spec from importlib.util.module_from_spec / spec_from_loader carried through the type map, or an importlib-rooted dotted name -- via the new `_expr_is_importlib_rooted` helper. The legitimate `spec.loader.exec_module(mod)` chain still fires. code-interpreter false negative: `_receiver_is_code_interpreter` never alias-normalized the inferred constructor type, so `import code as c; c.InteractiveInterpreter().runsource(...)` evaded detection. The inferred name now runs through `apply_import_aliases`, which also covers `from code import InteractiveInterpreter`. Production wiring: `behavioral_ast` now threads import aliases into `canonical_sibling_method` (the taint analyzer already did via `resolve_to_canonical_sink`), so both detection paths apply the gate. Corpus: +2 positive rows (aliased code interpreter, aliased importlib spec loader) and +1 false-positive neighbour (arbitrary `.loader.exec_module`), all exercised end-to-end through the real analyzers as a red->green oracle. Signed-off-by: Zied Jlassi <6190550+zied-jlassi@users.noreply.github.com>
…lags Since NVIDIA#517, main reports os.__dict__["system"](x), vars(builtins)["exec"](x) and builtins.__dict__["exec"](x) as AST9 on the subscript. The canonical fallback also resolved the outer call, so the same expression came out a second time as AST5/AST1. _covered_by_ast9 now mirrors every read AST9 owns on main: allowlisted getattr (joined names included, NVIDIA#544), the module-dict subscript, and the .get/.setdefault/.pop reads (NVIDIA#517). The fallback stays quiet on those and keeps the spellings AST9 doesn't see (__builtins__["exec"], getattr on names outside the allowlist). The corpus test now requires exactly one sink finding on the sink line instead of "at least one". It fails on the three rows from the review without the guard and passes with it. Signed-off-by: Zied Jlassi <6190550+zied-jlassi@users.noreply.github.com>
Head branch was pushed to by a user without write access
c1c4141 to
baa3b9a
Compare
3 participants
Implements the proposal in #181: a single canonicalization chokepoint that collapses every spelling of a dangerous callable to one canonical sink id, paired with a shared evasion corpus wired as a fitness gate.
I've opened this as a concrete PR (rather than only waiting on a yes/no in #181) so the approach can be judged on real, tested code — happy to adjust the direction or close if you'd prefer.
This branch is stacked on #180 (
fix: detect builtins.* and importlib.import_module sink evasions): the canonical resolver reusesresolve_dynamic_import_call/_strip_builtins_prefixintroduced there. Becausemaindoes not yet contain #180, this PR's diff currently includes #180's commit8bdc3bc(thecommon.pyhelpers + its behavioral tests). Once #180 lands I'll rebase ontomainso only the canonical layer remains. Suggested merge order: #180 first, then this.What this PR adds on top of #180
canonical_sink.py—resolve_to_canonical_sink(node, aliases, type_map): one chokepoint reducing bare / alias /from/as,builtins.*,importlib.import_module, reflectivegetattr("lit"), subscript__builtins__["exec"]/<mod>.__dict__["lit"], and importlib / runpy / code sibling machinery to a single canonical sink id, reusing the existing alias / type-map / dynamic-import machinery.behavioral_ast+behavioral_taint_trackingwired to call it. Reflectivesubprocessinvocations grade AST9-HIGH (parity with reflectiveos.system); a directsubprocess.*call keeps baseline AST4-MEDIUM; a_covered_by_ast9guard keeps AST9 and the canonical fallback complementary so they never double-fire. Benign neighbours never canonicalize to a sink.tests/evasion_corpus/— shared parametrized corpus as a fitness gate: every spelling → its canonical id and a real production sink rule; FP-neighbours → no sink. Adding a sink becomes "add corpus rows", not "add a detector branch + hope it's complete".Scope / non-goals
In scope: statically-resolvable spellings. Out of scope (documented, left to the generic reflective-access AST7-LOW rule): genuinely dynamic idioms — non-literal
getattr(os, name), variable subscript key, computed attribute string, module name from a variable, local-variable indirection (f = os.system; f(x)). These have no static canonical id and would need def-use / taint tracking; kept explicitly as non-goals so the layer doesn't oversell completeness.Validation (real analyzers + repo toolchain, isolated container)
test_static_patterns::TestRunStaticPatternsAgentSnooping::test_no_same_line_duplicatefailure is unrelated and present on the base tooruff==0.15.2check +ruff format+mypyall cleanAI disclosure: prepared with AI assistance; every line was reviewed and validated by me, and the results above come from running the repo's real analyzers + test toolchain.