fix(cron): close NUL-padded script bypass in lifecycle guard (#77927) - #77928
Closed
Mengchee118 wants to merge 1 commit into
Closed
fix(cron): close NUL-padded script bypass in lifecycle guard (#77927)#77928Mengchee118 wants to merge 1 commit into
Mengchee118 wants to merge 1 commit into
Conversation
The NousResearch#76762 binary check treats any NUL byte in the first chunk as "compiled binary, nothing to scan": if b"\x00" in data: return None, False "Contains a NUL" and "is a compiled binary" are different questions, and the gap between them is a guard bypass. `bash` executes a *text* script straight past an embedded NUL, so one pad byte disables the entire scan while the script still runs: #!/bin/bash # pad<NUL> hermes gateway restart scan("bash padded.sh") -> False (not blocked) bash padded.sh -> executes the lifecycle command This shape was blocked before NousResearch#76762, so the crash fix traded a loud failure for a silent one. Keying the check on a leading `#!` is not sufficient: a shebang-less file with a NUL on any line but the first also executes normally. (A NUL on line 1 of a shebang-less file is the one shape bash rejects, exit 126 — but that same file is still executable via `. file`.) Fix: identify binaries by MAGIC NUMBER — ELF, Mach-O (incl. byte-swapped and universal/fat), PE/COFF, static archive, gzip, zip — with a shebang always winning. A NUL-bearing *text* file is scanned with its NULs stripped; stripping can only splice tokens together, never apart, so it fails closed. File extensions are deliberately not consulted, so a suffixless shell script is still scanned. The size check now runs BEFORE the strip: stripping shrinks the buffer, so checking afterwards would let an oversized file slip under the threshold and skip the fail-closed branch. (Caught by test_oversized_nul_bearing_text_still_fails_closed, which failed on the first cut of this patch.) Return values are unchanged, so this does not conflict with the in-flight crash-class fixes to the same function. Tests (tests/hermes_cli/test_gateway_restart_loop.py), 3 of which fail on main: - test_nul_padded_script_is_still_scanned - test_nul_padded_script_without_shebang_is_scanned - test_oversized_nul_bearing_text_still_fails_closed - test_elf_binary_is_not_scanned_as_script (NousResearch#76762 stays fixed) - test_macho_binary_is_not_scanned_as_script (incl. fat binary) - test_clean_script_without_lifecycle_command_not_blocked
Mengchee118
added a commit
to Mengchee118/hermes-agent
that referenced
this pull request
Aug 18, 2026
Upstream's rewritten guard sniffs the file prefix and bails on ANY NUL byte, which makes the post-read magic-number check unreachable dead code and silently reopens the NUL-padded script bypass (NousResearch#77928). bash executes a text script straight past an embedded NUL and rejoins the halves, so "contains a NUL" and "is a compiled binary" are different questions. Bailing on the former lets a single pad byte -- or a keyword split across one -- skip the scan entirely. Both sites now use the same predicate. Added a warning comment so the NUL test is not reintroduced at the early sniff. Same defect shape as 2026-08-07: a clean cherry-pick left the restored patch unreachable because upstream added a competing implementation earlier in the flow. No merge conflict means textual independence, not semantic compatibility. Verified: 134 tests pass, 22/22 bypass harness, behavioural probe confirms a NUL-padded referenced script is blocked again.
Contributor
|
Merged via PR #93411 (a9e4622) — your commit cherry-picked onto current main with authorship preserved in git log, plus a follow-up on our side: a newer main-side sniff fast-path also keyed on NUL-in-head and would have short-circuited your magic-number check, so it was re-keyed to executable magic only. Earliest submitter (Aug 3) — thanks for the thorough incident writeup and the size-before-strip catch. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Closes a guard bypass introduced by the
#76762binary check. That checktreats any NUL byte in a file's first chunk as "compiled binary, nothing to
scan" — but
bashexecutes a text script straight past an embedded NUL, so asingle pad byte disables the scan while the script still runs its lifecycle
command.
This shape was blocked before #76762, so the crash fix traded a loud failure
for a silent one. "Contains a NUL" and "is a compiled binary" are different
questions, and the bypass lives in the gap between them.
Fix
Identify binaries by magic number — ELF, Mach-O (incl. byte-swapped and
universal/fat), PE/COFF, static archive, gzip, zip — with a shebang always
winning. NUL-bearing text is scanned with its NULs stripped, which can only
splice tokens together and never apart, so it fails closed. As a side benefit
hermes gateway rest\x00artinside a file stops evading the matcher.Extensions are deliberately not consulted: a suffixless shell script must
still be scanned, and an oversized one must still fail closed.
Return values are unchanged (
(None, False)for a binary), so this does notconflict with the in-flight crash-class fixes to the same function.
Related Issue
Fixes #77927
Type of Change
Changes Made
cron/lifecycle_guard.py_BINARY_MAGICStuple +_has_binary_magic()helper, documenting why aNUL census is the wrong test and why a shebang wins.
_read_referenced_script: match on signature; strip NULs from text insteadof skipping the file.
checking afterwards let an oversized file slip under the threshold and skip
the fail-closed branch. This was a real bug in the first cut of this patch,
caught by the oversize test below.
tests/hermes_cli/test_gateway_restart_loop.py— six tests inTestLifecycleGuardModule.How to Test
Reproduce on
main:Swap the payload for
echo EXECUTEDto confirm bash really runs these withoutrestarting anything. With this PR both become
True.Automated:
cron/lifecycle_guard.pytomainand keeping the new testsfails exactly 3 — the two bypass shapes and the oversize case — so the tests
cover the defect rather than passing vacuously.
Checklist
Code
pytest tests/hermes_cli/test_gateway_restart_loop.py -qand all tests pass (88 passed)Documentation & Housekeeping
_has_binary_magicdocuments the reasoningcli-config.yaml.example— N/A, no config keysCONTRIBUTING.md/AGENTS.md— N/A, no architecture changeMZ) and ELF are in the magiclist alongside Mach-O, so Windows and Linux binaries are recognised too;
previously they were caught only incidentally by their NUL bytes
Notes for reviewers
On the shebang-only shortcut. My first attempt keyed the check on a leading
#!and it was insufficient — a shebang-less file with a NUL on any line but thefirst also executes. Measured
bash filebehaviour:Only the last is unrunnable via
bash file, and even that one is executable via. file. Hence magic-number detection rather than a shebang test.On duplicates / overlap. The open PRs on this function (#77383, #77729,
#77806, #77894) all address the crash class. This is the bypass class. Worth
flagging specifically: #77383's
_looks_like_scripthelper preserves the samepremise — its docstring says "real scripts are NUL-free text; shebang or
not" — so
b"\x00" not in headkeeps this hole open. I executed that helperdirectly against the shapes above to confirm rather than reading it. Happy to
rebase on whichever crash fix lands first; this patch touches the decision, not
the return contract.
On NUL in argv vs in a file. A NUL inside a command-line argument is
harmless —
execve(2)takes NUL-terminated strings, so such a command isunrunnable. That is not true inside a script file, which is why this needs
fixing at the file-read boundary. Same byte, different channel, opposite
conclusion.