Skip to content

fix: memfd_secret close-on-exec, honest Windows termination, release gate that fails closed - #57

Closed
deadpoets wants to merge 2 commits into
mainfrom
fix/review-batch-1
Closed

fix: memfd_secret close-on-exec, honest Windows termination, release gate that fails closed#57
deadpoets wants to merge 2 commits into
mainfrom
fix/review-batch-1

Conversation

@deadpoets

Copy link
Copy Markdown
Owner

Three findings from the external review. Unrelated to each other except that all
three are cases where something reported success it had not earned — which is
the failure mode this repo cares most about.

Independent of #56 (the HIGH janitor-key fix); no overlapping files.


1. memfd_secret fd was inheritable across exec — MEDIUM

mlock_linux.go:177. The descriptor was created with no flags and stays open
across ftruncate, the guard reservation, and the MAP_FIXED. A fork+exec
from any other goroutine during that window hands the child a live descriptor
to the secret pages
— making the strongest allocation tier the one with the
worst exec posture.

The trap in the fix. The flag is O_CLOEXEC, not the FD_CLOEXEC that
memfd_secret(2)'s man page names. The kernel tests flags & O_CLOEXEC and
EINVALs anything outside SECRETMEM_FLAGS_MASK | O_CLOEXEC. Passing bit 0
would not merely have failed to set close-on-exec — it would have failed the
syscall outright and silently dropped every allocation to the weaker anon
path
, which is worse than the leak being fixed.

This file cannot be executed from the maintainer's platform (Windows), so
EINVAL is not trusted to mean one specific thing: it retries bare and sets
close-on-exec with fcntl(F_SETFD). Slightly wider window than the atomic form,
but a tier downgrade is impossible either way.

Cross-compiled and vetted for linux/amd64 and linux/arm64.

2. InstallTerminationWipe claimed a termination it cannot perform — MEDIUM

terminationwipe.go:67. Verified empirically on go1.26/windows-amd64 rather
than from memory:

Signal(os.Interrupt)    -> not supported by windows
Signal(syscall.SIGTERM) -> not supported by windows

os.Process.Signal on Windows supports only os.Kill. The error was discarded
with _ =, so the process ran on past Ctrl-C with every secret already
zeroed
— reads returning zeros, mutations returning ErrWiped — while the doc
comment said it terminated. That is the worst of the three possible outcomes.

Now logged at warn level, with the platform limit documented on the exported
function. Deliberately not escalated to a forced os.Exit: this installer
explicitly promises never to take the exit out from under a co-installed
graceful shutdown, and signal.Notify is additive, so such a handler already
received the signal independently. On Windows the exit is the application's job
— the difference is that it now says so instead of being discovered.

If you would rather it force-exit on Windows, that is a behaviour change and
your call; say so and I will make it.

3. release.sh failed open on the exact case it exists for — MEDIUM

.github/scripts/release.sh:122. The ordering check asked only whether the
required in-repo version was published.

The failure that motivated the entire script — secmem-crypto/v0.3.0,
permanently inert — required a version that was published perfectly well. It was
simply the previous one, because the tag was cut before the floor-raise PR
merged. So the gate reported success on precisely the footgun it was written to
prevent.

It now also requires that version to be the newest published one, and fails
closed
when the proxy cannot be reached — an unreachable proxy is exactly when
someone is most tempted to shrug and tag anyway. SECMEM_ALLOW_STALE_DEP=1
covers a deliberately older floor.

version_lt is unit-tested, including the numeric-vs-lexical trap that catches
naive comparisons:

ok   version_lt(v0.3.0,v0.4.0)=0     # stale go.mod -> refuse
ok   version_lt(v0.10.0,v0.9.0)=1    # numeric, not lexical
ok   version_lt(v0.9.0,v0.10.0)=0
ok   version_lt(,v0.4.0)=1           # unparseable -> not older

And verified end to end against the live proxy: it refuses v0.3.0 against a
published v0.4.0, and passes the repo's current, correct state
(core @latest = v0.3.0, secmem-crypto requires v0.3.0).


