Conversation
samjay121566
approved these changes
Sep 26, 2024
doc-hex
added
new-feature
The `bad txn version` check (`txn_version in {0,1,2,3}`) lives in
parse_txn(), which only runs for PSBTv0. On the PSBTv2 path read_psbt()
skips parse_txn(), so PSBT_GLOBAL_TX_VERSION was used unvalidated: a v2
PSBT with nVersion 4 (or a negative value) could be approved and signed,
producing a transaction the network will not relay.
validate() already enforces PoR-specific versions (0 only for BIP-322,
{0,2} for PoR txns) but had no upper bound for ordinary transactions. Add
the matching non-PoR bound (txn_version in {1,2,3}) so both v0 and v2
reject out-of-range versions while the BIP-322 PoR path is unchanged.
Add test_psbt_v2_bad_txn_version (versions 4, -1, 100 rejected on v2).
Verified to fail pre-patch (v2 reaches 'OK TO SEND?') and pass with the
fix; full test_bip322.py suite still passes.
Presence of the redeem script is not enough: the input is signed as segwit based on the witness script, while witness_utxo_is_provably_segwit decides amount trust from the redeem script field. A present-but-wrong redeem script passed the new presence check yet still failed the provably-segwit test, leaving the fee unverifiable with only a warning before approval - the same unknown-fee hole as the missing case. Require the redeem script to be exactly the P2WSH wrapper of the witness script. Add regression test with a junk redeem script (verified to reach the approval UX without this fix), and update the changelog.
Gate on required_key instead of num_our_keys: keypath metadata is attacker-controlled, and the zero-xfp placeholder is rewritten to our fingerprint on the fly, so a foreign input carrying a forged keypath plus a partial signature looked both 'ours' and already-signed, and was never rejected nor signed - while the SIGHASH_ALL signature over input zero still committed to its prevout. required_key is only set after the input's pubkey/scripts validate against our seed or WIF store, so gating on it fails closed for all three variants: no keypaths, forged keypaths, and presigned inputs. Add regression test with a foreign UTXO (different seed) carrying a zero-xfp keypath and a garbage partial signature.
bugfix: defer OVC persistence until signing enhancement: improve OVC implementation and size 30 -> 128
In Delta Mode, the Trick PINs menu allowed a coercer to (a) press (6) to read a duress wallet's full seed words / XPRV off screen, and (b) Activate Wallet to load the duress secret as the active temporary seed -- all without a wipe. Every other secret-revealing flow calls wipe_if_deltamode(); these two paths were missed. The master seed itself is safe (hardened derivation), but the duress wallet's funds and the user's deniability were exposed. Keep menu browsing working (Delta Mode should look like normal operation), but call wipe_if_deltamode() before the (6) viewer reads the slot and before Activate Wallet applies the secret. Add test_deltamode_duress_secret_view_wipes: browsing the menu works in delta mode, but viewing the duress secret and activating the wallet each trigger a wipe. Verified to fail pre-patch (no wipe) and pass with the fix.
FW_MAX_LENGTH_MK4 now bounds only the firmware itself to the world-checksum-covered flash region (4k aligned, no DFU-wrapper slack carved out). New FW_MAX_DFU_SIZE_MK4 adds room for the bootloader element and DFU wrapper so multi-element -factory.dfu images remain selectable in the MicroSD file picker.
psram_copy_file() and psram_mmap_file() trusted FAT metadata exposed over USB Mass Storage: a cluster chain covering more bytes than the directory entry's file size made 'len = actual_len - so_far' underflow to ~4GB, causing an out-of-bounds memcpy across PSRAM from a compromised USB host. - validate cluster-chain coverage against file size before each fragment - check destination bounds with 64-bit arithmetic *before* writing (the old post-memcpy check was too late, and dest+len+3 could wrap) Q1 shares this file via symlink; both platforms are fixed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Two residual hostile-FAT shapes bypassed the guards: - num_clusters (DWORD from the link map) was narrowed to uint16_t by block_to_ptr(); 65536 wrapped to 0 and passed the range check, mapping fragments outside the PSRAM window. Now validated as u32 (1..BLOCK_COUNT) before the call in both psram_copy_file() and psram_mmap_file(). - The declared final remainder was not checked against the final fragment's physical capacity: a 1024-byte directory entry backed by one 512-byte cluster made the final branch copy/map past the fragment. Now the fragment capacity is preserved and the final remainder must fit it (checked before the copy path's align-4). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Review nit from scgbckbone. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…ing fragments Finding 1: CREATE_LINKMAP reports run lengths in clusters, but the code treats them as 512-byte sectors; a hostile host can rewrite the BPB so FatFs mounts a filesystem with csize > 1, breaking block_to_ptr() and capacity math. Reject vfs.fatfs.csize != 1 right after f_mount() in both psram_mmap_file() and psram_copy_file() (the explicit-reject option). Finding 2: when preceding fragments exactly cover objsize, the final branch computed len == 0 and accepted the oversized chain, making mmap() return a spurious zero-length tuple. Now reject a final fragment whose remainder is < 1 (covers zero-size files with an allocated chain). Also merged PR #763 head (dec0158) into this branch (changelog conflict resolved, keeping both entries) and updated the changelog wording. Validated with a python logic harness and a gcc-compiled C harness replicating both fixed loops: legit shapes pass, all hostile FAT shapes (csize>1, u16 wrap, oversized chains, trailing fragments, edge clusters) are rejected. Co-authored-by: Amiga500 <16525337+Amiga500@users.noreply.github.com>
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.
This will be a long-lived PR / branch containing "edge" firmware differences.