Skip to content

fix(analyzer): detect bound shell truthiness - #577

Open
chrisknvidia wants to merge 3 commits into
NVIDIA:mainfrom
chrisknvidia:fix/christopherk/issue-475-shell-truthiness-core
Open

chrisknvidia wants to merge 3 commits into
NVIDIA:mainfrom
chrisknvidia:fix/christopherk/issue-475-shell-truthiness-core

Conversation

@chrisknvidia

Copy link
Copy Markdown
Contributor

Summary

  • resolve definitely-truthy straight-line bindings used as shell= for ordinary Python subprocess calls
  • preserve direct TM1 severity, confidence, source coordinates, contextual classification, and public evidence contracts
  • invalidate facts conservatively across rebinding, imports, control flow, receiver mutation, protocol-consuming calls, and potentially observable stores
  • keep unsupported aliases, generators/async functions, indirect calls, .pyw/shebang classification, recursive reporting, and platform parsing outside this focused change

Review context

This is the narrow issue #475 core split requested in the review of #497. It contains no scan-budget/CLI/reporting work and no Python execution-surface expansion.

Validation

  • exact core contract: 15 expected failures before implementation, then 52 passes
  • six independent review regressions verified red then green, covering finalizer/protocol mutation, receiver ordering, exact long-line identity, and global/nonlocal stores
  • affected AST/security E2E suite: 157 passed
  • full standard suite on the final head: 5,474 passed, 14 skipped, 39 deselected, 4 expected xfails
  • built and installed wheel contains the new analyzer; installed-wheel probe emitted exactly one HIGH TM1 at the bound call
  • Ruff lint/format, mypy, and diff checks passed

Fixes #475.
Split from #497 for focused architectural and security review.

Signed-off-by: Christopher Kevin <christopherk@nvidia.com>
Signed-off-by: Christopher Kevin <christopherk@nvidia.com>
Signed-off-by: Christopher Kevin <christopherk@nvidia.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.

Reviewed exact head bd09251621e1edabcfb4615b04d420dbf83d02b6. Requesting changes because the issue #475 literal-versus-bound bypass remains when a later argument is effectful.

For enabled = True; subprocess.run(command, shell=enabled, env=build_env()), Python resolves the receiver and captures shell=True before evaluating the later env expression. The runtime shell value is therefore definitely true, but this head emits zero TM1 findings; the equivalent shell=True call emits one. Moving env=build_env() before shell=enabled correctly makes the value uncertain, so the two orders must not be treated identically.

_call_arguments_are_passive currently rejects the whole call when any argument is impure, including expressions evaluated after shell. Please make invalidation evaluation-order-aware and add before/after-shell parity regressions.

@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]

Reviewed exact head bd09251621e1edabcfb4615b04d420dbf83d02b6 and the complete bound-shell dataflow implementation.

The existing blocking review remains valid: _call_arguments_are_passive() rejects a call when any later argument is effectful, even though Python has already evaluated and captured an earlier shell=enabled value. This leaves the issue #475 bypass for shell=enabled, env=build_env() while the equivalent literal is detected. Make invalidation evaluation-order-aware and add before/after-shell parity regressions.

No duplicate inline comment was added.

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.

shell=True detection can be bypassed through variable assignment (malicious skill example that bypasses this static detection model)

2 participants