Verification

  • Core go test -race ./... green; gofmt clean over tracked files.
  • GOOS=linux build + vet for amd64 and arm64 (the mlock_linux.go change
    cannot be exercised on the maintainer's platform).
  • bash -n on release.sh, plus the helper unit tests above.

…ase gate that fails closed

Three findings from the external review, unrelated to each other beyond all
being cases where something reported success it had not earned.

memfd_secret fd was inheritable across exec
--------------------------------------------
Created with no flags, and the fd stays open across ftruncate, the guard
reservation and the MAP_FIXED. A fork+exec from any other goroutine in that
window hands the child a live descriptor to the secret pages — making the
strongest allocation tier the one with the worst exec posture.

The flag is O_CLOEXEC, NOT the FD_CLOEXEC that memfd_secret(2) names: the kernel
tests `flags & O_CLOEXEC` and EINVALs anything outside
SECRETMEM_FLAGS_MASK|O_CLOEXEC. Passing bit 0 would not have merely failed to
set close-on-exec, it would have failed the syscall and silently dropped every
allocation to the weaker anon path — a worse outcome than the leak. Since this
file cannot be executed from the maintainer's platform, EINVAL is not trusted to
mean one specific thing: it retries bare and sets close-on-exec via
fcntl(F_SETFD). Slightly wider window than the atomic form, never a downgrade.

InstallTerminationWipe claimed a termination it cannot perform on Windows
-------------------------------------------------------------------------
os.Process.Signal on Windows supports only os.Kill and rejects os.Interrupt and
SIGTERM with "not supported by windows" — verified empirically on go1.26,
windows/amd64, rather than from memory. The error was discarded, so the process
ran on past Ctrl-C with every secret already zeroed while the doc comment said
it terminated.

Now logged at warn level with the platform limit documented. Deliberately not
escalated to a forced os.Exit: the installer promises never to take the exit
away from a co-installed graceful shutdown, and signal.Notify is additive, so
such a handler already received the signal independently.

release.sh ordering check failed open on the case it exists for
---------------------------------------------------------------
It asked only whether the required in-repo version was published. The failure
that motivated the whole script — secmem-crypto/v0.3.0, permanently inert —
required a version that was published perfectly well. It was just the previous
one, because the tag was cut before the floor-raise PR merged. The gate reported
success on precisely the footgun it was written to prevent.

It now also requires that version to be the newest published one, and fails
CLOSED when the proxy cannot be reached, because an unreachable proxy is exactly
when someone is most tempted to shrug and tag anyway.
SECMEM_ALLOW_STALE_DEP=1 covers a deliberately older floor.

version_lt is unit-tested against the numeric-vs-lexical trap (v0.10.0 > v0.9.0)
and verified end to end against the live proxy: it refuses v0.3.0-against-v0.4.0
and passes the repo's current, correct state.
TestScrub_ScrubsShallowCallTree addresses its planted markers through a raw
uintptr into this goroutine's stack. The GC neither tracks nor adjusts that
value, so a stack shrink between planting and reading frees the segment back to
the pool and the read lands on whatever now occupies the address.

The fault case is the obvious one. The quiet case matters more: unrelated memory
holds no 0xA5 markers, so countMarkers returns 0 and the assertion PASSES having
observed nothing at all. A security regression test that can silently succeed
without testing anything is worse than one that fails, and this one guards the
reserve-then-wipe fix specifically.

shrinkstack only runs while the collector is scanning the goroutine, so the
collector is disabled for the window — placed before the control read so it
covers that too.

Also corrects countMarkers' comment, which asserted the read was safe because
"stack segments are pooled, not unmapped". That holds only while the segment is
still this goroutine's; after a shrink it can be scavenged. The caller owns that
constraint now, and the comment says so.

Flagged by the external review as a candidate for the open windows/amd64
corruption. I do not think it is: this is a READ, and it cannot corrupt the
goroutine free list, while the observed crash was `stack not a power of 2`
thrown from stackfree via gfget. A faulting read reports an unexpected fault
address instead. Fixed on its own merits, not as a crash theory.
@deadpoets

Copy link
Copy Markdown
Owner Author

Landed on main via the integration train in #61:

  • b0b2e23 — memfd_secret close-on-exec, honest Windows termination, release gate that fails closed
  • b844011 — keep the collector out of the scrub test's dead-stack read window

Rebased locally so both carry your SSH signature; different SHAs, hence the manual close.

Note the release.sh change is now live: it refuses a tag whose in-repo dependency is not the newest published version, and fails closed when the proxy is unreachable.

@deadpoets deadpoets closed this Aug 19, 2026
@deadpoets
deadpoets deleted the fix/review-batch-1 branch August 19, 2026 00:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant