Skip to content

fix(cron): guard against NUL-byte crash in lifecycle_guard scanners - #78572

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

Shunkleburger wants to merge 1 commit into
NousResearch:mainfrom
Shunkleburger:fix/lifecycle-guard-nul-byte

Conversation

@Shunkleburger

Copy link
Copy Markdown

Summary

The terminal lifecycle_guard scanner crashes with ValueError: embedded null byte when a scanned command references a script whose decoded binary contents tokenize into a NUL-containing path. os.open() raises ValueError, but the crash sites only catch OSError — so a single bad token blocks every terminal command until the gateway recovers. This is the sibling of #76762, which fixed the resolve() path but missed the script-content path.

Root cause

_read_referenced_script() does os.open(path, flags) inside except OSErroros.open raises ValueError: embedded null byte for NUL-containing paths, which escapes uncaught.

Upstream, the crash propagates through both scanners:

  • scan_for_referenced_scripts() (cron script: values) via _resolve_script_path()
  • scan_command_for_gateway_lifecycle_commands() (terminal commands) via the shell-tokenized command segments — NUL bytes leak into tokens when a scanned script's binary contents are decoded

Fix (3 defensive layers, same class as #76762)

Site Change
_iter_command_segments() Strip \x00 at the tokenizer — covers all three scanners (path refs, bash -c payloads, launchctl detection)
_read_referenced_script() except (OSError, ValueError) at the os.open crash site
_resolve_script_path() NUL-strip before Path() construction (cron script: values)

NUL bytes are never valid shell characters or path components, so stripping them cannot change legitimate behavior.

Verification

  • tests/hermes_cli/test_gateway_restart_loop.py82 passed
  • Direct repro: NUL-containing path through the full scanner → no crash, safe return
  • Behavior unchanged: hermes gateway restart still blocked, normal cron script values resolve identically

Test plan for CI

  • Existing suite: tests/hermes_cli/test_gateway_restart_loop.py
  • Recommended addition: a regression test feeding a command containing a NUL byte (e.g. bash /tmp/evil\x00path.sh) through contains_gateway_lifecycle_command_or_referenced_script() and asserting it returns False without raising.

Sibling of NousResearch#76762: os.open() raises ValueError ('embedded null byte')
on NUL-containing paths, but _read_referenced_script() only caught
OSError, letting the crash escape and block every terminal command
when a scanned script's decoded binary contents tokenize into a
NUL-containing path.

- _iter_command_segments: strip \x00 at the tokenizer (covers all
  three scanners: path refs, bash -c payloads, launchctl detection)
- _read_referenced_script: catch ValueError alongside OSError
- _resolve_script_path: NUL-strip before Path() construction

Verified: tests/hermes_cli/test_gateway_restart_loop.py 82 passed;
NUL-path repro returns safely; gateway lifecycle detection unchanged.
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/cron Cron scheduler and job management tool/terminal Terminal execution and process management needs-decision Awaiting maintainer decision before any implementation labels Aug 4, 2026
@kshitijk4poor

Copy link
Copy Markdown
Contributor

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

comp/cron Cron scheduler and job management needs-decision Awaiting maintainer decision before any implementation 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