fix(confirm): stop review() reporting success when the host took the screen away - #475
Merged
Merged
Conversation
…screen away
All four review*() variants called confirm_helper() as (void) and then
`return true`. confirm_helper() returns false for exactly one reason -- this
device has one button and no reject, so false means a host-sent
Cancel/Initialize -- and discarding it meant a host could suppress a screen and
still be told the review happened.
Propagating the result is one line each. What matters is the one caller where
the operation CONTINUED afterwards.
passphrase_sm.c: the "passphrase confirmation" screen is the only place a user
sees the passphrase their keys will be derived from. passphrase_request()
ignored the result and set ret = true, so a host that answered that screen with
a Cancel suppressed the display and still reached
session_cachePassphrase(). The session then derived a different wallet than the
user believed they were opening, with nothing shown on the OLED. All five
callers of passphrase_protect() already test its result, so returning the
review's verdict is enough to close it.
The other ten call sites were audited and none has the same consequence:
- fsm_msg_ton.h:108,182, fsm_msg_solana.h:485, ethereum.c:809 are "Blocked"
screens followed immediately by fsm_sendFailure + abort. Suppressing the
screen hides an explanation for a refusal that happens regardless.
- transaction.c:456 warns about a duplicate transaction and then sets
retval = -1. The transaction is refused either way.
- recovery_cipher.c:604,608,614 report a dry-run result and then send the
matching Success/Failure. Nothing is signed and no state changes.
- authenticator.c:257,264 gate and annotate an OTP display.
They now receive a meaningful value rather than an unconditional true, which is
worth having even where today's consequence is nil.
COVERAGE GAP, STATED DELIBERATELY. There is no automated test for this.
review() drives confirm_screen()'s loop, which blocks on a button press or an
injected USB tiny-message, and no unit test in this tree sets that up -- the
confirm-driver suites are why firmware-unit does not complete unfiltered. The
test belongs in the python-keepkey integration suite, which can send
PassphraseAck followed by Cancel and assert the passphrase is not cached; that
suite is currently pinned to a commit predating this work. Until then this is
verified by reading, by the ARM build, and by the hardware round.
This closes the suppression primitive in #428. It does NOT close #428: the
render measurement still covers only layout_standard_notification, so the
custom-layout address and xpub screens remain outside the measured guarantee.
Verified: ARM cross-compile in the pinned builder image; cppcheck clean under
CI's exact invocation (0 findings); clang-format clean across CI's entire lint
scope.
Refs #428
BitHighlander
force-pushed
the
fix/review-propagates-host-cancel
branch
from
August 17, 2026 22:58
b4c89a5 to
b286dc0
Compare
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.
#428(a) only. Deliberately does not attempt #428(b).
The defect
All four
review*()variants calledconfirm_helper()as(void)and thenreturn true:This device has one button and no reject, so
confirm_helper()returns false for exactly one reason: a host-sentCancel/Initialize. Discarding it means a host can suppress a screen and still be told the review happened.Where it actually costs something
Propagating is one line per variant. What matters is the single caller where the operation continued afterwards.
passphrase_sm.c:146— the"passphrase confirmation"screen is the only place a user sees the passphrase their keys will be derived from.passphrase_request()ignored the result and setret = true, so a host answering that screen with aCancelsuppressed the display and still reachedsession_cachePassphrase()(passphrase_sm.c:187). The session then derives a different wallet than the user believes they are opening, with nothing shown on the OLED.All five callers of
passphrase_protect()already test its result (storage.c:1863,1886,1932,fsm_msg_common.h:171,312), and each responds withFailure_ActionCancelled. So returning the review's verdict is sufficient — no caller changes.The other ten sites, audited
None has the same consequence, and each was checked rather than assumed:
fsm_msg_ton.h:108,182,fsm_msg_solana.h:485,ethereum.c:809fsm_sendFailure+ aborttransaction.c:456retval = -1recovery_cipher.c:604,608,614Success/Failureauthenticator.c:257,264They now receive a meaningful value instead of an unconditional
true, which is worth having even where today's consequence is nil.Coverage gap, stated deliberately
There is no automated test for this.
review()drivesconfirm_screen()'s loop, which blocks on a button press or an injected USB tiny-message, and no unit test in this tree sets that up — the confirm-driver suites are precisely whyfirmware-unitdoes not complete unfiltered.The test belongs in the python-keepkey integration suite, which can send
PassphraseAckthenCanceland assert the passphrase is not cached. That suite is pinned to a commit predating this work (see the repin blocker in the release notes). Until then this rests on reading, the ARM build, and the hardware round.Scope
Closes the suppression primitive in #428. Does not close #428 — the render measurement still covers only
layout_standard_notification, so the custom-layout address and xpub screens remain outside the measured guarantee. Gate 2 stays formally unmet, and the release notes should not say otherwise.Verified
ARM cross-compile in the pinned builder image; cppcheck clean under CI's exact invocation (0 findings); clang-format clean across CI's entire lint scope.
Depends on #474 for a usable CI signal —
secret-scanis currently red repo-wide from an unpinned gitleaks, which skips the whole build graph.