From d696e799d923d7e8ef2708fc4717371d188fd03d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=BF=97=E5=AE=87?= Date: Mon, 15 Jun 2026 19:13:09 +0000 Subject: [PATCH] fix(signer): match BIP32 origin via `matches`, not string prefix `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) --- src/signer.rs | 103 ++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 74 insertions(+), 29 deletions(-) diff --git a/src/signer.rs b/src/signer.rs index d0866c3d..f371a096 100644 --- a/src/signer.rs +++ b/src/signer.rs @@ -1,6 +1,4 @@ use alloc::collections::BTreeMap; -use alloc::string::ToString; -use alloc::vec::Vec; use bitcoin::{ psbt::{GetKey, GetKeyError, KeyRequest}, @@ -34,28 +32,30 @@ impl GetKey for Signer { } (_, desc_sk) => { for desc_sk in desc_sk.clone().into_single_keys() { - if let KeyRequest::Bip32((fingerprint, derivation)) = &key_request { - if let DescriptorSecretKey::XPrv(k) = desc_sk { - // We have the xprv for the request - if let Ok(Some(prv)) = - GetKey::get_key(&k.xkey, key_request.clone(), secp) - { - return Ok(Some(prv)); - } - // The key origin is a strict prefix of the request derivation - if let Some((fp, path)) = &k.origin { - if fingerprint == fp - && derivation.to_string().starts_with(&path.to_string()) - { - let to_derive = derivation - .into_iter() - .skip(path.len()) - .cloned() - .collect::>(); - let derived = k.xkey.derive_priv(secp, &to_derive)?; - return Ok(Some(derived.to_priv())); - } - } + if let (DescriptorSecretKey::XPrv(k), KeyRequest::Bip32(key_source)) = + (desc_sk, &key_request) + { + // 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)); + } + // Otherwise let miniscript's `matches` confirm this key + // represents the request (handling the key origin and + // wildcard); a raw prefix check would also derive paths the + // descriptor never declares, e.g. a sibling of the wildcard. + if k.matches(key_source, secp).is_some() { + // `xkey` is anchored at the origin, so strip the origin + // prefix from the master-relative request path; with no + // origin the whole path is relative to `xkey`. + let (_, derivation) = key_source; + let to_derive = match &k.origin { + Some((_, origin)) => &derivation[origin.len()..], + None => derivation.as_ref(), + }; + let derived = k.xkey.derive_priv(secp, &to_derive)?; + return Ok(Some(derived.to_priv())); } } } @@ -70,6 +70,7 @@ impl GetKey for Signer { #[cfg(test)] mod test { use crate::bitcoin::bip32::ChildNumber; + use alloc::string::ToString; use core::str::FromStr; use std::string::String; @@ -147,11 +148,6 @@ mod test { desc: format!("tr([{fp}/{path}]{derived}/0/*)"), derivation: format!("{path}/0/7"), }, - TestCase { - name: "key origin matches request derivation", - desc: format!("tr([{fp}/{path}]{derived}/0/*)"), - derivation: path.to_string(), - }, ]; for test in cases { @@ -172,6 +168,33 @@ mod test { Ok(()) } + // `matches` only signs for keys the descriptor actually represents. Requests + // that share the origin as a prefix but fall outside the declared derivation + // are rejected: the account-level key itself (request == origin) and a sibling + // of the wildcard branch (`/9/*` when the descriptor declares `/0/*`). + #[test] + fn get_key_bip32_rejects_paths_outside_descriptor() -> anyhow::Result<()> { + let secp = Secp256k1::new(); + let xprv: Xpriv = "tprv8ZgxMBicQKsPdy6LMhUtFHAgpocR8GC6QmwMSFpZs7h6Eziw3SpThFfczTDh5rW2krkqffa11UpX3XkeTTB2FvzZKWXqPY54Y6Rq4AQ5R8L".parse()?; + let fp = xprv.fingerprint(&secp); + let path: DerivationPath = "86h/1h/0h".parse()?; + let derived = xprv.derive_priv(&secp, &path)?; + let desc = format!("tr([{fp}/{path}]{derived}/0/*)"); + + for derivation in [path.to_string(), format!("{path}/9/7")] { + let deriv: DerivationPath = derivation.parse()?; + let req = KeyRequest::Bip32((fp, deriv)); + let (_, keymap) = Descriptor::parse_descriptor(&secp, &desc)?; + let res = Signer(keymap).get_key(req, &secp); + assert!( + matches!(res, Ok(None)), + "expected None for {derivation}: {res:?}" + ); + } + + Ok(()) + } + #[test] fn get_key_xpriv_with_key_origin() -> anyhow::Result<()> { let secp = Secp256k1::new(); @@ -202,4 +225,26 @@ mod test { Ok(()) } + + // The origin "84h/1h/0h/1" is a string prefix of "84h/1h/0h/10" even though + // m/84'/1'/0'/1 is NOT a derivation-path prefix of m/84'/1'/0'/10. The signer + // must reject this request instead of leaking the key at the origin path. + #[test] + fn get_key_bip32_string_prefix_not_path_prefix() -> anyhow::Result<()> { + let secp = Secp256k1::new(); + let xprv: Xpriv = "tprv8ZgxMBicQKsPdy6LMhUtFHAgpocR8GC6QmwMSFpZs7h6Eziw3SpThFfczTDh5rW2krkqffa11UpX3XkeTTB2FvzZKWXqPY54Y6Rq4AQ5R8L".parse()?; + let fp = xprv.fingerprint(&secp); + + let origin_path: DerivationPath = "84h/1h/0h/1".parse()?; + let derived = xprv.derive_priv(&secp, &origin_path)?; + let desc = format!("wpkh([{fp}/{origin_path}]{derived}/*)"); + let (_, keymap) = Descriptor::parse_descriptor(&secp, &desc)?; + + let request_path: DerivationPath = "84h/1h/0h/10".parse()?; + let req = KeyRequest::Bip32((fp, request_path)); + let res = Signer(keymap).get_key(req, &secp); + + assert!(matches!(res, Ok(None)), "expected None, got: {res:?}"); + Ok(()) + } }