From b9a12043b0a93a2fe4ac79eeda65f85c8fafc302 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Tue, 22 Apr 2025 14:43:39 +0000 Subject: [PATCH] bip32: return error when trying to derive a path too deep This restores the Result return from derive_xpriv which we had removed in a previous PR, but such is life. Fixes #4308 --- bitcoin/examples/bip32.rs | 2 +- bitcoin/examples/ecdsa-psbt-simple.rs | 10 +++++---- bitcoin/examples/ecdsa-psbt.rs | 5 +++-- bitcoin/examples/taproot-psbt-simple.rs | 10 +++++---- bitcoin/examples/taproot-psbt.rs | 18 ++++++++++----- bitcoin/src/bip32.rs | 29 +++++++++++++++---------- bitcoin/src/psbt/mod.rs | 16 +++++--------- bitcoin/tests/bip_174.rs | 3 ++- 8 files changed, 55 insertions(+), 38 deletions(-) diff --git a/bitcoin/examples/bip32.rs b/bitcoin/examples/bip32.rs index 6cc56697e..7cde6c646 100644 --- a/bitcoin/examples/bip32.rs +++ b/bitcoin/examples/bip32.rs @@ -38,7 +38,7 @@ fn main() { // derive child xpub let path = "84h/0h/0h".parse::().unwrap(); - let child = root.derive_xpriv(&secp, &path); + let child = root.derive_xpriv(&secp, &path).expect("only deriving three steps"); println!("Child at {}: {}", path, child); let xpub = Xpub::from_xpriv(&secp, &child); println!("Public key at {}: {}", path, xpub); diff --git a/bitcoin/examples/ecdsa-psbt-simple.rs b/bitcoin/examples/ecdsa-psbt-simple.rs index ac5d84b43..21b9f6cad 100644 --- a/bitcoin/examples/ecdsa-psbt-simple.rs +++ b/bitcoin/examples/ecdsa-psbt-simple.rs @@ -62,11 +62,12 @@ fn get_external_address_xpriv( ) -> Xpriv { let derivation_path = BIP84_DERIVATION_PATH.into_derivation_path().expect("valid derivation path"); - let child_xpriv = master_xpriv.derive_xpriv(secp, &derivation_path); + let child_xpriv = + master_xpriv.derive_xpriv(secp, &derivation_path).expect("only deriving three steps"); let external_index = ChildNumber::ZERO_NORMAL; let idx = ChildNumber::from_normal_idx(index).expect("valid index number"); - child_xpriv.derive_xpriv(secp, &[external_index, idx]) + child_xpriv.derive_xpriv(secp, &[external_index, idx]).expect("only deriving two more steps") } // Derive the internal address xpriv. @@ -77,11 +78,12 @@ fn get_internal_address_xpriv( ) -> Xpriv { let derivation_path = BIP84_DERIVATION_PATH.into_derivation_path().expect("valid derivation path"); - let child_xpriv = master_xpriv.derive_xpriv(secp, &derivation_path); + let child_xpriv = + master_xpriv.derive_xpriv(secp, &derivation_path).expect("only deriving three steps"); let internal_index = ChildNumber::ONE_NORMAL; let idx = ChildNumber::from_normal_idx(index).expect("valid index number"); - child_xpriv.derive_xpriv(secp, &[internal_index, idx]) + child_xpriv.derive_xpriv(secp, &[internal_index, idx]).expect("only deriving two more steps") } // The address to send to. diff --git a/bitcoin/examples/ecdsa-psbt.rs b/bitcoin/examples/ecdsa-psbt.rs index e6bbaf1e6..376604911 100644 --- a/bitcoin/examples/ecdsa-psbt.rs +++ b/bitcoin/examples/ecdsa-psbt.rs @@ -118,11 +118,12 @@ impl ColdStorage { // Hardened children require secret data to derive. let path = "84h/0h/0h".into_derivation_path()?; - let account_0_xpriv = master_xpriv.derive_xpriv(secp, &path); + let account_0_xpriv = + master_xpriv.derive_xpriv(secp, &path).expect("derivation path is short"); let account_0_xpub = Xpub::from_xpriv(secp, &account_0_xpriv); let path = INPUT_UTXO_DERIVATION_PATH.into_derivation_path()?; - let input_xpriv = master_xpriv.derive_xpriv(secp, &path); + let input_xpriv = master_xpriv.derive_xpriv(secp, &path).expect("derivation path is short"); let input_xpub = Xpub::from_xpriv(secp, &input_xpriv); let wallet = ColdStorage { master_xpriv, master_xpub }; diff --git a/bitcoin/examples/taproot-psbt-simple.rs b/bitcoin/examples/taproot-psbt-simple.rs index 8181573a2..d2ca8f9e2 100644 --- a/bitcoin/examples/taproot-psbt-simple.rs +++ b/bitcoin/examples/taproot-psbt-simple.rs @@ -60,11 +60,12 @@ fn get_external_address_xpriv( ) -> Xpriv { let derivation_path = BIP86_DERIVATION_PATH.into_derivation_path().expect("valid derivation path"); - let child_xpriv = master_xpriv.derive_xpriv(secp, &derivation_path); + let child_xpriv = + master_xpriv.derive_xpriv(secp, &derivation_path).expect("only deriving three steps"); let external_index = ChildNumber::ZERO_NORMAL; let idx = ChildNumber::from_normal_idx(index).expect("valid index number"); - child_xpriv.derive_xpriv(secp, &[external_index, idx]) + child_xpriv.derive_xpriv(secp, &[external_index, idx]).expect("only deriving two more steps") } // Derive the internal address xpriv. @@ -75,11 +76,12 @@ fn get_internal_address_xpriv( ) -> Xpriv { let derivation_path = BIP86_DERIVATION_PATH.into_derivation_path().expect("valid derivation path"); - let child_xpriv = master_xpriv.derive_xpriv(secp, &derivation_path); + let child_xpriv = + master_xpriv.derive_xpriv(secp, &derivation_path).expect("only deriving three steps"); let internal_index = ChildNumber::ONE_NORMAL; let idx = ChildNumber::from_normal_idx(index).expect("valid index number"); - child_xpriv.derive_xpriv(secp, &[internal_index, idx]) + child_xpriv.derive_xpriv(secp, &[internal_index, idx]).expect("only deriving two more steps") } // Get the Taproot Key Origin. diff --git a/bitcoin/examples/taproot-psbt.rs b/bitcoin/examples/taproot-psbt.rs index 92af85156..d2914bd7f 100644 --- a/bitcoin/examples/taproot-psbt.rs +++ b/bitcoin/examples/taproot-psbt.rs @@ -299,7 +299,7 @@ fn generate_bip86_key_spend_tx( .ok_or("missing Taproot key origin")?; let secret_key = - master_xpriv.derive_xpriv(secp, &derivation_path).to_private_key().inner; + master_xpriv.derive_xpriv(secp, &derivation_path)?.to_private_key().inner; sign_psbt_taproot( secret_key, input.tap_internal_key.unwrap(), @@ -393,8 +393,11 @@ impl BenefactorWallet { // We use some other derivation path in this example for our inheritance protocol. The important thing is to ensure // that we use an unhardened path so we can make use of xpubs. let derivation_path = format!("101/1/0/0/{}", self.next).parse::()?; - let internal_keypair = - self.master_xpriv.derive_xpriv(&self.secp, &derivation_path).to_keypair(&self.secp); + let internal_keypair = self + .master_xpriv + .derive_xpriv(&self.secp, &derivation_path) + .expect("derivation path is short") + .to_keypair(&self.secp); let beneficiary_key = self.beneficiary_xpub.derive_xpub(&self.secp, &derivation_path)?.to_x_only_public_key(); @@ -486,6 +489,7 @@ impl BenefactorWallet { let new_internal_keypair = self .master_xpriv .derive_xpriv(&self.secp, &new_derivation_path) + .expect("derivation path is short") .to_keypair(&self.secp); let beneficiary_key = self .beneficiary_xpub @@ -538,6 +542,7 @@ impl BenefactorWallet { let secret_key = self .master_xpriv .derive_xpriv(&self.secp, &derivation_path) + .expect("derivation path is short") .to_private_key() .inner; sign_psbt_taproot( @@ -660,8 +665,11 @@ impl BeneficiaryWallet { for (x_only_pubkey, (leaf_hashes, (_, derivation_path))) in &psbt.inputs[0].tap_key_origins.clone() { - let secret_key = - self.master_xpriv.derive_xpriv(&self.secp, &derivation_path).to_private_key().inner; + let secret_key = self + .master_xpriv + .derive_xpriv(&self.secp, &derivation_path)? + .to_private_key() + .inner; for lh in leaf_hashes { let sighash_type = TapSighashType::All; let hash = SighashCache::new(&unsigned_tx).taproot_script_spend_signature_hash( diff --git a/bitcoin/src/bip32.rs b/bitcoin/src/bip32.rs index 735734499..e94c32e2a 100644 --- a/bitcoin/src/bip32.rs +++ b/bitcoin/src/bip32.rs @@ -185,6 +185,9 @@ impl ChildNumber { /// Returns the child number that is a single increment from this one. pub fn increment(self) -> Result { + // Bare addition in this function is okay, because we have an invariant that + // `index` is always within [0, 2^31 - 1]. FIXME this is not actually an + // invariant because the fields are public. match self { ChildNumber::Normal { index: idx } => ChildNumber::from_normal_idx(idx + 1), ChildNumber::Hardened { index: idx } => ChildNumber::from_hardened_idx(idx + 1), @@ -706,7 +709,7 @@ impl Xpriv { &self, secp: &Secp256k1, path: &P, - ) -> Xpriv { + ) -> Result { self.derive_xpriv(secp, path) } @@ -717,16 +720,20 @@ impl Xpriv { &self, secp: &Secp256k1, path: &P, - ) -> Xpriv { + ) -> Result { let mut sk: Xpriv = *self; for cnum in path.as_ref() { - sk = sk.ckd_priv(secp, *cnum) + sk = sk.ckd_priv(secp, *cnum)?; } - sk + Ok(sk) } /// Private->Private child key derivation - fn ckd_priv(&self, secp: &Secp256k1, i: ChildNumber) -> Xpriv { + fn ckd_priv( + &self, + secp: &Secp256k1, + i: ChildNumber, + ) -> Result { let mut engine = HmacEngine::::new(&self.chain_code[..]); match i { ChildNumber::Normal { .. } => { @@ -750,14 +757,14 @@ impl Xpriv { let tweaked = sk.add_tweak(&self.private_key.into()).expect("statistically impossible to hit"); - Xpriv { + Ok(Xpriv { network: self.network, - depth: self.depth + 1, + depth: self.depth.checked_add(1).ok_or(DerivationError::MaximumDepthExceeded)?, parent_fingerprint: self.fingerprint(secp), child_number: i, private_key: tweaked, chain_code: ChainCode::from_hmac(hmac), - } + }) } /// Decoding extended private key from binary data according to BIP 32 @@ -910,7 +917,7 @@ impl Xpub { Ok(Xpub { network: self.network, - depth: self.depth + 1, + depth: self.depth.checked_add(1).ok_or(DerivationError::MaximumDepthExceeded)?, parent_fingerprint: self.fingerprint(), child_number: i, public_key: tweaked, @@ -1188,7 +1195,7 @@ mod tests { let mut pk = Xpub::from_xpriv(secp, &sk); // Check derivation convenience method for Xpriv - assert_eq!(&sk.derive_xpriv(secp, &path).to_string()[..], expected_sk); + assert_eq!(&sk.derive_xpriv(secp, &path).unwrap().to_string()[..], expected_sk); // Check derivation convenience method for Xpub, should error // appropriately if any ChildNumber is hardened @@ -1203,7 +1210,7 @@ mod tests { // Derive keys, checking hardened and non-hardened derivation one-by-one for &num in path.0.iter() { - sk = sk.ckd_priv(secp, num); + sk = sk.ckd_priv(secp, num).unwrap(); match num { Normal { .. } => { let pk2 = pk.ckd_pub(secp, num).unwrap(); diff --git a/bitcoin/src/psbt/mod.rs b/bitcoin/src/psbt/mod.rs index 955af4372..3cdb9b32b 100644 --- a/bitcoin/src/psbt/mod.rs +++ b/bitcoin/src/psbt/mod.rs @@ -775,14 +775,14 @@ impl GetKey for Xpriv { KeyRequest::XOnlyPubkey(_) => Err(GetKeyError::NotSupported), KeyRequest::Bip32((fingerprint, path)) => { let key = if self.fingerprint(secp) == *fingerprint { - let k = self.derive_xpriv(secp, &path); + let k = self.derive_xpriv(secp, &path).map_err(GetKeyError::Bip32)?; Some(k.to_private_key()) } else if self.parent_fingerprint == *fingerprint && !path.is_empty() && path[0] == self.child_number { let path = DerivationPath::from_iter(path.into_iter().skip(1).copied()); - let k = self.derive_xpriv(secp, &path); + let k = self.derive_xpriv(secp, &path).map_err(GetKeyError::Bip32)?; Some(k.to_private_key()) } else { None @@ -915,8 +915,8 @@ impl_get_key_for_xonly_map!(HashMap); #[derive(Debug, Clone, PartialEq, Eq)] #[non_exhaustive] pub enum GetKeyError { - /// A bip32 error. - Bip32(bip32::ParseError), + /// A bip32 derivation error. + Bip32(bip32::DerivationError), /// The GetKey operation is not supported for this key request. NotSupported, } @@ -930,7 +930,7 @@ impl fmt::Display for GetKeyError { use GetKeyError::*; match *self { - Bip32(ref e) => write_err!(f, "a bip23 error"; e), + Bip32(ref e) => write_err!(f, "bip32 derivation"; e), NotSupported => f.write_str("the GetKey operation is not supported for this key request"), } @@ -949,10 +949,6 @@ impl std::error::Error for GetKeyError { } } -impl From for GetKeyError { - fn from(e: bip32::ParseError) -> Self { GetKeyError::Bip32(e) } -} - /// The various output types supported by the Bitcoin network. #[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)] #[non_exhaustive] @@ -1454,7 +1450,7 @@ mod tests { ChildNumber::from_normal_idx(31337).unwrap(), ]; - sk = sk.derive_xpriv(secp, &dpath); + sk = sk.derive_xpriv(secp, &dpath).unwrap(); let pk = Xpub::from_xpriv(secp, &sk); diff --git a/bitcoin/tests/bip_174.rs b/bitcoin/tests/bip_174.rs index c9ecef166..90f4f2118 100644 --- a/bitcoin/tests/bip_174.rs +++ b/bitcoin/tests/bip_174.rs @@ -315,7 +315,8 @@ fn parse_and_verify_keys( let path = derivation_path.into_derivation_path().expect("failed to convert derivation path"); - let derived_priv = ext_priv.derive_xpriv(secp, &path).to_private_key(); + let derived_priv = + ext_priv.derive_xpriv(secp, &path).expect("derivation path too long").to_private_key(); assert_eq!(wif_priv, derived_priv); let derived_pub = derived_priv.public_key(secp); key_map.insert(derived_pub, derived_priv);