security: audit and harden 7.14.3 Bitcoin-only + Dice - #604
Conversation
Backported from release/7.15, where the collection UI and the physical
short-press/long-hold classification are hardware-verified. Eight files
arrive byte-identical to that tree (dice_input.{c,h}, rng_health.{c,h},
rng.{c,h}, u2f.c, keepkey_flash.c) along with both unit-test suites; the
reset.c / fsm_msg_common.h / fsm_msg_debug.h integration is ported
surgically and diffs clean against 7.15 except for the one deliberate
divergence below.
ResetDevice.dice_entropy collects 50/75/99 rolls on the device (short press
advances 1-6/undo, 800 ms hold commits) and folds
SHA256(int_entropy || rolls) into the internal entropy BEFORE
EntropyRequest, so the host contribution arrives strictly after the device
has committed to its own. The completion screen shows the first 8 bytes of
SHA256(rolls) so the user can recompute it from their written rolls; the
same digest is served over DebugLink for test parity. docs/DiceEntropy.md
covers what the digest proves and what it deliberately does not.
Divergence from 7.15, on purpose: display_random is RETAINED, because
already-shipped 7.14-line hosts request the Internal Entropy screen and the
pinned python suite asserts it below 7.15.0. The two are mutually exclusive
by refusal -- ResetDevice with both flags fails with a SyntaxError, since
the screen shows the POST-mix value, which is the seed pre-image once
ext_entropy is known.
The seed-time RNG gate covers every call site on rng_health.h's documented
list, so the header's COMPLETE LIST claim is true in this tree: reset_init
(fail with FirmwareError), the storage encryption and wipe-code keys and
the V1-upgrade PIN-KDF salt (storage_drawKeyMaterial, halt), U2F key-handle
derivation (refuse registration), and the one-shot OTP randomness block
(skip the write, leave the block claimable by a healthy boot).
Pins: deps/device-protocol advances to 8545cd5b -- the exact pin
release/7.15 builds against; every bytes/string field new to the compiled
protos verified to carry nanopb sizing (the options files here were already
pre-staged for it, plus three Solana LUT lines). deps/python-keepkey
advances to 9aaaa84, which moves the dice end-to-end test, the ceremony
re-entry regression, and report Section K from a 7.15.0 gate to 7.14.3 --
exact for the fleet, since no firmware between 7.14.3 and 7.15.0 exists
without dice. Those tests previously SKIPPED against this branch and CI
went green with zero device-level dice coverage.
ci: build-arm-firmware compiles both release variants on every PR, so a
change that only breaks the bitcoin-only image can no longer go green.
Verified in CI-parity docker (same base image digest): bitcoin-only ARM
links at .text 372,928 and full at 523,600; firmware-unit 103/103 including
Dice.* (5) and RngHealth.* (15); the full python-integration compose suite
passes 449/449 selected with test_reset_device_dice and
test_reset_reentry_disarms_entropy_ack RUNNING and green; clang-format-20
clean on every touched file.
BitHighlander
changed the title
Exact-head audit attestationAudited candidate: Exact-head CI: https://github.com/BitHighlander/keepkey-firmware/actions/runs/32945400323 All required jobs and the aggregate CI gate completed successfully, including full-feature and Bitcoin-only ARM builds, both integration-test lanes, native firmware/board/crypto tests, secret scanning, cppcheck, clang-format, emulator, and dylib checks. Reconciled test evidence
Shipping artifact identity
Artifacts: Auditor-of-auditor evidenceThe first proposed remediation for #610 made the targeted OP_RETURN regression pass but caused unexpected signing confirmations in both structured and raw many-output tests. That remediation was rejected. The corrected lifecycle fix resets only the current signing digest at signing start while preserving the prior completed-transaction warning, and the entire Bitcoin-only lane was rerun successfully. This negative result and correction are recorded on #610. The audit also found that the repository-wide secret scan could ingest disconnected fork refs and block CI (#544). The fix binds scanning to the event revision range, pins and verifies the scanner download, and introduces no allowlist or suppression. This remains a draft until the SOP's human line-review attestations and release-signing/provenance gates are explicitly completed. Green CI is evidence for the tested candidate; it is not, by itself, a claim that every manual security-review gate is complete. |
Adversarial exact-head audit — NOT APPROVEDAudited product: firmware 7.14.3, Bitcoin-only + Dice I completed the SOP line-review units, reconciled the linked issues and PR discussion, reproduced both shipping builds, inspected generated parser descriptors and the Bitcoin-only link surface, and ran the required negative control. This head is not certifiable for merge or release because the audit found six new defects, four rated high. New fork findings
All six issues are on the BitHighlander fork tracker; nothing was filed upstream. Pre-existing fork issues confirmed live in the audited productThe audit also reconfirmed related open findings rather than duplicating them: #399 (retained The fixes claimed for #446, #449, #528, #531, #544, #597, #608, #609, and #610 are present at this exact head and their targeted regression evidence is consistent. That does not offset the new blockers above; those issues should remain merge-coupled as planned. Surface disposition and review accountingModel attestation: OpenAI Codex, GPT-5 model family. The runtime does not expose exact inference-token telemetry per audit section. I will not fabricate it. The deterministic accounting below is the exact reviewed source footprint and
Gross reviewed-footprint sum (with cross-surface duplication): 26,949 lines, 955,440 bytes, approximately 238,866 source tokens. Reproduction and evidence
Certification decisionDo not merge, tag, sign, or promote this head. Resolve #613–#618, rerun the exact SOP and positive control on the new immutable head, then complete the still-pending independent human line-review and release-signing/provenance attestations. Green CI here certifies the tested mechanics and artifacts only; it does not certify the security properties that failed this review. |
Security re-review attestation — exact head
|
Finding #616 — ClearSession leaves Bitcoin signing active: resolvedAt exact head d842f1e, ClearSession reaches the centralized Bitcoin signing abort and scrubs signer statics, private/root/derived nodes, and digests. Device-wire tests interrupt every TxRequest stage and confirm signing cannot continue. Both exact-head CI gates pass. Issue: #616 |
Finding #617 — Recovery/reset retains mnemonic state: resolvedAt exact head d842f1e, reset/recovery aborts wipe the BIP39 workspace and all recovery fragments, and invalid word counts disarm the ceremony. Re-review also found and corrected a hidden U2F-counter commit that aborted recovery before mnemonic storage. The complete recovery-cipher suite and both exact-head CI matrices pass. Issue: #617 |
Finding #614 — Authenticator passphrase-rejection bypass: resolvedAt exact head d842f1e, passphrase rejection returns before authenticator dispatch, cancellation/rejection propagates to the caller, and plaintext passphrase, HMAC, OTP, and temporary secret buffers are cleared. Native and device-wire regressions pass, as do both exact-head CI gates. Issue: #614 |
Finding #615 — ARM/emulator checksum divergence: resolvedAt exact head d842f1e, both checksum passes hash script type using a canonical four-byte little-endian representation, removing host-width dependence. Checksum regression coverage, full and Bitcoin-only ARM builds, emulator builds, and both exact-head CI gates pass. Issue: #615 |
Finding #613 — RNG fault latch bypass: resolvedAt exact head d842f1e, RNG hardware and continuous-health failures latch across subsequent cached checks. Transient and persistent hardware faults, degenerate output, and wipe-on-failure behavior have regression coverage. Both exact-head CI gates pass. Issue: #613 |
Finding #618 — Invalid multisig change can lock funds: resolvedAt exact head d842f1e, signing output validation and both multisig compilers require has_m and 1 <= m <= n <= 15. Invalid external and change outputs cannot serialize or sign. Native and device-wire negative tests and both exact-head CI gates pass. Issue: #618 |
1 participant
Fork target and immutable identity
This PR is entirely on the BitHighlander fork:
BitHighlander/keepkey-firmwaredeveloprelease/7.14.3-bitcoin-only2c4b7021041bd14667dd6d9c1ac282974e24d6c01d446ccbc9cf32a6e499990e65bcf7b358f1325be353ce5ef05d96e8f85568bd2a7a7420b11713c4The PR remains a draft while the final-head CI and the remaining SOP gates are
audited. It is not a certification of the regular multi-coin product.
Normative SOP and definition of “line review”
The in-tree, versioned audit contract is
docs/security/7.14.3-bitcoin-only-dice-audit-sop.md.For this audit, line review means reading every executable line at the
recorded commit in each unit below, including error paths and compile-time
branches. File/function anchors are authoritative; mutable line numbers are
only navigation aids. The SOP defines the procedure, evidence contract,
auditor-of-auditor controls, exclusions, and merge gates so the claimed surface
cannot silently drift away from the reviewed tree.
Audit surface, unit by unit
CMakeLists.txt;lib/firmware/CMakeLists.txt;coins.def;coins.h;coins.cBITCOIN_ONLYis binary-valued; excluded coin engines and tables are absent rather than UI-hidden; only Bitcoin and Testnet remain reachable.lib/firmware/messagemap.def;fsm.c;fsm_msg_coin.h; Bitcoin-reachablefsm_msg_common.hGetCoinTable.signing.c;transaction.c;txin_check.c; signing portions ofcrypto.capp_confirm.c;app_layout.cdice_input.c;reset.c;dice_input.hlib/rand/rng.c;rng_health.c; their headers; everyrandom_buffer_checkedcallerreset.c;recovery_cipher.c;storage.c; public headersauthenticator.c; storage session functions; lock/wipe/initialize/session-clear handlers; signing cachesinclude/pb.h;lib/transport/pb_decode.c; generated descriptors; Bitcoin/setup.optionslib/board/signatures.c; bootloader validation;code-signing-keysgitlink; release workflow and manifests.github/workflows/ci.yml;scripts/emulator/*; native manifests; pinned Python test/report selection.gitmodulesand every recursive gitlinkThe regular multi-coin image is a regression-build boundary only because the
release workflow publishes both variants. Its non-Bitcoin transaction semantics
are out of certification scope and require their own audit record.
Confirmed findings and fixes in this PR
1 <= m <= n <= 15before multisig quorum reaches fee orscript accounting.
disclosure; decode supported Omni only from an exact, aligned-safe 20-byte
payload.
absent or empty.
cp, somake xunit's exit code is discarded #528: preserve xUnit reports while returning the captured test failure code.KK_BITCOIN_ONLY=ONimage, with distinct artifacts and aggregate failure.constrain
GetCoinTableto that boundary.while preserving the prior completed transaction used by its warning.
finding; checksum/version-assert the single scanner artifact and default the
workflow token to
contents: read.The #610 issue comment also records a rejected first fix: the complete product
suite caught unexpected prompts in both many-output regressions. That candidate
was replaced and retested before this branch was published.
Candidate evidence
Pinned Docker base:
kktech/firmware@sha256:7438e53933d47d53157ed6d96d864cb208597e62dce26235ace09d1063427fa2(AMD64 image executed on an ARM64 host).
passed
git diff --check: passedactionlint: no finding in the modified matrix/scanner steps; the workflow retainspre-existing shellcheck findings outside this change
The local evidence authorizes this draft and does not replace final-head CI.
Artifacts, skips, firmware hashes, and signing provenance must be reconciled in
the final attestation before this PR is made ready.
Companion fork PR
The test harness change is also entirely on the fork:
BitHighlander/python-keepkey#62.
It lowers the Bitcoin-only product-test gate specifically to 7.14.3 and marks
the Mayachain memo test as full-feature-only. This firmware PR pins the exact
companion commit rather than following its branch tip.
Auditor-of-auditor / merge gates
The independent check must reconcile claims against source, fork issue state,
immutable commits, JUnit, screenshots, firmware hashes, and CI artifacts. A
green aggregate badge, elapsed waiting time, or this prose alone is not
evidence. In particular it must prove tests executed rather than skipped and
that every resolved issue has a reachable fix plus regression.
Do not merge until the SOP checklist is complete, including the clean regular
regression build, final-head variant tests, failure-propagation positive
control, static/format/submodule/secret gates, release manifest provenance, and
closure of every in-scope finding.
Closes #446
Closes #449
Closes #528
Closes #531
Closes #597
Closes #608
Closes #609
Closes #610
Closes #544