fix(terminationwipe): exit on Windows instead of wiping and running on - #62
Merged
Conversation
…nd running on Measured rather than reasoned. A real CTRL_C_EVENT delivered to a child in its own process group, using the actual installer: first Ctrl-C -> every secret wiped, process KEPT RUNNING (19 heartbeats) second Ctrl-C -> exited, status 0xc000013a os.Process.Signal on Windows implements only os.Kill and rejects os.Interrupt and SIGTERM, and the console event that triggered the handler has already been consumed and answered "handled", so there is nothing left to re-deliver. The error was discarded, so the failure was invisible. Why this is worse than "press Ctrl-C twice" -------------------------------------------- WipeAllSecrets deliberately leaves regions MAPPED so a late access reads zeros instead of faulting. That trade is justified entirely by "the process is terminating imminently" — and reads still SUCCEED. A process that survives the wipe therefore keeps every key buffer readable and full of zeros, so an application that treats the signal as "begin shutdown" can go on to sign with an all-zero key, derive from zeros, or write zeros where a secret belonged, each call reporting success. That is the one state the emergency wipe does not support, and it is worse than either terminating or never wiping. Status choice ------------- 0xC000013A is STATUS_CONTROL_C_EXIT, what Windows produces for an un-intercepted console Ctrl-C. Verified identical in the harness: the run that exited through Windows' own default and the run that exited through this constant both report 0xc000013a. Matching it is the point — installing the wipe must not change how a parent, a batch file or a CI step reads a cancellation. Written as the signed 32-bit value so it fits an int on 386 as well as amd64. Opt-out ------- InstallTerminationWipeNoExit keeps the previous behaviour for callers whose own handler performs a graceful shutdown that must not be truncated. It is exported API, so the next core release is a minor bump. Its doc says plainly that the secrets are already gone and readable as zeros, so that handler should exit promptly. Unix is untouched: the re-raise there is a real kill(2) against a restored default disposition and already terminated correctly. Testing ------- The re-raise and exit steps are parameters of completeTermination rather than direct calls, because a test that really re-raised would kill the test binary and one that really exited would take the suite with it. The four-way table (re-raise works / impossible x default / NoExit) is a unit test; live console delivery stays in the manual harness, which is deliberately NOT wired into CI — generating console control events on a shared runner risks the job's own process group, and a diagnostic that can kill CI is not worth the coverage.
…compiles
t.Errorf takes its arguments as interface{}, so an untyped 0xC000013A defaults
to int — which overflows on a 32-bit word. Both 386 jobs caught it, which is
exactly what they exist for: the changelog records windows/386 failing to
compile once before, and the cross-compile row was added in response.
The forcedExitStatus constant itself was fine; only the test literal was not.
Typed as uint32 now, and verified by compiling the test binaries for all eight
GOOS/GOARCH pairs CI covers rather than trusting vet alone.
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.
Implements the decision on the
InstallTerminationWipequestion: exit bydefault on Windows, with an explicit opt-out.
Measured, not reasoned
A real
CTRL_C_EVENTdelivered to a child in its own process group, using theactual installer:
os.Process.Signalon Windows implements onlyos.Killand rejectsos.Interruptand SIGTERM; the console event that triggered the handler hasalready been consumed and answered "handled", so there is nothing left to
re-deliver. The error was discarded, so the failure was invisible.
Why this was worse than "press Ctrl-C twice"
WipeAllSecretsdeliberately leaves regions mapped so a late access readszeros instead of faulting. That trade is justified entirely by "the process is
terminating imminently" — and reads still succeed.
A process that survives the wipe therefore keeps every key buffer readable and
full of zeros. An application treating the signal as "begin shutdown" can go on
to sign with an all-zero key, derive from zeros, or write zeros where a secret
belonged — each call reporting success. That is the one state the emergency
wipe does not support, and it is worse than either terminating or never wiping.
Status choice
0xC000013AisSTATUS_CONTROL_C_EXIT, what Windows produces for anun-intercepted console Ctrl-C. Verified identical in the harness: the run that
exited through Windows' own default and the run that exited through this
constant both report
0xc000013a.Matching it is the point — installing the wipe must not change how a parent, a
batch file or a CI step reads a cancellation. Written as the signed 32-bit value
so the constant fits an
inton 386 as well as amd64.Opt-out
InstallTerminationWipeNoExitkeeps the previous behaviour for callers whose ownhandler performs a graceful shutdown that must not be truncated. Its doc says
plainly that the secrets are already gone and readable as zeros, so that handler
should exit promptly.
This is exported API, so the next core release is a minor bump, consistent
with the precedent in the changelog.
Unix is untouched — the re-raise there is a real
kill(2)against a restoreddefault disposition and already terminated correctly.
Testing, and one thing deliberately not tested in CI
completeTerminationtakes the re-raise and exit steps as parameters, because atest that really re-raised would kill the test binary and one that really exited
would take the suite with it. The four-way table (re-raise works / impossible ×
default / NoExit) is a unit test, plus a check that the status matches
STATUS_CONTROL_C_EXITon Windows.Live console delivery stays in a manual harness and is deliberately not wired
into CI: generating console control events on a shared runner risks the job's
own process group, and a diagnostic that can kill CI is not worth the coverage.
Verified: core + redact
-racegreen,GOOS=linuxandGOOS=darwinbuild andvet clean,
gofmtclean.