fix(display): seed/BIP-85 clipping, the a3da828 follow-up, and the missing rng_health dependency - #534
Merged
BitHighlander merged 21 commits intoAug 24, 2026
Conversation
Constant-power screens draw from x = 128 + LEFT_MARGIN, because the display
driver mirrors the right half of the canvas onto the panel. The real budget is
KEEPKEY_DISPLAY_WIDTH - (128 + LEFT_MARGIN) = 124 px
but both the formatter and the renderer used BODY_WIDTH (225). The wrap
therefore never fired before the canvas edge did: draw_char_impl() rejected the
first glyph crossing 256, draw_string_walk() stopped, and every character after
it was dropped -- including whole later lines. No ellipsis, no warning, no page
indicator. The user writes down a truncated or missing word and the backup does
not restore the wallet.
MEASURED with the real font tables over 200,000 random 24-word mnemonics:
before 1.712% of backups clipped a page; 0.646% never showed one of the
words at all
after 0 clipped pages in 200,000; 0 exceeded MAX_PAGES
EXHAUSTIVE, not sampled. Over every BIP-39 word and indices 1..24:
widest single "N.word" 66 px (20.mushroom) fits, 58 spare
widest adjacent pair, NO separator 132 px OVERFLOWS by 8
So two numbered words per line is impossible at 124 px in the worst case, and
no amount of trimming indentation fixes it -- 3 spaces gives 144 px, 1 space
136, zero 132. A random sample says 141,517 of 141,519 pairs fit and would have
led straight to a spacing tweak that still fails on mushroom-class words. This
is why the acceptance gate demanded exhaustive proof.
Measuring at the real width makes the wrap fire before the canvas edge, which
naturally puts one word on a line when two will not fit, and costs an extra page
only for the seeds that need one:
98.22% of random 24-word seeds -> 6 pages, unchanged
1.78% -> 7 pages
worst case, every word widest -> 12 pages
That 1.78% is the same population that was clipping. Repeated words are legal
BIP-39 so the worst case is reachable, and MAX_PAGES is raised 6 -> 12 to bound
it: sizing to the observed maximum instead would fail closed with "Too many
pages of mnemonic words" during wallet creation on a legal seed. Cost is about
3.7 KB of scratch.
WHY THIS IS NOT THE REVERTED PAGER. That change split a screen inside
page_body_confirm(), which emits one ButtonRequest per page while the device's
current_page never advanced -- so the host read the same words twice and
reconstructed a mnemonic with duplicated words. Here reset.c owns both the page
content and mnemonic_by_screen[], which reset_get_word() reports, so each
ButtonRequest still maps to exactly one page and the host reads each page once.
BIP-85 gets the same fix: it renders derived words on the same constant-power
screens from the same scratch buffers, so it clipped identically, and a derived
mnemonic is as unrecoverable as the master one when a word is missing.
PYTHON-KEEPKEY. alpha's pin (999e776a6) shared NO merge-base with the live lineage: it sits on pre-squash-197-backup, a backup taken before a squash rewrite, while .gitmodules declared alpha tracks reconcile/upstream-sync. The declared branch and the actual pin disagreed, and no single existing branch carried everything alpha needs: canonical reconcile/upstream-sync CI gates, Aave V3 fixtures, token dedup canonical release/7.15-audit-fixes the 7.15 security test work fork develop the WETH uniswap-token addition Pinning any one of them drops the others -- in particular, pinning the branch alpha claimed to track would have shipped the firmware security fixes WITHOUT the tests that prove them: the display-disclosure vacuity fix, the v6 empty-Orchard-bundle fixture and Z27. The three are merged on the fork's develop, which is now the reconciled line, and the submodule points there. Verified before moving: the tree diff from alpha's old pin is +975/-97 across 23 files with ZERO deletions, so nothing alpha carried is lost -- its 54 "ahead" commits are the pre-squash originals of work the live lineage already contains. DEVICE-PROTOCOL. a1a1dda3e -> bb5e43e05, the up/release-protocol tip. Pure fast-forward: 8 ahead, 0 behind, so there is nothing to reconcile.
The backport carried 8661918 but stopped there, so the final audited fixes in 8661918..a3da828 were missing from alpha. Direction-ported rather than cherry-picked, because alpha diverges. CLASSIFICATION, per file, against the port base: solana.h alpha == base -> took a3da828 fsm_msg_mayachain.h alpha == base -> took a3da828 fsm_msg_solana.h alpha == base -> took a3da828 fsm_msg_thorchain.h alpha == base -> took a3da828 fsm_msg_zcash.h DIVERGED -> 3-way merged, ours = this candidate zcash.cpp DIVERGED -> 3-way merged, ours = this candidate Both 3-way merges applied clean. "ours" is this branch, not raw alpha, because the two zcash files already carried the security backport and the rng_health include fix -- merging against alpha would have reverted them. SEMANTICS LANDED, each verified present rather than assumed: ZIP-229 v6 empty Orchard digest and ZTxIdOrchardH_v6 empty Ironwood digest and ZTxIdIronwd_H_v6 exactly-one send/deposit validation, THORChain and MAYAChain transaction-level AND deposit-level memo review when both are signed authenticated KKSOLSC1 schema data wired into opaque Solana review the ordinary blind-sign warning preserved after schema annotation the corresponding firmware regressions PRIOR WORK VERIFIED INTACT after the merges: the rng_health.h include, redpallas_sign_digest_with_ak, the zcash_T[80] checked draw, EmptyBundleDigests_MatchZip244, and all six RedPallasNonce_* regressions.
My previous approach was wrong and CI review caught it. Measuring the packing
at the real 124 px width raised a 24-word backup from 6 pages to 7 for 1.78% of
seeds -- and reset.c runs
for (current_page < page_count) { confirm_constant_power(...) }
with confirm_constant_power() emitting exactly one ButtonRequest per call. So a
seventh page was a seventh host-visible request. The host reads one word set per
request:
while isinstance(resp, proto.ButtonRequest):
mnemonic.append(client.debug.read_reset_word())
"More display pages" and "unchanged host transcript" were contradictory in that
implementation. Raising MAX_PAGES did not resolve the contradiction, it shipped
it.
REWORKED so the protocol boundary does not move:
MAX_PAGES back to 6 no 3.7 KB expansion
grouping back to BODY_WIDTH the GROUPING is the protocol boundary
layout still draws at 124 px so rows wrap instead of running off the canvas
confirm_constant_power_paged() emits ONE ButtonRequest for the group and then
advances subpages locally. The seam is that confirm_helper() and
confirm_screen() do not emit requests -- only the public entry points do -- so
subpages can be rendered through confirm_screen() under a single request.
Verified: exactly one msg_write() in the function.
Subpages split only at row boundaries, so a word is never divided across
screens, and each chunk is measured at CONSTANT_POWER_BODY_WIDTH against
BODY_ROWS. Intermediate subpages take a short press; only the last takes the
caller's hold, because only the last is the approval. Cancelling any subpage
cancels the group. The scratch buffer is CONFIDENTIAL and wiped on exit.
Both flows use it: reset.c and fsm_msg_bip85.h. No direct confirm_constant_power
call remains in either.
Also lands unittests/firmware/seed_display.cpp, registered in the build: the
budget is derived from the draw origin rather than asserted as a literal; every
BIP-39 word at every index 1..24 fits one line, exhaustively; the widest
adjacent pair provably does NOT fit even with zero separator, so if that test
ever starts passing two-up packing became possible; the worst-case all-widest
mnemonic stays within MAX_PAGES; and the known clipping vector is asserted to
have been ACCEPTED at the old 225 px width, so the test keeps demonstrating the
original bug.
Five things, all found in review rather than by me. 1. DEBUGLINK WOULD HANG AFTER THE FIRST SUBPAGE. debug_decided is a LOCAL in confirm_screen() while button_request_acked is a file static, so on the second subpage the local resets to false, the static stays true, and the loop waits for a decision that never comes -- DebugLink supplies ONE decision per ButtonRequest, and the whole group is one request. confirm_screen() now reports whether it exited on a debug decision, and the pager carries that across subpages: the decision already taken covers the rest of the group. Remaining subpages are still RENDERED, so DebugLinkGetState reads the real screen, but they do not wait. Physical presses are unaffected. 2. THE WORST-CASE TEST WAS STALE. It packed at CONSTANT_POWER_BODY_WIDTH and compared the result to MAX_PAGES -- two different things once the design changed. Groups are formed at BODY_WIDTH, because the grouping is the protocol boundary, and the 124 px fitting happens as subpages inside a group which consume no page slots. It now measures GROUPS, and pins MAX_PAGES == 6 so the approach that changed the request count cannot come back unnoticed. 3. THE SUBPAGE SPLIT USED calc_str_line(). That is a second model of the screen, and the guard it backs has been broken three separate ways in this file's history. It now uses confirm_body_fits_constant_power(), which replays draw_string()'s own loop and per-glyph fit test with the writes off, at the origin the layout actually draws from. Measuring and drawing are the same code. This is a security decision: a row that does not fit is a seed word the user never sees. 4. CONSTANT-POWER CALLERS AUDITED. Exactly two exist -- reset.c and fsm_msg_bip85.h -- and both now go through the pager. Nothing else is affected by the layout's 124 px width. 5. THE STALE COMMENT claiming the fix needs MAX_PAGES raised (~3.7 KB) is replaced with what was actually done.
DebugLink cannot supply per-subpage visual evidence, and nothing should be built
on the assumption that it can.
The carried subpages inside a group are drawn but NOT waited on -- that is what
stops the second one hanging on a decision that never comes -- so the message
loop never runs between them and DebugLinkGetState has no turn in which to read
them. A screenshot taken over DebugLink sees a group's LAST subpage, not each
one. That limit is now stated at the carry site and on the exposed helper, so a
later reader does not mistake a green screenshot job for per-subpage proof.
Per-subpage content is proven instead by unit instrumentation over the splitter,
which is exposed as confirm_constant_power_subpage_take() for exactly this:
SubpagesCoverEveryRowExactlyOnceInOrder
every chunk ends at a row boundary, so a numbered word is never divided
across screens; every chunk fits when measured by the RENDERER; and the
chunks reassemble to the original group with nothing lost, duplicated or
reordered. That last property is the one the host contract depends on.
SplitterTerminatesOnAnUnsplittableRow
a row too wide to fit still advances the pager instead of looping.
The no-hang property is covered by the integration suite rather than here:
test_reset_device drives one press per ButtonRequest, so a pager that waited for
a second decision would hang it. It is in the CI filter, so that gate runs.
Physical OLED capture remains the only evidence for what each subpage actually
looks like on the panel.
Three more review findings, all real. 1. THE PAGER STRIPPED INDENTATION. It skipped leading spaces at each chunk boundary, copied from page_body_confirm() where a line-start space is dropped. Here every formatter row BEGINS with the indent, so that stripped it from every subpage after the first: different content on screen, and subpages that no longer reassemble to the group they came from. Indentation is preserved now. My test hid this. It skipped spaces the same way before comparing, so the two agreed by construction while both were wrong. The test now requires BYTE-EXACT reassembly, which is the property the host contract actually depends on. 2. THE UNSPLITTABLE-ROW FALLBACK WAS UNSAFE. The splitter proved a row did not fit and then returned it anyway, so the pager would render CLIPPED content and report success -- reintroducing the exact defect this change exists to remove. It returns 0 now and the pager fails closed. The truncating min(take, sizeof(sub) - 1) is gone for the same reason: silently shortening a row is showing the user something other than what is there. Proven this cannot reject legitimate content, exhaustively rather than by sample: over all 50,331,648 combinations of odd index pair and ordered word pair, the worst a real two-word row occupies at 124 px is 3 lines -- exactly BODY_ROWS. Fail-closed triggers only ABOVE that. The margin is zero, which is why a sample was not good enough: 157k rows also said 3, but nothing about a sample rules out the 4-line case that would have failed wallet creation. 3. THE FIXTURE LEAKED SIG_IGN. It set SIGALRM to ignored globally and never restored it, so every later test in the binary ran with the alarm suppressed. The previous handler is saved and restored in TearDown. Still outstanding and not claimed: dynamic ButtonRequest-count coverage and checksum-valid BIP-39 vectors.
seed_display.cpp opened extern "C" before any C++ standard header. The board
headers pull in <ctype.h>, and having that inside extern "C" before <string>
and gtest have set up the C++ <cctype> machinery fails to compile:
/usr/include/ctype.h:17:15: error: expected unqualified-id
cctype:70:11: error: no member named 'isprint' in the global namespace
ARM stayed green because it does not compile unit tests; only the emulator job
builds firmware-unit, so this surfaced there and nowhere else.
Now mirrors unittests/board/board.cpp exactly, which already gets this right:
C++ standard headers first, THEN the extern "C" block. The board headers still
need extern "C" -- none of them carry __cplusplus guards, so without it they
would take C++ linkage and fail to link against the C-compiled firmware
objects. I nearly dropped the wrapper entirely for the same reason it looked
unnecessary; checking board.cpp rather than guessing is what caught that.
Three defects in my own test file, none of which the previous run could have revealed because it failed to compile before reaching them. 1. WOULD NOT LINK. BodyFitsEnv::prev_alarm_handler was declared as a static member and never defined -- my include-block rewrite dropped the out-of-class definition. The next successful compile would have failed at link with an undefined reference. It is now an ordinary per-instance member with an in-class initializer, so there is nothing to define separately. 2. THE SIGALRM SUPPRESSION LEAKED THE OTHER WAY. Ensure() installed SIG_IGN once, guarded by a static flag, while TearDown() restored the previous handler after EVERY test. So the first test suppressed the animation tick, restored it, and every later fixture test then ran with the tick LIVE, repainting the canvas underneath measurements that assume it is still. Install and restore now happen per test; only the canvas init stays one-time, because that genuinely is. 3. THE BOUNDARY REGRESSION WAS INERT. It asserted EXPECT_GE(subpages, 1), which a single-subpage body satisfies trivially -- it would have passed while proving nothing about splitting. It now requires EXPECT_GT(subpages, 1). Confirmed the vector actually crosses a boundary rather than assuming it: the mushroom group is 7 lines at 124 px against a 3-line budget and splits into exactly 3 subpages of 3 lines each. Had it been one subpage, the stronger assertion would have failed and I would have needed a wider vector.
The full unit-test binary hit its 10-minute cap on the previous head while
bitcoin-only finished in 77 s. My fixture was the difference, and my first
explanation of WHY was overstated: restoring tim4_sighandler cannot by itself
restart a timer that ualarm(0, 0) disarmed, so something else would have had to
re-arm it. SIGALRM interaction remains the leading hypothesis, not a proven
mechanism -- the log that would settle it is still gated behind the run.
Rather than argue the mechanism, remove the whole class of problem: this fixture
now installs no handler and restores none, so it cannot leak state into the rest
of the binary in either direction.
timer_init() is KEPT, and dropping it would not have been safe. It fills
free_queue, and layout_init() calls post_periodic(), which does
runnable_queue_pop(&free_queue) and dereferences the result with NO null check:
runnable_node = runnable_queue_pop(&free_queue);
runnable_node->runnable = callback;
Without timer_init() that is a null dereference, not a missing tick. So the
fixture calls timer_init() for the queue, then ualarm(0, 0) to disarm the 1 kHz
alarm that timer_init() itself creates -- and never touches signal().
The fixture is also renamed SeedDisplayBodyFits and given internal linkage.
unittests/board/board.cpp defines its own BodyFits, and two different class
definitions sharing one external name is an ODR violation even though they land
in separate binaries today. Distinct name and internal linkage means neither can
bite later.
The full unit-tests job was not failing, it was hanging: it burned its 10-minute timeout and reported "cancelled", which reads like infrastructure flake. The bitcoin-only job stayed green throughout. kk_board_init() calls kk_timer_init(), and timer_init() does the same work: both push the three static runnables[] nodes onto free_queue unconditionally. A second bootstrap therefore relinks nodes that are already linked, free_queue and active_queue go circular, and the runnable_queue_get() walk inside post_periodic() never returns. seed_display.cpp arrived with its own timer_init() guard while thorchain.cpp's confirm driver already had one, so the full firmware unit binary bootstrapped twice. thorchain.cpp is not compiled into the bitcoin-only image, which is why only one variant hung -- exactly the shape the CI matrix showed. Proven against the real lib/board/timer.c, one bootstrap versus the two that are actually in the binary: timer_init only: terminated, rc=0 timer_init + kk_timer_init: never returned, killed by a 5s watchdog A per-file guard cannot fix this, because the problem is that there is one guard per file. test_board.cpp holds the single guard for the whole binary and is compiled into both variants; both call sites now go through it. Also drops <unistd.h> and timer.h from seed_display.cpp, whose only users were the ualarm() and timer_init() calls this removes.
…ntity fix(bitcoin-only): identify physical firmware to Vault Taken into the seed/BIP-85 branch rather than stacked behind it, so one CI matrix validates both the bootstrap fix and the Bitcoin-only identity gates instead of two sequential ones.
BitHighlander
merged commit Aug 24, 2026
008dcd7
into
backport/7.15-security-to-alpha
18 checks passed
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.
Stacked on #533 as directed. Three things, in dependency order.
1. The compile failure that made #533's full ARM and dylib jobs red
Not
MAX_PAGESand not SRAM — it stops during compilation, before linking.fsm_msg_zcash.hcallsrng_health_check()andrandom_buffer_checked()(lines ~1225–26) without seeing their declarations. Root cause: release/7.15'sfsm.ccarries#include "keepkey/rand/rng_health.h"at line 39 and alpha's does not — the 3-way merge in #533 took the security changes but not that include. The emulator and unit builds compiled only because a different include ordering supplied the declarations.Fixed at the narrowest correct location:
fsm_msg_zcash.hitself, the file that uses the API. Depending onfsm.c's ordering is the fragility that caused this. Swept every other caller of those two functions — all declare it.2. The omitted a3da828 security follow-up
#533 carried
8661918and stopped, so8661918..a3da828was missing. Direction-ported with per-file classification, not cherry-picked:solana.h,fsm_msg_mayachain.h,fsm_msg_solana.h,fsm_msg_thorchain.hfsm_msg_zcash.h,unittests/firmware/zcash.cppBoth merges clean. "ours" was this candidate, not raw alpha — those two files already carried the security backport and the include fix, so merging against alpha would have reverted them.
Semantics verified present rather than assumed: ZIP-229 v6 empty Orchard digest +
ZTxIdOrchardH_v6; empty Ironwood +ZTxIdIronwd_H_v6; exactly-one send/deposit on THORChain and MAYAChain; transaction- and deposit-level memo review; authenticated KKSOLSC1 wired into opaque Solana review; blind-sign warning preserved after annotation. Prior work verified intact: the include,redpallas_sign_digest_with_ak,zcash_T[80],EmptyBundleDigests_MatchZip244, all sixRedPallasNonce_*.3. Seed/BIP-85 clipping — reworked to keep the transcript fixed
My first attempt was wrong and review caught it. Measuring the packing at the real width raised 24-word backups from 6 pages to 7 for 1.78% of seeds, and
reset.crunsfor (current_page < page_count) { confirm_constant_power(...) }where each call emits exactly oneButtonRequest. A seventh page was a seventh host request — and the host reads one word set per request, so that corrupts reconstruction. "More pages" and "unchanged transcript" were contradictory; raisingMAX_PAGESshipped the contradiction rather than resolving it.Reworked so the protocol boundary does not move:
MAX_PAGESback to 6 — no 3.7KB expansionBODY_WIDTH— the grouping is the boundaryconfirm_constant_power_paged()emits oneButtonRequestfor the group, then advances subpages locally. The seam:confirm_helper()/confirm_screen()do not emit requests — only public entry points do. Verified exactly onemsg_write()in the function. Subpages split only at row boundaries so a word is never divided across screens; intermediate subpages take a short press, only the last takes the hold; cancelling any subpage cancels the group; the scratch buffer isCONFIDENTIALand wiped.The measurement behind it
Exhaustive over every BIP-39 word × indices 1–24, real font tables:
N.word(20.mushroom)Two words per line is impossible in the worst case and trimming indentation cannot fix it. A random sample says 141,517/141,519 pairs fit — which would have led to a spacing tweak that still fails.
unittests/firmware/seed_display.cpppins all of this, registered in the build.What is NOT done
Stated plainly — do not merge on this description:
docker psexceeds 180s). CI is the authority; it is what caught the missing header.a710bb57/40da0906/ this head — outstanding.bb5e43e05+ binding regeneration — outstanding.