Skip to content

fix(signer): match BIP32 origin via matches, not string prefix#81

Open
evanlinjin wants to merge 1 commit into
masterfrom
fix/origin_matching
Open

fix(signer): match BIP32 origin via matches, not string prefix#81
evanlinjin wants to merge 1 commit into
masterfrom
fix/origin_matching

Conversation

@evanlinjin

@evanlinjin evanlinjin commented Jun 15, 2026

Copy link
Copy Markdown
Member

Description

Signer::get_key decided whether a key origin (fingerprint, path) covered a KeyRequest::Bip32 derivation by stringifying both paths and using starts_with:

derivation.to_string().starts_with(&path.to_string())

Because DerivationPath renders its ChildNumber components as /-joined decimals, this matched on character boundaries rather than path components. That produces false positives whenever the origin's stringified form is a substring of the request's:

  • origin m/1 vs request m/10 ("10".starts_with("1"))
  • origin m/84'/1'/0'/1 vs request m/84'/1'/0'/10
  • unhardened origin .../0 vs hardened request .../0'

When a spurious match fires and the two paths share a component count, skip(path.len()) leaves an empty to_derive, so the signer returns the private key at the origin path while claiming to satisfy an unrelated request — leaking a key the descriptor was never meant to expose.

This PR delegates the origin/derivation matching to miniscript's DescriptorXKey::matches, which confirms the key actually represents the request (accounting for the key origin and wildcard) instead of doing a raw prefix check. On a match it strips the origin prefix and derives the remainder from the xkey.

Notes to the reviewers

matches performs an equality check (modulo the wildcard), so it is stricter than the previous prefix logic: it rejects requests that share the origin as a prefix but fall outside what the descriptor declares (e.g. the account-level key itself, or a sibling of the wildcard branch). This is the intended behavior.

Tests:

  • get_key_bip32_string_prefix_not_path_prefix — origin m/84'/1'/0'/1, request m/84'/1'/0'/10: old code returns Some(key at origin), fixed code returns None.
  • get_key_bip32_rejects_paths_outside_descriptor — confirms a request equal to the origin and a sibling of the wildcard branch both return None.

Original bug reported via llm-code-review (CWE-697). Discovered by Project Loupe.

Follow-up: remove Signer once upstream is fixed

Signer only exists because miniscript's own GetKey for key maps mis-derives BIP32 requests for descriptors that carry key-origin info (the KeyMapWrapper in 12.3.x derives the full path without stripping the origin; 13.0.0 strips the wrong amount). The upstream fix is rust-miniscript#872, which is not yet released. Once it ships in a release we can bump our minimum minor version of miniscript to, we should drop bdk_tx::Signer entirely and use miniscript's GetKey impl directly.

Changelog notice

Fixed: Signer matched a BIP32 key origin against a derivation request by string prefix instead of path-component prefix, which could cause it to return a private key for an unrelated derivation path.

Before submitting

🤖 Generated with Claude Code

@codecov

codecov Bot commented Jun 15, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 84.61538% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 54.64%. Comparing base (d14f144) to head (d696e79).
⚠️ Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
src/signer.rs 84.61% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #81      +/-   ##
==========================================
+ Coverage   54.01%   54.64%   +0.63%     
==========================================
  Files          12       12              
  Lines        1620     1625       +5     
  Branches       62       61       -1     
==========================================
+ Hits          875      888      +13     
+ Misses        716      712       -4     
+ Partials       29       25       -4     
Flag Coverage Δ
rust 54.64% <84.61%> (+0.63%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@evanlinjin evanlinjin force-pushed the fix/origin_matching branch from 3e7e399 to 58fa5e5 Compare June 15, 2026 19:20
@evanlinjin evanlinjin self-assigned this Jun 15, 2026
@evanlinjin evanlinjin added the bug Something isn't working label Jun 15, 2026
@evanlinjin evanlinjin marked this pull request as ready for review June 15, 2026 19:21
Comment thread src/signer.rs Outdated
`get_key` decided whether a key origin covered a `KeyRequest::Bip32`
derivation by stringifying both paths and using `starts_with`. Since
`DerivationPath` renders components as `/`-joined decimals, this matched
on character boundaries rather than components: `m/1` spuriously matched
`m/10`, and an unhardened origin matched its hardened sibling. The signer
could then return a private key for a derivation the descriptor never
meant to expose.

Delegate the check to miniscript's `DescriptorXKey::matches`, which
confirms the key actually represents the request (handling the key origin
and wildcard), then strip the origin prefix and derive the remainder.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@evanlinjin evanlinjin force-pushed the fix/origin_matching branch from 58fa5e5 to d696e79 Compare June 27, 2026 01:04
@evanlinjin evanlinjin changed the title fix(signer): match BIP32 origin by path prefix, not string prefix fix(signer): match BIP32 origin via matches, not string prefix Jun 27, 2026
@evanlinjin evanlinjin requested a review from oleonardolima June 27, 2026 01:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants