Skip to content

fix: make lifecycle guard total against NUL-byte paths - #77894

Closed
luckrucksack wants to merge 1 commit into
NousResearch:mainfrom
luckrucksack:fix/guard-nul-byte-crash
Closed

fix: make lifecycle guard total against NUL-byte paths#77894
luckrucksack wants to merge 1 commit into
NousResearch:mainfrom
luckrucksack:fix/guard-nul-byte-crash

Conversation

@luckrucksack

Copy link
Copy Markdown

Summary

The gateway lifecycle guard (cron/lifecycle_guard.py) can crash with ValueError: embedded null byte when a terminal command references a binary (e.g. .venv/bin/python). Issue class #76762 was previously fixed at one crash site only — Path.resolve() — while sibling sites (os.open, script-directory resolution) still raised. A security guard must be a total function: every input maps to a decision, none raises.

Root cause

  • The guard treats any /-containing executable as a "referenced script" and scans it.
  • _read_referenced_script correctly detects binaries and skips them locally — but the caller then fires a remote fallback (tools/terminal_tool.pycat <binary>), which succeeds on binaries and decodes the raw bytes as UTF-8 with NUL bytes intact.
  • The guard recursively tokenizes that binary garbage as shell text; a NUL-containing "path" reaches os.open → uncaught ValueError. The terminal tool: lifecycle_guard crashes on absolute-path executables (ValueError: embedded null byte), blocks all such commands #76762 fix covered resolve() but missed this sibling (the incomplete-fix pattern).

Two real consequences: any command referencing a binary can crash the guard (observed in production, crashing the terminal tool on ordinary commands), and decoded binary content can contain literal lifecycle-command strings (Python binaries carry docstrings) → false-positive blocking of innocent commands.

Fix

  • Tolerate ValueError at every path-resolution site in the guard's referenced-script walk (in addition to the existing OSError handling).
  • Fail closed with a size bound on referenced-script reads.
  • Reject NUL bytes in the terminal tool's script reader — both local and remote fallback branches — so binary content never enters the guard as text.
  • Regression tests for all three crash sites + the binary-reference path.

Testing

  • 3 new regression tests in tests/hermes_cli/test_gateway_restart_loop.py; all failed pre-fix with the exact bug signatures (red proven), pass post-fix.
  • Full file: 85/85 green on current origin/main.
  • No collateral damage in adjacent suites.

The NousResearch#76762 fix covered Path.resolve() only; sibling sites (os.open,
script-directory resolve) still raised ValueError: embedded null byte
when a referenced binary was decoded as text and re-tokenized. The
terminal tool's remote-script fallback resurrects binaries as decoded
text, feeding machine code into the scan.

- tolerate ValueError at every path-resolution site in the guard walk
- fail closed with a size bound on referenced-script reads
- reject NUL bytes in the terminal tool's script reader (local+remote)
- regression tests for all three crash sites (red before, green after)
@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.

@Nexlab-One Nexlab-One mentioned this pull request Aug 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cron Cron scheduler and job management 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