Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion cron/lifecycle_guard.py
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,12 @@ def _read_referenced_script(path: Path) -> tuple[Optional[str], bool]:
flags = os.O_RDONLY | getattr(os, "O_NONBLOCK", 0)
try:
descriptor = os.open(path, flags)
except OSError:
except (OSError, ValueError):
# ValueError: ``open: embedded null character in path``. A path
# candidate carrying a NUL byte can only come from tokenizing
# non-script bytes, so there is nothing to scan — and a guarded
# path must never crash the guard (#76762). ``_resolve_script_path``
# below already treats ValueError this way; os.open did not.
return None, False
try:
metadata = os.fstat(descriptor)
Expand Down
67 changes: 67 additions & 0 deletions tests/hermes_cli/test_gateway_restart_loop.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import json
import os
from argparse import Namespace
from pathlib import Path

import pytest

Expand Down Expand Up @@ -695,6 +696,72 @@ def test_absolute_path_binary_does_not_crash_guard(self):
)
assert result is False

def test_nul_path_from_remote_reader_does_not_crash_guard(self, tmp_path):
"""#76762 follow-up: the same crash came back through the OTHER reader.

The test above passes because it omits ``read_remote_script`` — but
tools/terminal_tool.py ALWAYS passes one (``_read_script_in_env``), and
that fallback returned replacement-decoded binaries. The scanner then
tokenized machine code into path candidates carrying NUL bytes and
``os.open`` raised ``ValueError: open: embedded null character in path``,
which the ``except OSError`` in ``_read_referenced_script`` did not catch.

Effect on a real deployment: every terminal command whose executable
contains a "/" failed — ``.venv/bin/python -m pytest``, ``/bin/ls -la`` —
after burning ~28s in the scan first.
"""
from cron.lifecycle_guard import (
contains_gateway_lifecycle_command_or_referenced_script,
)
binary = tmp_path / "fake-interpreter"
# Shaped like decoded machine code: NUL bytes glued to path-ish tokens.
binary.write_bytes(b"\x7fELF\x00\x00./setup.sh\x00/tmp/x.sh\x00\x01\x02")

def _decode_anything(script_path):
"""Exactly what _read_script_in_env used to do.

``except Exception`` matches the real callback — and is load-bearing:
the scanner feeds junk tokens back in, and ``Path.read_bytes`` on one
raises ``ValueError: embedded null byte``, not OSError.
"""
try:
return Path(script_path).read_bytes().decode("utf-8", errors="replace")
except Exception:
return None

result = contains_gateway_lifecycle_command_or_referenced_script(
f"{binary} --version",
cwd=str(tmp_path),
read_remote_script=_decode_anything,
)
assert result is False

def test_remote_reader_binary_rejected_before_scanning(self):
"""The root-cause fix: a binary payload is never handed to the scanner.

Covers ``tools.terminal_tool._script_text_if_not_binary``, which both
branches of ``_read_script_in_env`` now route through. Catching the
ValueError alone is not enough — it stops the crash but leaves the
scanner grinding through decoded machine code (measured at >80s per
command on a real interpreter, worse than failing fast).
"""
from tools.terminal_tool import _script_text_if_not_binary

# Binaries: rejected, as bytes and as already-decoded text (NUL is valid
# UTF-8, so it survives an errors="replace" decode).
assert _script_text_if_not_binary(b"\x7fELF\x00\x02\x01") is None
assert _script_text_if_not_binary("\x7fELF\x00\x02\x01") is None
assert _script_text_if_not_binary(None) is None

# Real shell scripts: passed through unchanged, so the guard can still
# walk them (see test_shell_script_reference_walk_still_works).
assert _script_text_if_not_binary(
b"#!/bin/sh\nhermes gateway restart\n"
) == "#!/bin/sh\nhermes gateway restart\n"
assert _script_text_if_not_binary("#!/bin/sh\ntrue\n") == "#!/bin/sh\ntrue\n"
# Invalid UTF-8 without NULs is still text-shaped; decode, do not drop.
assert _script_text_if_not_binary(b"#!/bin/sh\n\xff\xfe\n") is not None

def test_shell_script_reference_walk_still_works(self, tmp_path):
"""The referenced-script walk still applies to real shell scripts:
a .sh script that itself invokes a lifecycle command is caught."""
Expand Down
36 changes: 34 additions & 2 deletions tools/terminal_tool.py
Original file line number Diff line number Diff line change
Expand Up @@ -373,6 +373,32 @@ def _check_all_guards(command: str, env_type: str,
has_host_access=has_host_access)


def _script_text_if_not_binary(data: "bytes | str | None") -> Optional[str]:
"""Return scannable script text, or ``None`` when *data* is a binary.

The lifecycle guard feeds whatever a script reader returns straight back
into its recursive scanner, so replacement-decoded ELF/Mach-O/PE bytes make
it tokenize machine code into path candidates — junk that carries NUL bytes
and then blows up path handling (``ValueError: open: embedded null
character in path``, #76762). ``cron.lifecycle_guard._read_referenced_script``
already refuses binaries this way for its own local reads; every other
reader handed to the guard has to agree, or a command as ordinary as
``/bin/ls -la`` fails.

NUL is valid UTF-8 and survives an ``errors="replace"`` decode, so the same
check is correct for raw bytes and for already-decoded text.
"""
if data is None:
return None
if isinstance(data, bytes):
if b"\x00" in data:
return None
return data.decode("utf-8", errors="replace")
if "\x00" in data:
return None
return data


# Allowlist: characters that can legitimately appear in directory paths.
# Covers Unicode letters/digits, path separators, Windows drive/UNC separators,
# tilde, dot, hyphen, underscore, space, plus, at, equals, and comma. Shell
Expand Down Expand Up @@ -2533,6 +2559,12 @@ def _read_script_in_env(script_path: str) -> Optional[str]:
For local backends the script path is on the host filesystem. For
SSH/Modal/Daytona the same path is remote; the local read misses, so we
fall back to ``env.execute('cat ...')``.

Binaries are rejected rather than returned as replacement-decoded
text — see ``_script_text_if_not_binary`` for why that matters
(#76762). Both branches must agree on it: the `cat` fallback below is
what actually fed decoded ELF bytes back into the scanner, because a
multi-megabyte interpreter fails the local size check and falls through.
"""
if env is None:
return None
Expand All @@ -2545,14 +2577,14 @@ def _read_script_in_env(script_path: str) -> Optional[str]:
if stat.S_ISREG(metadata.st_mode) and metadata.st_size <= 1024 * 1024:
data = local_path.read_bytes()
if len(data) <= 1024 * 1024:
return data.decode("utf-8", errors="replace")
return _script_text_if_not_binary(data)
except Exception:
pass
# Remote / sandboxed backend: read via the environment's shell.
try:
result = env.execute(f"cat {shlex.quote(script_path)}")
if result.get("returncode", -1) == 0:
return result.get("output", "")
return _script_text_if_not_binary(result.get("output", ""))
except Exception:
pass
return None
Expand Down