Skip to content

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

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

Description

@2002lcy0401
2002lcy0401
opened on Sep 2, 2026

Summary

The scanner's Tool Misuse detection can be bypassed by assigning the shell argument to a variable instead of using shell=True directly.

Original:

command = f'python a.py'
result = subprocess.run(
    command,
    shell=True,
    capture_output=True,
    text=True
)

Result:

Risk score: 76
Severity: HIGH
Recommendation: DO_NOT_INSTALL
TM1: Tool Parameter Abuse detected

Variant:

command = f'python a.py'
a = True
result = subprocess.run(
    command,
    shell=a,
    capture_output=True,
    text=True
)

Result:

Risk score: 46
Severity: MEDIUM
Recommendation: CAUTION
TM1 finding is missing

Expected Behavior

Both variants should receive equivalent findings because a statically resolves to True.

Impact

A simple variable assignment removes the TM1 high-severity finding and significantly reduces the overall risk score, which may affect installation recommendations.

Suggested Fix

Add constant propagation / variable resolution for sensitive parameters such as shell, so values that statically resolve to True are treated the same as direct shell=True.

Reproduction results

Here are the comparative JSON results provided

adv.json
ori.json

Activity

  1. 2002lcy0401
    changed the title [-]shell=True detection can be bypassed through variable assignment[/-] [+]shell=True detection can be bypassed through variable assignment (malicious skill example that bypasses this static detection model)[/+] on Sep 7, 2026

  2. chrisknvidia commented on Sep 8, 2026

    @chrisknvidia
    chrisknvidia
    on Sep 8, 2026
    Collaborator

    Reproduced on current main (704bc954, SkillSpector 2.11.1) with the real static graph and --no-llm JSON CLI:

    • direct shell=True: TM1 is present
    • use_shell = True; shell=use_shell: TM1 is absent while AST4 remains
    • in the minimal full-scan fixture, this changes the result from score 45 / CAUTION to score 16 / SAFE

    The root cause is that TM1 currently recognizes the direct spelling lexically, but does not resolve a statically definite name at the call site. I opened draft PR #497. Its first commit intentionally pins the failing runner-level regression; I am implementing the bounded AST augmentation and will push it after focused tests and independent local review passes.

    The fix will keep unresolved or control-flow-ambiguous values on the existing AST4 path rather than guessing, and will preserve TM1-TM4's normalized/declared-marker/windowed lexical scans.

  3. chrisknvidia commented on Sep 8, 2026

    @chrisknvidia
    chrisknvidia
    on Sep 8, 2026
    Collaborator

    Implementation is complete and ready for review in #497 at exact head 005fff55874b45775e7f3fc789265145382caefe.

    The fix adds a bounded shared-AST companion to TM1 while preserving the existing lexical TM1-TM4 path. It now detects the issue's straight-line forms, including both a = True and the reporter attachment's a = 'True', when the name is passed as shell=a to a direct one-level subprocess.* or bare Popen call. It reuses the existing TM1 contextual classification and avoids duplicate direct-literal findings.

    Verification on the final head:

    • installed-wheel CLI: direct True, bound True, and bound 'True' each produced exactly one HIGH/0.9 TM1 with the same score (38); false and compound-boundary controls produced no TM1
    • focused/shared/runner matrix: 95 passed; legacy reconstruction: 527 passed; public surfaces: 46 passed
    • full repository suite: 4,018 passed, 14 skipped, 38 deselected, 4 xfailed
    • source/wheel build and twine check, Ruff, formatting, mypy, local Docker smoke, and real GitHub URL scan all passed
    • GitHub CI: changes, lint, unit, DCO, and Docker smoke all passed
    • independent multi-pass review found no remaining issue within the supported contract; an adversarial 827 KB nested-control-flow input completed in 0.83 seconds

    The analysis stays intentionally conservative beyond that contract: unsupported expressions, compound control flow, import aliases, arbitrary nested-call effects, and dynamic global mutation discard tracked facts rather than guessing. The PR uses Fixes #475, so this issue will close when it merges.

  4. chrisknvidia commented on Sep 8, 2026

    @chrisknvidia
    chrisknvidia
    on Sep 8, 2026
    Collaborator

    Superseding my earlier readiness comment for head 005fff55874b45775e7f3fc789265145382caefe: PR #497 is now ready for review at exact head 89ccd3681cf72e06691ff9ec0e4c9eddcf3f0770.

    The final implementation detects the reported straight-line a = True; shell=a form and the attached a = 'True' variant for direct one-level subprocess.* and bare Popen calls. It preserves the direct shell=True TM1 severity, confidence, tags, fingerprint/risk behavior, source location, output caps, and report/ledger contracts. False and control-flow-ambiguous values do not gain TM1; unresolved calls retain the existing AST4 behavior.

    Exact-candidate verification:

    • 889 affected/shared/public-surface tests passed
    • full static suite: 4,457 passed, 14 skipped, 38 deselected, 4 xfailed (89% coverage)
    • isolated built-wheel CLI matrix passed for direct/bound Boolean/bound string/false/control-flow cases across JSON, terminal, Markdown, and SARIF
    • source/wheel build, twine check, isolated install, pip check, Ruff, format, and mypy on all touched production modules passed
    • no-cache Docker build and smoke passed, including a real GitHub clone/scan; private reconciliation evidence did not leak
    • fresh GitHub CI on the exact SHA passed: changes, lint, test-unit, DCO, and Docker smoke
    • three independent defect-focused review passes found no remaining actionable issue within the documented supported contract

    The analysis remains intentionally conservative for unsupported expressions, compound control flow, class/method or other dynamic effects, import aliases, and return/yield positions. Near-limit normalization-heavy inputs also have limited runtime headroom and continue to use the existing fail-closed partial/runtime-limit ledger contract.

    PR: #497

  5. chrisknvidia commented on Sep 9, 2026

    @chrisknvidia
    chrisknvidia
    on Sep 9, 2026
    Collaborator

    TG follow-up superseding the readiness statement for exact head 89ccd36: independent installed-wheel and public-output testing found that executable extensionless Python (with a Python shebang) and .pyw files still preserve the #475 bypass. Direct shell=True is detected as HIGH TM1, while the equivalent flag=True; shell=flag form emits no TM1 and can remain SAFE with analysis_completeness=true. The cause is extension-only Python AST eligibility. I moved PR #497 back to draft. No source changes have been made for this finding yet; the proposed remediation is to use one bounded positive Python-source classifier for .py/.pyw and trusted Python shebangs across AST prewarm and static analysis, with public direct/bound parity and negative-classification tests before republishing readiness.

  6. chrisknvidia commented on Sep 10, 2026

    @chrisknvidia
    chrisknvidia
    on Sep 10, 2026
    Collaborator

    Superseding the extension-only blocker reported earlier: the remediation is now pushed to PR #497 at exact head c986c3e4da2e7f6cee623ca2eac298a61a99a107.

    The fix now covers .py, .pyw, trusted extensionless Python shebangs, nested/transitive artifacts, strict source decoding, and the relevant CPython/env execution forms. Runtime-dependent source intent remains conservatively analyzed but is published as incomplete. Recursive failures and known risk also remain fail-closed under output caps, with parseable and secret-safe JSON/SARIF/Markdown output.

    Final local evidence includes:

    • 5,223 repository tests passed with only the environment-blocked MCP stdio timing test excluded; 14 skipped, 39 deselected, 4 expected xfails
    • 1,060 focused parser/TM1 tests passed independently on both Python 3.13 and 3.14
    • fresh source and installed-wheel matrices passed 35/35 each; recursive source/wheel reporting passed 18/18
    • live Darwin and GNU execution matrices passed 25/25 each; native/Docker PTY and bare -c/-m execution probes passed
    • real-root transitive cap 1/2 fault probes failed closed with exact typed diagnostics and no secret leakage
    • sdist/wheel, isolated install, dependency/compile checks, no-cache arm64 Docker build/runtime, Ruff, formatting, and multiple independent review passes passed

    One local environment gap remains explicit: endpoint-security and unrelated test I/O caused the fixed 15-second MCP stdio handshake test to time out; a simultaneous clean origin/main control timed out identically, while every other MCP server test and eight real installed-wheel MCP scans passed.

    Fresh GitHub CI has now completed on exact head c986c3e4: changes, lint, test-unit, DCO, and Docker smoke all passed. The PR is open, not draft, mergeable, and reports a clean merge state.

    PR: #497

  7. rng1995 commented on Sep 16, 2026

    @rng1995
    rng1995
    on Sep 16, 2026
    Collaborator

    Current implementation work is tracked in PR #497, the comprehensive indirect-shell-truthiness fix, with a narrower literal-assignment implementation also open in PR #560. Keeping this issue open until an accepted implementation merges and the cross-surface regressions are verified.

  8. rng1995 commented on Sep 21, 2026

    @rng1995
    rng1995
    on Sep 21, 2026
    Collaborator

    Updated implementation links: PR #497 was closed without merging after its work was split for focused review.

    • Core bound shell= truthiness fix: PR #577, open.
    • TM1 cross-window identity/reconciliation: PR #578, open.
    • Additional Python execution surfaces: PR #579, open.
    • The independent recursive scan/reporting split, PR #576, has merged; it does not itself fix the reported indirect-shell bypass.

    The narrower alternative PR #560 also remains open. Keeping this issue open until the core fix is accepted and merged, with the cross-surface follow-ups tracked separately.

  9. rng1995
    closed this as completedin #560on Sep 22, 2026

  10. rng1995 commented on Sep 22, 2026

    @rng1995
    rng1995
    on Sep 22, 2026
    Collaborator

    Resolution verification: PR #560 merged and automatically closed this issue. It detects the reported local literal assignment passed as shell=, with regression coverage for reassignment and scope boundaries. Those tests pass on current main at 844ac30.

    This supersedes my earlier in-progress comment: the narrower accepted implementation has landed. The additional work in #577, #578, and #579 remains separate follow-up scope. The merged fix is not yet in the latest published release, v2.11.2.

  11. rng1995 commented on Oct 8, 2026

    @rng1995
    rng1995
    on Oct 8, 2026
    Collaborator

    Follow-up merge update checked on 2026-10-08: PR #577 has now merged, extending the earlier accepted fix in PR #560.

    The follow-up preserves lexical HIGH findings unless a retained bound-shell finding replaces them or a scoped replacement proof justifies abstention. Fresh offline verification on main 8f4ca7d6: 390 bound-shell, lexical-abstention, and cached-import regressions passed.

    This issue remains closed. PR #578 (cross-window identity) and PR #579 (additional execution surfaces) remain open as separate follow-up scope.

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions