backport(security): the 7.15 audit fixes into alpha - #533
Merged
Conversation
alpha carries the fork's full integration history and had NONE of the security
work done on release/7.15 -- every one of EMPTY_ORCHARD_DIGEST,
redpallas_sign_digest_with_ak, CHECK_NOT_BITCOIN_ONLY_LOCKED and
rng_seed_error_latched was absent.
WHAT THIS IS NOT. release/7.15 is 28 commits ahead of alpha, but 9 of those are
SQUASHED, release-shaped re-packagings of work alpha already carries unsquashed
-- alpha is 400 commits ahead of release/7.15. Merging the branch would have
touched 288 files and replaced alpha's own history with squashed equivalents.
That is precisely the failure this repo has already had once, where a merge went
green while 24 of 79 files were taken wholesale from the wrong side because the
gate checked symbol existence rather than resolution DIRECTION.
So this backports the 19 security commits only, and every file was classified
before being touched:
15 files alpha identical to the base the fixes were built on -> taken whole
6 files alpha diverged -> 3-way merged with git merge-file
1 pin deps/python-keepkey -> deliberately NOT touched (see below)
Of the six 3-way merges, four applied clean and two conflicted:
fsm.c alpha's CHECK_NOT_INITIALIZED predates the lock test.
Resolved to 7.15 after verifying, with whitespace
normalised, that every alpha line survives in it.
fsm_msg_zcash.h alpha's side was EMPTY -- a pure addition of the checked
80-byte T draw. Resolved to 7.15.
deps/python-keepkey is left alone on purpose: alpha's pin has 54 commits that
7.15's lacks and 7.15's has 794 that alpha's lacks. Dragging either direction
would silently discard work. Alpha's pyk has no Zcash PCZT device tests, so
nothing there conflicts with the firmware changes.
WHAT LANDS. The RedDSA nonce fix (via the crypto repin to the canonical keepkey
tip), the v6 unverified-Orchard-bundle theft path, rk validation, Zcash session
teardown on every path including auto-lock and PIN failure, hash contexts
holding seeds and keys, blank amounts on overflow, the multisig out-of-bounds
write, the bitcoin-only lock on four handlers, the RNG seed-error latch, and the
CI gate that could not fail.
VERIFIED. Zero conflict markers, clang-format clean, Pallas boundary gate
passes, shell syntax clean. The diverged files show small ADDITIVE diffs
(+20/-2, +11/-1, +26/-4) -- alpha's own content preserved with the security
changes layered on, which is the shape a correct resolution produces. Handler
gating re-checked per function body: ChangePin, ChangeWipeCode, ApplySettings
and ApplyPolicies gated; WipeDevice deliberately not, because wiping is the way
out of the lock.
NOT IN SCOPE. The 7.14.3 feature reconciliation is a separate, larger problem:
develop carries diverged IMPLEMENTATIONS of features alpha already has -- 253
lines of signing.c, 135 of transaction.c, a 500-line bip340.cpp, storage and
build-variant work. alpha has its own PAYTOTAPROOT and STORAGE_VERSION_BTC_ONLY.
That needs its own per-file direction audit and must not ride along here.
7.14.2 needs nothing: it is already fully contained in alpha (0 files differ).
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.
Owner
Author
Correction: two commit messages cite the wrong issue numbersRecording this here rather than opening new issues, per the upstream-freeze handoff ("do not open additional public security issues"). Two findings in this backport were low severity. The bulk issue-filing run only filed critical/high/medium, so no issue was ever created for them — but the commit messages cite numbers anyway, and both are wrong:
The fixes themselves are unaffected and are described in full in their own commit messages. Both findings are recorded here so the trail is not lost: Bitcoin-only lock (low). A locked bitcoin-only wallet resets the RAM shadow at boot, so handlers that persist settings run normally: the setter updates the shadow, RNG seed-error latch (low, but it weakens a security control). The remaining commit references in this branch (#511, #513, #514, #517, #519, #527, #528) were checked against the issue list and are correct. |
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.
fix(display): seed/BIP-85 clipping, the a3da828 follow-up, and the missing rng_health dependency
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.
alpha carries the fork's full integration history and had none of the security work done on
release/7.15. Every one ofEMPTY_ORCHARD_DIGEST,redpallas_sign_digest_with_ak,CHECK_NOT_BITCOIN_ONLY_LOCKEDandrng_seed_error_latchedwas absent.Why this is not a branch merge
release/7.15is 28 commits ahead of alpha — but 9 of those are squashed, release-shaped repackagings of work alpha already carries unsquashed. alpha is 400 commits ahead ofrelease/7.15. A branch merge would have touched 288 files and replaced alpha's own history with squashed equivalents.That is exactly the failure this repo has had once already: a merge went green while 24 of 79 files were taken wholesale from the wrong side, because the gate checked symbol existence rather than resolution direction.
Verified the distinction rather than assuming it — alpha already contains the Group-A feature content (
zcash_orchard_compute_cmx,PAYTOTAPROOT,STORAGE_VERSION_BTC_ONLY,rng_health_check,signed_metadata_clear_signers).Method
Every file classified before being touched:
git merge-filedeps/python-keepkeyFour of the six 3-way merges applied clean. The two conflicts:
fsm.c— alpha'sCHECK_NOT_INITIALIZEDpredates the bitcoin-only lock test. Resolved to 7.15 after verifying, with whitespace normalised, that every alpha line survives in it.fsm_msg_zcash.h— alpha's side was empty; a pure addition of the checked 80-byteTdraw. Resolved to 7.15.deps/python-keepkeyis left alone on purpose. alpha's pin has 54 commits 7.15's lacks; 7.15's has 794 alpha's lacks. Dragging either direction silently discards work. alpha's pyk has no Zcash PCZT device tests, so nothing there conflicts with these firmware changes.What lands
RedDSA nonce fix (via the crypto repin to the canonical
keepkeytip) · v6 unverified-Orchard-bundle theft path ·rkvalidation · Zcash session teardown on every path including auto-lock and PIN-failure · hash contexts holding seeds and keys · blank amounts on overflow · multisig out-of-bounds write · bitcoin-only lock on four handlers · RNG seed-error latch · the CI gate that could not fail.Verification
Zero conflict markers · clang-format clean · Pallas boundary gate passes · shell syntax clean.
The diverged files show small additive diffs (+20/−2, +11/−1, +26/−4) — alpha's content preserved with the security changes layered on, which is the shape a correct resolution produces. A wrong-side resolution would show large replacements.
Handler gating re-checked per function body:
ChangePin,ChangeWipeCode,ApplySettings,ApplyPoliciesgated;WipeDevicedeliberately not, because wiping is the documented way out of the lock.Not in scope
7.14.3 feature reconciliation is a separate, larger problem.
developcarries diverged implementations of features alpha already has: 253 lines ofsigning.c, 135 oftransaction.c, a new 500-linebip340.cpp, plus storage and build-variant work — while alpha has its ownPAYTOTAPROOTandSTORAGE_VERSION_BTC_ONLY. That needs its own per-file direction audit and must not ride along here.7.14.2 needs nothing — already fully contained in alpha (0 files differ).