From 5226cf41b8d3ba883d1e412057bf2accaaa45fec Mon Sep 17 00:00:00 2001 From: highlander Date: Mon, 10 Aug 2026 22:56:37 -0600 Subject: [PATCH 1/5] fix(release): say what each published hash is for 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. --- .github/workflows/release.yml | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 33b7fe9af..233776f6a 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -163,11 +163,20 @@ jobs: for f in *.bin; do [ -f "$f" ] || continue FULL_HASH=$(sha256sum "$f" | awk '{print $1}') - echo "sha256 (full) $f $FULL_HASH" >> HASHES.txt + echo "$f" >> HASHES.txt + echo " sha256 (full image) $FULL_HASH" >> HASHES.txt + echo " Compare against the firmware hash your device reports" >> HASHES.txt + echo " (Features.firmware_hash, shown in KeepKey Vault)." >> HASHES.txt + echo " This proves the release binary is what is installed." >> HASHES.txt FILE_SIZE=$(stat -c%s "$f") if [ "$FILE_SIZE" -gt 256 ]; then PAYLOAD_HASH=$(tail -c +257 "$f" | sha256sum | awk '{print $1}') - echo "sha256 (payload) $f $PAYLOAD_HASH" >> HASHES.txt + echo " sha256 (payload) $PAYLOAD_HASH" >> HASHES.txt + echo " Compare against your own reproducible build, with" >> HASHES.txt + echo " 'tail -c +257' applied to BOTH files -- a local build" >> HASHES.txt + echo " has no signatures in its 256-byte descriptor." >> HASHES.txt + echo " This proves the release binary came from the source." >> HASHES.txt + echo " It is NOT the hash the device reports." >> HASHES.txt fi echo "" >> HASHES.txt done From 36ae0b5681c32adc05ebcdbcc61ac195f5569df5 Mon Sep 17 00:00:00 2001 From: highlander Date: Tue, 11 Aug 2026 17:03:16 -0600 Subject: [PATCH 2/5] fix(release): hash the artifacts that actually get published 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. --- .github/workflows/release.yml | 59 +++++------ scripts/release/hash-manifest.sh | 170 +++++++++++++++++++++++++++++++ 2 files changed, 195 insertions(+), 34 deletions(-) create mode 100755 scripts/release/hash-manifest.sh diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 233776f6a..2f439644c 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -155,33 +155,8 @@ jobs: --budgets tools/sram-budgets.json \ --variant "${{ matrix.variant }}" - - name: Compute hashes - working-directory: release - run: | - echo "# KeepKey Firmware v${{ needs.validate.outputs.fw_version }} (${{ matrix.variant }}) — Hash Manifest" > HASHES.txt - echo "" >> HASHES.txt - for f in *.bin; do - [ -f "$f" ] || continue - FULL_HASH=$(sha256sum "$f" | awk '{print $1}') - echo "$f" >> HASHES.txt - echo " sha256 (full image) $FULL_HASH" >> HASHES.txt - echo " Compare against the firmware hash your device reports" >> HASHES.txt - echo " (Features.firmware_hash, shown in KeepKey Vault)." >> HASHES.txt - echo " This proves the release binary is what is installed." >> HASHES.txt - FILE_SIZE=$(stat -c%s "$f") - if [ "$FILE_SIZE" -gt 256 ]; then - PAYLOAD_HASH=$(tail -c +257 "$f" | sha256sum | awk '{print $1}') - echo " sha256 (payload) $PAYLOAD_HASH" >> HASHES.txt - echo " Compare against your own reproducible build, with" >> HASHES.txt - echo " 'tail -c +257' applied to BOTH files -- a local build" >> HASHES.txt - echo " has no signatures in its 256-byte descriptor." >> HASHES.txt - echo " This proves the release binary came from the source." >> HASHES.txt - echo " It is NOT the hash the device reports." >> HASHES.txt - fi - echo "" >> HASHES.txt - done - cat HASHES.txt - + # Renaming has to happen BEFORE hashing, or the manifest names files that + # are never published. - name: Rename artifacts working-directory: release run: | @@ -190,14 +165,21 @@ jobs: [ -f firmware.keepkey.bin ] && mv firmware.keepkey.bin "firmware.keepkey.v${VER}${SUFFIX}.bin" [ -f firmware.keepkey.elf ] && mv firmware.keepkey.elf "firmware.keepkey.v${VER}${SUFFIX}.elf" [ -f bootloader.bin ] && mv bootloader.bin "bootloader.v${VER}${SUFFIX}.bin" - # The regular variant carries an EMPTY suffix, so this rename is a - # no-op for it -- and `mv HASHES.txt HASHES.txt` is not a harmless - # no-op to GNU coreutils, it is "are the same file" and exit 1, which - # the step's default `bash -e` turns into a failed release. The three - # renames above are safe because each one also inserts the version. - [ -z "$SUFFIX" ] || mv HASHES.txt "HASHES${SUFFIX}.txt" ls -lh + # The manifest CI can produce describes the UNSIGNED build, because the + # signatures do not exist yet: hash-manifest.sh reads that state off the + # artifacts and says so in the file. Key holders re-run the same script + # over the signed binaries before publishing -- see the release body's + # signing checklist -- so the published hashes describe the published + # bytes rather than a draft nobody installs. + - name: Compute hashes + run: | + scripts/release/hash-manifest.sh release \ + "${{ needs.validate.outputs.fw_version }}" \ + "${{ matrix.variant }}" \ + "${{ matrix.suffix }}" + - name: Record build provenance working-directory: release run: | @@ -317,12 +299,21 @@ jobs: > **DRAFT TEST CANDIDATE** — RC artifacts are unsigned and intended for > release-candidate testing. Firmware must be signed by 3/5 key holders > before publishing a production release. + > + > The attached \`HASHES*.txt\` describes the **unsigned** build. Signing + > rewrites the 256-byte metadata descriptor, so the device-image hash + > changes; only the payload hash survives it. Do not pin a hash from a + > draft manifest. ### Signing Checklist (per variant) - - [ ] Built on multiple machines, hashes match + - [ ] Built on multiple machines, payload hashes match - [ ] Signed on air-gapped machine (3/5 signers) - [ ] Storage upgrade tested on production device - [ ] Signed .bin uploaded, replacing unsigned + - [ ] \`HASHES*.txt\` regenerated from the signed binaries and re-uploaded: + \`scripts/release/hash-manifest.sh --require-signed ${VER} \` + (fails if any image is missing its quorum; the device-image hash it + prints is what Vault should pin) - [ ] Release notes finalized EOF diff --git a/scripts/release/hash-manifest.sh b/scripts/release/hash-manifest.sh new file mode 100755 index 000000000..aaa240dba --- /dev/null +++ b/scripts/release/hash-manifest.sh @@ -0,0 +1,170 @@ +#!/bin/sh +# Generate the published hash manifest for a directory of release artifacts. +# +# This exists as a script rather than as steps inside release.yml because the +# manifest has to be produced TWICE: once by CI over the unsigned build, and +# again by the key holders over the signed binaries they are about to upload. +# When only CI could generate it, the published full-image hash described the +# unsigned draft -- the binary nobody installs -- while the checklist quietly +# swapped the signed one in underneath it. That is the most likely origin of +# the wrong v7.14.1 hash that ended up pinned in KeepKey Vault. +# +# Usage: +# scripts/release/hash-manifest.sh [suffix] +# scripts/release/hash-manifest.sh --require-signed [suffix] +# scripts/release/hash-manifest.sh --self-test +# +# Writes HASHES.txt into . With --require-signed it exits non-zero +# unless every application firmware image carries three distinct signer slots +# and three non-zero signatures -- run it that way before publishing. +# +# Application metadata descriptor (include/keepkey/board/memory.h): +# 0x00 4 magic 'KPKY' 0x08 1 sig_index1 0x40 64 signature 1 +# 0x04 4 codelen (LE) 0x09 1 sig_index2 0x80 64 signature 2 +# 0x0A 1 sig_index3 0xC0 64 signature 3 +# 0x0B 1 sig_flag +set -eu + +sha256() { { command -v sha256sum >/dev/null && sha256sum; } || shasum -a 256; } +digest() { sha256 | awk '{print $1}'; } + +# Little-endian uint32 at byte offset $2 of file $1. NR==1 because od closes +# with a trailing offset line that awk would otherwise emit as a second value. +le32() { + od -An -tu1 -j"$2" -N4 "$1" | + awk 'NR == 1 {print $1 + $2 * 256 + $3 * 65536 + $4 * 16777216}' +} +u8() { od -An -tu1 -j"$2" -N1 "$1" | awk 'NR == 1 {print $1}'; } +is_kpky() { [ "$(od -An -c -N4 "$1" | tr -d ' \n')" = "KPKY" ]; } + +# Three distinct non-zero signer slots and three non-zero signatures. This is a +# structural quorum check, NOT signature verification -- it catches "the +# unsigned binary got uploaded", which is the failure actually observed. +has_quorum() { + _i1=$(u8 "$1" 8); _i2=$(u8 "$1" 9); _i3=$(u8 "$1" 10) + [ "$_i1" -ne 0 ] && [ "$_i2" -ne 0 ] && [ "$_i3" -ne 0 ] || return 1 + [ "$_i1" -ne "$_i2" ] && [ "$_i1" -ne "$_i3" ] && [ "$_i2" -ne "$_i3" ] || return 1 + _sigs=$(od -An -tx1 -j64 -N192 "$1" | tr -d ' \n') + [ -n "$(printf '%s' "$_sigs" | tr -d '0')" ] +} +signer_slots() { printf '%s,%s,%s' "$(u8 "$1" 8)" "$(u8 "$1" 9)" "$(u8 "$1" 10)"; } + +self_test() { + d=$(mktemp -d) + trap 'rm -rf "$d"' EXIT + # 256-byte descriptor + 4 bytes of "code": magic, codelen=4, no signatures. + printf 'KPKY\004\000\000\000' > "$d/f.bin" + dd if=/dev/zero bs=1 count=248 >> "$d/f.bin" 2>/dev/null + printf 'code' >> "$d/f.bin" + is_kpky "$d/f.bin" || { echo "self-test: magic not detected"; exit 1; } + [ "$(le32 "$d/f.bin" 4)" = "4" ] || { echo "self-test: codelen misread"; exit 1; } + has_quorum "$d/f.bin" && { echo "self-test: unsigned image claimed quorum"; exit 1; } + # Sign it: slots 1,2,4 and non-zero signature bytes. + printf '\001\002\004\001' | dd of="$d/f.bin" bs=1 seek=8 conv=notrunc 2>/dev/null + printf '\052' | dd of="$d/f.bin" bs=1 seek=64 conv=notrunc 2>/dev/null + has_quorum "$d/f.bin" || { echo "self-test: signed image failed quorum"; exit 1; } + # Repeated slots are not a quorum, however non-zero. + printf '\001\001\004' | dd of="$d/f.bin" bs=1 seek=8 conv=notrunc 2>/dev/null + has_quorum "$d/f.bin" && { echo "self-test: duplicate slots passed quorum"; exit 1; } + echo "self-test: ok" +} + +[ "${1:-}" = "--self-test" ] && { self_test; exit 0; } + +REQUIRE_SIGNED=0 +if [ "${1:-}" = "--require-signed" ]; then REQUIRE_SIGNED=1; shift; fi + +DIR=$1; VERSION=$2; VARIANT=$3; SUFFIX=${4:-} +cd "$DIR" + +OUT="HASHES${SUFFIX}.txt" +: > "$OUT" + +# Signed-ness is read off the artifacts rather than passed in, so the manifest +# cannot claim a state the bytes do not support. +UNSIGNED=0 +for f in *.bin; do + [ -f "$f" ] || continue + is_kpky "$f" || continue + has_quorum "$f" || UNSIGNED=1 +done + +{ + echo "# KeepKey Firmware v${VERSION} (${VARIANT}) — Hash Manifest" + echo "#" + if [ "$UNSIGNED" -eq 1 ]; then + echo "# THESE ARE THE UNSIGNED BUILD ARTIFACTS. Signing rewrites the 256-byte" + echo "# metadata descriptor, which CHANGES every 'device image' and 'whole file'" + echo "# hash below. Regenerate this file from the signed binaries before" + echo "# publishing:" + echo "# scripts/release/hash-manifest.sh --require-signed . ${VERSION} ${VARIANT} ${SUFFIX}" + echo "# Only the 'payload' hash survives signing unchanged." + else + echo "# Generated from the signed release artifacts." + fi + echo "" +} >> "$OUT" + +for f in *.bin *.elf; do + [ -f "$f" ] || continue + WHOLE=$(digest < "$f") + + if ! is_kpky "$f"; then + # No KPKY descriptor: a bootloader image or a build product. The + # device-image and payload framings below simply do not apply to it, and + # applying them is how 'tail -c +257' ended up recommended for + # bootloader.bin. + { + echo "$f (no KPKY application descriptor)" + echo " sha256 (whole file) $WHOLE" + echo " The file as published. This is NOT what Features.firmware_hash" + echo " reports, and 'tail -c +257' does not apply to it." + echo "" + } >> "$OUT" + continue + fi + + CODELEN=$(le32 "$f" 4) + SIZE=$(wc -c < "$f" | tr -d ' ') + DEVICE=$(head -c $((256 + CODELEN)) "$f" | digest) + PAYLOAD=$(tail -c +257 "$f" | digest) + + if has_quorum "$f"; then + STATE="signed, signer slots $(signer_slots "$f")" + else + STATE="UNSIGNED" + fi + + { + echo "$f (application firmware, ${STATE})" + echo " sha256 (device image) $DEVICE" + echo " Compare against the firmware hash your device reports" + echo " (Features.firmware_hash, shown in KeepKey Vault)." + echo " Covers the 256-byte metadata descriptor -- signatures included --" + echo " plus codelen (${CODELEN}) bytes of application code." + echo " Signing CHANGES this hash." + echo " sha256 (whole file) $WHOLE" + if [ "$SIZE" -eq $((256 + CODELEN)) ]; then + echo " The file as published; identical to the device image hash above." + else + echo " The file as published. It is ${SIZE} bytes against a" + echo " 256+codelen device image of $((256 + CODELEN)), so the two hashes" + echo " DIFFER. Pin the device image hash, not this one." + fi + echo " sha256 (payload) $PAYLOAD" + echo " Compare against your own reproducible build, with" + echo " 'tail -c +257' applied to BOTH files -- a local build" + echo " has no signatures in its 256-byte descriptor." + echo " This proves the release binary came from the source." + echo " Signing does NOT change this hash, and it is NOT the hash" + echo " the device reports." + echo "" + } >> "$OUT" +done + +cat "$OUT" + +if [ "$REQUIRE_SIGNED" -eq 1 ] && [ "$UNSIGNED" -eq 1 ]; then + echo "ERROR: an application firmware image is missing its 3-of-5 quorum." >&2 + exit 1 +fi From 45ecfa367e1e5f2db14cff5ba701cbad6a846ff0 Mon Sep 17 00:00:00 2001 From: highlander Date: Tue, 11 Aug 2026 18:56:42 -0600 Subject: [PATCH 3/5] fix(release): the quorum gate was passing unsigned images 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. --- scripts/release/hash-manifest.sh | 104 ++++++++++++++++++++++++++----- 1 file changed, 87 insertions(+), 17 deletions(-) diff --git a/scripts/release/hash-manifest.sh b/scripts/release/hash-manifest.sh index aaa240dba..7dd016407 100755 --- a/scripts/release/hash-manifest.sh +++ b/scripts/release/hash-manifest.sh @@ -28,27 +28,53 @@ set -eu sha256() { { command -v sha256sum >/dev/null && sha256sum; } || shasum -a 256; } digest() { sha256 | awk '{print $1}'; } +# EVERY od CALL PASSES -v. Without it od collapses repeated identical lines to +# a single '*', so 192 zero bytes render as one line of zeros plus '*' -- and +# the '*' survives `tr -d '0'`, which made an entirely unsigned image read as +# signed. That defect is the reason this file's checks are per-region below +# rather than one concatenated blob. +# # Little-endian uint32 at byte offset $2 of file $1. NR==1 because od closes # with a trailing offset line that awk would otherwise emit as a second value. le32() { - od -An -tu1 -j"$2" -N4 "$1" | + od -v -An -tu1 -j"$2" -N4 "$1" | awk 'NR == 1 {print $1 + $2 * 256 + $3 * 65536 + $4 * 16777216}' } -u8() { od -An -tu1 -j"$2" -N1 "$1" | awk 'NR == 1 {print $1}'; } -is_kpky() { [ "$(od -An -c -N4 "$1" | tr -d ' \n')" = "KPKY" ]; } +u8() { od -v -An -tu1 -j"$2" -N1 "$1" | awk 'NR == 1 {print $1}'; } +is_kpky() { [ "$(od -v -An -c -N4 "$1" | tr -d ' \n')" = "KPKY" ]; } + +# True if the 64-byte signature slot $2 (0..2) of file $1 is not all zeroes. +sig_present() { + _off=$((64 + $2 * 64)) + [ -n "$(od -v -An -tx1 -j"$_off" -N64 "$1" | tr -d ' \n' | tr -d '0')" ] +} -# Three distinct non-zero signer slots and three non-zero signatures. This is a -# structural quorum check, NOT signature verification -- it catches "the -# unsigned binary got uploaded", which is the failure actually observed. +# A 3-of-5 quorum: three signer slots, each in the valid range 1..5, all +# distinct, and each of the three 64-byte signature regions independently +# non-zero. +# +# This is a STRUCTURAL check, not signature verification -- it catches "the +# unsigned binary got uploaded", which is the failure actually observed. It +# cannot catch a well-formed forgery, and must not be described as if it could. has_quorum() { _i1=$(u8 "$1" 8); _i2=$(u8 "$1" 9); _i3=$(u8 "$1" 10) - [ "$_i1" -ne 0 ] && [ "$_i2" -ne 0 ] && [ "$_i3" -ne 0 ] || return 1 + for _i in "$_i1" "$_i2" "$_i3"; do + [ "$_i" -ge 1 ] && [ "$_i" -le 5 ] || return 1 + done [ "$_i1" -ne "$_i2" ] && [ "$_i1" -ne "$_i3" ] && [ "$_i2" -ne "$_i3" ] || return 1 - _sigs=$(od -An -tx1 -j64 -N192 "$1" | tr -d ' \n') - [ -n "$(printf '%s' "$_sigs" | tr -d '0')" ] + sig_present "$1" 0 && sig_present "$1" 1 && sig_present "$1" 2 } signer_slots() { printf '%s,%s,%s' "$(u8 "$1" 8)" "$(u8 "$1" 9)" "$(u8 "$1" 10)"; } +fail() { echo "self-test: $1"; exit 1; } + +# Writes 64 non-zero bytes into signature slot $2 (0..2) of $1. +sign_slot() { + _o=$((64 + $2 * 64)) + dd if=/dev/zero bs=1 count=64 2>/dev/null | tr '\000' '\052' | + dd of="$1" bs=1 seek="$_o" conv=notrunc 2>/dev/null +} + self_test() { d=$(mktemp -d) trap 'rm -rf "$d"' EXIT @@ -56,16 +82,45 @@ self_test() { printf 'KPKY\004\000\000\000' > "$d/f.bin" dd if=/dev/zero bs=1 count=248 >> "$d/f.bin" 2>/dev/null printf 'code' >> "$d/f.bin" - is_kpky "$d/f.bin" || { echo "self-test: magic not detected"; exit 1; } - [ "$(le32 "$d/f.bin" 4)" = "4" ] || { echo "self-test: codelen misread"; exit 1; } - has_quorum "$d/f.bin" && { echo "self-test: unsigned image claimed quorum"; exit 1; } - # Sign it: slots 1,2,4 and non-zero signature bytes. + is_kpky "$d/f.bin" || fail "magic not detected" + [ "$(le32 "$d/f.bin" 4)" = "4" ] || fail "codelen misread" + has_quorum "$d/f.bin" && fail "unsigned image claimed quorum" + + # THE od REGRESSION. Valid distinct in-range slots, but the whole 192-byte + # signature area is still zero. Under `od` without -v that area renders as one + # zero line plus '*', and the '*' survived `tr -d '0'`, so this exact shape -- + # an unsigned binary with its indices filled in -- passed the gate. printf '\001\002\004\001' | dd of="$d/f.bin" bs=1 seek=8 conv=notrunc 2>/dev/null - printf '\052' | dd of="$d/f.bin" bs=1 seek=64 conv=notrunc 2>/dev/null - has_quorum "$d/f.bin" || { echo "self-test: signed image failed quorum"; exit 1; } + has_quorum "$d/f.bin" && fail "all-zero signature area passed quorum (od -v)" + + # One signature present is not three. The previous self-test wrote a single + # byte here and declared the quorum valid, which is why none of this was + # caught. + sign_slot "$d/f.bin" 0 + has_quorum "$d/f.bin" && fail "one signature passed a 3-of-5 quorum" + sign_slot "$d/f.bin" 1 + has_quorum "$d/f.bin" && fail "two signatures passed a 3-of-5 quorum" + sign_slot "$d/f.bin" 2 + has_quorum "$d/f.bin" || fail "three signed slots failed quorum" + + # Slots outside 1..5 are not signers, however non-zero. + printf '\001\002\006' | dd of="$d/f.bin" bs=1 seek=8 conv=notrunc 2>/dev/null + has_quorum "$d/f.bin" && fail "out-of-range signer slot 6 passed quorum" + printf '\001\002\000' | dd of="$d/f.bin" bs=1 seek=8 conv=notrunc 2>/dev/null + has_quorum "$d/f.bin" && fail "zero signer slot passed quorum" + # Repeated slots are not a quorum, however non-zero. printf '\001\001\004' | dd of="$d/f.bin" bs=1 seek=8 conv=notrunc 2>/dev/null - has_quorum "$d/f.bin" && { echo "self-test: duplicate slots passed quorum"; exit 1; } + has_quorum "$d/f.bin" && fail "duplicate slots passed quorum" + + # And --require-signed must refuse a directory with no application image + # rather than reporting success over nothing. + e=$(mktemp -d) + if sh "$0" --require-signed "$e" 0.0.0 full "" >/dev/null 2>&1; then + rm -rf "$e"; fail "--require-signed passed an empty directory" + fi + rm -rf "$e" + echo "self-test: ok" } @@ -82,13 +137,25 @@ OUT="HASHES${SUFFIX}.txt" # Signed-ness is read off the artifacts rather than passed in, so the manifest # cannot claim a state the bytes do not support. +# +# APPS counts application images. Without it "no unsigned image found" and "no +# image found at all" were the same answer, so --require-signed exited 0 on an +# empty directory and announced "Generated from the signed release artifacts" -- +# a gate that passes when there is nothing to gate. UNSIGNED=0 +APPS=0 for f in *.bin; do [ -f "$f" ] || continue is_kpky "$f" || continue + APPS=$((APPS + 1)) has_quorum "$f" || UNSIGNED=1 done +if [ "$REQUIRE_SIGNED" -eq 1 ] && [ "$APPS" -eq 0 ]; then + echo "ERROR: no KPKY application image in $(pwd) — nothing to publish." >&2 + exit 1 +fi + { echo "# KeepKey Firmware v${VERSION} (${VARIANT}) — Hash Manifest" echo "#" @@ -100,7 +167,10 @@ done echo "# scripts/release/hash-manifest.sh --require-signed . ${VERSION} ${VARIANT} ${SUFFIX}" echo "# Only the 'payload' hash survives signing unchanged." else - echo "# Generated from the signed release artifacts." + echo "# Generated from the signed release artifacts: ${APPS} application" + echo "# image(s), each carrying three distinct signer slots in 1..5 and three" + echo "# non-empty signatures. That is a STRUCTURAL check -- it proves the" + echo "# unsigned binary was not published, NOT that the signatures verify." fi echo "" } >> "$OUT" From fc092c8f0ccca2749537da16a360b83d74df9ba9 Mon Sep 17 00:00:00 2001 From: highlander Date: Tue, 11 Aug 2026 19:42:23 -0600 Subject: [PATCH 4/5] fix(release): bind the manifest to one named image, and stop overstating 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. --- scripts/release/hash-manifest.sh | 46 +++++++++++++++++++++++++++----- 1 file changed, 39 insertions(+), 7 deletions(-) diff --git a/scripts/release/hash-manifest.sh b/scripts/release/hash-manifest.sh index 7dd016407..e4503cd23 100755 --- a/scripts/release/hash-manifest.sh +++ b/scripts/release/hash-manifest.sh @@ -53,9 +53,16 @@ sig_present() { # distinct, and each of the three 64-byte signature regions independently # non-zero. # -# This is a STRUCTURAL check, not signature verification -- it catches "the -# unsigned binary got uploaded", which is the failure actually observed. It -# cannot catch a well-formed forgery, and must not be described as if it could. +# WHAT THIS PROVES, EXACTLY: that the canonical unsigned draft -- zero indices, +# zero signature area -- was not published. Nothing more. A region holding a +# single non-zero byte passes, so this cannot distinguish a real ECDSA +# signature from a placeholder, and it cannot detect a forgery at all. +# +# The five signing public keys are 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. Until +# that exists, do not let this check be described as proof the release is +# correctly signed. has_quorum() { _i1=$(u8 "$1" 8); _i2=$(u8 "$1" 9); _i3=$(u8 "$1" 10) for _i in "$_i1" "$_i2" "$_i3"; do @@ -142,17 +149,29 @@ OUT="HASHES${SUFFIX}.txt" # image found at all" were the same answer, so --require-signed exited 0 on an # empty directory and announced "Generated from the signed release artifacts" -- # a gate that passes when there is nothing to gate. +# Exactly one application image, and it must be the one this invocation names. +# A directory holding both variants' artifacts -- which is precisely what the +# release job's merge-multiple download produces -- would otherwise hash the +# bitcoin-only image into the full variant's manifest and vice versa. +EXPECTED_APP="firmware.keepkey.v${VERSION}${SUFFIX}.bin" UNSIGNED=0 APPS=0 for f in *.bin; do [ -f "$f" ] || continue is_kpky "$f" || continue + if [ "$f" != "$EXPECTED_APP" ]; then + echo "ERROR: unexpected application image '$f'; this manifest is for" >&2 + echo " '${EXPECTED_APP}'. Generate each variant from its own" >&2 + echo " directory, or the variants cross-contaminate." >&2 + exit 1 + fi APPS=$((APPS + 1)) has_quorum "$f" || UNSIGNED=1 done if [ "$REQUIRE_SIGNED" -eq 1 ] && [ "$APPS" -eq 0 ]; then - echo "ERROR: no KPKY application image in $(pwd) — nothing to publish." >&2 + echo "ERROR: no application image '${EXPECTED_APP}' in $(pwd) —" >&2 + echo " nothing to publish." >&2 exit 1 fi @@ -168,9 +187,12 @@ fi echo "# Only the 'payload' hash survives signing unchanged." else echo "# Generated from the signed release artifacts: ${APPS} application" - echo "# image(s), each carrying three distinct signer slots in 1..5 and three" - echo "# non-empty signatures. That is a STRUCTURAL check -- it proves the" - echo "# unsigned binary was not published, NOT that the signatures verify." + echo "# image, carrying three distinct signer slots in 1..5 and three" + echo "# non-empty signature regions." + echo "#" + echo "# That is a STRUCTURAL check with a narrow meaning: it proves the" + echo "# canonical UNSIGNED DRAFT was not published. It does NOT verify the" + echo "# signatures, and a region holding one non-zero byte would pass it." fi echo "" } >> "$OUT" @@ -196,6 +218,16 @@ for f in *.bin *.elf; do CODELEN=$(le32 "$f" 4) SIZE=$(wc -c < "$f" | tr -d ' ') + + # head -c stops at EOF without complaining, so a truncated image would be + # hashed over fewer bytes than its own descriptor claims and published as if + # it were whole. + if [ "$SIZE" -lt $((256 + CODELEN)) ]; then + echo "ERROR: '$f' is ${SIZE} bytes but its descriptor claims 256+${CODELEN}" >&2 + echo " = $((256 + CODELEN)). Truncated image; refusing to hash it." >&2 + exit 1 + fi + DEVICE=$(head -c $((256 + CODELEN)) "$f" | digest) PAYLOAD=$(tail -c +257 "$f" | digest) From af933a53f852af431245f2eb9742dd855e5954ee Mon Sep 17 00:00:00 2001 From: highlander Date: Wed, 12 Aug 2026 01:28:39 -0600 Subject: [PATCH 5/5] =?UTF-8?q?fix(release):=20publish=20firmware=20only?= =?UTF-8?q?=20=E2=80=94=20never=20a=20bootloader?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The build produces bin/bootloader.bin, this workflow copied it into release/, the rename turned it into bootloader.v.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. --- .github/workflows/release.yml | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 2f439644c..5b220a194 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -140,7 +140,6 @@ jobs: mkdir -p /root/keepkey-firmware/release && \ cp bin/firmware.keepkey.bin /root/keepkey-firmware/release/ && \ cp bin/firmware.keepkey.elf /root/keepkey-firmware/release/ && \ - cp bin/bootloader.bin /root/keepkey-firmware/release/ 2>/dev/null || true && \ find . -name '*.su' -print0 | tar czf /root/keepkey-firmware/release/stack-usage.tgz --null -T - && \ chmod -R a+rw /root/keepkey-firmware/release" @@ -155,6 +154,16 @@ jobs: --budgets tools/sram-budgets.json \ --variant "${{ matrix.variant }}" + # A RELEASE PUBLISHES FIRMWARE ONLY -- never a bootloader. + # + # The build produces bin/bootloader.bin, and this step used to copy it + # into release/, where the rename turned it into bootloader.v.bin and + # release-assets/* published it. A bootloader is a separately signed + # artifact with its own rollout, and shipping one as a side effect of + # tagging firmware is how a device gets bricked by an image nobody + # reviewed as a bootloader release. It is no longer copied, so it cannot + # be renamed, hashed or attached. + # # Renaming has to happen BEFORE hashing, or the manifest names files that # are never published. - name: Rename artifacts @@ -164,7 +173,6 @@ jobs: SUFFIX="${{ matrix.suffix }}" [ -f firmware.keepkey.bin ] && mv firmware.keepkey.bin "firmware.keepkey.v${VER}${SUFFIX}.bin" [ -f firmware.keepkey.elf ] && mv firmware.keepkey.elf "firmware.keepkey.v${VER}${SUFFIX}.elf" - [ -f bootloader.bin ] && mv bootloader.bin "bootloader.v${VER}${SUFFIX}.bin" ls -lh # The manifest CI can produce describes the UNSIGNED build, because the @@ -276,6 +284,13 @@ jobs: artifacts/PROVENANCE*.txt release-assets/ ls -lh release-assets/ + # Firmware only. If a bootloader ever reaches this directory again, + # stop rather than publish it. + if ls release-assets/ | grep -i bootloader; then + echo "::error::A bootloader artifact reached the release assets." + exit 1 + fi + - name: Generate release body run: | VER="${{ needs.validate.outputs.fw_version }}"