fix(smoke): de-flake 10c by corrupting the signature instead of removing it - #692
Merged
Conversation
…ing it Smoke test 10c (Phase 10 binary security E2E) verifies that an arm64 binary with an invalid code signature is SIGKILLed (exit 137). It did `codesign --remove-signature` then ran the binary, expecting the kernel to kill it. But since macOS 11, a binary with NO LC_CODE_SIGNATURE is ad-hoc re-signed on exec by newer macOS and RUNS (exit 0) -- so the test went flaky and then consistently red on updated CI runner images (dry-runs 28350650225 and 28354735368 both failed only here; every other job, including the full cross-platform test matrix, passed). Corrupt the signature blob in place instead, leaving the LC_CODE_SIGNATURE load command intact: AMFI then sees "signed but invalid" and rejects the binary before any user code runs (deterministic 137). Only the signature blob is garbled (not the code), so the later 10e re-sign step stays valid -- it replaces the blob and the code is untouched. x86_64 keeps remove-signature (code signing is not enforced there). Refs: github.com/garrytan/gstack#997, github.com/nodejs/node#40827 Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Refines the previous 10c change on this branch. Garbling only the signature blob still ran (exit=0 on dry-run 28360363173): since macOS 11 a binary with a missing/invalid signature is ad-hoc re-signed on exec by newer macOS and RUNS, so neither remove-signature nor a corrupt blob triggers the kill. The reliable "tampered binary is SIGKILLed (137)" trigger is tampering the SIGNED CODE while leaving the valid signature attached: the kernel validates each executed page against the intact CodeDirectory hash, finds the mismatch, and kills the process before user code runs. Zero the entry-point instructions (LC_MAIN entryoff, extracted dynamically) plus a span of early __text on a SEPARATE copy, leaving the Mach-O header + load commands intact so it still parses. The original binary is untouched, so the later 10e re-sign step stays valid. x86_64 keeps remove-signature (code signing is not enforced there). Refs: github.com/garrytan/gstack#997, github.com/nodejs/node#40827 Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
The "tampered arm64 binary is SIGKILLed (137)" premise is empirically false on current macOS CI runners for an ad-hoc-signed CLI binary -- the binary has no CS_KILL/hardened-runtime flag, so a tampered code page is not killed: it executes the garbage and crashes with SIGILL (exit 132, run 28365724001), not 137. (remove-signature and corrupt-blob both ad-hoc re-sign on exec and run to exit 0.) So no runtime exit code is a deterministic guard here, and "tamper -> crash" is near-tautological (zeroed code crashes regardless of signing). Assert the real, deterministic integrity invariant instead: `codesign --verify` REJECTS a tampered copy (the CodeDirectory page hashes no longer match the modified code), while the untampered binary verifies cleanly (10a). It is a pure userspace hash check -- no tampered code is executed. The copy is separate, so the original binary stays intact for the 10e re-sign test. Refs: github.com/garrytan/gstack#997, github.com/nodejs/node#40827 Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
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
1 participant
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.
De-flakes smoke test 10c (Phase 10 binary security E2E).
Root cause: the test's premise — "a tampered/unsigned arm64 binary is SIGKILLed (exit 137)" — is empirically false on current macOS CI runners for an ad-hoc-signed CLI binary. The cbm binary has no
CS_KILL/hardened-runtime flag, so the kernel does not kill a bad page at runtime. Observed across four dry-runs:codesign --remove-signaturethen run28350650225,283547353682836036317328365724001So no runtime exit code is a deterministic guard here, and "tamper → crash" is near-tautological (zeroed code crashes regardless of signing).
Fix: assert the real, deterministic integrity invariant —
codesign --verifyrejects a tampered copy (the CodeDirectory page hashes no longer match the modified code), while the untampered binary verifies cleanly (10a above). It's a pure userspace hash check; no tampered code is ever executed. Tampering is done on a separate copy ($SECURITY_BIN.tampered), so the original is untouched and the 10e re-sign step stays valid. x86_64 unchanged.This is neither a skip nor a weakening — it replaces a false runtime premise with the actual security property the test exists to verify (the signature detects tampering). Unrelated to the seal fix (#677). Verified via a full dry-run on this branch.
Refs: garrytan/gstack#997, nodejs/node#40827