Repository navigation
Conversation
screen_command classified the command name as tokens[0].lower(), so the same program was caught bare and missed when written with its path. The module already owns the canonical normalizer for this - _program() strips the directory and any Windows extension - and screen_egress and screen_install both use it. Resolve the program through it here too, so all three screens agree on what a program is called. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This branch has not been deployed
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.
Why
Follow-up to #195 (which closed the whitespace / flag-order bypass in issue #128), and of the same kind as #176.
screen_commandmatches the program name against_classifyusingtokens[0].lower(), so it classifies on the literal token rather than on the program it names. Every other screen in the module resolves the program through_program()first:_program()is the module's own definition of "what a program is called" — it drops the directory and any Windows extension:Because
screen_commandskipped it, the identical command was caught when typed bare and passed when typed with its path:screen_commandrm -rf /recursive force remove/bin/rm -rf /None❌sudo rebootprivilege escalation/usr/bin/sudo rebootNone❌dd if=/dev/zero of=/dev/sdaraw disk write/bin/dd if=/dev/zero of=/dev/sdaNone❌mkfs.ext4 /dev/sda1raw disk write/sbin/mkfs.ext4 /dev/sda1None❌chmod 777 /etcworld-writable permissions/bin/chmod 777 /etcNone❌ls | /usr/bin/rm -rf /None❌This is the exact gap the module docstring says it exists to close: "That closes the whitespace / flag-order / flag-spelling gaps without pretending to be exhaustive." Path qualification is one more spelling gap, and the helper for it was already in the file.
Change
One line — resolve the program through
_program()before classifying, matching the two sibling screens:_program()already lowercases, so the existingRM -rfcase is unaffected.Still defense-in-depth, not the boundary
Unchanged from #195, and deliberately so: the sandbox in
core.harness.sandboxremains the enforcement boundary,screen_commandstill never raises, and an untokenisable command is still passed through. This only makes the shallow screen catch what it already claims to. I did not touch the sandbox.Tests
Added to
tests/test_command_guard.py(no new file):test_blocks_path_qualified_destructive_commands— 13 cases: absolute (/bin/rm,/usr/bin/sudo,/sbin/mkfs.ext4), relative (./rm --recursive --force), Windows extension stripping (/usr/bin/rm.exe,C:/Windows/System32/shutdown.exe), and path-qualified stages inside a sequence and a pipeline.test_allows_path_qualified_benign_commands— 4 cases, so normalising the program does not turn every absolute path into a hit (/usr/bin/rm notes.txtstays allowed, as before).Test-validity self-check. With the one-line fix reverted and the new tests kept, all 13 destructive cases fail on the unmodified
main; restoring the fix makes all 53 pass:Differential fuzz. 28,200 generated commands (programs × flag spellings × paths × shell operators, plus 4,000 random multi-segment pipelines) run through the pre-fix and post-fix implementations: 635 newly blocked, 0 newly allowed. Every one of the 635 contains a segment whose normalised program is genuinely destructive (checked programmatically against the classified set) — no unintended behaviour change, and spot-checked that
touch rm-rf-notes.txt,grep -r foo ./src,cat chmod.txt,git commit -m 'drop sudo'still pass.Full suite:
2279 passed, 11 skippedin 2m26s, on the fix applied — same as baseline.ruff checkandruff format --checkclean on both touched files.Note on Windows backslash paths
C:\Windows\System32\shutdown.exestill passes, for an unrelated reason:shlex.split(..., posix=True)consumes the backslashes as escapes, so the token arrives asC:WindowsSystem32shutdown.exebefore_program()ever sees it. That is a tokenizer-level behaviour rather than a program-name mismatch, and fixing it means deciding how this POSIX-oriented tokenizer should treat Windows paths — a larger design question than this PR's one-line scope, so I left it alone rather than fold it in silently.