Repository navigation
Require on-device confirmation for host XPUB/fingerprint requests - #11
schnuartz-ai wants to merge 16 commits into
Conversation
Manual test scripts for reviewersTwo small standalone helper scripts to exercise this change against real hardware over USB-serial, exactly like the wire protocol in How to test
|
|
Pushed an update: added scoped multi-XPUB authorization on top of the individual-confirmation flow already in this PR (this addresses the "one confirmation per XPUB is painful for BIP-138-style recovery" concern raised on the upstream PR at cryptoadvance#381). New: Key security properties (full writeup in
Also added New testing scripts added in this update:
Tests run this session:
Generated by Claude Code |
|
Per feedback, the new test scripts have been dropped from the committed tree (see the follow-up commit) and are posted here instead. They're not part of the PR diff - paste them locally under the paths noted if you want to run them. All three passed against this branch before removal:
|
|
Companion script for manually testing this PR against real USB hardware: RequirementsThen: The script finds the device automatically (USB serial / simulator TCP ports), connects, and shows a numbered menu. Just enter a number + Enter — no need to type raw protocol commands. How it's builtIt's a thin menu layer over
Option 5 is pure client-side logic ( How to test this PR with itA) Core feature — one authorization, many queries with no further confirmation
B) Outside the scope still prompts individually C) Regression check — with no active authorization, everything behaves exactly like before this PR D) Ending an authorization E) Budget/consumption
|
e48704c to
4249b20
Compare
Two hardening fixes for the scoped xpub authorization threat model (malicious host): 1. Bound host input before it is decoded/parsed. The `xpubauth` and `xpub` command handlers previously did `stream.read().strip().decode()` with no length limit, so a multi-megabyte line was buffered as a Python str and stripped before MAX_SCOPE_LEN was ever checked - a realistic memory-exhaustion path on the STM32F469. Both handlers now read at most a fixed cap (MAX_SCOPE_COMMAND_LEN / MAX_XPUB_PATH_LEN) and reject anything longer before .decode(). 2. Fail closed when a new `xpubauth begin` arrives. A cancelled or malformed replacement request used to leave the previous (possibly broader) authorization active - a user reacting to a suspicious prompt by pressing Cancel could reasonably assume nothing is authorized. `xpubauth_begin` now drops any prior authorization up front, before parsing or displaying the new scope, so Cancel/error always leaves no authorization at all. Docs and native/integration tests updated accordingly.
`except:` around decode()/bip32.parse_path masked any internal error as "Invalid path". Catch only (ValueError, IndexError) - the exceptions those two calls actually raise for bad host input (UnicodeError is a ValueError subclass; parse_path raises ValueError/IndexError on a malformed or empty component) - so genuine bugs propagate instead. Add a regression test that malformed and non-UTF-8 paths still surface as AppError.
xpubauth begin <scope> already refuses to authorize a scope whose
standard-purpose coin type doesn't match the network active on the
device. A plain, non-scoped `xpub <path>` request had no such guard: the
firmware would happily derive and prompt "Share Xpub?" for e.g. a
testnet-coin-type path while the device is set to mainnet, and hand it
over on Confirm.
Apply the same rule there. When a standard-purpose path (44'/48'/49'/
84'/86'/87') has a coin type that doesn't match the device's active
network, refuse before ever showing the normal Confirm/Cancel prompt:
- Show the local user an explanation screen ("Host tried to get access
to the following key") naming the requested derivation and the
network the device is actually on, with a single OK to dismiss - this
is not a Confirm/Cancel, there is nothing to approve.
- Reply to the host with a machine-parseable error
("network mismatch: device is on <network>") instead of the generic
"User cancelled"/silence a Cancel would produce, so companion
software can tell its user which network to switch the device to
instead of just failing.
New src/apps/xpubs/scope.py:standard_path_coin_type_mismatch(path,
expected_coin_type) applies the same check _validate_standard_coin_type
already applies to a scope entry, to a single already-parsed path.
Non-standard/custom paths are left alone, same as for scopes - they carry
no implied network semantics.
Companion to cryptoadvance/specter-desktop#2702, which parses this error
on the client side to show "your device is on X, switch to Y" instead of
a generic failure.
Several pre-existing tests used mainnet-coin-type paths (m/84h/0h/0h, m/48h/0h/7h/2h) against a keystore/app fixture configured for network "test", and a testnet-coin-type path (m/84h/1h/0h) against a "main" fixture in the xpubauth out-of-scope test. That was harmless before this branch (no network check existed), but now correctly trips the new network-mismatch guard instead of reaching the normal Confirm/Cancel prompt these tests expected. Fix the paths to match each fixture's actual network, and add a dedicated test asserting the mismatched one is refused outright (not just "still prompts because it's out of scope").
The refusal screen said "switch the device to the matching network first" without saying which one. Name it: coin type 0' -> "Mainnet", 1776' -> "Liquid", 1' -> "Testnet, Signet or Regtest" (that coin type is shared, so it can only name the candidates). Unrecognized coin types keep the generic wording. New scope.py:network_hint_for_path(path).
The refusal screen was OK-only - the user had to find the network picker themselves. Make it a two-button prompt: "OK" dismisses, "Network settings" takes them straight to the network selection menu (via a new root cross-app command `select_network`, same pattern as the existing `set_mnemonic` one). The host request still fails either way - after switching, the host retries. 117/117 native tests pass.
Replace the b"select_network" root cross-app command (which faked a host stream to trigger navigation) with a NetworkSwitchRequested exception the app raises and Specter.process_host_request() handles: it opens the network picker and then still reports the failure to the host. The app no longer needs to know how navigation happens or to construct a fake stream; Specter, which already owns the host-request flow and the GUI, decides. NetworkSwitchRequested is a plain Exception (not BaseError) so it isn't mistaken for a host-facing error. 118/118 native tests pass.
When the user tapped "Network settings" on the wrong-network refusal screen, process_host_request awaited select_network(), which loads the picker with gui.menu() -> load_screen(). That deletes the lvgl screen the main menu loop is still awaiting a result on, so after picking a network (or backing out) navigation was wedged and the back button did nothing. select_network() now takes an optional show_fn; when it's driven from a host-command handler it renders the picker through that show_fn (a popup), exactly like the refusal Prompt right before it - the background menu screen stays alive and is restored when the popup closes. The settings-menu caller is unchanged.
…onal The wrong-network screen no longer offers a jump-to-network-picker action; it's a single-button informational Alert, then the host still gets the machine-parseable "network mismatch: device is on <net>". Opening the picker from here meant driving the main() menu loop's navigation from the host-command task, which this firmware has no primitive for - the workaround was to stack the picker as a popup over the current menu. That's a lot of plumbing (plus a NetworkSwitchRequested signal type and a show_fn path through select_network) for one button, so it's reverted. A UX note in xpubs.py records that the button would be a real usability win and is worth adding if cross-task navigation ever lands. src/app.py and src/specter.py are back to their pre-guard state.
- title/body say "Xpub" instead of "key" (the host is asking for an extended public key, not a raw key - "key" read as more alarming than it is) - "switch the device to X in the settings first" so the user knows where to go, now that there's no button taking them there
Reorder the `xpub <path>` handler to parse -> depth-check -> normalize -> network validation -> authorization check -> derive -> prompt/return. Previously the xpub was derived first and the network/coin-type check ran after it; the key was never leaked on a mismatch, but a request that's going to be refused shouldn't cost a derivation. Also cap the path depth of a plain request at scope.MAX_PATH_DEPTH (8) - the same limit the scoped parser already enforces. A non-scoped request was previously bounded only by the 1024-byte input cap, which still allows a ~500-level path and the BIP32 derivation work that comes with it.
9bfeb11 to
353ccaf
Compare
353ccaf to
9bfeb11
Compare








Summary
XpubApp.process_host_command()previously returned the master fingerprint or an XPUB at any host-selected derivation path immediately once the device was unlocked, with no trusted-screen confirmation. A malicious or compromised host could silently enumerate the wallet fingerprint and XPUBs for address-monitoring purposes.fingerprintstays non-interactive — it's used by companion software (Specter Desktop, Liana) for device discovery/identification and must respond immediately. Still gated by the unchanged locked-device check.xpub <path>now requires an on-device Confirm/Cancel before responding, showing the normalized derivation path and the derived XPUB. Reject returns no key material via the existingerror: User cancelledprotocol — no wire-protocol changes.hwidevice.py(this repo's bundled reference HWI client) no longer applies a 3-second response timeout toxpubrequests, since a human now has to physically confirm on-device and that can legitimately take longer.get_master_fingerprintkeeps its short timeout.Companion-repo compatibility
The same 3-second-timeout incompatibility exists in Specter Desktop's independent
specter_diy.pyHWI client, and a related cancellation-mapping bug exists inasync-hwi(used by Liana). Both have their own focused fixes in flight:get_pubkey_at_path, adds regression tests.xpubrequest toSpecterError::UserCancelled/HWIError::UserRefused, matching howsign/add_walletalready handle the same cancellation string.async-hwi's already-parsed error type.Test plan
test/run_native_tests.py(CPython + pipembit) — 15/15 pass, includingtest/tests_native/test_xpubs_host_command.py: fingerprint returns immediately without invokingshow_screen; xpub still requires confirmation with the actual requested path and derived xpub shown; reject returns no key material; locked-device check still fires before either branch; malformed/empty paths fail before confirmation.make unix) and ran the frozen-firmware test suite under it (make test→test/run_tests.pyonbin/micropython_unix) — 14/14 pass.test/integration) against the built simulator end-to-end, driving the real GUI event loop —test_get_xpub(fingerprint non-interactive, xpub confirmed) andtest_get_xpub_rejected(xpub rejection) — 4/4 pass.