Skip to content

fix(guard): handle binary executables in lifecycle script scan - #77233

Closed
mohitagrawal-marvis wants to merge 1 commit into
NousResearch:mainfrom
mohitagrawal-marvis:fix/lifecycle-guard-binary-null-byte
Closed

fix(guard): handle binary executables in lifecycle script scan#77233
mohitagrawal-marvis wants to merge 1 commit into
NousResearch:mainfrom
mohitagrawal-marvis:fix/lifecycle-guard-binary-null-byte

Conversation

@mohitagrawal-marvis

Copy link
Copy Markdown

Summary

The gateway lifecycle guard (cron/lifecycle_guard.py) crashes with ValueError: embedded null byte whenever a terminal command references a binary executable by path — e.g. .venv/bin/python -m src.main or /usr/local/opt/python@3.14/bin/python3.14 --version. The guard reads the referenced file, decodes it, tokenizes the content as shell text, and Path.resolve() blows up on NUL bytes found in the binary. The result: legitimate commands get hard-blocked with a confusing error, and any workflow that shells out to a venv interpreter breaks.

I hit this immediately after upgrading to the version that introduced the referenced-script scanning — the very first command referencing the events-scraper's venv python crashed the guard.

Root Cause

  • _iter_referenced_shell_scripts treats any executable containing / (or ending in .sh/.bash/.zsh) as a script to scan.
  • A Python interpreter qualifies (path contains /), so the guard opens and reads the binary.
  • data.decode("utf-8", errors="replace") keeps NUL bytes intact (they're valid UTF-8 code points).
  • The decoded binary text is then scanned as shell content; _iter_referenced_shell_scripts yields tokenized "paths" containing \x00, and Path.resolve() raises ValueError("embedded null byte").
  • Only OSError was caught, so the ValueError propagated up and killed the terminal call.

Fix

Two changes, both in cron/lifecycle_guard.py:

  1. _read_referenced_script — strip NUL bytes from the decoded text before returning it. Binary content can then still be scanned as text (preserving the fail-closed guarantee that a crafted binary can't hide a lifecycle command behind non-UTF-8 bytes) without poisoning downstream path resolution.

  2. _contains_unsafe_gateway_action — also catch ValueError from Path.resolve() alongside OSError, as defense-in-depth for any other path that could smuggle a NUL byte in.

Test Plan

Added regression tests to tests/hermes_cli/test_gateway_restart_loop.py:

  • test_binary_executable_referenced_in_command_does_not_crash — a NUL-byte-laden binary executable (ELF-ish signature) passes through the terminal tool cleanly (exit 0, command executed).
  • test_binary_executable_hiding_lifecycle_command_still_blocked — a binary embedding hermes gateway restart is still blocked (fail-closed preserved).

Manually verified all four behavior classes against the patched module:

  • ✅ Binary executable, no lifecycle command → passes (was: crash)
  • ✅ Binary hiding a lifecycle command → still blocked
  • ✅ Plain lifecycle command → still blocked
  • ✅ Benign shell script → still passes

Repro (before fix)

$ cd ~/.hermes/events-scraper && ./.venv/bin/python --version
Failed to execute command: embedded null byte
ValueError: embedded null byte  (traceback through cron/lifecycle_guard.py)

Commands referencing a binary executable by path (e.g. a venv
interpreter like .venv/bin/python) crashed the gateway lifecycle
guard with ValueError('embedded null byte'): the referenced file was
read and decoded, then its content tokenized as shell text, and
Path.resolve() blew up on NUL bytes in the resulting tokens. Every
terminal call touching such a binary was blocked with a hard error.

Fix in two parts:
- _read_referenced_script strips NUL bytes after decoding, so binary
  content can still be scanned as text without poisoning path
  resolution. The scan still runs, preserving the fail-closed
  guarantee that a crafted binary cannot hide a lifecycle command
  behind non-UTF-8 bytes.
- _contains_unsafe_gateway_action also catches ValueError from
  Path.resolve() as defense-in-depth, treating it like OSError.

Adds regression tests: a NUL-byte binary executable passes through
cleanly, while a binary embedding the gateway restart command is
still blocked.
@alt-glitch alt-glitch added type/bug Something isn't working comp/cron Cron scheduler and job management tool/terminal Terminal execution and process management P2 Medium — degraded but workaround exists needs-decision Awaiting maintainer decision before any implementation labels Aug 3, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: #77201 and #76797 address the same absolute-binary lifecycle-guard failure with different scan-selection policies. This patch retains binary scanning after NUL removal; maintainer selection is needed.

@mohitagrawal-marvis

Copy link
Copy Markdown
Author

Acknowledging the overlap flagged in triage — #76797 is the better fix, closing mine in favor of it.

Comparison:

Why #76797's scan-selection policy is more correct than mine: a binary executable is never interpreted by a shell, so scanning its bytes for a lifecycle command string produces only false positives (my test_binary_executable_hiding_lifecycle_command_still_blocked encodes exactly that false positive — a binary merely containing the string gets blocked even though it would never execute it as a shell command). Skipping binaries entirely is the right call.

Closing this PR in favor of #76797 to reduce maintainer selection burden. My regression tests are redundant with theirs (test_full_path_non_shell_binary_is_not_scanned, test_embedded_null_path_does_not_raise, etc.).

@mohitagrawal-marvis

Copy link
Copy Markdown
Author

Closing in favor of #76797 (superset fix). Details in my analysis comment above.

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