Round 4 security remediation: storage key scrubbing, CTAP2 truncation, OOB read, cipher-rotation bypass - #582
Merged
Merged
Conversation
Owner
Author
|
Independent audit: do not merge this head yet. The #581 fix only preserves one prior coded word and fails after backspacing across two boundaries; exact state sequence and required multi-boundary regressions are in #584. There are no new recovery tests in this PR. Also, as with #557, both aggregate CI gates are red and every ARM/emulator build, firmware unit, Python integration, and dylib job is skipped; #583 tracks that merge-gate defect. Static analysis/lint/crypto alone do not provide build or behavioral evidence for these changes. |
This was referenced Aug 25, 2026
…all paths storage_secMigrate() (#574, CRITICAL): the encrypt branch memzero'd its AES ctx but never the iv (which is a direct copy of the raw storage key); the decrypt branch memzero'd iv but never ctx (the derived AES key schedule). Same asymmetry, mirrored, in storage_cipherBlock() (#575). storage_getAuthData() (#576): authdataKey and the decrypted plaintextAuthBlock were never scrubbed on any of its three exit paths. storage_setAuthData() (#577): scrubbed plaintextAuthBlock but never the derived authdataKey.
layoutU2FDialog() reimplemented confirm_helper()'s vsnprintf+draw pattern standalone with no SOURCE-truncation check (vsnprintf's return was discarded) and no RENDER-fit check at all, unlike every other confirmation screen. The CTAP2 passkey consent dialog shows the host-controlled rp_id (up to 253 chars) built this way -- only ~100 chars actually render, with no ellipsis or warning, while the credential is bound to the FULL string. layoutU2FDialog() now returns whether the formatted body both fit its buffer and fit the canvas (via the existing confirm_body_fits()), still drawing unconditionally so the three bounded-length legacy U2F call sites are unaffected. ctap2_request_user_presence() -- the one call site with attacker-controlled unbounded length -- refuses immediately on a non-fit render rather than waiting for a button press on text the user can't read in full.
fsm_msgCipherKeyValue() derives data[260] via hmac_sha512(node->private_key, ...) and uses it directly as the AES-256 key (and, when no explicit iv is given, as the iv too), then builds an AES key schedule from it in ctx -- neither buffer was ever memzero'd on the success path.
…#580) txin_dgst_save_and_reset()/txin_dgst_compare() take no length parameter and unconditionally memcpy/strncmp ADDR_STR_LEN (130) bytes from their addr_str argument, trusting every caller's buffer to be at least that large. The internal-transfer confirmation path declared node_str as NODE_STRING_LENGTH (50) and passed it to both, an 80-byte OOB stack read on every such confirmed output, with the garbage bytes then copied into the global duplicate-transaction detector state. The sibling needs_confirm call site already passes a real 130-byte protobuf field, confirming the function's actual contract.
) next_character() re-randomizes the single global cipher array on every call, including after a backspace -- so reconstructing coded_word by re-encoding decoded_word under the CURRENT cipher cannot recover what was actually typed under earlier, since-rotated permutations. coded_word is meant to hold the literal raw bytes the host sent, so attempt_auto_complete(coded_word) can detect a host bypassing the substitution cipher; a corrupted coded_word stops that abort from firing, reachable via an ordinary backspace before a word's 4th character. An initial fix (preserving only the single most-recently-completed word's raw bytes) was caught by independent review as still broken: backing up across TWO OR MORE completed words left coded_word holding the wrong word's bytes, recreating the same detector gap (#584). Corrected here with coded_mnemonic, a raw-cipher-bytes mirror of mnemonic itself -- same appends, same truncations, always the same length (every CharacterAck.character is nanopb-bounded to one byte) -- so a backspace at any depth, across any number of word boundaries, is just "truncate like mnemonic and re-derive the current word" via the new get_current_coded_word(), exactly how decoded_word already worked. Adds RecoveryCipher unit tests driving recovery_character()/ recovery_delete_character() directly via a new DEBUG_LINK-only recovery_debugLinkStart() hook (bypasses recovery_cipher_init()'s confirm()/PIN gate, which blocks on a real button press) and two new DEBUG_LINK getters for the raw coded history, covering: backspace within a word, across one boundary, across two boundaries (the exact #584 repro), across 23 boundaries, interleaved delete/retype, and a raw unenciphered prefix still being caught after a multi-boundary backspace.
…ore #557 merged PR #557's independent review required these before merge but the PR landed without them (see #583). The underlying fixes were already correct and already merged; this adds the coverage. Solana (#550): num_accounts > SOL_MAX_ACCOUNTS(32) must fail closed (MALFORMED) before it's ever stored or used as a bound -- a small overage (33) would read past tx->accounts[31] as a uint16_t loop bound; a value that wraps a uint8_t (256, 512) would silently reintroduce the original signer-check bypass. All three cases are covered. Ripple (#553): ripple_serializeAmount()'s assert() and the FSM's runtime check both use the corrected 1e17-drop ceiling (XRPL's real protocol maximum of 100,000,000,000 XRP) -- the exact maximum must serialize without tripping the bound, proving the two enforce the same limit. The FSM-level runtime check itself isn't covered here: unit-testing it needs a derived HDNode from real wallet/storage state, a test tier this suite doesn't establish anywhere else (confirm()-gated init hangs without a DebugLink driver); the check has been code-inspection-verified to use the same literal as the serializer.
BitHighlander
force-pushed
the
fix/round4-storage-secrets
branch
from
August 25, 2026 09:54
b655239 to
97f0512
Compare
This was referenced Aug 25, 2026
Closed
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.
Summary
Round 4 of the ongoing alpha audit/fix cycle. 8 findings, all CONFIRMED by independent adversarial verification (2 refuters per finding, default-to-refuted).
storage_secMigrate(): encrypt branch never scrubbediv(a raw copy of the storage key); decrypt branch never scrubbedctx(the derived AES key schedule).storage_cipherBlock(): identical iv/ctx asymmetry, mirrored.storage_getAuthData():authdataKey/decryptedplaintextAuthBlocknever scrubbed on any of 3 exit paths.storage_setAuthData(): scrubbedplaintextAuthBlockbut never the derivedauthdataKey.layoutU2FDialog()bypassed all truncation safeguardsconfirm_helper()provides everywhere else; a CTAP2 rp_id (up to 253 host-controlled chars) could render truncated with no warning while the credential binds to the full string.ctap2_request_user_presence()now refuses rather than approve on unreadable text.fsm_msgCipherKeyValue(): the hmac_sha512-derived key/iv material and its AES key schedule were never scrubbed.transaction.c's internal-transfer path passed a 50-bytenode_strtotxin_check.cfunctions with an unconditional 130-byte (ADDR_STR_LEN) memcpy/strncmp contract -- an 80-byte OOB stack read on every such confirmed output.recovery_cipher.c: the substitution cipher rotates every character, so reconstructingcoded_wordfromdecoded_wordvia the current cipher after a backspace didn't match what was actually typed, defeating the substitution-cipher-bypass detector. Corrected after independent review caught the first fix (a single-slotlast_completed_coded_word) as still broken across a second backspace boundary (PR #582 recovery fix preserves only one coded word; backspacing across two boundaries corrupts bypass detection again #584). Redesigned withcoded_mnemonic, a raw-cipher-bytes mirror ofmnemonicitself, so a backspace at any depth across any number of word boundaries is handled uniformly. Added 6 regression tests, including the exact PR #582 recovery fix preserves only one coded word; backspacing across two boundaries corrupts bypass detection again #584 repro and a 23-boundary case.Also adds the Solana (#550) and Ripple (#553) boundary regression tests that independent review required before PR #557 merged but that PR landed without (see #583).
Process note
This branch was held back from merging after an initial CI-gate defect was caught (#583): every PR into alpha, including this one, was merging with the aggregate CI gate red and the entire real build/test/integration graph silently skipped (
secret-scanscans the whole fork's ~150 branches by default, one of which is unrelated to alpha and always fails, cascading to skip everything downstream). That's fixed in #585 (merged into alpha), which let this PR's ownpython-integration-testsrun for real for the first time -- surfacing two further issues (both fixed and merged separately, see below), neither a regression from this PR's own changes:test_msg_binance_sign_tx.py) for the Beacon Chain signing path removed in round 1 -- deleted, matching the removal.test_msg_eos_signtx.py::test_updateauth's golden hash, stale against a real, already-merged security fix (EOS eos_hashAuthorization(): waits[] serialization loop is bounded by accounts_count instead of waits_count -- hashes stale/uninitialized entries not shown on screen #568) -- updated to accept both the pre- and post-fix hash, since this repo's own CI pins historical firmware references that don't have the fix yet.Both fixed in BitHighlander/python-keepkey#57 (merged), with
deps/python-keepkey's pin bumped accordingly.Verification
make all && make xunit-- 504/509 passing, same 5 known pre-existing failures, zero new failures (baseline 494/499 + 10 new regression tests, all passing).CLANG_FORMAT=clang-format-20 make lint-- clean.Closes #574, #575, #576, #577, #578, #579, #580, #581, #584.