fix(passphrase): keep the plaintext off the stack, and zero it - #491
Merged
Conversation
Closes #476. passphrase_protect() held the BIP-39 passphrase in a stack local that was never zeroed, on every path -- success, host cancel, and the early-out where passphrase protection is off. The passphrase selects the wallet: it is the difference between the visible wallet and a hidden one. Nothing else in the firmware treats key material that way. confirm_sm.c uses 'static CONFIDENTIAL char strbuf[]'; the session cache is 'static SessionState CONFIDENTIAL session' and is zeroed in session_clear_impl. This one buffer opted out of both. Not a regression: unchanged from the shipped base 1af2ffe. The only delta in this file for 7.14.2 is #475's verdict propagation. CONFIDENTIAL is not a documentation marker. On device builds CMakeLists defines it as __attribute__((section("confidential"))), a NOLOAD region that the bootloader wipes at boot (tools/bootloader/main.c:83, keepkey.ld:23); on emulator builds it is empty. A section attribute cannot apply to an automatic, so the buffer has to leave the stack to get that protection -- hence static. passphrase_protect() is not reentrant: fsm handlers call it on the single main loop and it blocks in passphrase_request() until the exchange completes. The section is only cleared at boot, so the buffer is also zeroed on entry and exit. Reboot-scope protection is not call-scope protection, and the residue between two calls is the part reachable without a power cycle. Bounded severity, and worth stating: the passphrase arrives FROM the host in plaintext by design -- the device displays it so the user can confirm which wallet they are opening. A compromised host already has it. This closes a device-local residue, not a disclosure path.
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.
Closes #476.
Answers to the three questions worth asking first
Is it the BIP-39 passphrase? Yes —
char passphrase[PASSPHRASE_BUF]inPassphraseInfo. It selects the wallet: the difference between the visiblewallet and a hidden one.
Is it a regression? No.
git diff 1af2ffe7de HEAD -- lib/firmware/passphrase_sm.cshows the only change in this file for 7.14.2 is #475's verdict propagation. The
missing zeroing shipped in 7.14.1 and earlier.
Is the design really "host sends it over the wire, device shows it"? Yes —
PassphraseAckcarries it and the device renders"If this is wrong, unplug/replug Keepkey:"so the user can confirm which wallet is opening. Thatbounds the severity of this issue: a compromised host already has the
passphrase in plaintext. What this closes is a device-local residue, not a
disclosure path.
The fix is not the one the issue proposed
#476 suggests marking the local
CONFIDENTIAL. That cannot work: on devicebuilds
CONFIDENTIALis__attribute__((section("confidential")))(
CMakeLists.txt:88), and a section attribute does not apply to an automatic.So the buffer has to leave the stack — which is exactly what the rest of the
firmware already does:
confirm_sm.cstrbufstatic CONFIDENTIALstatic SessionState CONFIDENTIAL sessionsession_clear_implpassphrase_protect()This makes it
static PassphraseInfo CONFIDENTIALand zeroes it on entry andexit.
Why both, and not just the section:
.confidentialisNOLOADand is wipedby the bootloader at boot (
tools/bootloader/main.c:83,keepkey.ld:23).That is reboot-scope protection. The residue that matters is the one between two
calls, without a power cycle — so it is zeroed explicitly as well.
Reentrancy: safe.
passphrase_protect()is called from fsm handlers on thesingle main loop and blocks inside
passphrase_request()until the exchangecompletes.
Scope
Three lines plus comments, no behaviour change, no wire change, no new screens.
The issue said "not proposed for 7.14.2" on the grounds that it is untestable at
the unit layer and unchanged from the base — both still true. Offered anyway
because it is small and self-contained, and an open issue reading "plaintext
passphrase on the stack" sits badly beside a release whose subject is disclosure.
Happy to defer to 7.15 if you would rather not touch this file again after the
hardware round.