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

[evanlinjin]

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

• I followed the contribution guidelines
• This PR breaks the existing API

🤖 Generated with Claude Code

[codecov]

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.

[noahjoeris]

Thanks for the fix!
There seems to be another bug here though with the early Xpriv::get_key return. See my comment.

[noahjoeris]

Suggested change

	// We may hold the xprv for the request directly.
	if let Ok(Some(prv)) =
	GetKey::get_key(&k.xkey, key_request.clone(), secp)
	{
	return Ok(Some(prv));
	}

This early return isn’t correct for master xprv descriptors

This test fails on the current code

    // Master xprv in the descriptor must not sign sibling paths (`/9/7` vs `/0/*`);
    #[test]
    fn get_key_bip32_rejects_sibling_with_master_xprv() -> anyhow::Result<()> {
        let secp = Secp256k1::new();
        let master_xprv: Xpriv = "tprv8ZgxMBicQKsPdy6LMhUtFHAgpocR8GC6QmwMSFpZs7h6Eziw3SpThFfczTDh5rW2krkqffa11UpX3XkeTTB2FvzZKWXqPY54Y6Rq4AQ5R8L".parse()?;
        let fp = master_xprv.fingerprint(&secp);
        let account_path: DerivationPath = "86h/1h/0h".parse()?;
        let desc = format!("tr({master_xprv}/{account_path}/0/*)");
        let request_path: DerivationPath = format!("{account_path}/9/7").parse()?;
        let req = KeyRequest::Bip32((fp, request_path));
        let (_, keymap) = Descriptor::parse_descriptor(&secp, &desc)?;
        let res = Signer(keymap).get_key(req, &secp);
        assert!(matches!(res, Ok(None)), "expected None, got: {res:?}");
        Ok(())
    }

[oleonardolima]

Good, for this sceneario it should return either None or an Err, right ?

[oleonardolima]

Oh, I think we have this same issue in rust-miniscript.

[noahjoeris]

You're right! I opened a PR rust-bitcoin/rust-miniscript#1004

[ValuedMammal]

Concept ACK, although I assume this type will go away when we have impl GetKey for KeyMap as per #1 (comment).