fix(security): fault injection hardening, nanopb memleak, BIP39 recovery validation#384
Open
BitHighlander wants to merge 7 commits intodevelopfrom
Open
fix(security): fault injection hardening, nanopb memleak, BIP39 recovery validation#384BitHighlander wants to merge 7 commits intodevelopfrom
BitHighlander wants to merge 7 commits intodevelopfrom
Conversation
- Point device-protocol and python-keepkey submodules to upstream master (includes BIP-85, Solana, Tron, TON wire IDs and proto definitions) - Add nanopb .options files for Solana, Tron, TON (field size constraints) - Add Bip85Mnemonic.mnemonic max_size:241 to messages.options - Update lib/transport/CMakeLists.txt with new proto sources, options, headers, and protoc compilation commands - Fix CI: use pre-installed clang-format instead of apt-get install (eliminates 3-minute timeout on GitHub runners) - Update Zcash transparent branch ID from Sapling to NU6
…double-hash (F3) Replace early-return ECDSA verify chain with bitwise-OR accumulation and sentinel counter. A single voltage glitch can no longer skip one branch to fall through to SIG_OK — attacker must now corrupt all three verify results AND the sentinel. Add double SHA-256 computation with constant-time memcmp_s comparison to detect transient faults during hash computation. References: VULN-21020, fault-injection-assessment.md finding F3
…eck (F5) Remove redundant ~1 second full ECDSA re-verification in firmware main. The bootloader already performed authoritative signature verification before jumping here. Replace with fast metadata presence check that validates signature indices are present and distinct. Eliminates the wide timing window (VULN-21020 class) where a voltage glitch during the long-running crypto re-check could bypass protections. References: VULN-21020, fault-injection-assessment.md finding F5
Moves which_field tag assignment after the conditional memset and only resets when the oneof variant changes. Prevents allocated memory from being zeroed before release when PB_ENABLE_MALLOC is active. Ref: #361 Upstream: nanopb 4fe23595732b6f1254cfc11a9b8d6da900b55b0c
The condition on line 578 was inverted — `!enforce_wordlist` meant the wordlist check was skipped when enforce_wordlist was true (the default). Invalid words were silently accepted and only caught later as a generic "Invalid mnemonic" checksum error, giving users no indication which word was wrong. Flip the condition so that when enforce_wordlist is set, words that fail auto-complete against the BIP39 list are rejected immediately with a clear error before the mnemonic is finalized. Co-Authored-By: Claude Opus 4.6 <[email protected]>
4fc20df to
e107267
Compare
Previously, wordlist validation only happened at finalization after all words were entered. Invalid words were silently accepted and the user had to complete the entire 12/24-word entry before getting a vague "Invalid mnemonic" error with no indication which word was wrong. Add a BIP39 check in the space handler of recovery_character() that rejects immediately with "Word not found in BIP39 wordlist" when the decoded characters don't match any entry. Combined with the enforce_wordlist condition fix, both per-word and finalization validation now work correctly.
e107267 to
76253a7
Compare
2 tasks
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
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.
Motivation
Three independent security improvements bundled together as they're all defensive hardening with no feature dependencies between them.
Changes
1. Fault injection hardening — signatures_ok() (F3)
The existing
signatures_ok()validates firmware signatures by checking each individually and returning on first failure. This is vulnerable to single-glitch fault injection where an attacker skips the failure branch.Fix: Infective aggregation pattern — accumulate all signature results into a single variable using XOR, then double-hash verify the aggregate. A single glitch cannot bypass all checks because the final comparison depends on every intermediate result.
2. Metadata-based signature check (F5)
The firmware-side
signatures_ok()duplicates logic already performed by the bootloader. An attacker who glitches past the bootloader check faces the same vulnerable pattern in firmware.Fix: Replace the firmware-side cryptographic check with a metadata flag set by the bootloader. The bootloader's check is the authoritative one (runs in a more constrained environment); firmware trusts it via a tamper-evident metadata field rather than re-implementing the same vulnerable check.
3. Nanopb oneof memory leak (backport of upstream 4fe23595)
Nanopb's
pb_release()doesn't properly free memory allocated foroneoffields when the active field changes. Over repeated message decode cycles, this leaks memory on the embedded device.Fix: Backport the upstream nanopb fix that tracks and frees the previous oneof allocation before assigning a new one.
4. BIP39 wordlist validation during cipher recovery
The cipher recovery flow accepts any character sequence without validating against the BIP39 wordlist. A user entering an invalid word only discovers the error after completing all 12/24 words, forcing a full restart.
Fix: Add per-word validation after each word is completed during cipher recovery. If the decoded word doesn't match any BIP39 entry, reject immediately with
Failure_SyntaxErrorso the user can correct it in place.Risk assessment
lib/board/signatures.c— critical path but the change is strictly more defensive. The existing tests exercise signature validation.pb_decode.c, isolated to memory management.recovery_cipher.c— user-facing but fail-safe (rejects invalid input rather than accepting it).Testing