release candidate: stage 7.16 after 7.15 - #621
Merged
Merged
Conversation
…ings The previous revision said all four code findings were closed. Two were not, and the head hashes for #366 and #368 were a commit behind. Both reopened findings were the same mistake wearing different clothes -- a fix correct as far as it went, summarised as if it went further. #366 gated a list of call sites and called it "every key-material draw", missing the Orchard RedPallas nonce in a submodule nobody re-audits. #369 removed the certificate's three-way field inconsistency and called the result "canonical" while every offset after 0x02 was still a '*'. Neither was a coding error; both were scope claims outrunning the work, and both were found by reading the claim against the code rather than against the diff. That is now stated at the top, because it is the most useful thing in this document for whoever reads it next. Also recorded: the #367 self-test passed throughout the period its gate was inoperative, because it exercised the one input shape in which the bug is invisible. A green check on a gate is evidence about the test, not the gate. Heads, statuses, the #369 blocker table (8 downgraded to proposed, 5 widened to gate both phases) and the test counts are updated to match.
…ing it
Three follow-ups from review.
**Cross-contamination.** The script hashed whatever KPKY images it found, so a
directory holding both variants -- which is exactly what create-release's
merge-multiple download produces -- would write the bitcoin-only image into the
full variant's manifest. It now requires exactly the image this invocation
names, firmware.keepkey.v${VERSION}${SUFFIX}.bin, and refuses any other
application image rather than silently including it.
**Truncation.** head -c stops at EOF without complaining, so an image shorter
than its own descriptor's 256+codelen was hashed over fewer bytes and published
as though whole. Now compared against the file size and refused.
**Overstatement.** The header said the structural check "proves the unsigned
binary was not published". It proves the canonical unsigned DRAFT -- zero
indices, zero signature area -- was not published, and nothing more: a region
holding a single non-zero byte passes it. The wording now says that, in the
header and at has_quorum.
Recorded while narrowing it: the five signing public keys are already in
include/keepkey/board/pubkeys.h, so real verification is not blocked on
obtaining them. It needs a host-side secp256k1 verifier over sha256 of the
image, which nobody has written. That is the outstanding dependency, and it is
now written down where the next person will look.
Two merge blockers, neither caught locally because the emulator build accepts both and I never cross-compiled. **_Alignas.** The ARM toolchain rejects it on this declaration, so full and bitcoin-only both failed at storage.c. Switched to __attribute__((aligned(4))), which is the form the rest of the tree already uses (fsm.c msg_resp, usb.c buffers). Verified by actually building: both variants now produce firmware.keepkey.bin and bootloader.bin in CI's own image, kktech/firmware@sha256:7438e539. **Catalog.** 280f3b6 renamed the storage test to PinKdfRewrapsToActiveVersionAfterCorrectPin without updating the pinned python-keepkey report catalog, which still required the old name, so catalog validation failed after the suite itself passed. Repinned to python-keepkey 417a613, which fixes K8 and adds K8b for the reboot regression. Also, per review: storage_setPin_impl now captures storage_rewrapPinKdfVersion() ONCE into a local and uses that same value for both the derivation and the pin_kdf_v2 flag. The helper is pure today, so calling it twice is harmless today -- and coupling the persisted description to the wrap actually produced, rather than to a second call, is the invariant this whole branch is about.
…unting **BLOCKER 9 — nothing bootstraps.** The proof session required "a certificate the device has already verified", while §6 acceptance requires satisfied freshness. A factory-fresh device, or one returning from bitcoin-only with no established freshness, has no certificate that passes acceptance, so it can never start the session that would establish the freshness acceptance demands. Split the word: AUTHENTICATED is encoding + root signature + device/domain bindings; AUTHORIZED is that plus epoch, freshness, usage/scope and transaction binding. Freshness proofs may reference an authenticated certificate, which is sound because every value a proof depends on is already covered by the root signature -- authentication alone fixes the constraints the host would otherwise choose. Warning-free rendering requires authorized. Note this loosens nothing: an authenticated-only certificate can drive the ratchet forward, and forward only ever REDUCES authority. **BLOCKER 10 — the global height throws the work witness away.** Each proof is checked against its own certificate's expiry_min_work, but what is committed is one global height that then evaluates every certificate. So a proof under a weak certificate advances the height far enough to expire a certificate whose higher threshold nothing ever demonstrated, and whoever holds the weakest policy ever issued sets the real cost of advancing everyone's clock. Still fail-closed for authorization; what it destroys is the cost bound that made permanent clear-sign DoS expensive. Three options stated, not resolved, plus: cap the committed height at the work-qualified checkpoint rather than crediting every accepted header, because the headers past that point are free to produce. Smaller: the root signature must be low-S canonical, or one certificate has two valid encodings and therefore two cert_hash values -- and cert_hash covers the signature. §9 no longer claims certificate encoding is settled, which contradicted §6's own "proposed, not ratified". Phase 3's release gates gain one test per new blocker.
Three defects in the inversion, all found by review because I never cross-compiled and reported CI as "running" instead of green. **Both ARM variants failed to link**, and so did board-unit and crypto-unit: `undefined reference to random_uniform`. rng.c called trezor-crypto's, which made kkrand depend on trezorcrypto -- and GNU ld resolves static archives left-to-right in one pass, so whether that linked came down to archive order. It had been resolving by accident; removing rng.c's random_buffer reference stopped dragging rand.o in early and the accident ended. Four lines of rejection sampling now live in rng.c, so kkrand depends on nothing. **crypto-unit failed on layout_warning_static**, for the mirror-image reason: rng_health.c reached UP into kkboard, which is listed BEFORE kkrand everywhere. rng_health_require() now halts with abort() and the module includes no board headers at all. Firmware paths that have somewhere to report -- reset_init, U2F registration, the OTP block -- still render a proper error by calling rng_health_check() first; abort() is the backstop for draws with no UI context, which is exactly the dependency draws it exists for. **memcmp_s pulled the fatal path into the bootloader by the back door.** It filled decoys with random_buffer_raw() and then shuffled them with the CHECKED permutation, and the bootloader verifies signatures through memcmp_s(). Decoy ORDER is a timing-equalisation detail exactly like decoy CONTENT, so it now uses random_permute_char_raw(). Recovery-cipher and PIN-matrix shuffles stay checked. **The default path did not feed the continuous test.** Only random_buffer_checked() folded bytes into rng_continuous, so ordinary draws -- RedPallas, ECDSA blinding, SecAESSTM32 -- enforced the boot verdict and nothing else, and a source going degenerate after the gate would not have been noticed by the RCT or APT that exist to notice exactly that. random32() now calls rng_health_observe() on every checked draw. Two tests cover it: a stuck run observed after the gate latches the verdict, and the next plain draw dies. MEASURED, not asserted, in CI's own image (kktech/firmware@sha256:7438e539): both ARM variants and the bootloader build; and my earlier "no ROM" claim for the bootloader was wrong. Bootloader text +784 bytes, firmware text +1408. The bootloader edge is trezor-crypto's generate_k_random, an ECDSA SIGNING nonce that gc-sections keeps but the bootloader never calls -- it verifies. So there is no reachable fatal path there, but there is real ROM, and against a 256 KiB partition holding ~103 KiB that is affordable rather than free.
…able Round-2 review reopened both code PRs. #368 failed both ARM builds on _Alignas and then failed catalog validation on a test name renamed in 280f3b6; #366 failed every link on random_uniform and layout_warning_static, and its continuous RCT/APT state was never fed by the default path. So the previous claim that #369 was the sole reason RC28 is not merge-ready was wrong, and both are now listed as reopened-and-fixed rather than closed. The header now carries the pattern instead of a fourth round of individual corrections, because it has repeated three times in different shapes: work correct as far as it goes, reported as if it went further. The instance worth acting on is the last one -- "verified locally" meant an emulator build, and an emulator build proves nothing about shipping firmware. _Alignas compiles under clang and is rejected by the ARM toolchain; a missing link edge only appears under the archive ordering the ARM target uses. Neither is visible from cmake -DKK_EMULATOR=ON. There is a mechanical fix rather than a resolution to try harder, so it is now in the header AND the build recipe: a four-minute Docker cross-compile in CI's own image, both variants, no toolchain install. Every ARM claim in this document was reproduced that way before being written down, including the ROM delta that corrects the earlier "no ROM" claim for the bootloader. Also records that #367's structural gate is not blocked on obtaining signing keys -- pubkeys.h already has all five; it needs a host-side secp256k1 verifier.
The header explains why; §5 is where someone actually looks before running anything.
Three defects, and one scope correction. **The triggering draw was still returned.** random32() observed the word and returned it regardless, so only the NEXT call aborted -- and random_buffer() is built from four-byte draws, so a run tripping on the final word handed back the whole degenerate buffer first. For a RedPallas nonce that is the disclosure this gate exists to prevent, delivered by the gate itself. rng_health_observe() now returns whether these very bytes tripped the test, and random32() aborts before returning them. The regression forces the raw source so the trip happens on a chosen draw; it fails if the abort is removed, which is the only way to prove the triggering word never escapes. **The firmware could not boot with a dead RNG.** signatures_ok() runs at keepkey_main.c:177, BEFORE kk_board_init(), and reaches the gate through ecdsa_verify_digest -> curve_to_jacobian -> generate_k_random. A device whose generator died would abort at boot with no display, and could not be used to move funds out either -- RFC6979 signing needs no entropy at all. Fixed at the right layer, in the crypto fork (7a4c3464): blinding draws raw. With USE_RFC6979 those callers are blinding only, never a nonce, so a weak draw degrades masking and discloses nothing. Verified from the built ELF: the firmware's only caller of checked random32() is now random_buffer(). **lib/rand was never in my clang-format sweep** -- CI's file list does not cover it and neither did I. Formatted. **Scope: every bootloader and blupdater edit is reverted.** tools/bootloader/main.c is byte-identical to develop again. A firmware release does not update anyone's bootloader, so a reachable gate in a bootloader we are not building-and-shipping cannot affect an RC28 user -- and carrying bootloader changes in an RC means they ship months later, reviewed under this deadline rather than their own. The bootloader's stack canary therefore still draws through checked random32(). **That is a hard gate on the next bootloader release, recorded in the RC28 handoff.**
#366 made random32() abort on a failed RNG verdict, and the bootloader's stack canary draws through it. A device with a dead RNG would abort inside the bootloader: no verify, no boot, no reflash. Unrecoverable, in the component that exists to recover from everything else. Deliberately NOT fixed here. A firmware release does not update anyone's bootloader, so it cannot reach an RC28 user, and fixing it in an RC28 PR means bootloader changes reviewed under a firmware deadline that ship months later. All bootloader edits were reverted; tools/bootloader/main.c is byte-identical to develop. Records both prototyped fixes and the objdump check to confirm it, because 'nothing calls it' has already been wrong twice in this module.
7.15 ships the narrow gate this PR was originally scoped as. The inverted
default -- random32() checked, so every consumer including deps/ inherited it
-- is removed, along with the crypto-submodule patch it needed.
Why it was descoped rather than fixed again:
- generate_k_random() is blinding ONLY while USE_RFC6979=1. The macro that
made it draw raw would, under a different configuration, control the actual
ECDSA signing nonce. rng_health.c opens by warning against trusting a
build-configuration macro to have the value its name suggests -- that is
the Coldcard lesson -- and this reintroduced exactly that.
- The raw path did not deliver what it promised anyway. generate_k_random()
loops `while (bn_is_zero(k) || !bn_is_less(k, prime))`, so a source stuck
at zero never leaves it. A dead-RNG device would hang rather than abort:
less diagnosable, no more recoverable.
- Making a dead-RNG device genuinely bootable and spendable needs a defined
degraded-RNG recovery mode across firmware and bootloader crypto. That is a
project, not an RC patch.
RedPallas stays uncovered. That is develop's existing behaviour, not a
regression introduced by 7.15, and it is now stated in the header rather than
implied.
WHAT THIS SHIPS. Six draws opt in by name via random_buffer_checked(): the
device half of the seed, the storage encryption key, the wipe-code key, the
PIN-KDF salt on the V1 upgrade path, the U2F key-handle path, and the one-shot
OTP randomness block. The first four halt on failure; U2F returns NULL and the
OTP write is skipped, leaving the block unlocked for a later healthy boot.
Kept from the wider work: the corrected RCT/APT (independent state, exact
cutoffs), the latched per-boot verdict, and the triggering-byte protection --
random_buffer_checked() observes the bytes it just drew and wipes them if THEY
tripped the test, rather than returning the output the test just rejected.
Coverage is OPT-IN and the header says so, listing every covered site. Two
earlier revisions of this branch described it as wallet-wide; both were wrong.
deps/ and the bootloader are untouched -- 0 files each. Bootloader text is
102656 bytes, identical to develop.
Verified: both ARM variants build in CI's image; firmware-unit 341,
crypto-unit 18, board-unit 5, RngHealth 15; clang-format 20 and cppcheck
clean; trezor-crypto's ecdsa.c compiles standalone under CI's -Werror set.
The build produces bin/bootloader.bin, this workflow copied it into release/, the rename turned it into bootloader.v<ver>.bin, and release-assets/* attached it. So tagging firmware published a bootloader as a side effect. A bootloader is a separately signed artifact with its own rollout and its own review. Shipping one because someone tagged a firmware RC is how a device gets bricked by an image nobody reviewed as a bootloader release -- and 7.15 has an explicit decision not to release a new bootloader at all. The copy is removed, so it cannot be renamed, hashed or attached, and the asset-preparation step now fails loudly if a bootloader ever reaches release-assets/ again rather than quietly publishing it.
…ranch died Coverage/RedPallas/degraded-RNG-recovery are one project, because each answer changes the others. All three were built during RC28 and descoped. The part worth keeping is why inverting the default failed, since it is not obvious and someone will try again: ECDSA blinding runs on the VERIFY path, so a checked draw aborts signatures_ok() before the display exists; making blinding raw does not help because generate_k_random() rejection-loops forever on a stuck source; and the raw patch is only safe while USE_RFC6979=1, since the same switch otherwise controls the real signing nonce. The prerequisite is a degraded-RNG recovery mode across firmware and bootloader crypto -- a device that still boots, still verifies, still signs with RFC6979 to move funds out, and still refuses to mint new key material. fix/blinding-draws-raw is deleted rather than left unmerged, so nobody repins to it on the strength of a commit message the loop condition disproves.
fix(storage): revert the flash format to V17 for the 7.15 release
fix(release): say what each published hash is for
feat(rng): gate wallet creation on a seed-time RNG self-test
A signed upgrade must never wipe. A downgrade wiping is correct, and stays
that way.
An upgrading device arrives carrying a blob written by the release it is
leaving. If the incoming firmware does not recognise that version,
version_from_int returns StorageVersion_NONE, storage_fromFlash returns
SUS_Invalid, and storage_init runs storage_reset + storage_commit. No prompt,
no warning, wallet gone at boot.
Exactly two edits cause that, and both compile clean today:
1. Lowering STORAGE_VERSION below a version that has shipped.
2. Removing, reordering, or skipping an entry in storage_versions.inc, so a
version that used to be recognised no longer is.
Neither is caught by any functional test, because tests create storage with
the firmware under test and never cross a release boundary. So both are now
static assertions:
STORAGE_VERSION >= STORAGE_VERSION_LAST_SHIPPED (new, in storage.h)
StorageVersion_##LAST == LAST
The second works because the enum is emitted in .inc order after
StorageVersion_NONE = 0, so a contiguous 1..N list makes StorageVersion_N == N.
Delete entry 5 and StorageVersion_17 becomes 16. It is checked on the last
entry because that is the only one whose value depends on every entry before
it. Both were verified to fire against the real .inc.
Compile-time and zero runtime cost. Disarming either one now means editing a
constant that documents why not to, which makes it a review item instead of an
accident.
docs/StorageVersionGate.md carries the audit SOP: the mechanism, the review
procedure for storage-touching changes, and the separate bootloader wipe path
(should_restore), including the consequence that an unsigned RC build cannot
prove the preserve path at all.
Users are being shown a digest and reasonably ask what it is worth. It proves capture: the rolls are ASCII '1'-'6', the digest is SHA256 over exactly rolls_needed bytes, and the first 8 are displayed. Anyone who wrote their rolls down can recompute it and confirm the device recorded that sequence, in order, with none dropped or substituted. It does not prove mixing. dice_mix is a separate step and neither int_entropy nor the mixed result is ever displayed, so firmware that showed a correct digest and skipped the mix would be indistinguishable from the honest one. That is deliberate, and the reasoning belongs next to the feature rather than buried in a source comment: displaying the mixed entropy as a "verifiable commitment" is strictly worse, because a host supplying ext_entropy and reading that screen once computes SHA256(shown || ext_entropy), the seed pre-image. There can be no host-side verifier for the mixing step. One would need the device to disclose seed-derived material, which is the disclosure the design refuses; a verifier that instead reports "the device says it mixed" relays a claim from the component whose honesty is in question, and manufactures false assurance. The chain is the digest, the published source, and the firmware hash, and the middle link is not delegable.
The contiguity check asserted StorageVersion_##LAST == LAST, on the reasoning that the last entry is the only one whose value depends on every entry before it. That is true, and it is not enough: the last entry's value pins the entry COUNT, not the entry VALUES. Renumbering ENTRY(16) to ENTRY(99) keeps 17 entries, so StorageVersion_17 is still 17 and the build is clean -- while version_from_int loses `case 16` and every device carrying storage version 16 is wiped on upgrade. That is exactly the failure this check exists to stop, and it walked straight through. Asserting on every entry closes it. STORAGE_VERSION_LAST falls back to STORAGE_VERSION_ENTRY inside the .inc, so defining ENTRY covers the last entry too and the check reads the same. Found by mutating the .inc and confirming the build still succeeded, rather than by reading the assert. Five mutations now rejected, baseline unchanged: lowering STORAGE_VERSION, deleting a middle entry, renumbering an entry, truncating the tail, and swapping two entries.
The RFC required that a power loss leave the device able to boot either the previous or the new image. The device cannot do that, and no firmware change makes it able to. The application is a single slot: sectors 7-11, 640 KiB total. A 7.15 image is ~568 KiB, so two resident copies need ~1.14 MB. A/B slots do not fit. Verifying before erase is unreachable for the same reason -- there is nowhere to stage a candidate, ~192 KiB of RAM cannot buffer a 568 KiB image, and the signature covers the whole image so it cannot be checked until the last byte arrives, by which point handler_erase has already cleared sectors 7-11. So the invariant is withdrawn and replaced with what actually happens. An interrupted update leaves the device recovery-only: no bootable application, but the bootloader is untouched and can always accept another image, and the application magic is installed only after verification so a partial image is never bootable. Storage survives under the usual should_restore() conditions. The update state machine is reordered to match: the epoch check is the only candidate check that can precede the erase, because the epoch is declared in metadata while hash and signature cover an image that has not arrived yet. Dual-slot is recorded as a hardware-revision requirement rather than a firmware backlog item, because leaving it in a backlog implies someone could pick it up. A spec that overstates a guarantee is worse than one that admits a limitation: integrators plan around the promise, and audits find the gap.
… cost Decided in one pass, biggest risk first. Three of the ten close by removing scope rather than solving it, and the document now says so rather than leaving a reader to infer it. D1 custody -- 1-of-1 on a stock signed KeepKey. The finding stands as written: one compromise yields globally warning-free false interpretations remediable only by a firmware release. What makes it defensible is recorded next to it -- DR is a seed restore rather than a new problem, rotation stays cheap until an anchor is pinned, and with a single root the compensating control is visibility (a published signing log) rather than quorum. D2 freshness -- descoped. clearsign_freshness was the only field that did not fit the OTP counter, and it existed to expire delegates. Expiry is replaced by epoch ordering: monthly rotation, revocation is rotating early. Monthly over a 256-step unary counter is ~21 years, so the substrate needs no journal, no generation binding, no wear budget and no power-loss state machine. Blockers 6 and 10 close with it. The residual risk is stated for the audit -- a device that never sees a newer epoch keeps accepting an old delegation indefinitely -- together with why it is tolerable here: the per-tx signer is server hosted, so a device using the delegated path is connected by definition and the epoch bump travels the same channel. D3 authority -- distinct keys, domain-tagged transcripts, context binding, and negative tests as a merge gate. Delegate certificates and per-tx blobs carry different tags so a stolen blob cannot be presented as a delegation. Firmware signing is unchanged and cannot be tagged retroactively; tagging the new authorities is sufficient because crossing them would need a preimage collision. D4 updater -- recovery-only, already amended in the anti-rollback RFC. D5 blind-sign policy -- session-scoped. This is the precedent signed_metadata.c already set when it rejected persistent trust anchors in the same unauthenticated section, it avoids a storage version bump, and it makes reset, variant-change and corrupted-policy behaviour trivially "off". During 7.15 testing a persistent AdvancedMode was left on and silently converted four suites' "rejected correctly" results into false greens, which is the usability argument for the same answer. D6 certificate -- ratified at 169 bytes, freshness fields removed rather than reserved, since format_version already refuses anything not exactly known. Sections describing work that will not be built now carry a DO NOT IMPLEMENT banner instead of being deleted. The analysis is why the decision went the way it did, and it is what has to be re-read if anyone proposes reinstating it.
hash-manifest.sh --require-signed is a STRUCTURAL gate. It proves the canonical unsigned draft was not published; a signature region holding a single non-zero byte passes it. It cannot detect a placeholder and it cannot detect a forgery. Until now nothing checked the actual signatures before a release shipped -- the first real verification happened in a user's bootloader. This mirrors lib/board/signatures.c:signatures_ok() exactly: magic, three key indices in range and mutually distinct, digest over image[256 : 256+codelen], and each 64-byte compact signature verified against the pubkey its index selects. Keys are parsed out of include/keepkey/board/pubkeys.h at runtime rather than copied here, so a key rotation cannot leave this verifying against a stale set. Uses `ecdsa`, which python-keepkey already depends on -- no new dependency. High-S signatures are reported, not rejected: the device accepts either form, and v7.10.0's second signature is in fact high-S, so enforcing canonicality here would reject a genuine shipped release. --self-test signs throwaway fixtures and asserts the checks fire, including the two that matter: a signature by the WRONG key, and a non-zero placeholder -- the exact case the structural gate waves through. Verified against real artifacts: v7.10.0 reports SIGNED with keys 1/2/3, and an unsigned development image is rejected on sig_index1 = 0.
AdvancedMode is the blind-sign gate -- 16 call sites across ethereum, eos,
solana, ton, tron, the clearsign attestor and signed_metadata read it. It lived
in the PUBLIC storage section, which has no authenticated integrity against
physical flash modification. That is the same section RC18 refused to persist
clear-sign trust anchors in, for the same reason, and signed_metadata.c says so
in a comment. A physical attacker flipped one bit and every "this contract
cannot be decoded, refusing" check was gone. No signature, no forgery, one bit.
So the policy is now session state: never written, never restored, off at every
boot, and off again whenever the device locks. Re-arming costs another on-device
confirmation.
Six changes, and two of them are the ones that matter:
* Both readers pass false for AdvancedMode instead of `flags & (1u << 12)`, and
both writers emit 0 for that bit. Bit 12 is now burned -- documented in
policy.h, because firmware <= 7.15 still reads it as the policy, so reusing it
would make a downgraded device read the new field as "blind signing on".
* storage_readStorageV1 no longer trusts the legacy policy record. This was a
LIVE BYPASS of everything above: for storage versions 2-10 the record supplied
policies[0]'s NAME from flash, storage_upgradePolicies only fills indices from
policies_count upward, and storage_isPolicyEnabled_impl returns on the first
name match scanning from index 0. A crafted record naming itself "AdvancedMode"
with enabled=1 was answered before the real entry at index 3 was ever reached.
Nothing is lost by discarding it: the only policy it could legitimately name is
ShapeShift, which every V11+ reader already forces to false and which has no
storage_isPolicyEnabled consumer anywhere. Removing the call also closes a
1-byte over-read (storage_readPolicyV1 guards len < 17 then reads ptr + 17) and
a strcmp over-read (memcpy fills all 15 bytes of policy_name with no NUL).
* storage_fromFlash reports SUS_Updated when the retired bit is still set, so
storage_init commits once and the writer scrubs it. Ignoring the bit is not
enough -- <= 7.15 honours it, so a downgrade would boot with blind signing
already on. It cannot be left to the next incidental commit either: a device
with no PIN may never make one, because storage_init's storage_isPinCorrect("")
returns PIN_GOOD rather than PIN_REWRAP once sca_hardened and v15_16_trans are
set, which 7.14 and 7.15 both guarantee.
* session_clear_impl disarms it on lock, gated on clear_pin. The gate is
load-bearing: clear_pin separates a real lock (screensaver, ClearSession,
recovery) from a soft re-init. fsm_msgInitialize calls session_clear(false),
and python-keepkey's apply_policy() itself calls init_device() straight after
applying -- so an ungated disarm would clear the policy inside the very call
that set it, and demand a fresh button press before every host operation. The
runtime signers survive Initialize only because the host can silently reload
them; a physical confirmation cannot be reloaded.
* read_u32_le reads through unsigned char. Pre-existing, and load-bearing here:
`char` is signed on x86, the flags word's byte 0 is always >= 0xC0 because bit
7 (Pin Caching) is hardcoded set, so the old read returned 0xffffffc0 --
and storage_writeStorageV17 read-modify-writes that same word, persisting the
corruption and SETTING bit 12. ARM defaults to unsigned char so shipping
firmware was unaffected, but plain `char` signedness is implementation-defined
and must not be relied on either way.
* The ApplyPolicies confirm now states the lifetime. "Enable policy?" reads as
permanent, and this screen is the only place the device says what enabling
costs.
Tests: three, all mutation-tested by reverting the corresponding source change
and confirming they fail. AdvancedModeIsNeverRestoredFromFlash drives all three
readers (V11 and V16 are separate call sites on live migration paths, so a
V17-only test would let a regression through) and asserts the scrub;
LegacyPolicyRecordCannotNameAdvancedMode is the crafted-record attack;
SessionClearDisarmsAdvancedMode asserts BOTH directions of the clear_pin gate.
StorageUpgrade_Normal's ShapeShift assertion moves from true to false: that value
came from the legacy flash record, and every V11+ reader already forced it false,
so the migrated true never survived the first commit. It was transient and
inconsistent with what the next boot would see.
Verified: 424 native tests pass; ARM cross-compiles in CI's pinned image for both
variants (.text 619900 full, 333232 bitcoin-only, 35,460 bytes free in the app
slot); clang-format-20 clean on all three C/H files; cppcheck reports 0 findings
on storage.c under CI's exact invocation.
…iggers
Gate 3 evidence for this PR, captured from the emulator rather than a phone:
python-keepkey drives ApplyPolicies with KEEPKEY_SCREENSHOT=1 and writes the
actual OLED bitmap, so the proof is reproducible in CI instead of being a
photograph nobody can regenerate. The emulator runs the same layout and font
code as the device.
Looking at the capture changed the wording. The first draft said the policy
turns off "when you unplug the device" -- true, but incomplete now that it also
disarms on lock (session_clear_impl, gated on clear_pin). The omission runs in
the surprising direction: lock the device, come back, and blind signing is off
with nothing having said it would be.
Enable AdvancedMode policy? It turns off when the device locks or is
unplugged.
79 characters, and it renders in 2 lines rather than 3. The previous wording sat
at exactly BODY_ROWS with zero headroom, where one more word would have flipped
it to two pages and a second ButtonAck -- a host-protocol change from an edit
that looks purely cosmetic. This buys that headroom back.
Evidence and the capture script live under
docs/security/evidence/session-scoped-advanced-mode/, matching the convention
the clearsign attestor evidence already uses.
424 native tests still pass.
feat(release): verify firmware signatures host-side, before publishing
…y-state docs(anti-rollback): withdraw a guarantee the hardware cannot provide
storage: make a wiping upgrade a build failure
…ed-mode fix(policy): AdvancedMode is session state, not a flash bit
The emulator native libs are one deliverable on two platforms, but CI treated only the macOS dylib that way, and only on alpha. Two gaps: 1. The Windows DLL could vanish silently. The cross-build was non-blocking and the publish job tolerated a missing DLL, so a macOS-only publish was a success by construction. That tolerance then hid a total loss: the publish job (2c14200) is not an ancestor of any current branch — it was dropped in a branch reset — so the rolling prerelease has served a stale libkkemu-win-x64.dll since 2026-06-30 while every run stayed green. 2. The libs only ever went to a rolling alpha prerelease. develop is the release line and tagged releases carried no emulator assets at all, so there was no way to run a specific RC in the emulator. Restore it as a coupling, on the release line: - Cross-build the DLL in python-dylib-tests, BLOCKING. A regression now fails the job instead of silently narrowing the release. - Stage both binaries under their published names and upload them as one artifact, gated on both paths being set, so a dylib-only artifact is never produced. - publish-emulator-libs publishes that pair to the rolling prerelease on pushes to develop OR alpha, asserting both assets first. Uses `gh release upload --clobber` so the release body stays stable. - release.yml attaches the same two files to every tagged release. They are NOT rebuilt: the tagged commit's CI run already built and tested them, and validate/ already required that run to be green, so the published binaries are the tested ones. Both present or the release fails. create-release gains actions:read — reading the CI run's artifacts needs it, and a declared permissions block zeroes every scope it omits. Windows still needs no runner: the DLL is cross-compiled from the same macOS runner via MinGW-w64.
Docs only. No code change proposed for 7.15.0.
Clear-signing's premise is that the host supplies asset meaning at transaction
time. Once that works the built-in ERC-20 table is a shipped snapshot of a list
that is wrong the day it compiles. This records what it costs, what it actually
buys, and the one ordering constraint that must not be missed.
Cost, measured rather than estimated (compiled the generated .def files into
one TU on a 64-bit host, read section sizes, converted the struct array to
ARM32 — the string pool is pointer-size independent):
struct array 1944 x 16 B 31.1 KB
string pool measured 41.1 KB
lookup code ~0.5-1 KB
---------
~72 KB
For scale, ROM reclaim #339 (printf, -24.2 KB) and #340 (blake2b, -12.1 KB)
recovered 34 KB combined. The compile-out path is already proven —
ethereum_tokens.h:29 sets TOKENS_COUNT 0 for the bitcoin-only image.
What it buys today: 1924 of 1944 rows are Ethereum mainnet. Base, Arbitrum,
Avalanche and Gnosis have ZERO rows, because the generator can only emit
networks listed in ethereum_networks.json and those are not in it. A USDC
transfer on Base therefore renders "Unknown token value" — no amount, no
ticker.
Ordering constraint, and the reason this is a doc and not a patch:
tokenByChainAddress returns UnknownToken on a miss, NOT NULL, while the
blind-sign gate tests `token == NULL`. So an unrecognised ERC-20 transfer
skips the AdvancedMode gate that an unrecognised CONTRACT CALL hits — the less
identifiable case gets the more permissive treatment. Deleting the table sends
every token transfer down that path, so the asymmetry must be fixed BEFORE
phase 3, not after.
Phasing:
1. prove tx-time injection — NO firmware change, runs on shipped rc firmware
(ERC-20 is not contractHandled, so data_needs_confirm stays true and the
metadata block at ethereum.c:795 fires; LoadClearsignSigner and
EthereumTxMetadata already exist in hdwallet). Caveat recorded: a loaded
signer is annotation-only, so "Unknown token value" still follows the
injected screens. Phase 1 proves the pipe, it does not remove UNKN.
2. curate to top-N + stables across all supported chains — 200 rows reclaims
~64 KB and gives Base/Arbitrum coverage for the first time.
3. delete the table — blocked on the delegation custody decision in
clearsign-key-delegation-roadmap.md, not on engineering.
Also records two build-hygiene findings: keepkeylib/eth/ethereum-lists is an
empty submodule in a fresh checkout and ethereum_tokens.py treats a missing
directory as "no tokens" and returns silently, so a clean build would
regenerate the table with ~0 rows and nothing would fail. The table is not
reproducible from a clean tree today, which makes any size claim about it
unverifiable in CI.
rc29 pinned python-keepkey at 417a613, which was NOT on the canonical branch (keepkey/python-keepkey PR #197, reconcile/upstream-sync). rev-list gave 0 5: the canonical branch was fully contained, but rc29 sat five commits past anything under review, reachable only from a fork feature branch (fix/catalog-pin-kdf-rename). Those five are now merged into the canonical branch as keepkey/python-keepkey #212 (merge 3e68c862), together with two fixes they needed: - test_reset_device_pin / test_failed_pin asserted the post-7.15 reset flow (no Internal Entropy screen, fw 320f0eb) with NO version gate, so they FAILED on any older emulator. They were red the whole time and nobody saw it, because the commits were never in a PR. Now branch on firmware version rather than skipping, so the PIN / EntropyRequest / mnemonic-derivation coverage keeps running everywhere. Both pass on the 7.10 CI emulator. - the zoo report counted SKIPPED tests as 'executed' (613 collected, 252 skipped = a 41% overstatement in the document used to audit a release). Now tracks skips separately and says 'collected'. Also adds test_ethereum_eip_1559_multibyte_chain_id, the missing regression for fw ed6db16. It SKIPs on the current CI emulator (pre-7.15) and is inert until that moves up — which the report change above now makes visible rather than hiding. Pin only; no firmware source change. Those five commits touch only scripts/generate-test-report.py and two test files, never keepkeylib/, so the rc29 BINARY was never affected — firmware CMake executes deps/python-keepkey/keepkeylib/eth/*.py at build time, which is why that distinction matters. What was affected is the evidence: the zoo report for rc29 was generated by tooling outside the canonical PR.
Independent alpha audit remediation: signing, storage, entropy, and CI evidence
feat(solana): certify Relay LUT routes for 7.16 alpha
Simulate the post-7.15 release state by merging the current alpha candidate after the exact upstream #462 tree. Alpha supplies the 7.16 code and storage V20 boundary; the upstream signature-verification checklist and two-product build documentation are retained.
This was referenced Aug 26, 2026
# Conflicts: # include/keepkey/firmware/signing.h # lib/firmware/signing.c # unittests/firmware/signing.cpp # unittests/firmware/transaction.cpp
13 tasks
BitHighlander
changed the title
BitHighlander
marked this pull request as ready for review
August 26, 2026 11:17
Owner
Author
|
Restack/re-review update at exact head 91130eb (tree e2541d82e99122934b9ce8e223f83cf0162679bf). This merges fork develop d7dab95 after #620 and resolves the security-sensitive overlap explicitly:
Local validation on the exact resulting tree:
The restack exposed and fixed one semantic merge defect before push: an early recovery-finalize cleanup still referenced removed local new_mnemonic after #620 moved the secret to final_mnemonic_scratch. The corrected path scrubs the actual file-scope buffer and the full recovery/setup suites pass. The checkout is clean. This is staging/review evidence only; no signing or release was performed. Awaiting exact-head PR CI before any merge decision. |
Owner
Author
|
Exact-head update: the prior run at Head |
Owner
Author
|
Final exact-head disposition for |
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.
7.16 release-candidate staging — merge order is mandatory
Stages firmware 7.16 after #620. Target remains
develop, so GitHub shows the combined 7.15 + 7.16 range until #620 merges; afterward this PR reduces naturally to the 7.16-only delta.This candidate is code-ready for CI and external release qualification. Do not merge it before #620, and do not publish it until every production gate below is checked by a human.
Construction
41ec6eeaaee78b1ffa12e8064e7937a0eefe6da7bAlpha supplies the 7.16 implementation and audit history. Open alpha PRs #611, #612, and #619 remain deliberately excluded. The release-process carry-forwards from 7.15 remain intact: real 3-of-5 signature verification before upload and the documented full/Bitcoin-only product split.
Included 7.16 boundary
docs/release/SRS-7.16.mdis the controlling release contract. It distinguishes the disabled legacy monolithic EIP-712 path from the AdvancedMode-gated streaming endpoint and records the exact custody, storage, browser, hardware, and approval requirements.Audit fixes in this candidate
Local verification
efe6da7b(full/Bitcoin-only ARM and emulator builds, unit, Python integration/dylib, crypto, static analysis, format, secret, submodule, report, aggregate gate)Production release gates
Do not publish artifacts or advance
STORAGE_VERSION_LAST_SHIPPEDuntil these gates are complete.