Security audit: THP pairing bypass (HIGH) + Monero availability and bitcoin-branch findings

[sethforprivacy]

Security audit — trezor-flutter (THP stack, Monero path, bitcoin branch)

Scope: trunk @ 7d27e19 (THP v2 stack, pairing, Monero sign/key-image paths — the live path used by Cake's cw_monero) and origin/bitcoin @ 9a19b05 (native Bitcoin signing + on-device passphrase; not yet wired into any app). Loss-of-funds focus.
Method: independent line-level review + three parallel deep-dives (THP/crypto, Monero, bitcoin branch), every finding's evidence re-verified against the code before inclusion. Cross-checked against trezor-firmware THP spec (docs/common/thp/specification.md), trezor-suite upstream, the Monero reference host (device_trezor.cpp/protocol.cpp), and a live mainnet Monero tx.
Tests: trunk 28/28 pass; bitcoin branch 33/33 pass.

Summary

#	Severity	Finding
1	HIGH	Rogue BLE device can skip the entire pairing ceremony — spec-mandated HH3 assert missing; silent passphrase capture + receive-address substitution
2	MEDIUM	isAutoconnectPaired reads a stale/arbitrary credential (_pairingCredentials[0]), crashes on empty list — amplifies #1
3	MEDIUM	Key-image sync sends all outputs in one unbatched message — wallets with many outputs can't sync → can't spend (live Monero path)
4	MEDIUM	Decoder off-by-one at exact packet boundary → hang, then nonce desync kills the channel
5	MEDIUM	RequestQueue: no in-flight timeout by default; the timeout path itself deadlocks the queue
6	LOW	Error path doesn't drain the in-flight response → next op receives the previous op's response
7	LOW	THP reliability layer absent (no retransmission, no sync-bit/ack validation)
8	LOW	Hand-rolled curve25519: variable-time on secrets, no low-order DH output check (not currently load-bearing)
9	LOW	Example app commits a live autoconnect credential incl. host static private key
10	LOW	Monero signed-tx serialization hardcodes unlock_time=0 and BP+ count=1
11	LOW	No offloaded-bulletproof (rsig) flow — Monero txs with >2 outputs abort on device
12	LOW	FinalAck tx-key material discarded → payment proofs (get_tx_proof) impossible
13	LOW	prevTxs key contract documents the wrong txid byte order (bitcoin branch — breaks first integration, fail-closed)
14	LOW	V1 client silently ignores onDevicePassphrase → empty-passphrase standard wallet with no prompt (bitcoin branch)
15	LOW	Passphrase auto-enable sniffs generic code == 3, is a one-way ratchet on a device-global setting, and throws an unexported exception (bitcoin branch)
16	INFO	Credential lifecycle deviations vs upstream; 6-digit code not normalized before CPace
17	INFO	Bitcoin: multisig signature slots overwrite per-index; device-supplied indexes raise raw RangeError; dead code + debug prints in new paths

---

1 — HIGH: rogue device skips pairing; missing spec-mandated HH3 assert

trezor-flutter/lib/src/connect/handshake.dart:52-62:

state.isPaired = requiresParing != 0;
...
if (state.isAutoconnectPaired || requiresParing == 2) {
  // State HC1 -> HC2 pairing complete
  await thpPairingEnd(connection, state);
}

The THP spec is explicit: when the Trezor is unknown (no credential matched), the host is in state HH3, and HH3 step 3 mandates Assert that trezor_state == STATE_UNPAIRED (specification.md:712-719). That assert does not exist in this port. The Noise handshake by itself provides no device authentication; the only control forcing an unknown endpoint into the user-interactive code-entry ceremony is this assert. Note also the spec's channel-replacement section (:1307-1318): STATE_PAIRED_AUTOCONNECT (0x02) is only ever supposed to follow a validated credential (channel replacement with the same host static key) — never a credential-less handshake.

Scenario: BLE-proximity attacker advertises a spoofed Trezor (on iOS there is no OS-level BLE pairing gate — trezor_gatt_gateway.dart:44-50 returns immediately on non-Android). The app connects; the attacker completes the handshake with its own keys and claims trezor_state=0x02. The app treats the channel as paired with zero user interaction — no "Allow pairing?", no 6-digit code. Then:

• thpCreateSession sends ThpCreateNewSession(passphrase: ...) (connect/pairing.dart:157) inside the attacker-terminated channel → wallet passphrase captured;
• every subsequent xpub/address request is answered by the attacker → the wallet displays attacker-controlled receive addresses → deposits are stolen.

Loss of funds with no trust-anchor involvement. (For fairness: trezor-suite upstream has the same state === 2 trust at packages/connect/src/device/thp/handshake.ts:135 — the port faithfully mirrors it — but Suite has compensating UI controls; this plugin has none, so the assert is the only gate.)

Fix: implement the HH3 assert — when no credential was offered/validated (cred.credentials == null), require requiresParing == 0 and abort otherwise; treat state == 2 without a validated credential as a protocol violation. Only skip the ceremony when the credential that matched this device has autoconnect == true. Also evict rejected credentials (state==0 with a credential offered → removePairingCredential) like upstream.

2 — MEDIUM: isAutoconnectPaired reads _pairingCredentials[0] (stale/arbitrary), crashes on empty list

trezor/protocol/v2/state.dart:154: bool get isAutoconnectPaired => isPaired && _pairingCredentials[0].autoconnect == true; — and setPairingCredentials (:140-147) accumulates via addAll (never replaced, no dedup); fromJson pre-loads persisted credentials before the handshake; handshake.dart:30 appends this device's matches at the end. So [0] may be a credential for a different device entirely.

• (a) Amplifies feat: add passphrase session support #1: a rogue claiming 0x01 (not just 0x02) is silently "paired" whenever the app holds any autoconnect credential.
• (b) Rogue claims 0x01 on a fresh install (empty list) → RangeError → unhandled async crash instead of a clean protocol abort.

Fix: replace instead of accumulate, and define autoconnect-paired from the credential that actually matched this handshake (cred.credentials), guarding the empty list.

3 — MEDIUM: key-image sync is unbatched (live Monero path)

connect/coins/monero/sync_keyimages.dart:37-62 builds one MoneroKeyImageSyncStepRequest containing every tdi and sends it in a single call. The reference host batches (batch_size = 10, device_trezor.cpp:340-361) and the firmware is designed for incremental steps (key_image_sync.py). A long-lived wallet with hundreds/thousands of outputs produces a multi-hundred-KB protobuf the device must buffer/parse in RAM; sync fails → spent status unknown → the user cannot construct a spend at all until the plugin is fixed. Funds aren't lost but are stranded through this integration.

Fix: chunk tdis (10/step) into repeated step requests, accumulate kis from each ack, send Final only after all chunks. The init hash covers the full ordered list and firmware verifies incrementally, so chunking weakens nothing.

4 — MEDIUM: decoder off-by-one at exact packet boundary

trezor/protocol/decoder.dart:42-45: if (length >= reader.remainingLength) { ... return TrezorPackageV2(headers: headers, payload: payload); } (no CRC → treated as "needs continuation"). A complete response whose payload+CRC exactly fills the padded transport packet (length == remainingLength, zero padding bytes) is misclassified as incomplete. Reassembly then waits for a continuation that never comes; the next device message gets spliced in, CRC validation throws — and because nonces only advance on successful GCM decrypt, host/device nonces are now desynced, so every subsequent decrypt fails: the channel is dead until full reconnect. Size-dependent; a MITM can nudge sizes via proxied requests. Mid-signing, this strands the session.

Fix: >= → >; assert continuation packets carry control byte 0x80 and the same channel id as the initiation packet; discard partial payloads when a new initiation packet arrives mid-reassembly (per spec) instead of splicing.

5 — MEDIUM: RequestQueue has no default timeout, and the timeout path deadlocks

trezor_connection.dart:11 constructs RequestQueue() with inFlightTimeout: null — no timeout ever fires. Even when set, concurrency/request_queue.dart:86-88 does await Future.any([resultFuture, completer.future]); completer.completeIfPending(await resultFuture); — after a timeout it still awaits the hung resultFuture, so _sendNext never returns and _isSending stays true forever. One lost BLE notification (or #4, or a device reboot mid-op) silently wedges every subsequent operation — the wallet spins forever with no error, including mid-pairing/mid-signing.

Fix: set a sane default timeout; on timeout, error-complete the request without awaiting resultFuture, mark the channel desynced, and force re-handshake before further ops.

6 — LOW: error path doesn't drain the in-flight response

trezor_gatt_gateway.dart:316-321 errors out the pending op on a malformed/injected packet, but the device's real response is still in flight; when it arrives it's routed to the next op (:150-152). Since the failed op never advanced recvNonce, the stale response decrypts successfully and is returned to the wrong caller. Response types are never validated against requests (state.dart:262 expectedResponses is dead code; thpCall accepts anything that isn't Failure/ButtonRequest). GCM prevents content forgery, so worst case is cross-request type confusion / exception cascade — but there is no defense-in-depth.

Fix: on any receive-path error, mark the channel desynced, drop queued ops, require re-handshake; populate/enforce expectedResponses per request as upstream does.

7 — LOW: THP reliability layer absent

No retransmission, no sync-bit/ack validation (thp_encrypted_operation.dart:63-78 ignores control byte/channel; ACKs discarded unchecked at gateway.dart:124-127), state.dart:289 recentMessage stored but unused, and the byte-exact dedup (gateway.dart:115,131-136) would suppress a legitimate device retransmission. Packet loss → deadlock (compounded by #5). All integrity failures throw — nothing is silently accepted — so this is availability only.

8 — LOW: hand-rolled curve25519 — variable-time, no low-order check

utils/curve25519.dart:96-102 conditionalSwap is a Dart ternary (the constant-time mask version is commented out); the ladder runs secret-dependent BigInt branches; curve25519 never checks for an all-zero DH output (RFC 7748 "MUST check"; the THP spec mandates the analogous device-side check in TP3a step 4). I traced all DH uses: with Noise XX's mixed DHs and credential-pinned static keys, a low-order injection does not currently yield session keys or bypass pairing — not load-bearing today, but it becomes critical if any future path relies on contributory behavior. (The port is otherwise byte-equivalent to trezor-suite's vector-tested TypeScript; the c4/a24 division bug was already fixed in d1289df.)

Fix: reject all-zero DH outputs at the callers (handleHandshakeInit steps 5/8/14, getSharedSecret) as defense-in-depth; long-term prefer a vetted constant-time X25519.

9 — LOW: example app commits a live autoconnect credential

example/lib/main.dart:281 hardcodes a complete ThpState.fromJson with trezorStaticPublicKey, credential, and hostStaticKey — the host static private key plus an autoconnect credential granting confirmation-free pairing to that dev device, and enough to impersonate that device to any host holding it (the credential mask proves pubkey knowledge, not privkey). Dev-phone data today, but it normalizes committing THP key material; ThpState JSON (credentials incl. hostStaticKey) is persisted unencrypted by design and must be treated as sensitive by integrators.

Fix: remove the committed credential; document that credentials + hostStaticKey are secrets to be kept in platform secure storage.

10 — LOW: Monero signed-tx serialization hardcodes unlock_time=0 and BP+ count=1

sign_transaction.dart:185 writes VarInt.encodeMoneroVarint(0) for unlock_time while the device hashes the real tsx_data.unlock_time; :215 hardcodes the bulletproofs_plus vector count to 1 (// Todo dynamic?). If cw_monero/monero_c ever produces a timelocked tx or multi-BP rsig grouping, the device signs fine but _verifyTransactionPrefix (:267-273) throws after the Final step — every such signing fails. Fails safe (nothing malformed broadcasts), but broken for those txs.

Fix: serialize the real unlockTime; derive the BP+ count from the rsig grouping (or assert single-batch at init).

11 — LOW: no offloaded-bulletproof flow — Monero txs with >2 outputs abort

sign_transaction.dart:71-84 never sends isOffloadedBp/rsigData. Firmware enables rsig offloading when output_count > 2 and then requires the host-computed Bulletproof+ (step_06_set_output.py: "Rsig expected, not provided"). A 2-recipient tx (2 recipients + change = 3 outputs) aborts mid-protocol. Fails safe; multi-recipient Trezor sends are impossible through this plugin.

Fix: implement the offload flow per the reference host (protocol.cpp should_compute_bp_now/compute_bproof, lines ~733-813).

12 — LOW: FinalAck tx-key material discarded → payment proofs impossible

sign_transaction.dart:126-129 uses only finalResponse.openingKey, dropping salt, randMult, and txEncKeys (present in MoneroTransactionFinalAck). The tx secret key r is generated on-device and only ever exported encrypted as txEncKeys; the reference host stores these (protocol.cpp:945-946, store_tx_aux_info) so MoneroGetTxKeyRequest can later recover the tx key for get_tx_proof. Without them, payment proofs are permanently impossible for Trezor-sent txs.

Fix: return salt/randMult/txEncKeys alongside the signed hex (they're view-key-encrypted, safe to expose) and have the caller persist them.

13 — LOW (bitcoin branch): prevTxs key contract documents the wrong txid byte order

coins/bitcoin/sign_transaction.dart:46-47 says "[prevTxs] must be keyed by lowercase txid hex (display byte order — the same bytes as TxInput.prevHash)" — self-contradictory: Trezor's wire protocol uses internal byte order for prev_hash (trezorlib: bytes.fromhex(txid)[::-1]), and :118 keys the map by hex.encode(details.txHash) (the device-echoed, internal-order bytes). An integrator following the doc hits one of two fail-closed errors on every sign. The new test can't catch it — its prevHash is 32×0xaa, byte-order-invariant.

Fix: key by hex.encode(TxInput.prevHash) (internal order) or accept display txid and reverse internally; add a non-palindromic prevHash test.

14 — LOW (bitcoin branch): V1 client silently ignores onDevicePassphrase

trezor_client.dart:71-74 (branch): createChannel({passphrase, onDevicePassphrase}) drops the flag (comment only) and the V1 handler answers PassphraseAck(_passphrase ?? ""). Caller requesting a hidden wallet on a V1 device gets the standard (empty-passphrase) wallet with no prompt — the app shows standard-wallet addresses while the user believes they're in their hidden wallet. Wrong-wallet deposits are recoverable, but it's a nasty footgun.

Fix: if (onDevicePassphrase) throw UnsupportedError(...) (or implement PassphraseAck(onDevice: true) where supported); never auto-ack an empty passphrase when a hidden wallet was explicitly requested.

15 — LOW (bitcoin branch): passphrase auto-enable flow

acquire.dart:51-60 (branch): treats any TrezorFailureException with code == 3 (generic Failure_DataError) as "passphrase feature is off" and fires ApplySettings(usePassphrase: true) — a persistent, device-global, one-way settings change (no disable path), triggered merely because the app passed onDevicePassphrase: true. The quoted premise error can't even fire after 9a19b05 (it required sending a passphrase field with on_device:true, which that commit removed), so the branch is likely dead code that only misfires. And the intended success-path signal, PassphraseEnabledReconnectRequired, isn't exported from the barrel (trezor_flutter.dart exports no exceptions at all) — callers can't catch it via the public API.

Fix: read Features via the (written-but-unused) thpIsPassphraseEnabled before attempting the session instead of error-sniffing; prompt the user explicitly before flipping a device-global setting; export the exceptions.

16 — INFO: credential lifecycle deviations

Non-autoconnect credentials are never offered (handshake.dart:22 filters autoconnect-only; upstream offers all, autoconnect-first) and never persisted (state.dart:309-310), so users re-do the 6-digit ceremony after every app restart (fail-closed friction). Rejected credentials are never evicted. The code-entry challenge is 32 bytes vs spec's 16 (harmless — hashed as transmitted). getCpaceHostKeys uses the raw entered code while the device zero-pads to 6 digits — entering "123" instead of "000123" fails pairing loudly (availability footgun; normalize to %06d).

17 — INFO (bitcoin branch): minor

• Multisig inputs: signatures has one slot per input index (sign_transaction.dart:64,83); cosigner signatures under the same signature_index overwrite — fine while callers use serializedTx (which is authoritative), but document it or key as Map<int, List<Uint8List>>.
• Device-supplied indexes (signatureIndex, extra-data window) can raise raw RangeError instead of TrezorProtocolException (:83, :158-161) — bounds-check and throw the typed error like the other checks.
• Dead code: thpIsPassphraseEnabled never called; the while (res.$1 == buttonRequest) loop and trailing failure check in pairing.dart:176-186 are unreachable (thpCall already handles both); print("[THP-CHANNEL]…") debug lines in library code (phase info only, no secrets).

---

Verified clean (keep)

• Monero input ordering (7d27e19): correct and complete. Comparator sorts by key image descending per consensus, moving vini/hmac/pseudoOut/alpha/spendKey/origIdx together; the device independently enforces the ordering twice, so pre-fix behavior was a fail-safe abort, never a wrong signature. Verified against a live mainnet tx and the firmware checks.
• Monero device anchor intact: outputs/change/fee all displayed and validated on-device (_check_change rejects foreign change); per-output HMAC pairing correct; sealing-key derivation byte-exact vs reference; spend keys only ever relayed as ciphertext; per-run ephemeral randomness is device-fresh, so retries can't reuse nonces; partial signatures stay sealed on abort. Serialization layout verified against a live mainnet tx.
• Key-image sync crypto: the tdi control hash matches firmware compute_hash field-for-field; subaddress lies abort on-device; returned key images carry ring signatures verified at import. Only the batching gap (Security audit: THP pairing bypass (HIGH) + Monero availability and bitcoin-branch findings #3).
• Bitcoin SignTx state machine: faithful request/response loop; prev-tx streaming fully implemented, so for non-Taproot inputs the device independently recomputes input amounts (host can't inflate fees); Taproot amounts committed via BIP-341 sighash; change handling is the correct trust split (device derives change from its own seed — a host can't mark an attacker output as change); fail-closed on unknown prev tx, out-of-range index, unexpected type, missing signature.
• THP handshake crypto: faithful port (transcript order, HKDF chain, IVs, credential mask equation all match spec/upstream); CPace code-entry byte-exact vs reference — a pure relay MITM can't downgrade or learn the code; AES-GCM tag verified on every decrypt (loud failure); channel keys never serialized, so (key, nonce) reuse across restarts is impossible; CRC32 used only as spec-mandated transport framing, never in place of the AEAD tag.
• Randomness: all keys/credentials from Random.secure(); X25519 clamping correct; no hardcoded/default credentials in the library path.
• No secrets logged anywhere in connect/ (grep-verified).

Bottom line

One release-blocker: #1 (+#2) — the pairing bypass is a genuine loss-of-funds vector for the BLE path and the fix is small (implement the HH3 assert + matched-credential autoconnect check). #3 should land soon after (it strands large Monero wallets on the live path). #4/#5 are robustness bugs that turn any hiccup into a dead channel. Everything else is LOW/INFO. Happy to walk through any of it.