diff --git a/docs/release/rc30-evidence/T5-cutoff-findings.md b/docs/release/rc30-evidence/T5-cutoff-findings.md new file mode 100644 index 000000000..7d77d093d --- /dev/null +++ b/docs/release/rc30-evidence/T5-cutoff-findings.md @@ -0,0 +1,91 @@ +# T5 — Cut Off pre-screen (#428/#481/#482), hardware round + +Device: 7.14.2, variant KeepKey, device_id 39353036114736342A004600, AdvancedMode=False. +Vehicle: `Ping` with `button_protection` — `fsm_msgPing` -> `confirm(..., "Ping", "%s", msg->message)`. + +## PASS — boundary and wire protocol + +| body | ButtonRequests | result | +|---|---|---| +| 100 ch | 1 | one screen, no warning | +| 117 ch | 1 | three full rows, no warning | +| 118 ch | 2 | CUT OFF — boundary is 118 | +| 119 ch | 2 | CUT OFF | +| 255 ch | 2 | CUT OFF | + +- **#481 confirmed on hardware.** Boundary is 118, not 119. A clipped final glyph + no longer reports as fitting (`draw.c:213-219`). +- **#482 confirmed on hardware.** The Cut Off screen emits its own ButtonRequest + (`code=1`, ButtonRequest_Other). An auto-approving host can no longer deadlock. +- No false positives at 100 or 117. + +## FINDING 1 — "Hold to view it anyway" discloses nothing + +`confirm_sm.c:441` re-draws the SAME truncated body after the warning: + + return confirm_screen(request_title, request_body, ...); + +`request_body` is unchanged and the generic `confirm()` path has no pager. The +byte-exact pager (`confirm_bytes()`, with n/m counters) exists only for the three +SignMessage handlers. So the hidden text stays hidden and the second hold buys +the user nothing. + +The screen tells the user it is about to disclose the remainder, and does not. +Either the copy is wrong or the pager is missing. Wrong copy on a consent screen. + +## FINDING 2 — the second consent is satisfied by the RELEASE of the first + +Three runs, varying only when the host's ButtonAck for BR2 lands: + +| run | ack timing | hands off after hold #1 | result | +|---|---|---|---| +| carry-over | immediate | yes | **Success @ 1.602s** | +| no-ack control | never sent | yes | silence 15s (screen is gated) | +| delayed-ack | +5s | yes | silence 15s (screen waits) | + +Immediate ack completes; ack delayed past the bounce window does not. The press +edge therefore arrives shortly after the user releases hold #1. + +**Mechanism.** `keepkey_button.c` has no debounce. EXTI is `EXTI_TRIGGER_BOTH` +and `buttonisr_usr()` decides press vs release from the GPIO level at interrupt +time. A mechanical release bounces low and dispatches `on_press_handler`. +`confirm_screen()` resets `state_info` to HOME and re-registers handlers +(`confirm_sm.c:198-220`), so screen 2 is armed and accepts that bounce as a +fresh press out of HOME, then runs its own hold timer to completion. + +**Consequence.** #482 added a second ButtonRequest so the user would be asked to +consent to a body the device admits it cannot fully display. On hardware that +second consent can be satisfied by the physical release of the first hold. The +user holds once and both screens pass. + +**Why CI cannot see this.** The emulator has no bounce and no physical button; +`keepkey_button_up()` is `return false` under EMULATOR. Only a hardware round +finds it — this is the case for keeping the hardware gate. + +## Proposed fix (not yet applied) + +Require the button to be observed UP before a confirm screen accepts a press. +Local to confirm_sm.c, no driver change, no timing constant: + +- at `confirm_screen()` init: `state_info.armed = keepkey_button_up();` +- in `handle_screen_press()`: ignore the press unless `si->armed` +- in `handle_screen_release()`: `si->armed = true;` + +A screen that opens while the button is still down (or bouncing) refuses presses +until a genuine release is seen. Costs one bool and two branches. + +## Reproduce + + cd deps/python-keepkey/tests + PROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION=python python3 cutoff_428_carryover.py # Success ~1.6s + PROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION=python python3 cutoff_428_noack.py # silence + PROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION=python python3 cutoff_428_delayack.py # silence + +Replug between runs; `hwpreflight.idle_or_die()` refuses to start on a dirty device. + +## Trap recorded + +An aborted run leaves a ButtonRequest queued that SURVIVES into the next session, +and the next run answers it silently — observed live as BR1 returning code=4 +instead of the Ping's own code=23. Every test in this plan needs the first-code +assertion or it can be invalidated by whatever ran before it. diff --git a/docs/release/rc30-evidence/artifact-0-device.md b/docs/release/rc30-evidence/artifact-0-device.md new file mode 100644 index 000000000..b486f1325 --- /dev/null +++ b/docs/release/rc30-evidence/artifact-0-device.md @@ -0,0 +1,24 @@ +# Artifact #0 — device identity, rc30 hardware round + +Captured before any test, over WebUSB, Vault stopped. + +| field | value | +|---|---| +| version | 7.14.2 | +| firmware_variant | KeepKey (NOT KeepKeyBTC — handler-dependent tests are valid) | +| device_id | 39353036114736342A004600 | +| label | rc30 | +| initialized | True | +| pin_protection | False | +| passphrase_protection | False | +| bootloader_mode | False | +| firmware_hash (first 16B) | 933dc8ff10c7861ab2b25d67e4cc86dd | +| policies | ShapeShift=False, Pin Caching=True, Experimental=False, AdvancedMode=False | + +AdvancedMode reads False at rest, matching the compiled default +(include/keepkey/firmware/policy.h). Every phase-B test re-asserts it anyway. + +Artifact was UNSIGNED: flashing wiped storage. Seed present at capture time is +disposable and will be wiped again by T1. + +TODO: sha256 of the flashed .bin (host-side, not device-readable). diff --git a/include/keepkey/board/confirm_sm.h b/include/keepkey/board/confirm_sm.h index 69e252cf5..c4eb04a6a 100644 --- a/include/keepkey/board/confirm_sm.h +++ b/include/keepkey/board/confirm_sm.h @@ -79,6 +79,11 @@ typedef struct { DisplayState display_state; ActiveLayout active_layout; bool immediate; + /* False until the button has been observed released while this screen owned + it. The button driver has no debounce, so releasing a hold bounces the line + and dispatches a press; a screen armed a moment earlier would otherwise + accept that bounce as consent. See #484. */ + bool armed; } StateInfo; #define isprint(c) ((c) >= 0x20 && (c) < 0x7f) diff --git a/lib/board/confirm_sm.c b/lib/board/confirm_sm.c index b12ab3fe7..c37e197fd 100644 --- a/lib/board/confirm_sm.c +++ b/lib/board/confirm_sm.c @@ -68,6 +68,14 @@ static void handle_screen_press(void* context) { StateInfo* si = (StateInfo*)context; + /* A screen that opened while the button was still down -- or still bouncing + from the previous screen's release -- has not seen a genuine press yet. + The driver has no debounce and reports a release bounce as a press, so + without this the release of one hold satisfies the NEXT screen. #484. */ + if (!si->armed) { + return; + } + if (button_request_acked) { switch (si->display_state) { case HOME: @@ -88,6 +96,10 @@ static void handle_screen_release(void* context) { StateInfo* si = (StateInfo*)context; + /* The button is up: any bounce from a previous screen has settled and the + next press on this screen is the user's own. #484. */ + si->armed = true; + switch (si->display_state) { case CONFIRM_WAIT: si->active_layout = LAYOUT_REQUEST_NO_ANIMATION; @@ -200,6 +212,15 @@ static bool confirm_screen(const char* request_title_param, state_info.immediate = immediate; state_info.display_state = HOME; state_info.active_layout = LAYOUT_REQUEST; + /* Arm immediately only if the button is already up. If it is down, this + screen waits for the release before it will accept anything. #484. + There is no button under EMULATOR -- keepkey_button_up() is a constant + false there -- so arm unconditionally or emulated presses never land. */ +#ifdef EMULATOR + state_info.armed = true; +#else + state_info.armed = keepkey_button_up(); +#endif /* Request */ state_info.lines[LAYOUT_REQUEST].request_title = request_title;