Skip to content

fix(cron): tolerate NUL-byte path tokens from the command string in lifecycle guard - #77898

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

fix(cron): tolerate NUL-byte path tokens from the command string in lifecycle guard#77898
namredips wants to merge 1 commit into
NousResearch:mainfrom
namredips:fix/lifecycle-guard-nul-path

Conversation

@namredips

Copy link
Copy Markdown
Contributor

Problem

_read_referenced_script catches only OSError around os.open(), but os.open() raises ValueError for a path containing an embedded NUL byte. A NUL can arrive in a path token straight from the command string — e.g. an agent emitting \x00 inside a JSON-encoded terminal command — and the guard crashes:

>>> from cron.lifecycle_guard import contains_gateway_lifecycle_command_or_referenced_script
>>> contains_gateway_lifecycle_command_or_referenced_script("bash /tmp/junk\x00fragment.sh")
ValueError: open: embedded null character in path

In production this failed every terminal tool call in the affected agent turn, and agent retry loops multiplied the damage (we measured this driving a runaway token burn on a cron-driven agent before diagnosing it).

Relation to #76762

#76762 fixed the sibling vector — NUL bytes read out of a referenced binary's contents — by skipping binaries in _read_referenced_script and tolerating ValueError at the Path.resolve() site. But the os.open() call two lines above the binary check still catches only OSError, so a NUL-bearing path token that arrives directly from the command string still crashes the guard before the content check can run.

Fix

Catch (OSError, ValueError) at the os.open() site, matching the treatment #76762 gave Path.resolve(). This is fail-safe: a NUL-bearing path can never exist on POSIX and can never reach execve, so there is no script to scan and nothing for the guard to miss.

Testing

Added test_nul_byte_in_command_string_does_not_crash_guard beside the existing #76762 regression test. Verified:

  • new test fails with ValueError before the fix, passes after
  • test_absolute_path_binary_does_not_crash_guard and test_binary_script_does_not_silently_bypass still pass
  • lifecycle detection intact (bash -c "hermes gateway restart" still returns True)

…ifecycle guard

NousResearch#76762 taught _read_referenced_script to skip binaries whose contents
contain NUL bytes, and to tolerate ValueError at Path.resolve(). But a
path token containing a literal NUL can also arrive straight from the
command string (e.g. an agent emitting "\x00" inside a JSON-encoded
terminal command). os.open() raises ValueError — not OSError — for such
a path, which escaped _read_referenced_script's handler and crashed the
guard, failing every terminal tool call in the agent turn.

A NUL-bearing path can never exist on POSIX and can never reach execve,
so treating it as 'nothing to read' is fail-safe.

Repro (before this fix):
  contains_gateway_lifecycle_command_or_referenced_script(
      "bash /tmp/junk\x00fragment.sh")
  ValueError: open: embedded null character in path
@Duncan822

Copy link
Copy Markdown

Follow-up: same root cause has a second symptom this PR doesn't cover

I was hit by this crash in production (agent terminal calls failing every turn). After applying this exact (OSError, ValueError) fix, the crash stopped — but the guard then false-blocked legitimate commands that reference a binary in a script position:

$ python /home/user/venv/bin/python /home/user/scripts/foo.py
Blocked: command or referenced script cannot restart or stop the gateway from inside the gateway process...

Root cause

_read_referenced_script skips binaries via the local NUL-byte check (if b"\x00" in data: return None, False). But in _contains_unsafe_gateway_action, when the local read returns None, the remote fallback runs:

if script_text is None and read_remote_script is not None:
    script_text = read_remote_script(str(script_path))

For local backends read_remote_script is env.execute(f"cat {path}") — it cats the ELF binary and decodes it as text (~8 MB with thousands of NUL bytes). The recursive scan of that decoded binary then walks pseudo-paths out of machine code until _MAX_REFERENCED_SCRIPT_DEPTH is hit, which returns True unconditionally → false block on every command whose first token is a venv python.

Fix (mirror the local NUL guard in the remote fallback)

        if script_text is None and read_remote_script is not None:
            # Local path missing; try the remote backend if one is available.
            script_text = read_remote_script(str(script_path))
            # The remote fallback may return raw binary decoded as text (e.g.
            # `cat` of an ELF executable). NUL bytes mean binary — treat as
            # "nothing to scan", mirroring the local-path NUL guard above,
            # so a binary can't explode the recursion or false-positive.
            if script_text and "\x00" in script_text:
                script_text = None

Tests

  • contains_gateway_lifecycle_command_or_referenced_script("python /venv/bin/python script.py", read_remote_script=cat_fallback)False (no longer blocked)
  • contains_gateway_lifecycle_command_or_referenced_script("hermes gateway restart")True (real lifecycle command still blocked)
  • NUL path token → (None, False), no crash

Verified in production after a gateway reload: the previously-blocked command passes, and the real lifecycle guard still fires on actual restart commands.

@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

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.

4 participants