From b87ddc0043697b51dea7a7ed41020ea7f6862cd0 Mon Sep 17 00:00:00 2001 From: Martin Habovstiak Date: Tue, 4 Mar 2025 20:25:45 +0100 Subject: [PATCH 1/6] Don't panic in `PrivateKey::from_slice` During upgrade of `secp256k1` a number of function calls needed to be rewritten to not use `from_slice` but `from_byte_array`. Unfortunately, the conversions wasn't correct and caused panics on invalid inputs rather than errors. This fixes it simply by calling `try_into` on the entire slice and converting the error. --- bitcoin/src/crypto/key.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bitcoin/src/crypto/key.rs b/bitcoin/src/crypto/key.rs index 7a045cfb5..d73f84180 100644 --- a/bitcoin/src/crypto/key.rs +++ b/bitcoin/src/crypto/key.rs @@ -466,7 +466,7 @@ impl PrivateKey { ) -> Result { Ok(PrivateKey::new( secp256k1::SecretKey::from_byte_array( - data[..32].try_into().expect("Slice should be exactly 32 bytes"), + data.try_into().map_err(|_| secp256k1::Error::InvalidSecretKey)?, )?, network, )) From 1778fea66ef860774e4a722ff109d000d10f5942 Mon Sep 17 00:00:00 2001 From: Martin Habovstiak Date: Tue, 4 Mar 2025 20:34:59 +0100 Subject: [PATCH 2/6] Add a test checking `PrivateKey::from_slice` This test checks the previous fix - if ordered before the previous commit it will fail. --- bitcoin/src/crypto/key.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/bitcoin/src/crypto/key.rs b/bitcoin/src/crypto/key.rs index d73f84180..2fcacba37 100644 --- a/bitcoin/src/crypto/key.rs +++ b/bitcoin/src/crypto/key.rs @@ -1600,4 +1600,11 @@ mod tests { panic!("Expected Invalid char error"); } } + + #[test] + fn invalid_private_key_len() { + use crate::Network; + assert!(PrivateKey::from_slice(&[1u8; 31], Network::Regtest).is_err()); + assert!(PrivateKey::from_slice(&[1u8; 33], Network::Regtest).is_err()); + } } From 0d5cd7af435f420e76d2e69f0e76be30d230238e Mon Sep 17 00:00:00 2001 From: Martin Habovstiak Date: Tue, 4 Mar 2025 20:39:47 +0100 Subject: [PATCH 3/6] Add `from_byte_array` to `PrivateKey`. Private keys have statically-known length of 32 bytes and we are migrating types with known lenths to use `from_byte_array` methods. This adds the method to `PrivateKey` as well and uses it to implement `from_slice`. --- bitcoin/src/crypto/key.rs | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/bitcoin/src/crypto/key.rs b/bitcoin/src/crypto/key.rs index 2fcacba37..ed16e7a72 100644 --- a/bitcoin/src/crypto/key.rs +++ b/bitcoin/src/crypto/key.rs @@ -459,17 +459,21 @@ impl PrivateKey { /// Serializes the private key to bytes. pub fn to_vec(self) -> Vec { self.inner[..].to_vec() } + /// Deserializes a private key from a byte array. + pub fn from_byte_array( + data: [u8; 32], + network: impl Into + ) -> Result { + Ok(PrivateKey::new(secp256k1::SecretKey::from_byte_array(&data)?, network)) + } + /// Deserializes a private key from a slice. pub fn from_slice( data: &[u8], network: impl Into, ) -> Result { - Ok(PrivateKey::new( - secp256k1::SecretKey::from_byte_array( - data.try_into().map_err(|_| secp256k1::Error::InvalidSecretKey)?, - )?, - network, - )) + let array = data.try_into().map_err(|_| secp256k1::Error::InvalidSecretKey)?; + Self::from_byte_array(array, network) } /// Formats the private key to WIF format. From 8efacd4dda359bffae2120dfd58942637f32a52c Mon Sep 17 00:00:00 2001 From: Martin Habovstiak Date: Tue, 4 Mar 2025 20:43:39 +0100 Subject: [PATCH 4/6] Deprecate `PrivateKey::from_slice` method Since arrays better convey the intention than slices when parsing fixed-sized bytes we're migrating to them. This deprecates the `from_slice` method similarly to how we do it elsewhere. --- bitcoin/src/crypto/key.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/bitcoin/src/crypto/key.rs b/bitcoin/src/crypto/key.rs index ed16e7a72..c1d67bb64 100644 --- a/bitcoin/src/crypto/key.rs +++ b/bitcoin/src/crypto/key.rs @@ -468,6 +468,7 @@ impl PrivateKey { } /// Deserializes a private key from a slice. + #[deprecated(since = "TBD", note = "use from_byte_array instead")] pub fn from_slice( data: &[u8], network: impl Into, @@ -1606,6 +1607,8 @@ mod tests { } #[test] + #[allow(deprecated)] // tests the deprecated function + #[allow(deprecated_in_future)] fn invalid_private_key_len() { use crate::Network; assert!(PrivateKey::from_slice(&[1u8; 31], Network::Regtest).is_err()); From bbe87eccf2b9f01295e3c94a06130c6e7ac9bfa4 Mon Sep 17 00:00:00 2001 From: Martin Habovstiak Date: Wed, 5 Mar 2025 13:05:04 +0100 Subject: [PATCH 5/6] Fix bug in PSBT `Deserialize` for `XOnlyPublicKey` During upgrade of `secp256k1` a number of function calls needed to be rewritten to not use `from_slice` but `from_byte_array`. Unfortunately, the conversion wasn't correct and caused panics on invalid inputs rather than errors. This fixes it simply by calling `try_into` on the entire slice and converting the error. --- bitcoin/src/psbt/serialize.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bitcoin/src/psbt/serialize.rs b/bitcoin/src/psbt/serialize.rs index 26bfc9b32..b21a5ac0c 100644 --- a/bitcoin/src/psbt/serialize.rs +++ b/bitcoin/src/psbt/serialize.rs @@ -261,7 +261,7 @@ impl Serialize for XOnlyPublicKey { impl Deserialize for XOnlyPublicKey { fn deserialize(bytes: &[u8]) -> Result { XOnlyPublicKey::from_byte_array( - bytes[..32].try_into().expect("statistically impossible to hit"), + bytes.try_into().map_err(|_| Error::InvalidXOnlyPublicKey)?, ) .map_err(|_| Error::InvalidXOnlyPublicKey) } From ecc5791930b88581fcbc8f2417b0221486bd1031 Mon Sep 17 00:00:00 2001 From: Martin Habovstiak Date: Wed, 5 Mar 2025 13:09:44 +0100 Subject: [PATCH 6/6] Add test checking `XOnlyPublicKey::deserialize` This test checks the previous fix - if ordered before the previous commit it will fail. --- bitcoin/src/psbt/serialize.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/bitcoin/src/psbt/serialize.rs b/bitcoin/src/psbt/serialize.rs index b21a5ac0c..89de08dcb 100644 --- a/bitcoin/src/psbt/serialize.rs +++ b/bitcoin/src/psbt/serialize.rs @@ -461,6 +461,12 @@ mod tests { assert!(sighash.is_ok()) } + #[test] + fn deserialize_xonly_public_key_len() { + assert!(XOnlyPublicKey::deserialize(&[1; 31]).is_err()); + assert!(XOnlyPublicKey::deserialize(&[1; 33]).is_err()); + } + #[test] #[should_panic(expected = "InvalidMagic")] fn invalid_vector_1() {