fix(storage): revert the flash format to V17 for the 7.15 release - #368
Merged
Conversation
RC27 writes storage version 19. Booting it once on an existing wallet
silently migrates 17 -> 19 and re-commits, with no prompt and no user
action. From that moment the device cannot be downgraded without being
wiped -- and nothing in the release stops the downgrade, including a
user dropping an older signed .bin on Vault's firmware drop zone.
The wipe itself is correct and stays. An unrecognised storage version
maps to StorageVersion_NONE and resets, so an attacker cannot flash an
older validly signed image with a known extraction bug and keep the
seed. What is wrong is shipping the one-way migration ahead of the
mechanism designed to make it unnecessary: the anti-rollback security
epoch, which is a design note today and implements nothing.
docs/security/pin-kdf-v19-migration.md says exactly this and was not
followed.
This firmware now reads and writes V17, matching shipped v7.14.1.
Kept, because they never depended on the version bump:
- the bitcoin-only seed lock (STORAGE_VERSION_BTC_ONLY is
10000 + STORAGE_VERSION, so it simply becomes 10017)
- SUS_BitcoinOnlyLocked, BIP-85, the recovery-cipher frame arena, and
the authenticator error-reporting fixes
Gated, not deleted: the PIN-KDF v19 implementation and its unit tests
stay, behind STORAGE_PIN_KDF_V19 == 0. The gate covers the REWRAP in
storage_isPinCorrect_impl, not just the serializer, because the v19
marker is one flag bit that only round-trips in version 19. Rewrapping
without persisting the flag wraps the key with v19 parameters and reads
it back as v15/v16 on the next boot -- a wallet nobody can ever unlock,
with flash otherwise intact. That is worse than a wipe.
The same trap rules out a "read V19, write V17" bridge, so devices that
ran RC27 wipe on installing this. That must be in the release notes;
testers need their recovery phrase first.
docs/security/storage-version-downgrade-policy.md records why the wipe
is deliberate, the full gate list for re-enabling V19, and why the
clear-sign identity block (all of what made V18) is dead -- a
KeepKey-issued schema signature verifies against a built-in anchor and
needs no device storage at all.
Verified: kkfirmware builds clean. The X-macro ladder plus the
default-less switch made the compiler enumerate every affected site.
firmware-unit could not be linked locally (deps/sca-hardening's nested
SecAESSTM32 will not populate in this worktree); the storage suite must
run in CI.
Three defects found by audit of 6bebde7, all confirmed locally with a working firmware-unit build. 1. WALLET LOCKOUT (worse than the audit's "tests expect V19"). The previous commit gated the rewrap in storage_isPinCorrect_impl but missed storage_setPin_impl, which hardcoded PIN_KDF_V19 and set pin_kdf_v2 = true. That is the function that CREATES the wrap, and it runs on wallet creation, on every PIN change, and inside the V1 upgrade path. Wrapping with v19 parameters while committing a V17 record -- where bit 20 cannot round-trip -- means the next boot derives v15/v16 and every PIN fails: an intact wallet that nobody can ever open. Exactly the trap the gate comment describes, in the one place the gate was not applied. Both the derivation and the flag now follow storage_rewrapPinKdfVersion(), so the flag describes the wrap that was actually produced rather than an aspiration. 2. CRC omitted the last meaningful byte. flash_temp was 2570 while the CRC is computed as sizeof/sizeof(uint32_t) WORDS -- 642 words = 2568 bytes -- so byte 2568, the final byte of the V17 record, sat outside it. A corrupted final byte could pass commit verification and surface later as a secret fingerprint failure, which reaches storage_wipe(). This was a latent defect in the pre-V19 code that the V19 work had fixed by rounding to 3480; the revert reintroduced it. Now 2572 (643 words), with static asserts pinning both the word alignment and the minimum size so neither can regress silently. 3. Tests reimplemented production's KDF selection instead of asking for it, so they could not track the gate. Added storage_activePinKdfVersion() and storage_rewrapPinKdfVersion() as the single source of truth and pointed both production and the tests at them. The rewrap test now asserts the GATE rather than assuming v19: with the gate off, an already-v16, already-hardened wrap has nothing to upgrade, so a correct PIN must return PIN_GOOD, leave the wrap byte-identical, and produce no v19 claim. That is the negative control proving the gate works, and it flips back to PIN_REWRAP automatically when STORAGE_PIN_KDF_V19 is enabled. Also fixes the static-analysis finding that pin_kdf_v2 could be const: the gate-off path now explicitly clears it, which is both semantically correct and keeps it an out-parameter in every configuration. No suppression added. Verified: storage suite 23/23 (baseline on 6bebde7 was 19 passed / 4 failed, matching the audit exactly). Build recipe for the record -- the nanopb generator needs a `python` shim and an "rU" patch, and macOS ARM64 needs -DPB_NO_PACKED_STRUCTS=1 or the link fails on unaligned nanopb field atoms.
…he reboot Two gaps the V17 CRC fix could not close on its own. The emulator's calc_crc32() disagreed with hardware on both the unit and the algorithm: crc_calculate_block() feeds word_len 32-bit WORDS to the STM32 CRC peripheral, while the emulator looped word_len BYTES of a reflected zlib CRC-32. For storage_commit()'s 643-word buffer that is 643 bytes of coverage instead of 2572, so the suite could not tell the corrected length from the truncated one -- it could not demonstrate the very byte (2568) the fix was about. The emulator now models CRC-32/MPEG-2 over words, which is what the peripheral computes, with golden vectors that are independently checkable as the MPEG-2 CRC of each word's big-endian bytes. flash_temp becomes explicitly _Alignas(uint32_t). The existing size assertion says the buffer is a whole number of words, not that it starts on one, and calc_crc32() casts it to uint32_t*. And the reboot regression, which the audit ran but never committed. All 23 storage tests stay in RAM; the wallet lockout lived on the serialize/reboot boundary, where setPin's wrap met a V17 record that could not describe it. PinUnlocksAfterRebootUnderV17 runs the whole round trip -- create, set PIN, serialize V17 as storage_commit() does, reload into fresh state, unlock, decrypt -- and reports PIN_WRONG when the hardcoded PIN_KDF_V19 is put back.
BitHighlander
added a commit
that referenced
this pull request
Aug 12, 2026
… ran The four code findings are closed; each keeps its original text so nobody re-derives it, with the resolution underneath. The correction worth reading: `firmware-unit` has never completed on a local macOS build. SIX suites hang at 100% CPU on the shared kkconfirm_preload driver -- Authenticator, Ethereum, Mayachain, Osmosis, Thorchain and Confirmation -- and all six were reproduced at the merge base 6ae3b96 with none of this work applied. So "the storage suite is 23/23" was always a filtered result, and any past claim of a clean full local run should be treated with suspicion. Excluding those six, #368 is 327/327 and #366 is 339/339, in about eight seconds. Section 4 keeps the one thing that can still undo #367: the checklist now tells key holders to regenerate the manifest from the signed binaries, and the first real release is the test of whether that instruction is followed.
lint-format has been red on this branch since 280f3b6: an 82-column comment on the (void)pin_kdf_v2 line, and a call clang-format packs differently. Both predate the CRC work; neither is a behaviour change.
static-analysis has been red on this branch since 280f3b6, alongside the formatting failure: cppcheck flags storage->pub.pin_kdf_v2 = (storage_rewrapPinKdfVersion() == PIN_KDF_V19); as knownConditionTrueFalse, and it is right — STORAGE_PIN_KDF_V19 is 0, so storage_rewrapPinKdfVersion() returns PIN_KDF_V16 unconditionally. The comparison is still the correct code. Writing `false` would leave the persisted flag agreeing with the KDF only by coincidence, so the next person to flip the gate ships a v19 wrap described as v16 -- the lockout this branch exists to fix. Inline suppression with the reason, matching the cppcheck-suppress style already used in lib/board/memory.c. Verified with CI's exact invocation locally (cppcheck 2.20 vs CI's 2.13): zero findings across lib/, include/keepkey/ and tools/.
BitHighlander
added a commit
that referenced
this pull request
Aug 12, 2026
Both Stage-1 gates were red since 280f3b6 -- lint-format on an 82-column comment, static-analysis on a deliberate always-false comparison -- and every build and test job downstream was SKIPPED behind them. So the branch's green storage suite was a local claim only; CI had not compiled it. Records cppcheck as the second locally-runnable gate, including where the report actually goes: --error-exitcode=1 under bash -e kills the step before it cats cppcheck_report.txt, so the findings are only in the uploaded artifact unless you run it yourself.
BitHighlander
added a commit
that referenced
this pull request
Aug 12, 2026
…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.
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.
BitHighlander
added a commit
that referenced
this pull request
Aug 12, 2026
…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.
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.
Addresses the High finding in the RC27 audit. Blocks nothing else in 7.15.
The problem
RC27 writes storage version 19. Booting it once on an existing wallet silently migrates 17 → 19 and re-commits — no prompt, no user action (
storage_fromFlashcase 17 stampsSTORAGE_VERSION, returnsSUS_Updated,storage_initcommits). From that moment the device cannot be downgraded without being wiped, and nothing in the release prevents the downgrade — including a user dropping an older signed.binon Vault's firmware drop zone.The wipe is correct and stays
An unrecognised version maps to
StorageVersion_NONE→storage_reset(). During the audit it was proposed to "refuse rather than reset" so wallets survive a downgrade. That would be a vulnerability: an attacker with physical access could flash an older validly signed image with a known extraction bug and keep the seed, making the wallet only as strong as the weakest firmware ever signed.What is wrong is shipping the one-way migration ahead of the mechanism that makes it unnecessary.
docs/security/pin-kdf-v19-migration.mdsays exactly this; the anti-rollback epoch is a design note that implements nothing.Kept — never depended on the bump
STORAGE_VERSION_BTC_ONLY = 10000 + STORAGE_VERSION, so it just becomes 10017.SUS_BitcoinOnlyLocked, BIP-85, recovery-cipher frame arena, authenticator error reporting.Gated, not deleted
The PIN-KDF v19 implementation and its unit tests stay, behind
STORAGE_PIN_KDF_V19 == 0.The gate covers the rewrap, not just the serializer — this is the subtle part. The v19 marker is one flag bit that only round-trips in version 19. Rewrapping without persisting the flag wraps the key with v19 parameters and reads it back as v15/v16 next boot: a wallet nobody can ever unlock, flash otherwise intact. Worse than a wipe. Invariant for review:
That same trap rules out a "read V19, write V17" bridge (dropping the flag while the key stays v19-wrapped; re-wrapping downward needs the PIN, unavailable at boot).
Installing this on a device that ran RC27 wipes it — rc28 does not recognise version 19, which is the policy working as designed. Testers need their recovery phrase before updating.
Testing
kkfirmwarebuilds clean. The X-macro ladder (_Static_assert(VAL == STORAGE_VERSION)) plus thedefault:-less switch made the compiler enumerate every affected site rather than relying on grep.firmware-unitcould not be linked locally —deps/sca-hardening's nestedSecAESSTM32will not populate in this worktree, unrelated to this change. The storage suite must be green in CI before merge.Follow-ups (deliberately not here)
clearsign_identitiesarray are still compiled but unreachable. Removing them reclaims SRAM; kept out to keep a wallet-critical diff reviewable.storage_write*takes alenit does not honour (storage_writeV17guardslen < 1024, writes to 2569), and.cppcheck-suppressionsblanket-suppressesbufferAccessOutOfBoundsforstorage.cso CI cannot see it. Audit finding 4.