Repository navigation
feat(security): close the process-sink gaps Bandit cross-check found - #69
Merged
Merged
Conversation
Cross-checked the security lens against Bandit on click/pygments/sqlalchemy — an external authority we do not control, so the check is non-circular. Within the scope the lens claims (code execution, deserialization, process spawning) it matched 25 of 27 Bandit findings. One of the two misses was a comparison artifact (Bandit attributes a multi-line call to a continuation line; we use the call's start). The other was a real gap: os.startfile was not catalogued. Closes that gap and the neighbouring ones: os.popen, os.startfile, the os.exec*/os.spawn*/posix_spawn family, and commands.getoutput. They are split the way Bandit splits B605 from B606 — handing a string to a shell (os.system, os.popen) rates high; launching a program without one rates medium. Also elevates a subprocess.* call passing a literal shell=True to high, the B602/B603 distinction. Only a literal True counts: a variable could be either, and guessing would inflate severity on evidence the parser does not have. With the gate's fail-on: high default, a newly-introduced shell=True now fails a PR while an ordinary subprocess.run still does not. Re-running the cross-check after the fix: click 7/9 -> 8/9, and the single remaining difference is the known line-attribution artifact. Documents a real wrinkle this exposes: because both sides of the gate are scanned with the same build, a release that adds catalogue entries can report sinks that were always in the code. Pin the Action's modscan-version to control that. Moddability core untouched; suite 24/24; benchmark unchanged at 9/20. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
MODScan attack-surface diff —
|
MODScan extension-point diffNo breaking changes to extension points |
4 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Closes #61, driven by a non-circular cross-check against Bandit (an external authority we don't control) — the M3 validation work.
The cross-check
Ran both tools on click / pygments / sqlalchemy, comparing only within the scope the lens claims (code execution, deserialization, process) — Bandit's weak-crypto/assert/SQL tests are excluded because the lens explicitly doesn't cover them.
subprocess.Popento line 533; we report the call's start at 531. Same finding.os.startfilewasn't catalogued.__reduce__(×35) and builtincompile, which Bandit has no tests for. We cover more here, not less.The fix
os.popen,os.startfile, theos.exec*/os.spawn*/posix_spawnfamily,commands.getoutput— split the way Bandit splits B605 from B606: shell-invoking calls rate high, no-shell launches rate medium.shell=Trueelevates to high (B602 vs B603). Only a literalTruecounts — a variable could be either, and guessing would inflate severity on evidence the parser doesn't have. With the gate'sfail-on: high, a newshell=Truenow fails a PR while an ordinarysubprocess.runstill doesn't.After the fix: click 7/9 → 8/9, and the only remaining difference is the line-attribution artifact.
Documented wrinkle
Both sides of the gate are scanned with the same build, so a release that adds catalogue entries can report sinks that were always in the code — new to the detector, not the codebase. Noted in the Action header + CHANGELOG;
modscan-versionis the mitigation. Verified this PR doesn't trip it: no newly-catalogued sink exists inmodscan/itself.Test plan
tests/test_security_sinks.py:shell=True→ high, plain call → medium, non-literalshell=flag→ not elevated; new process sinks present;os.path.joinstill not a sink.🤖 Generated with Claude Code