fix(release): say what each published hash is for - #367
Merged
Conversation
HASHES.txt printed two bare sha256 lines labelled "full" and "payload" with no statement of which comparison either one serves. They answer different questions and are not interchangeable: full image -> release binary vs the device (Features.firmware_hash) payload -> release binary vs your own reproducible build Getting them backwards produces a mismatch that reads as a compromised device. That is not hypothetical: v7.14.1's HASHES.txt went further and labelled the payload hash "matches device-verifiable build hash", which is simply wrong -- the device reports the FULL-file hash, because memory_firmware_hash() covers the whole 256-byte meta descriptor (FLASH_META_DESC_LEN = 0x100) plus the app code. That label cost real debugging time while wiring firmware verification into Vault. Each hash now carries its purpose and, for the payload hash, an explicit "NOT the hash the device reports". Rendering only; no build change.
The manifest was generated before the rename and before signing, so it
described neither the names nor the bytes that ship.
- it listed firmware.keepkey.bin / bootloader.bin, files no release ever
publishes;
- the full-image hash was taken from the unsigned build, and the checklist
then told key holders to swap the 3-of-5 signed binary in underneath it
without regenerating anything. memory_firmware_hash() covers the whole
256-byte descriptor, signatures included, so that hash necessarily
changes when the descriptor is signed. The published hash therefore
described a draft nobody installs -- the likeliest origin of the wrong
32155c11 v7.14.1 pin in Vault's table;
- "compare against Features.firmware_hash" and "strip 256 bytes" were
applied to every *.bin including bootloader.bin, which has no KPKY
application descriptor and neither instruction fits.
Rename now precedes hashing, and generation moves into
scripts/release/hash-manifest.sh so key holders can re-run the identical
recipe over the signed binaries instead of reaching for sha256sum by hand.
The script reads signed-ness off the artifacts rather than being told, so an
unsigned manifest says so; --require-signed makes that a hard gate before
publishing. It also derives the device-image hash from codelen rather than
assuming the file is exactly 256+codelen bytes, and reports the two
separately when they differ.
Per-artifact framing replaces the one-size loop: an image without a KPKY
descriptor gets its whole-file hash and an explicit note that neither the
device comparison nor 'tail -c +257' applies to it.
--self-test covers the descriptor offset math and the quorum rule.
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.
Review found the check ineffective, and it was worse than ineffective -- it
was a gate that reported success on nothing at all.
**od collapses repeated lines.** `od -An -tx1 -j64 -N192` on a 192-byte
all-zero signature area emits one line of zeros followed by '*', and the '*'
survives `tr -d '0'`, so the emptiness test found a non-empty string and
declared the image signed. Every od call now passes -v. This is the whole
failure: an unsigned binary with its signer indices filled in walked through.
The old self-test could not catch it because it wrote ONE byte into the first
signature and called the three-signature quorum valid -- a single non-zero
byte defeats od's line compression, so the test exercised the one input shape
where the bug does not appear.
Also repaired:
- each 64-byte signature region is checked independently, not as one
concatenated blob;
- signer slots must be in 1..5, not merely non-zero;
- --require-signed refuses a directory containing no KPKY application image.
Previously "no unsigned image found" and "no image found" were the same
answer, so an empty directory exited 0 under "Generated from the signed
release artifacts."
The manifest header now states plainly that this is a structural check: it
proves the unsigned binary was not published, not that the signatures verify.
The self-test covers all of it and was checked against the reviewed
has_quorum: it fails there on the all-zero signature area, which is the
reported defect.
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.
…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.
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.
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.
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.
Why
HASHES.txtpublishes two sha256 values labelledfullandpayloadwith no statement of which comparison either serves. They answer different questions and are not interchangeable:Features.firmware_hash)tail -c +257)Using the wrong one produces a mismatch that reads to a user as "my device is compromised."
This is not hypothetical. v7.14.1's published
HASHES.txtlabels the payload hash"matches device-verifiable build hash", which is wrong — the device reports the FULL-file hash, becausememory_firmware_hash()(lib/board/memory.c:232) covers the entire 256-byte meta descriptor (FLASH_META_DESC_LEN = 0x100) plus the app code. That label sent firmware verification work in Vault down the wrong path for a while.Confirmed empirically against published assets: v7.14.0 (
446fd7ac…) and v7.10.0 (518ad416…) full-file hashes match the values Vault pins for on-device verification exactly; their payload hashes match nothing.What changes
Rendering only — same two hashes, each now carrying its purpose, and the payload hash carrying an explicit "NOT the hash the device reports". No build change, no artifact change.
Testing
YAML parses; the shell block was extracted and run standalone against a dummy 600 KB
.binto check the rendering (output above). Nothing else in the release job is touched.