diff --git a/cron/lifecycle_guard.py b/cron/lifecycle_guard.py index 6c7a5eaad062f..4532f8cf84915 100644 --- a/cron/lifecycle_guard.py +++ b/cron/lifecycle_guard.py @@ -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) diff --git a/tests/hermes_cli/test_gateway_restart_loop.py b/tests/hermes_cli/test_gateway_restart_loop.py index bd90e99130101..ad22826cd8a33 100644 --- a/tests/hermes_cli/test_gateway_restart_loop.py +++ b/tests/hermes_cli/test_gateway_restart_loop.py @@ -9,6 +9,7 @@ import json import os from argparse import Namespace +from pathlib import Path import pytest @@ -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.""" diff --git a/tools/terminal_tool.py b/tools/terminal_tool.py index d929947f41e07..ce47f479ece36 100644 --- a/tools/terminal_tool.py +++ b/tools/terminal_tool.py @@ -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 @@ -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 @@ -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