Skip to content

fix(taint): retain sources across reassignment - #754

Closed
efegokdemir wants to merge 1 commit into
NVIDIA:mainfrom
efegokdemir:fix/taint-source-reassignment
Closed

efegokdemir wants to merge 1 commit into
NVIDIA:mainfrom
efegokdemir:fix/taint-source-reassignment

Conversation

@efegokdemir

Copy link
Copy Markdown
Contributor

Fixes #750

The taint worklist now retains and propagates one fact per source for each variable, so reassignment cannot hide a stronger source classification. Regression coverage exercises both assignment orders and propagation through an alias into a shell sink.

Tests:

  • pytest -q tests/nodes/analyzers/test_behavioral_taint_tracking.py (78 passed)
  • ruff check on the changed files
  • ruff format --check on the changed files
  • git diff --check

Signed-off-by: Efe Gökdemir <gokdemirefe1903@gmail.com>
rng1995 added a commit that referenced this pull request Oct 8, 2026
…l site

Review follow-up for the function-scoped taint keys.

Resource bounds:
- Keys are small integers built from a scope or class id plus the
  identifier. No key copies a qualified name per reference, so memory
  no longer grows as name length times references. A 10,000-character
  function name called with 100,000 arguments peaks at 98 MB, the same
  as main.
- Redefinitions of a class merge into one class, and method lookups
  are cached, so same-named classes no longer cost a scan per call.
  1 MB of `class C:` plus `C()` calls takes 4.9 s, where it overran a
  30 s deadline by about 350 s.
- check_runtime now also runs per statement, per function and per
  post-indexing loop, not only per visited node and per dequeue.

Receivers. `obj.m(...)` resolves through obj's class, typed in obj's
own scope:
- self/cls and class objects; `Base.m(self, x)` passes self explicitly;
- `x = C()`, `x: C = C()`, `with C() as x`, `self.attr = C()`;
- `C().m()` and `cls().m()`;
- super() and super(C, self), along the in-file MRO.

`cls(...)` binds to `__init__`, and @Property reads return the getter's
value. Return values are read for every resolved receiver. Literal,
library and builtin receivers never bind into module methods. Unknown
receivers still bind arguments, but never for dunder methods.

Classes. Attributes are per class. A store in C reaches a read in E
when the two classes share a descendant (subclass, base, or mixin), so
sibling subclasses no longer share attributes or methods. Hierarchies
of more than 32 classes keep one shared namespace.

Call-site context. Taint that reached a function only through its own
parameters no longer enters its return value. Each call site reads its
own arguments, so one caller's secret no longer taints every other call
of a shared helper.

Sources:
- Each key keeps one fact per source (#754's per-variable source sets),
  and a sink reports the most severe rule. A helper's network or file
  source can no longer turn a TT3 into TT2 or TT4.
- A source call nested in a sink is reported once, as a direct flow,
  rather than again as a tainted flow through a helper in the same
  sink.

Scopes:
- Lambdas and comprehensions are scopes. A walrus binds in the lambda,
  or in the function enclosing a comprehension.
- Lambdas assigned to names are callable.
- A comprehension's iterable flows into its targets.
- Callback receivers may be attribute chains, constructors or
  parameters.

Messages keep main's wording: the variable read at the sink and the
line of the statement that last tainted it, with plain variables cited
first. A baseline generated on main suppresses the same TT findings on
this branch, both its exact fingerprints and its message-glob rules.
On the corpus, 49 of the 78 findings both versions report keep
identical text; 27 of the other 29 cited a same-named variable in
another function.

Corpus (141 Python skills): main 92 TT findings, this branch 109.
- Removed: 11 name-clash findings.
- Upgraded: 3 TT2 now TT3 on the same sink.
- Added: 31 flows main missed.
- Compared with the previous revision: no real flow is lost; 2 more
  flows are found through comprehension targets; 2 helper-return false
  positives are gone.
- Of the 28 targets with changed TT findings, 1 changes recommendation
  and exit code, from a new TT3 where an environment API key is passed
  into an Authorization header.

Tests: 144 taint tests, including #754's regression test. New tests
cover every review finding, and the deadline expiring inside scope
indexing, the statement loop and the worklist. 37 mutants of the new
paths each fail a test.

Signed-off-by: Narendran Raghavan <nraghavan@nvidia.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[SkillSpector Review]

@efegokdemir thank you for picking up #750, and for testing both assignment orders through an alias into a shell sink.

Value and readiness

The change does what #750 asks. Each name now keeps one fact per source, so a reassignment can no longer hide input() behind an earlier os.getenv() (TT5 vs TT2), and the worklist still terminates. I found no correctness defect. The branch merges cleanly with main, and CI is green.

I'm marking this Manual Review Needed rather than approving, for two reasons. It changes how many findings a sink reports. And the open maintainer PR #799 rewrites the same functions with a different model for the same problem. The PIC should choose one before either lands.

Material findings

None blocking.

  • Optional: behavioral_taint_tracking.py:661-676 emits one finding per distinct rule across a variable's sources, because _emit dedupes on (rule_id, node).

    • In the PR's own test, subprocess.run(command, shell=True) now reports both TT5 (CRITICAL, from input) and TT2 (MEDIUM, from os.getenv).
    • Similarly, a token read from os.getenv and later refreshed from requests.get reports both TT3 and TT2 at a requests.post sink.

    #750 allows either "the strongest applicable source" or "all of them". The PR body doesn't say which this implements, and the extra findings appear as new entries against existing baselines. Please state the choice in the PR body, and pin it in the test by asserting the full rule set at the sink, not just any(... == "TT5").

  • Optional: add the network-sink counterpart #750 started from: a credential source and a network-input source reach requests.post, in both orders, and the test asserts TT3.

PIC tradeoffs

  • Overlap with #799 ("scope taint to functions and bind call-site arguments"):
    • The two PRs conflict textually in behavioral_taint_tracking.py.
    • #799 replaces _collect_tainted and _mark_targets with a scoped graph. For each sink lane it keeps the most severe source, plus the first-arriving source for messages. It lists per-source fact sets as an approach it tried and dropped.
    • If #799 lands, this PR is superseded. If this lands first, #799 has to reconcile the dict-of-dicts shape.
    • The PIC should decide the reporting model: every source's rule (this PR) or the strongest rule per sink (#799).
  • Cost. Work is now bounded by assignments × distinct source names, not by assignments alone. There are at most len(_ALL_SOURCES) source names, about 23. That is still linear in file size, but up to about 23 times main's worst case. The timing tests use a single source.

Verification and gaps

  • I read the full diff and the head versions of _collect_tainted, _mark_targets, _find_tainted_names_in_args, _pick_rule and the sink loop in _analyze_python. I traced both orders of the new test, and the cyclic x/y/z test, by hand.
  • Nothing outside this module consumes _collect_tainted, and deduplicate keeps findings with different rules separate.
  • The new test fails on main for the getenv-then-input order: the first writer keeps os.getenv, so only TT2 is reported. The test therefore catches the regression.
  • git merge-tree against current main is clean, GitHub reports MERGEABLE, and all 6 checks pass at 86e43cd. I didn't run the code or the tests.

@efegokdemir

Copy link
Copy Markdown
Contributor Author

Closing this as superseded by #799, which merged on 14 October and addresses the same source-tracking issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

taint tracking: keep a set of sources per name so reassignment can't drop the stronger TT rule

2 participants