Skip to content

fix(core): never crash the terminal guard on NUL-byte paths - #79326

Closed
Shotflame wants to merge 2 commits into
NousResearch:mainfrom
Shotflame:fix/terminal-guard-nul-byte-pr
Closed

fix(core): never crash the terminal guard on NUL-byte paths#79326
Shotflame wants to merge 2 commits into
NousResearch:mainfrom
Shotflame:fix/terminal-guard-nul-byte-pr

Conversation

@Shotflame

Copy link
Copy Markdown

What

Make the terminal security guard (tools/tirith_security.py::check_command_security) total against commands containing embedded NUL bytes. A guarded command must never crash the guard.

Why

subprocess.run() raises ValueError: embedded null byte when handed an argv element containing \x00 (POSIX cannot represent NUL in a filename/argv element). The guard feeds the raw user command verbatim into such a call, so a path like /tmp/foo\x00bar used to crash the entire command-security check with an unhandled exception instead of returning a clean verdict.

What changed

  • Up-front reject: check_command_security now returns a block verdict with rule_id: nul-byte (severity HIGH) when the command contains an embedded NUL, mirroring the lifecycle-guard fix (terminal tool: lifecycle_guard crashes on absolute-path executables (ValueError: embedded null byte), blocks all such commands #76762).
  • Defense-in-depth: an explicit except ValueError at the subprocess boundary converts any NUL-bearing argv that slips through into the same block/nul-byte verdict instead of letting the exception escape.
  • Regression test: TestNulByteSafeGuard in tests/tools/test_command_guards.py drives the full interactive guard flow with a NUL-aware subprocess.run stand-in and asserts the guard returns a clean verdict rather than crashing.

Verification

  • RED on pre-fix: ValueError: embedded null byte escapes the guard.
  • GREEN on fix: 167/167 targeted tests pass (command_guards 30, tirith 41, approval 94); smoke test: clean cmd → allow, /tmp/foo\x00barblock/nul-byte, no crash.
  • Independent review approved (2 reviews).

Complements #79279 which covers the sibling crash sites in cron/lifecycle_guard.py + tools/terminal_tool.py.

ops and others added 2 commits August 5, 2026 07:32
Add TestNulByteSafeGuard to the command-guard suite. A file path containing
an embedded NUL byte (e.g. /tmp/foo\x00bar) is fed verbatim by the guard to
tirith, which hands it to subprocess.run(); stdlib raises
'ValueError: embedded null byte' for NUL-bearing argv, propagating up through
check_all_command_guards and crashing the terminal guard.

This test drives the guard through the full (interactive) flow with tirith
enabled and a NUL-aware subprocess.run stand-in, then asserts the guard
returns a clean verdict instead of crashing. It FAILS (red) against the
current code, proving the bug before the fix (issue fix(core) NousResearch#79279).
…sResearch#79279)

A command/path containing an embedded NUL byte (\x00) caused tirith's
subprocess.run to raise ValueError: embedded null byte, which escaped the
guard and crashed every guarded terminal call. POSIX filenames and argv
elements are NUL-terminated, so a NUL-bearing command can never be valid.

Now reject NUL-bearing commands up front with a clear, user-facing block
verdict (rule_id 'nul-byte') and add a ValueError handler at the subprocess
boundary as defence-in-depth, mirroring the lifecycle guard fix (NousResearch#76762).

The regression test added in d7f16b20a now passes (was RED); all
existing command-guard / tirith / approval tests remain green.
@alt-glitch alt-glitch added type/bug Something isn't working tool/terminal Terminal execution and process management P2 Medium — degraded but workaround exists labels Aug 5, 2026
@kshitijk4poor

Copy link
Copy Markdown
Collaborator

Closing as superseded by #80258, which fixes this whole bug class architecturally rather than per-callsite: path candidates are sanitized once at the ingestion boundary (NUL/empty/unexpandable tokens rejected before any OS call), text from any read_remote_script callback is sanitized at the recursion boundary (NUL = binary = nothing to scan; >1 MiB = fail closed), the remote fallback read is bounded at the source (head -c, so oversized binaries never cross the wire), and the public guard is total by construction — an unexpected walk failure logs and falls back to the direct-scan verdict instead of breaking every terminal command.

Your report and fix targeted a real member of this class — thank you. The per-callsite patches kept leaving sibling frames exposed (#76762#77703#77780#78256 each crashed one frame away from the previous fix), which is why we went with the boundary fix instead of merging the fragments individually. #80258 carries regression tests for the NUL-path, binary-callback, oversized-read, unset-HOME, and walk-crash cases plus an adversarial never-raises sweep.

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

Labels

P2 Medium — degraded but workaround exists tool/terminal Terminal execution and process management type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants