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(()) + } }