release(7.15): stage alpha onto develop with upstream-resolvable pins - #507
Open
BitHighlander wants to merge 290 commits into
Open
release(7.15): stage alpha onto develop with upstream-resolvable pins#507BitHighlander wants to merge 290 commits into
BitHighlander wants to merge 290 commits into
Conversation
First of three steps toward Taproot support. Adds nothing to the signing
path yet -- this lands the primitive and proves it against the spec.
- pin deps/crypto/trezor-firmware at the BIP-340 implementation
- compile bip340.c into trezorcrypto
- unittests/crypto/bip340.cpp: all 19 official BIP-340 vectors
Byte-exact signatures for the 8 vectors with secret keys, correct
rejection of all 10 must-fail cases (pubkey off-curve, pubkey >= field
size, has_even_y(R) false, sG - eP infinite with x(inf) as both 0 and 1,
sig[0:32] = field size, sig[32:64] = curve order), plus out-of-range
private keys and a check that the output buffer is zeroed on failure.
ROM cost measured with arm-none-eabi-gcc -Os -mcpu=cortex-m3 -mthumb:
1278 bytes of .text, no data, no bss.
Taproot is already further along in this tree than it looks: coins.def
carries taproot=true for Bitcoin and Testnet, segwit_addr.c selects
bech32m for witness versions above 0, and PAYTOTAPROOT outputs already
build and size correctly. What is missing is the input side, which the
next two steps cover:
2. BIP-86 output key tweak + SPENDTAPROOT in compute_address, which
removes the `return 0` at transaction.c:188 and unblocks GetAddress
for bc1p
3. BIP-341 sighash + the SPENDTAPROOT signing path + confirm UX
Depends on keepkey/trezor-firmware#5; the submodule pin points at that
branch and needs re-pointing at its merge commit before this lands.
The buffer-zeroing assertion in SignRejectsOutOfRangeKeys passed whether or
not bip340_sign() cleared the output, because sig started zero-initialised.
Pre-fill with 0xFF so it proves something.
Adds two cases:
- XOnlyPubkeyZeroesOnFailure, covering the matching contract now that
bip340_get_xonly_pubkey() zeroes on failure too
- ZeroSTakesTheSpecPath, pinning the ABSENCE of an s == 0 guard. s == 0
is in range per BIP-340 and must reject on the x-coordinate comparison
after computing R = -eP, not bail out early.
Bumps the crypto pin to pick up the guard removal and the restored Bitcoin
ABC copyright notice.
8/8 green.
Second of three steps. Removes the `return 0` at transaction.c:188 and
makes GetAddress return bc1p addresses for m/86' paths.
- compute_address() handles SPENDTAPROOT: tweak the x-only internal key
per BIP-86, then bech32m encode it at witness version 1
- path_mismatched() gains an m/86' branch, in BOTH copies (fsm_msg_coin.h
and coins.c, per the keep-in-sync note above them)
- three official BIP-86 vectors, driven from the published internal keys
and again end to end from the mnemonic
Two guards worth calling out, because both would have failed silently:
- taproot multisig is rejected up front. Without it the request fell
through to the p2sh branch and returned a p2sh address for a taproot
ask -- a wrong address, not an error.
- exactly 32 bytes are passed to segwit_addr_encode(), which only
length-checks the witness program for version 0 (segwit_addr.c:182).
Any other length would have encoded into a plausible-looking bc1p.
Also fixes a pre-existing bug in the PAYTOTAPROOT output gate. It tested
`!coin->has_taproot`, but has_taproot is the nanopb presence flag and every
coin in coins.def sets it -- only the `taproot` VALUE distinguishes them.
Bitcoin and Testnet have taproot=true; the other 41 coins have taproot=false
and were all passing the gate, building p2tr outputs for chains that cannot
spend them. Now tests the value.
Device build clean (MAKE_EXIT=0, no warnings, all variants). Retained cost
in firmware.keepkey.elf is 306 bytes; bip340_sign and bip340_verify still
garbage-collect out until step 3 references them.
11/11 unit tests green. Depends on keepkey/trezor-firmware#6.
Bumps deps/python-keepkey by one commit to pick up tests/test_msg_getaddress_taproot.py (BitHighlander/python-keepkey#28), which drives the emulator through the full SPENDTAPROOT GetAddress path and asserts the three official BIP-86 addresses. Verified against a locally built kkemu: 1 passed, and mutation checked so the assertions are known not to be vacuous. The test gates on firmware 7.16.0 and CMakeLists is 7.15.0, so it SKIPS until the project version bumps. Deliberate: gating at 7.15.0 would make released 7.15.0-rcN firmware without taproot fail rather than skip.
Third and last step: SPENDTAPROOT inputs are now signed.
- signing.c accumulates sha_amounts and sha_scriptpubkeys over every input.
BIP-341 commits to the amount and scriptPubKey of ALL inputs, not just
the taproot ones, which BIP-143 never required.
- signing_hash_bip341() delegates the SigMsg assembly to bip341_sighash()
in the crypto lib, so the field ordering is unit-testable against the
published vectors instead of only reviewable.
- the witness is a single 64-byte element. SIGHASH_DEFAULT omits the
trailing sighash byte; appending 0x00 would be a different signature and
would fail verification.
- signing keys go through bip340_tweak_seckey(), so the signature verifies
against the output key in the scriptPubKey rather than the internal key.
- transaction.c gains address_to_script_pubkey() and
fill_input_script_pubkey() to derive each input's scriptPubKey.
- taproot inputs are rejected on coins with taproot=false, matching the
output-side gate.
SRAM: +448 B bss, which is ABOVE the 256 B single-commit threshold that
tools/sram-budgets.json says needs explicit review -- flagging rather than
sneaking it through. It was +1072 B until the two new accumulators were
changed from Hasher to SHA256_CTX: BIP-341 fixes them to plain SHA256, while
a Hasher carries a union sized by GROESTL512_CTX and cost ~1.2 KB for no
benefit. The remainder is two SHA256 contexts, two 32-byte digests, the
tweaked key and one HDNode.
ROM: +2928 B text, 1900 B of it bip340/bip341. Nothing garbage-collects out
now that the signing path references it.
Device build clean, no warnings, all variants. Crypto validated against
BIP-341's published transaction: tweaked privkey, sigHash and the 64-byte
witness all match byte for byte.
NOT yet verified: signing on the emulator, and no OLED proof.
Emulator cross-check against an independent BIP-340/341 implementation
caught the device signing a valid signature over the wrong commitment.
BIP-143 hashes prevouts, sequences and outputs with DOUBLE sha256 --
curve->hasher_sign is HASHER_SHA2D for Bitcoin -- while BIP-341 specifies
SINGLE sha256. Reusing hash_prevouts/hash_sequence/hash_outputs was
therefore wrong in a way nothing self-consistent could detect: the
signature verified fine against the sighash the device computed, and that
sighash committed to a transaction nobody had authorised. Trezor's
reference keeps hash_prevouts143 next to hash_prevouts for this reason.
Adds the parallel single-sha256 set and points the sighash at it.
Also wires taproot through three classifiers it was missing, each of which
failed closed rather than silently:
- is_internal_input_script_type: a taproot input may carry address_n
- is_change_output_script_type: taproot CHANGE was being rejected
- the phase-1 dispatch, which routed only SPENDWITNESS/SPENDP2SHWITNESS
down the segwit path
and extracts prepare_input_node() from compile_input_script_sig(), so a
taproot input gets the same re-validation and derivation without building a
scriptSig it does not have. Skipping that would also have skipped the
guard that the host has not swapped address_n between phases.
Emulator: witness matches the independent implementation byte for byte.
Both device variants build clean, no warnings.
SRAM +856 B bss over the step-2 baseline, above the 256 B single-commit
threshold in tools/sram-budgets.json and flagged accordingly: five SHA256
contexts, five digests, the tweaked key and one HDNode. ROM +3216 B text.
Gate-3 OLED capture found the address verification screen silently truncating any bech32 address longer than one line. displayed bc1p5cyxnuxmeuwuvkwfem96lqzszd02n6xdcjrs20 actual bc1p5cyxnuxmeuwuvkwfem96lqzszd02n6xdcjrs20cac6yqjjwudpxqkedrcr 20 characters dropped, with no indication anything was missing -- and the QR beside it encodes the full address, so the two disagreed. A user "verifying" a receive address was checking two thirds of it. NOT a taproot bug. The threshold is ~42 characters at body font, so it already affects native segwit MULTISIG (p2wsh, 62 chars) on shipping firmware; p2wpkh is 42 and fits exactly, which is why it went unnoticed. Confirmed on the emulator for both p2wsh and p2tr. Cause: the address is drawn at TOP_MARGIN_FOR_ONE_LINE + font_height + ADDRESS_TOP_MARGIN = y=46 with line height 14 on a 64px canvas, so a second line starts at 60 and draw_char_with_shift() refuses to draw it -- it checks img->height + p->y <= canvas->height and returns false, and draw_string() then stops without reporting anything. Fix, in order of preference so the smallest lever is used first: - close the inter-line padding for multi-line addresses (14 -> 10) - only if that is still short, raise the block by exactly the overflow Raising alone was tried first and is wrong: the QR is drawn after the text and overwrites the start of a raised first line, which cost the first six characters. Closing the padding alone is also not enough -- the font is 10px, so two lines from y=46 need 66. Together they land the second line at y=54..64, clear of the QR at ~41. Single-line addresses are untouched: the whole branch is gated on calc_str_line() > ONE_LINE. Verified on the emulator: both 62-char addresses now render in full and legibly, the p2pkh confirm screen is unchanged, and the existing test_msg_getaddress_segwit suite still passes. Both device variants build clean with no warnings.
Lets a host ask the device whether it can derive and spend P2TR instead of inferring it from a firmware version. Version inference breaks the moment the feature is retargeted to a different release, and forces every client to carry a version table. Also unblocks the host side: Pioneer's taproot flag currently has no way to ask whether the connected firmware can verify a bc1p on screen or spend from one, so it has to guess. Bumps device-protocol for Features.supports_taproot (field 27; 19 and 20 are gaps with no reserved markers and are not safe to reuse against historical wire data), and python-keepkey for the regenerated bindings plus the tests that now gate on the capability rather than a version.
Deterministic signing is a choice a reviewer will question. It is spec-permitted, matches this firmware's RFC6979 ECDSA, and the nonce still depends on key and message so it is never reused across transactions. Fresh randomness would only add side-channel hardening, at the cost of making signatures unreproducible and therefore untestable against a published vector.
… host Response to the July 2026 COLDCARD incident (~1,367 BTC across 4,585 addresses). That was not a broken RNG: a board config left the hardware-RNG macro defined-but-zero, the supporting library tested only whether the macro was *defined* rather than enabled, and seed generation silently used the wrong source for five years. The substituted generator passed every statistical test -- it was simply seeded with ~40 bits -- so no amount of host-side entropy testing would have found it. Only the build configuration was wrong, and nobody could check. KeepKey is not exposed the way Coldcard was: reset.c mixes host entropy into the seed unconditionally (SHA256(int_entropy || ext_entropy)), so even a dead device RNG still yields a 256-bit seed, and random32() has no weak-PRNG fallback -- the emulator branch uses the host OS CSPRNG and aborts rather than degrading. This change hardens the two things the incident showed actually matter. 1. lib/rand/rng.c -- compile-time assertion on RNG *selection*. __arm__ comes from the compiler's own target definition, not from a board config or CMake option, so a mistaken -DEMULATOR cannot satisfy both conditions: firmware targeting the STM32 can only ever compile the RNG_DR path. Zero ROM, zero RAM. 2. GetEntropy is now auditable. It confirmed on every call, capped at 1 KiB, which made bulk RNG audits (bias tests, birthday/collision scans) impossible on real hardware -- so nobody ever ran one. Raise Entropy.entropy to 8 KiB and allow 64 KiB per boot without a press; the confirm returns once that budget is spent, and a replug refreshes it. Both are RAM-neutral: msg_resp and frame_arena.tx are already sized to MAX_FRAME_SIZE (12 KiB), and sizeof(Entropy) goes 1026 -> 8194, still under the existing _Static_assert. Seven messages already carry 2 KiB fields. The press was never protecting a secret -- the bytes are drawn fresh and discarded, never reused as key material, and the STM32 RNG is a free-running noise source rather than a seeded DRBG, so observing output reveals nothing about other draws. What it did buy is a cap on bias characterization: random32() returns RNG_DR raw with no whitening. A per-boot budget keeps that cap against a remote hostile host (which cannot replug) while leaving an audit ample room. Verified on kkemu via scripts/emulator/entropy-budget-check.py: 64 KiB collected in 8 x 8 KiB calls with no button press, all blocks distinct, and the next call correctly falls back to ButtonRequest.
Gate-3 OLED capture found the address verification screen silently truncating any bech32 address longer than one line. displayed bc1p5cyxnuxmeuwuvkwfem96lqzszd02n6xdcjrs20 actual bc1p5cyxnuxmeuwuvkwfem96lqzszd02n6xdcjrs20cac6yqjjwudpxqkedrcr 20 characters dropped, with no indication anything was missing -- and the QR beside it encodes the full address, so the two disagreed. A user "verifying" a receive address was checking two thirds of it. NOT a taproot bug. The threshold is ~42 characters at body font, so it already affects native segwit MULTISIG (p2wsh, 62 chars) on shipping firmware; p2wpkh is 42 and fits exactly, which is why it went unnoticed. Confirmed on the emulator for both p2wsh and p2tr. Cause: the address is drawn at TOP_MARGIN_FOR_ONE_LINE + font_height + ADDRESS_TOP_MARGIN = y=46 with line height 14 on a 64px canvas, so a second line starts at 60 and draw_char_with_shift() refuses to draw it -- it checks img->height + p->y <= canvas->height and returns false, and draw_string() then stops without reporting anything. Fix, in order of preference so the smallest lever is used first: - close the inter-line padding for multi-line addresses (14 -> 10) - only if that is still short, raise the block by exactly the overflow Raising alone was tried first and is wrong: the QR is drawn after the text and overwrites the start of a raised first line, which cost the first six characters. Closing the padding alone is also not enough -- the font is 10px, so two lines from y=46 need 66. Together they land the second line at y=54..64, clear of the QR at ~41. Single-line addresses are untouched: the whole branch is gated on calc_str_line() > ONE_LINE. Verified on the emulator: both 62-char addresses now render in full and legibly, the p2pkh confirm screen is unchanged, and the existing test_msg_getaddress_segwit suite still passes. Both device variants build clean with no warnings.
Self-review of the press-free path found a regression this PR introduced.
GetEntropy has no PIN gate and no initialization gate -- the button press
WAS the human gate. Dropping it unconditionally meant anyone holding a
locked, initialized device could harvest raw RNG_DR output silently, and
replug to refresh the budget and repeat.
The exposure is bounded (ECDSA nonces are RFC6979-deterministic, so bias
cannot weaken signatures, and the returned bytes are never key material),
but it is a real change in what a locked device does with no user present,
and it is not what the press-free path is for.
Restrict press-free collection to states where there is either nothing to
protect or a user demonstrably present:
- uninitialized: no seed exists yet. This is the case that motivated the
change -- auditing the RNG *before* trusting it to generate a seed.
- no PIN configured: nothing is locked, so the press guards nothing that
physical possession does not already defeat.
- PIN already cached this session: the user is right there.
An initialized, PIN-protected, locked device now falls back to the confirm
exactly as before this PR.
entropy-budget-check.py grows a case for it: load a seed with a PIN,
ClearSession, then assert a fresh-budget GetEntropy still returns
ButtonRequest. Verified the assertion has teeth by forcing the predicate
true and confirming the check fails (returns Entropy instead).
isCrossAccountSegwitChangeForbidden() enforced purpose/script-type agreement for BIP44, BIP49 and BIP84 change paths, but purpose 86' was never added when taproot support landed. A change output at m/86'/.../1/i declaring PAYTOADDRESS therefore skipped both mixed-mode guards and fell through to the generic path check in check_change_bip32_path(), which accepted it: same length, same account prefix, change chain, in-range index. The output was consequently treated as change, so its confirmation screen was suppressed, while transaction.c serialized it as P2PKH. A malicious host could route the entire change amount into a script that no BIP86 wallet scans for. The funds stay under the device's seed, but they are invisible to normal recovery until the path is derived by hand, and nothing was shown on the display. The reverse direction (44'/49'/84' claiming PAYTOTAPROOT) was already covered by the existing arms; this closes the one-sided hole. Rejecting here does not fail the signing session -- the output simply stops being treated as change and gets confirmed on screen like any other recipient. unittests/firmware/signing.cpp pins both directions. Verified failing without the new arm (make xunit -> Error 2) and passing with it.
The report generator has shipped a --screenshot-audit for a while: every catalog entry that DECLARES an OLED screen must have captured one. Nothing ever called it. Predictably it had rotted. Seven entries declared screens their test cannot draw -- five getaddress paths that return the address ON THE WIRE (the drawn one is the show_address sibling, catalogued separately), plus a metadata classification assert and an in-memory reference-vector check. No run ever said so, because no run ever asked. That is the whole bug class: an unrun gate is not a gate. It rots to the point where nobody trusts it, and then the release that actually stops drawing a confirmation screen sails past a check that was written precisely to catch it. So the wrapper runs it, between rendering and catalog validation -- render first so a failing candidate still leaves a truthful diagnostic PDF. It runs ONLY when the screenshots artifact arrived. That download is continue-on-error, and a flaked upload must not be reported as firmware that stopped drawing; it warns instead, which is the honest signal. Repins python-keepkey for the two report fixes: the seven bad declarations (#42), and the console breakdown that printed the run-wide skip count inside the catalog's own totals (#43).
SRS §3.4 said "NOT YET IMPLEMENTED -- this is the one genuine firmware build item in 7.15." It is built (#500, 146 added lines, no new crypto primitive). Rewritten as the requirement it now is, with the preimage spelled out -- "KeepKeySolanaTxAccounts/1" || sha256(raw_tx) || count || keys -- because sha256(raw_tx) inside it is load-bearing and invisible from the message definition. It is what stops one honest attestation being replayed to describe a transaction the provider never saw: the accounts would be real, and the transaction spending them would not be. Each of the three properties that follow names its test in section S. Exit criterion 1 is met. Criterion 4 had its second half pasted twice. New §6, the landing plan, because "ready to PR into develop" was an intention with no document behind it. alpha is AHEAD of 7.15 -- it carries 7.16 work -- so the cut is a selection, not a fast-forward, and until it is written down nobody can tell which. Seven changes go, in a stated order: the token budget lands directly after the defect fixes, because it frees the flash the rest spends and a ROM overflow found after five features have landed is a bisect through five features. §6.5 states the thing CI cannot close. Every display bound in this release is a claim about pixels, and the emulator's framebuffer is not the OLED. Gate 3 is hardware and it is still NOT PERFORMED -- the one item between a green develop and a signable candidate. Also corrects a comment I wrote in the report wrapper. It claimed nothing was calling the screenshot audit. Something was: python-keepkey-tests.sh runs it inside the emulator container right after capture. This second call is still worth its lines, but for a different reason -- the container gate answers "did the firmware draw?", and this one answers "does the PDF being shipped have the screens it claims?", which is a question about a DOWNLOADED artifact that nothing else asks.
feat(solana): KKSOLSW1 — show provider-attested lookup-table accounts
…erged #500 landed the firmware, so the atlas entries can follow. Held until now deliberately: S26-S29 catalog tests that gate on the KKSOLSW1 message, and repinning first would have put four entries in the catalog for a device that could not answer them -- four red rows describing a feature that was merely not merged yet. Brings in: #42 seven entries stopped declaring screens their test cannot draw (this is the gate that just failed #500's own run) #43 the console breakdown counted the run-wide skips, not the catalog's #44 distinct-test counting, and the assertion that the four numbers add up #45 S26-S29, and a version-aware MUST_RUN_MODULES
Turning the policy off left the provider in RAM. Every consumer in signed_metadata.c already refuses a runtime slot while AdvancedMode is off, so with the policy off the two behaviours are indistinguishable -- metadata fails closed either way. The difference is on the way back: re-enabling the policy brought the old signer straight back to VERIFIED with no second trust screen, so a user who disabled AdvancedMode to drop a provider had not dropped it, and the screen that re-armed it named the policy and never the signer. docs/security/clearsign-provider-tier.md already listed disabling AdvancedMode among the events that clear identities. It was right and the code was not: 7.15 is safe without a key-management programme precisely because trust dies on its own, and a revocation that only suspends is not a revocation. Four lines: clear the signers when the policy that authorized them is turned off. Re-loading costs one LoadClearsignSigner consent, which names the alias and fingerprint -- the screen that should appear whenever trust begins. Repins python-keepkey for I6, which now asserts the signer is GONE after the policy round-trip rather than documenting that it survives.
…ne item fewer R-5.1 was a decision deferred to 7.16 -- erase loaded signers on disable, or correct the tier document. It is answered in 7.15 by the four lines in this branch, so it is struck from SRS-7.16 rather than carried, and R-2.2 states the full list of events that clear an identity instead of listing three and footnoting the fourth as a deviation. Also wires DESIGN-716-reductive.md and TOKEN-TABLE-BUDGET.md into the roadmap's document list. Both were written and neither was reachable from the index a reader starts at.
Updates the guide for two changes to the report it describes. The header example carried the old numbers, from before the catalog's skip count was rebound to the run-wide census -- a breakdown quoting a population different from the one it was summarising. The guide now states the property rather than just showing numbers: the four counts add up to the total, the report asserts it before printing, and the total counts DISTINCT tests because a few are deliberately catalogued twice. And a note the seven bad screenshot declarations earned. An empty screenshot list already MEANS something in this format -- a refusal path whose evidence is the absence of a ButtonRequest -- so an entry empty for any other reason has to say why on the line, or the next reader cannot tell intent from omission. Plus the two invariants now asserted on every render, and what MUST_RUN_MODULES is for.
The verification table carried SRAM figures from before KKSOLSW1. Replaced with the measured ARM cross-build of the candidate: full 17,716 B, bitcoin-only 31,648 B, both against a 16,384 B floor, both PASS. The full variant lost 456 B and the row says why rather than absorbing it: fsm_msg_solana.h flattens the nanopb array into a contiguous lut_keys[SOL_MAX_LUT_ACCOUNTS][SOL_PUBKEY_SIZE] so the attested keys can be hashed. SRAM on this part is spent once, and an unexplained 456 B is the kind of thing that only surfaces when the NEXT feature does not fit. Also records the token budget as the build actually applied it -- 350 of 1378 and 150 of 568, the numbers the ARM build prints -- rather than as an intended cap.
C27 and test_msg_getentropy have described this policy since 2026-08-03, CI has been setting KK_EXPECT_ENTROPY_BUDGET=1 to enforce it, and no firmware ever implemented it. fsm_msgGetEntropy confirmed unconditionally, so the test failed the moment the suite ran far enough to reach it -- which it had not, because earlier failures were aborting the run first. It is the only red on alpha. The policy, exactly as the atlas states it: initialized device -> confirm, always uninitialized, budget left -> serve press-free, spend the bytes uninitialized, budget spent -> confirm again Why a press-free budget exists at all: telling a working hardware RNG from a stuck or grossly biased one needs a bulk sample, and a button press per 8 KiB turns the vault's pre-PIN health check into an eight-press ceremony -- which users learn to click through without reading, so the presses buy nothing and cost the check its credibility. Why it is safe: an uninitialized device holds no seed and no secret, so raw RNG output discloses nothing. The instant it holds one, storage_isInitialized() puts the press back. Both halves are the test, not just the comment. Two details that are load-bearing: - the budget is denominated in BYTES, not requests, so a caller cannot buy more of it by asking for a bigger chunk; - a wipe resets it, because a wipe returns the device to exactly the state the budget exists for. Without that, a device initialized once could never be RNG-audited again without a press per chunk. 64 KiB, which is the 8 x 8192 the test spends and then proves exhausted.
fix(rng): implement the entropy audit budget C27 has been asserting
First half of structured EIP-712: the part that decides what bytes get hashed and what characters get drawn. The walk and the FSM handler follow; this lands alone because it is independently testable and it is where being wrong is silent. Ported from OneKey firmware-classic1s (ethereum_typed_data.h @ 885e51d3, LGPL-3.0-or-later, carrying the Trezor copyright chain). Attribution is in both file headers. What came across is the encodeData rules and the leaf validation -- the parts that are the SPEC rather than one device's arrangement. What did NOT come across is the memory design, and it could not have. OneKey declares a ~31 KB TypedDataEnvelope on the stack; this device has 17,716 B of SRAM reserve above a 16,384 B linker floor, so their schema store alone is about 1.8x our entire runtime SRAM. Their 1S is a GD32F470VK with 256K; we have half that plus a 32K .confidential reserve. Three things here are deliberately stricter than the code we replaced: 1. Integer widths must be canonical AND declared. eip712.c accepted a bare "uint" and treated it as 256 bits, which is not EIP-712 -- and it hashed the host's spelling verbatim, so "uint0256" produced a type string no verifier reproduces while looking identical at OLED resolution. 2. Values are raw big-endian bytes of the declared width. The old path parsed decimal with strtoll and refused anything above 2^63-1, which is every unlimited ERC-20 approval ever issued -- the single most common permit there is. There is now no ceiling and no decimal-to-binary step that could disagree with what the host meant. 3. Strings reject control bytes as well as malformed UTF-8. An embedded NUL is how bytes past a terminator get signed and never drawn, which is the exact defect 7.14.2 closed for message signing; overlong encodings and surrogates break the same injectivity property from the other direction. The user's consent rests on two different strings being unable to draw the same screen. Arrays are spelled from array_levels in written order -- int16[2][][4] -- so encodeType is assembled from the wire description rather than from a recursive type the host could shape. 21 gtest cases, including keccak vectors for the dynamic forms and the unlimited-approval case that the old encoder refused.
The type graph, and the bug it exists to fix. encodeType(S) = seg(S) || seg(D1) || seg(D2) || ... where D1..Dn is every struct S transitively references, SORTED BY NAME. eip712.c appends referenced definitions in DISCOVERY order. There is no sort call anywhere in that file. So a document naming two structs out of alphabetical order hashes a type string no compliant verifier reproduces -- and nothing on the device can notice, because the device is internally consistent in exactly that case. Display matches hash, hash matches nothing. That failure is worse than the one 7.14.2 withdrew the path for. "Displayed is not hashed" produces a signature over the wrong thing on THIS device. "Everything agrees on-device, and disagrees with the world" produces a signature that is either worthless or valid for a document nobody meant. There is now a test for it -- ReferencedStructsAreSortedByName, a schema naming Zebra before Apple -- which is the differential the design review asked for before changing parseType. It is a genuine bug fix, not a tidy-up. Nothing is stored. Each segment streams into a keccak context as it is fetched, so only the closure's NAMES are held: EIP712_MAX_STRUCTS x 48 bytes. Cyclical schemas terminate on the already-present check rather than recursing -- EIP-712 leaves them undefined, and a signing device's answer to undefined is "do not hang". The lookup is a CALLBACK. Firmware backs it with the streaming state machine; the tests back it with a fixture table, which is what makes the type graph testable without an emulator. Five vectors: the canonical spec Mail/Person example, the sort canary, Permit2 PermitSingle nesting PermitDetails (the exact payload a flat-structs-only design cannot sign), a missing struct, and a cycle. Budget note. The digest pool and frame stack come down to 16 slots and depth 4 after checking what the arena can lend. frame_arena_scratch2049 cannot be borrowed here: it is a UNION with the rx and tx frame buffers, and this walk sends and receives while holding state, so it would be corrupted by the very round trips it depends on. Only .bss counts against the linker gap -- that gap IS the stack -- so the transient SHA3_CTX costs nothing against it.
The build failed with:
/kkemu/include/messages-ethereum.pb.h: pb_callback_t address_n;
/kkemu/include/messages-ethereum.pb.h: pb_callback_t primary_type;
/kkemu/include/messages-ethereum.pb.h: pb_callback_t struct_name;
/kkemu/include/messages-ethereum.pb.h: pb_callback_t array_levels;
/kkemu/include/messages-ethereum.pb.h: pb_callback_t name;
pb_callback_t forbidden. missing .options entry?
Two mistakes, one of them documented in our own new-message checklist and made
anyway.
1. The bounds went into deps/device-protocol/messages-ethereum.options. The
BUILD reads include/keepkey/transport/messages-ethereum.options --
lib/transport/CMakeLists.txt:27 names it explicitly. The submodule copy is
the canonical protocol definition and is not consulted when generating
firmware headers, so every field arrived unbounded and nanopb refused to
emit a static struct.
2. EthereumSignTypedData.address_n had no max_count at all, in either file.
Every other message declares its own (EthereumSignTx.address_n max_count:8);
there is no global default to inherit.
Both files now carry the bounds, so the canonical definition and the one the
firmware compiles against agree. Nested messages take the fully-qualified
Outer.Inner.field form, as BinanceTransferMsg.BinanceInputOutput.* already does.
Verified by generating in kktech/firmware:v8 rather than by pushing and hoping:
zero pb_callback_t across the five new messages, and
EthereumTypedDataStructAck_size = 6048, comfortably inside MAX_DECODE_SIZE
(13 KB) that fsm.c static-asserts against.
…signers fix(policy): disabling AdvancedMode revokes loaded clear-sign signers
An adversarial review of this code found the canary weaker than its commit message claimed. I called it "the differential that proves the sort". It is not. With two dependencies discovered as [Zebra, Apple], reversing the discovery list produces [Apple, Zebra] -- the same as sorting it. So an implementation that merely reverses passes. The review also found three of the four typeHash tests still pass with the sort deleted outright, because they have at most one dependency and order cannot be observed. Three dependencies separate the three hypotheses: discovery [Bravo, Charlie, Alpha] reversed [Alpha, Charlie, Bravo] SORTED [Alpha, Bravo, Charlie] <- the only correct one Two more cases the closure has to survive: - TRANSITIVE reference. Aardvark is reachable only through Zulu, and must sort among the dependencies rather than trailing the struct that introduced it. - ARRAY-ONLY reference. A struct reached only as Person[] must still be collected: an array member carries data_type STRUCT with array_levels set, so a collector keying off the dimensions instead of struct_name would drop it. Trezor fixed exactly this in 2.5.1. The lesson is about the test rather than the code. A canary whose expected and buggy outputs coincide is not a canary, and "the test passes" said nothing about whether the sort worked. Three plausible implementations now give three different answers.
The typeHash tests built their expectation by keccak-ing a type string written
inside the test. The implementation builds the same string. So a shared
misreading of encodeType would agree with itself and go green -- the tests
could only ever prove the code is consistent with me, never that it is
consistent with the world. That is exactly the failure mode this whole effort
exists to avoid, sitting in the tests meant to catch it.
Replaced with literals published by independent references:
Mail/Person typeHash
a0cedeb2dc280ba39b857546d74f5549c3a1d7bdc2dd96bf881f76108e23dac2
from assets/eip-712/Example.js in ethereum/EIPs -- the reference
implementation the spec itself links to. Republished by Example.sol in the
same directory, by MetaMask eth-sig-util's hashStruct snapshots for V3 AND
V4, and by Mrtenz/eip-712.
EIP-2612 Permit typeHash
6e71edae12b1b97f4d1f60370fef10105fa2faae0126114a169c64845d6126c9
published as a literal constant by Circle in the deployed USDC
FiatTokenV2_2 source (contracts/v2/EIP2612.sol), and independently computed
by OpenZeppelin's ERC20Permit.
The keccak-of-a-string assertion stays alongside the literal, so a failure says
which half diverged: if both fail the closure is wrong, if only the literal
fails my type string is wrong.
Also lands the walk's session state and sizes its constants to a MEASURED
budget rather than an assumed one. Built for ARM locally in the pinned Docker
image: _ebss 0x2001b2c4, _stack 0x2001f7f8, gap 17,716 B against the linker's
16,384 B floor, so 1,332 B to spend. Depth 2, 8 slots, 3 structs, 32-char names
fit inside that with margin.
One thing that measurement CANNOT yet show, and it matters: the module is
currently unreachable, and the build uses -Wl,--gc-sections, so every byte of
that state is collected and _ebss has not moved. The real cost is unmeasurable
until the walk is wired into the FSM dispatch. Nobody should read the unchanged
gap as evidence this fits -- it is evidence the code is not linked in yet.
The device can now sign structured EIP-712. Domain first, then the message, under one session, hashing each leaf in the same call that displays it. RESUMABLE, because it has to be. KeepKey has no blocking request/response primitive -- wait_for_tiny_msg is a 64-byte channel for ButtonAck and PinAck, and a StructAck is 6 KB. OneKey drives its walk from a re-entrant call() that pumps usbPoll() from inside a handler; that cannot be transplanted. So each handler emits one request and returns, and the next Ack resumes the machine. The walk NEVER writes a message. RESP_INIT uses msg_resp, which is private to fsm.c, so the machine describes what it wants next (Eip712Next) and three thin FSM handlers emit it. That was forced by the compiler and it is better design: key material and the wire stay out of the walk, which is what lets the whole machine be unit-tested. The member_path is rebuilt from the frame stack rather than maintained alongside it, so the cursor and the stack cannot drift apart. SRAM, measured rather than assumed, in the pinned Docker image: before _ebss 0x2001b2c4 gap 17,716 margin 1,332 first attempt _ebss 0x2001b80c gap 16,364 LINK FAILED, 20 bytes short now _ebss 0x2001b77c gap 16,508 margin 124 The first attempt did not link: "Insufficient runtime SRAM: require 16 KiB stack/heap reserve". The gate caught it in two minutes on this laptop, before CI, before review, before hardware. What went to pay for it: the session-wide typeHash memo. A typeHash is needed exactly from the moment its frame is entered until that frame folds, so it lives in the frame now -- 64 bytes instead of 192. A struct appearing twice is hashed twice, which costs round trips, and round trips are the thing this device has to spare. 124 bytes of margin is thin, and worth saying out loud rather than burying: the next feature that touches .bss has to measure before it writes. Arrays are refused for now, with a message that says so. Permit2 PermitSingle, EIP-2612, DAI permit and EIP-3009 all sign without them; PermitBatch and Seaport do not. A refusal sends the host to the AdvancedMode-gated hashed path, which is what every typed-data payload gets today, so nothing regresses. Behind AdvancedMode. Structured display is strictly more information than the blind path it replaces, but this is new parser surface reachable from a website, and it stays gated until there is hardware evidence behind it.
Arrays work. PermitBatch and Seaport-shaped documents now walk instead of being refused, and the margin is BETTER than it was without them. The SRAM came from the right place, which was not where I first looked. Cutting the token table further does nothing: `tokens` lives at 0x080c0e74 in section T, i.e. FLASH. The 8,000 bytes the 500-entry budget saved were never SRAM, and the linker gate measures _stack - _ebss. MAX_DECODE_SIZE is what had slack. 13 KB provisioned for a worst-case message, against a largest new message of 6,048 B. Reduced to 12 KB: arrays + 13 KB buffer gap 16,180 B FAILS the 16,384 B gate by 204 B arrays + 12 KB buffer gap 17,196 B margin 812 B (no arrays, 13 KB) gap 16,508 B margin 124 B 10 KB does NOT compile, so the true floor is between 10 and 12 KB and this is the last kilobyte available without shrinking a message. That bound is enforced, not assumed: fsm.c static-asserts sizeof() of every registered inbound message against MAX_DECODE_SIZE, so a message that outgrows it fails the build rather than overflowing at runtime. The walk itself: - An array's LENGTH is requested before its frame is pushed, so the member_path the device sends still points AT the array rather than into it -- which is precisely the path whose value is the length. - What an element IS comes from the type, never from anything accompanying the value: another array while dimensions remain, a struct if the leaf type is one, otherwise a leaf. - A FIXED dimension is checked against the length. It is part of the type string and therefore part of typeHash, so accepting a different count would sign a document whose type declares another -- and nothing downstream could notice, because the count is the only thing the device is told. - An empty array hashes as keccak of no bytes, which is what EIP-712 says and what a zero-length struct array needs. - Arrays fold WITHOUT a typeHash prefix, per the spec: enc(array) is the keccak of the concatenated element encodings, nothing more. Element counts are bounded by the slot pool and refused past it, so a long array is a clear failure rather than a corrupted hash.
…e2e test Brings in the python client that drives the walk, the protoc-3.5.1 bindings for messages 1704-1708, and test_msg_eip712_streaming.py -- four cases against real firmware, including the device's own hashes matching the values published by the EIP-712 reference implementation.
Three findings, reproduced locally rather than read out of a CI log. cppcheck, zero-warning policy: - "Condition 'shown != len' is always true". The hex renderer truncated to 64 bytes and then checked whether it had truncated, which is convoluted on its face and unreachable-false on the path cppcheck analyses. Replaced with a buffer sized for a WHOLE leaf: confirm_helper paginates a long body across screens, which is the exact-byte disclosure rule 7.14.2 established, so nothing signed is cut off the screen and no long-but-valid value is refused. ~2 KB of stack inside a 16 KB stack, and stack is not what the linker gate measures. - Two frames only ever read through their pointer; both now const. clang-format: I broke include/keepkey/firmware/fsm.h when adding the three handler declarations. Reformatted. messagemap.def is also unformatted, and is left alone: it was already that way on alpha, it is a column-aligned macro table that clang-format would mangle, and CI only checks *.c and *.h. Re-verified after the change: ARM links at the same _ebss (0x2001b4cc, 812 B of margin), cppcheck silent, formatting clean, and the four emulator tests still pass -- including the device's own hashes matching the published spec values.
The firmware I built and flashed came off this branch, which was cut from alpha BEFORE #502 (entropy audit budget) and #501 (AdvancedMode revokes signers) landed. So the device was running the original unconditional-confirm GetEntropy, and the press-free budget looked broken on hardware when it simply was not present.
Carries the hardware evidence from the 2026-08-21 K1-14AM session into the report: nine screens correct, 42-character addresses rendered in full, and the published EIP-712 hashes matched on silicon.
cppcheck constVariablePointer. The pointer is never written through, and saying so is worth the line -- fold_frame is the one place that turns a completed frame into a hash, and it has no business mutating it.
SortIsNotMerelyReversedDiscoveryOrder never ran until now. It sat behind static-analysis, and a failed Stage-1 gate SKIPS the whole downstream graph -- so the run summary looked clean while the test was red. When it did run it failed, and not for the reason its name suggests: the encoder REFUSED. The test used three dependencies, but the closure holds EIP712_MAX_STRUCTS names INCLUDING the primary type, so M plus three dependencies is one wider than the device accepts. Three dependencies are not needed. Discovering TWO in alphabetical order separates sorted from reversed just as well, because reversal is then the one thing that gets it wrong: discovery [Alpha, Bravo] reversed [Bravo, Alpha] SORTED [Alpha, Bravo] The two canaries are now complementary and neither is redundant: Zebra/Apple catches "no sort at all", this one catches "reversed". Verified by injecting a reversal into sort_closure_tail -- Zebra/Apple PASSES under it, which is precisely why it was not enough on its own, and the new canary fails. Also added RefusesADocumentWiderThanTheClosure, which pins the capacity limit as a tested contract rather than an undocumented edge. A truncated closure would still produce a well-formed 32-byte typeHash -- one no verifier reproduces -- so refusing is the only safe behaviour and now something a test asserts. 28/28 Eip712Stream tests pass.
feat(eip712): the encoder core for device-driven structured signing
alpha is 287 ahead of develop and 0 behind, so this is a fast-forward in content -- no contested files, and none of the reconcile machinery in ALPHA-MERGE-HANDOFF.md applies. That machinery was written for the 7.14.2-vs-7.15 divergence, which no longer exists. SUBMODULE PINS RETARGETED, which is the whole reason this is a merge commit and not a fast-forward. alpha pins fork masters, by design. develop cannot: a PR into upstream carries its pins, and a reviewer cannot resolve a commit that exists only on a fork. Per docs/release/BRANCHING-SOP.md the legitimate shapes are a commit already on the upstream submodule's master, or the head of an OPEN upstream PR. These are the second kind -- the dress-rehearsal pins: deps/device-protocol 0f050d61ec93 head of keepkey/device-protocol#112 deps/python-keepkey f9849f67f4f6 head of keepkey/python-keepkey#197 Both branches were brought up to date on UPSTREAM first, so the pins resolve for a reviewer today rather than after some future merge: - device-protocol: up/release-protocol carried two build fixes that exist only upstream (the cimg/node:20.11 CI pin, and moving the zcash file options into the preamble for pbjs). Merged, not rebased, so nothing was force-pushed over an open PR. Verified afterwards that both survived and that the EIP-712 messages came across. - python-keepkey: reconcile/upstream-sync was a pure fast-forward, 0 upstream-only commits against 47 on the fork. deps/crypto/trezor-firmware stays at cdc05bebe9e6: its .gitmodules URL is the fork, and that commit is contained in the fork's canonical `keepkey` branch, so it already satisfies the rule. 7.16 is deliberately NOT here. clearsign_root and the passkey work are open PRs against alpha and stay there; this branch is 7.15 only, and the staging was taken BEFORE landing them for exactly that reason.
BitHighlander
force-pushed
the
fix/715-develop-staging
branch
from
August 22, 2026 06:13
30c991f to
99ee71a
Compare
The previous pin carried a storage-version gate that asserted STORAGE_VERSION == 20 and the literal "case StorageVersion_18:". Both are true on the 7.16 passkeys branch and both are FALSE here, where STORAGE_VERSION is 17 and nothing is burned -- so develop was pinning a test suite guaranteed to fail against its own firmware. 006142da70e4 derives the ladder, the burned set and LAST_SHIPPED from the tree under test instead. Verified green on both lines from one file: 10 passed / 5 skipped here, 15 passed on the 7.16 tree. It also carries the integration-CI repair, which matters for a release branch: that job had been ending "cancelled" at exactly 30 minutes with zero assertions run, behind a check that reported success. It now finishes in 2m46s with 636 passed, 32 skipped, 0 failed, against an emulator built from current firmware rather than a five-month-old published image. Still the head of the open upstream PR keepkey/python-keepkey#197, which was fast-forwarded to this commit first, so the pin stays resolvable for an upstream reviewer.
BitHighlander
force-pushed
the
fix/715-develop-staging
branch
from
August 22, 2026 06:53
99ee71a to
9bafd99
Compare
…release
YAGNI triage of the 7.15 release diff. Nine documents come out; one is
carried forward in reduced form. No source file, no test, and no
CI-invoked script is touched, so this carries no build or coverage risk.
Removed, with what each cost:
1602 clearsign-key-delegation-roadmap.md Phases 0-3 of the DELEGATION
tier. Zero lines of it are implemented here -- signed_metadata.h
defines only LEGACY 0x01 and SCHEMA 0x02. It is 7.16's, and it
stays on alpha.
499 rc28-open-findings-handoff.md A working record: per-PR
state, "RC28 is not merge-ready", a build recipe. See below.
425 zcash-on-device-ua.md Header says "design -- not
yet implemented". Zero inbound references.
297 zcash-clearsign-handoff.md Session handoff from
2026-05-20 that also leaked local /Users paths into a
develop-bound PR. zcash-pczt-clearsign.md carries the same threat
model as product doc and is kept.
153 next-wave-hardening.md "Status: proposed", zero
inbound references anywhere.
146 docs/zoo/reports/zcash-report.md Its only referrer was
zcash-on-device-ua.md, also cut. Removes the whole docs/zoo tree.
144 DESIGN-716-reductive.md Titled "7.16".
138 SRS-7.16.md "Depends on 7.15.0 shipping".
98 SRS-7.17.md "Status: outline".
The rc28 handoff was NOT a plain deletion, because shipping code points
at it -- rng_health.h:110 and storage.c:578 both cite it to explain why
RNG coverage is opt-in rather than wallet-wide. Two sections are carried
into docs/security/rng-coverage-scope.md: the coverage-scope statement,
and "STOP -- hard gate on the NEXT BOOTLOADER RELEASE", which records
that the bootloader draws its stack canary through random32(). Both
citations repointed; nothing in lib/ or include/ still names the removed
file.
That STOP section is a FORWARD gate, and the carried doc now says so.
It was written when random32() consulted the RNG verdict and could
abort. That was descoped: in this tree random32() does no such check,
tools/bootloader/main.c is byte-identical to develop, and the checked
path is confined to storage_drawKeyMaterial(). The hazard needs BOTH the
checked path made default AND a bootloader cut, and 7.15 is neither.
Eleven inbound links repaired across four surviving docs so nothing
dangles.
NOT cut, though they looked like candidates: the merge gates
(DEFECTS-2026-08.md names merge_direction_gate.py as the remediation
that CLOSED D-09), check_pallas_ct_disassembly.py (invoked from
ci.yml:479, an RC18 blocker), the anti-rollback RFC (one link in a chain
reaching live code at storage.h:163), clearsign-provider-tier.md (it IS
this release's scope statement), and token-table-retirement.md
(referenced from a pinned submodule's source).
Known and deliberately not repaired: generate-test-report.py in the
pinned python-keepkey cites "SRS-7.16 R-4.1, R-4.2". Repairing it needs
a repin, and repinning during a release cut is the worse trade.
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.
Stages the 7.15 release from
alphaontodevelop, so the upstream PR can follow.Why this is a merge commit and not a fast-forward
alphais 287 ahead ofdevelopand 0 behind, so the content is a fast-forward — no contested files, and none of the reconcile machinery inALPHA-MERGE-HANDOFF.mdapplies. That was written for the 7.14.2-vs-7.15 divergence, which no longer exists.The merge commit exists to retarget the submodule pins.
Submodule pins — the dress-rehearsal shape
alphapins fork masters, by design.developcannot: a PR into upstream carries its pins, and a reviewer cannot resolve a commit that exists only on a fork. Perdocs/release/BRANCHING-SOP.mdthe two legitimate shapes are a commit already on the upstream submodule's master, or the head of an open upstream PR. These are the second kind:deps/device-protocol0f050d61ec93deps/python-keepkeyf9849f67f4f6deps/crypto/trezor-firmwarecdc05bebe9e6keepkeybranch; its.gitmodulesURL is the forkBoth upstream branches were brought up to date on upstream first, so these pins resolve for a reviewer today rather than after some future merge:
up/release-protocolcarried two build fixes that exist only upstream: thecimg/node:20.11CI pin, and moving the zcash file options into the preamble sopbjscan parse them. Merged rather than rebased, so nothing was force-pushed over an open PR. Verified afterwards that both survived and thatEthereumSignTypedData/EthereumTypedDataStructAck/EthereumTypedDataValueAckcame across — a clean auto-merge keeps hunks from both sides, which is exactly how one side quietly loses. All 18.protofiles parse underprotoc.reconcile/upstream-syncwas a pure fast-forward: 0 upstream-only commits against 47 on the fork.7.16 is deliberately not here
clearsign_rootand the passkey work are open PRs againstalpha(#506, #504) and stay there. This branch is 7.15 only, and the staging was taken before landing them for exactly that reason — merging either intoalphafirst would have forced a cherry-pick instead of a merge.Verified:
git ls-tree -r origin/alpha | grep -c 'clearsign_root|passkey'→ 0, whileeip712_streamis present.Verification
Built from this tree with the retargeted pins resolved:
_stack - _ebss= 17,188 B against the 16,384 B gate (+804).protoparseCI on this branch is the authoritative run.
What this PR does not claim
No hardware run. Gate 3 (OLED screenshots) has not been attempted against this exact tree — the 7.15 clear-signing screens were verified on hardware earlier from the same content on
alpha, but not from this staging commit.