Skip to content

fix(security): normalize the program path in the destructive-command screen - #255

Open
D41910 wants to merge 1 commit into
HKUDS:mainfrom
D41910:fix/command-guard-program-path-normalization
Open

D41910 wants to merge 1 commit into
HKUDS:mainfrom
D41910:fix/command-guard-program-path-normalization

Conversation

@D41910

@D41910 D41910 commented Oct 9, 2026

Copy link
Copy Markdown

Why

Follow-up to #195 (which closed the whitespace / flag-order bypass in issue #128), and of the same kind as #176. screen_command matches the program name against _classify using tokens[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:

core/harness/command_guard.py:467    if _program(tokens[0]) not in _FETCHERS:      # screen_egress
core/harness/command_guard.py:600    program = _program(tokens[0])                 # screen_install
core/harness/command_guard.py:609    program = _program(args[1])                   # screen_install
core/harness/command_guard.py:158    reason = _classify(tokens[0].lower(), ...)    # screen_command

_program() is the module's own definition of "what a program is called" — it drops the directory and any Windows extension:

def _program(token: str) -> str:
    """The bare program name, without directory or Windows extension."""
    name = token.replace("\\", "/").rsplit("/", 1)[-1].lower()
    for suffix in (".exe", ".cmd", ".bat", ".ps1"):
        ...

Because screen_command skipped it, the identical command was caught when typed bare and passed when typed with its path:

command screen_command reality
rm -rf / recursive force remove destructive
/bin/rm -rf / None ❌ same binary, same flags
sudo reboot privilege escalation destructive
/usr/bin/sudo reboot None ❌ same
dd if=/dev/zero of=/dev/sda raw disk write destructive
/bin/dd if=/dev/zero of=/dev/sda None ❌ same
mkfs.ext4 /dev/sda1 raw disk write destructive
/sbin/mkfs.ext4 /dev/sda1 None ❌ same
chmod 777 /etc world-writable permissions destructive
/bin/chmod 777 /etc None ❌ same
ls | /usr/bin/rm -rf / None ❌ still split segment-by-segment

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:

-        reason = _classify(tokens[0].lower(), tokens[1:])
+        reason = _classify(_program(tokens[0]), tokens[1:])

_program() already lowercases, so the existing RM -rf case is unaffected.

Still defense-in-depth, not the boundary

Unchanged from #195, and deliberately so: the sandbox in core.harness.sandbox remains the enforcement boundary, screen_command still 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.txt stays 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:

FAILED test_blocks_path_qualified_destructive_commands[/bin/rm -rf /]
FAILED ... [/usr/bin/rm -rf /tmp/x]        ... 13 failed, 40 passed   (fix reverted)
53 passed                                                              (fix applied)

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 skipped in 2m26s, on the fix applied — same as baseline. ruff check and ruff format --check clean on both touched files.

Note on Windows backslash paths

C:\Windows\System32\shutdown.exe still passes, for an unrelated reason: shlex.split(..., posix=True) consumes the backslashes as escapes, so the token arrives as C:WindowsSystem32shutdown.exe before _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.

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

No deployments
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.

1 participant