From b38ae97eafe8d07612bb35e59bbc4eeafea834ec Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Thu, 17 Nov 2022 15:56:35 +1100 Subject: [PATCH] Implement stable comparison functionality Currently we rely on the inner bytes with types that are passed across the FFI boundry when implementing comparison functions (e.g. `Ord`, `PartialEq`), this is incorrect because the bytes are opaque, meaning the byte layout is not guaranteed across versions of `libsecp26k1`. Implement stable comparison functionality by doing: - Implement `core::cmp` traits by first coercing the data into a stable form e.g., by serializing it. - Add fast comparison methods to `secp256k1-sys` types that wrap types from libsecp, add similar methods to types in `secp256k1` that wrap `secp256k1-sys` types (just call through to inner type). - In `secp256k1-sys` feature gate the new `core::cmp` impls on `not(fuzzing)`, when fuzzing just derive the impls instead. Any additional methods added to `secp256k1-sys` types are private, justified by the fact the -sys is meant to be just a thin wrapper around libsecp256k1, we don't want to commit to supporting additional API functions. Please note, the solution presented in this patch is already present for `secp256k1::PublicKey`, this PR removes that code in favour of deriving traits that then call down to the same logic in `secp256k1-sys`. --- secp256k1-sys/src/lib.rs | 202 ++++++++++++++++++++++++++++++++++ secp256k1-sys/src/macros.rs | 35 ++++++ secp256k1-sys/src/recovery.rs | 53 ++++++++- src/ecdsa/mod.rs | 3 +- src/key.rs | 47 +------- src/macros.rs | 27 +++++ 6 files changed, 324 insertions(+), 43 deletions(-) diff --git a/secp256k1-sys/src/lib.rs b/secp256k1-sys/src/lib.rs index c5fc383..f92535e 100644 --- a/secp256k1-sys/src/lib.rs +++ b/secp256k1-sys/src/lib.rs @@ -169,6 +169,59 @@ impl PublicKey { pub fn underlying_bytes(self) -> [c_uchar; 64] { self.0 } + + /// Serializes this public key as a byte-encoded pair of values, in compressed form. + fn serialize(&self) -> [u8; 33] { + let mut buf = [0u8; 33]; + let mut len = 33; + unsafe { + let ret = secp256k1_ec_pubkey_serialize( + secp256k1_context_no_precomp, + buf.as_mut_c_ptr(), + &mut len, + self, + SECP256K1_SER_COMPRESSED, + ); + debug_assert_eq!(ret, 1); + debug_assert_eq!(len, 33); + }; + buf + } +} + +#[cfg(not(fuzzing))] +impl PartialOrd for PublicKey { + fn partial_cmp(&self, other: &PublicKey) -> Option { + Some(self.cmp(other)) + } +} + +#[cfg(not(fuzzing))] +impl Ord for PublicKey { + fn cmp(&self, other: &PublicKey) -> core::cmp::Ordering { + let ret = unsafe { + secp256k1_ec_pubkey_cmp(secp256k1_context_no_precomp, self, other) + }; + ret.cmp(&0i32) + } +} + +#[cfg(not(fuzzing))] +impl PartialEq for PublicKey { + fn eq(&self, other: &Self) -> bool { + self.cmp(other) == core::cmp::Ordering::Equal + } +} + +#[cfg(not(fuzzing))] +impl Eq for PublicKey {} + +#[cfg(not(fuzzing))] +impl core::hash::Hash for PublicKey { + fn hash(&self, state: &mut H) { + let ser = self.serialize(); + ser.hash(state); + } } /// Library-internal representation of a Secp256k1 signature @@ -209,6 +262,54 @@ impl Signature { pub fn underlying_bytes(self) -> [c_uchar; 64] { self.0 } + + /// Serializes the signature in compact format. + fn serialize(&self) -> [u8; 64] { + let mut buf = [0u8; 64]; + unsafe { + let ret = secp256k1_ecdsa_signature_serialize_compact( + secp256k1_context_no_precomp, + buf.as_mut_c_ptr(), + self, + ); + debug_assert!(ret == 1); + } + buf + } +} + +#[cfg(not(fuzzing))] +impl PartialOrd for Signature { + fn partial_cmp(&self, other: &Signature) -> Option { + Some(self.cmp(other)) + } +} + +#[cfg(not(fuzzing))] +impl Ord for Signature { + fn cmp(&self, other: &Signature) -> core::cmp::Ordering { + let this = self.serialize(); + let that = other.serialize(); + this.cmp(&that) + } +} + +#[cfg(not(fuzzing))] +impl PartialEq for Signature { + fn eq(&self, other: &Self) -> bool { + self.cmp(other) == core::cmp::Ordering::Equal + } +} + +#[cfg(not(fuzzing))] +impl Eq for Signature {} + +#[cfg(not(fuzzing))] +impl core::hash::Hash for Signature { + fn hash(&self, state: &mut H) { + let ser = self.serialize(); + ser.hash(state); + } } #[repr(C)] @@ -248,6 +349,55 @@ impl XOnlyPublicKey { pub fn underlying_bytes(self) -> [c_uchar; 64] { self.0 } + + /// Serializes this key as a byte-encoded x coordinate value (32 bytes). + fn serialize(&self) -> [u8; 32] { + let mut buf = [0u8; 32]; + unsafe { + let ret = secp256k1_xonly_pubkey_serialize( + secp256k1_context_no_precomp, + buf.as_mut_c_ptr(), + self, + ); + assert_eq!(ret, 1); + }; + buf + } +} + +#[cfg(not(fuzzing))] +impl PartialOrd for XOnlyPublicKey { + fn partial_cmp(&self, other: &XOnlyPublicKey) -> Option { + Some(self.cmp(other)) + } +} + +#[cfg(not(fuzzing))] +impl Ord for XOnlyPublicKey { + fn cmp(&self, other: &XOnlyPublicKey) -> core::cmp::Ordering { + let ret = unsafe { + secp256k1_xonly_pubkey_cmp(secp256k1_context_no_precomp, self, other) + }; + ret.cmp(&0i32) + } +} + +#[cfg(not(fuzzing))] +impl PartialEq for XOnlyPublicKey { + fn eq(&self, other: &Self) -> bool { + self.cmp(other) == core::cmp::Ordering::Equal + } +} + +#[cfg(not(fuzzing))] +impl Eq for XOnlyPublicKey {} + +#[cfg(not(fuzzing))] +impl core::hash::Hash for XOnlyPublicKey { + fn hash(&self, state: &mut H) { + let ser = self.serialize(); + ser.hash(state); + } } #[repr(C)] @@ -287,6 +437,58 @@ impl KeyPair { pub fn underlying_bytes(self) -> [c_uchar; 96] { self.0 } + + /// Creates a new compressed public key from this key pair. + fn public_key(&self) -> PublicKey { + unsafe { + let mut pk = PublicKey::new(); + let ret = secp256k1_keypair_pub( + secp256k1_context_no_precomp, + &mut pk, + self, + ); + debug_assert_eq!(ret, 1); + pk + } + } +} + +#[cfg(not(fuzzing))] +impl PartialOrd for KeyPair { + fn partial_cmp(&self, other: &KeyPair) -> Option { + Some(self.cmp(other)) + } +} + +#[cfg(not(fuzzing))] +impl Ord for KeyPair { + fn cmp(&self, other: &KeyPair) -> core::cmp::Ordering { + let this = self.public_key(); + let that = other.public_key(); + this.cmp(&that) + } +} + +#[cfg(not(fuzzing))] +impl PartialEq for KeyPair { + fn eq(&self, other: &Self) -> bool { + self.cmp(other) == core::cmp::Ordering::Equal + } +} + +#[cfg(not(fuzzing))] +impl Eq for KeyPair {} + +#[cfg(not(fuzzing))] +impl core::hash::Hash for KeyPair { + fn hash(&self, state: &mut H) { + // To hash the key pair we just hash the serialized public key. Since any change to the + // secret key would also be a change to the public key this is a valid one way function from + // the key pair to the digest. + let pk = self.public_key(); + let ser = pk.serialize(); + ser.hash(state); + } } extern "C" { diff --git a/secp256k1-sys/src/macros.rs b/secp256k1-sys/src/macros.rs index 761ab94..247ee12 100644 --- a/secp256k1-sys/src/macros.rs +++ b/secp256k1-sys/src/macros.rs @@ -19,6 +19,34 @@ macro_rules! impl_array_newtype { ($thing:ident, $ty:ty, $len:expr) => { impl Copy for $thing {} + impl $thing { + /// Like `cmp::Ord` but faster and with no guarantees across library versions. + /// + /// The inner byte array of `Self` is passed across the FFI boundry, as such there are + /// no guarantees on its layout and it is subject to change across library versions, + /// even minor versions. For this reason comparison function implementations (e.g. + /// `Ord`, `PartialEq`) take measures to ensure the data will remain constant (e.g., by + /// serializing it to a guaranteed format). This means they may be slow, this function + /// provides a faster comparison if you know that your types come from the same library + /// version. + pub fn cmp_fast_unstable(&self, other: &Self) -> core::cmp::Ordering { + self[..].cmp(&other[..]) + } + + /// Like `cmp::Eq` but faster and with no guarantees across library versions. + /// + /// The inner byte array of `Self` is passed across the FFI boundry, as such there are + /// no guarantees on its layout and it is subject to change across library versions, + /// even minor versions. For this reason comparison function implementations (e.g. + /// `Ord`, `PartialEq`) take measures to ensure the data will remain constant (e.g., by + /// serializing it to a guaranteed format). This means they may be slow, this function + /// provides a faster equality check if you know that your types come from the same + /// library version. + pub fn eq_fast_unstable(&self, other: &Self) -> bool { + self[..].eq(&other[..]) + } + } + impl AsRef<[$ty; $len]> for $thing { #[inline] /// Gets a reference to the underlying array @@ -28,6 +56,9 @@ macro_rules! impl_array_newtype { } } + // We cannot derive these traits because Rust 1.41.1 requires `std::array::LengthAtMost32`. + + #[cfg(fuzzing)] impl PartialEq for $thing { #[inline] fn eq(&self, other: &$thing) -> bool { @@ -35,14 +66,17 @@ macro_rules! impl_array_newtype { } } + #[cfg(fuzzing)] impl Eq for $thing {} + #[cfg(fuzzing)] impl core::hash::Hash for $thing { fn hash(&self, state: &mut H) { (&self[..]).hash(state) } } + #[cfg(fuzzing)] impl PartialOrd for $thing { #[inline] fn partial_cmp(&self, other: &$thing) -> Option { @@ -50,6 +84,7 @@ macro_rules! impl_array_newtype { } } + #[cfg(fuzzing)] impl Ord for $thing { #[inline] fn cmp(&self, other: &$thing) -> core::cmp::Ordering { diff --git a/secp256k1-sys/src/recovery.rs b/secp256k1-sys/src/recovery.rs index a5c7fa4..7a89112 100644 --- a/secp256k1-sys/src/recovery.rs +++ b/secp256k1-sys/src/recovery.rs @@ -15,7 +15,7 @@ //! # FFI of the recovery module -use crate::{Context, Signature, NonceFn, PublicKey, CPtr, impl_array_newtype}; +use crate::{Context, Signature, NonceFn, PublicKey, CPtr, impl_array_newtype, secp256k1_context_no_precomp}; use crate::types::*; use core::fmt; @@ -27,6 +27,23 @@ impl_array_newtype!(RecoverableSignature, c_uchar, 65); impl RecoverableSignature { /// Create a new (zeroed) signature usable for the FFI interface pub fn new() -> RecoverableSignature { RecoverableSignature([0; 65]) } + + /// Serializes the signature in compact format. + fn serialize(&self) -> [u8; 65] { + let mut buf = [0u8; 65]; + let mut recid = 0; + unsafe { + let ret = secp256k1_ecdsa_recoverable_signature_serialize_compact( + secp256k1_context_no_precomp, + buf.as_mut_c_ptr(), + &mut recid, + self, + ); + debug_assert!(ret == 1); + } + buf[64] = (recid & 0xFF) as u8; + buf + } } impl Default for RecoverableSignature { @@ -59,6 +76,40 @@ impl fmt::Debug for RecoverableSignature { } } +#[cfg(not(fuzzing))] +impl PartialOrd for RecoverableSignature { + fn partial_cmp(&self, other: &RecoverableSignature) -> Option { + Some(self.cmp(other)) + } +} + +#[cfg(not(fuzzing))] +impl Ord for RecoverableSignature { + fn cmp(&self, other: &RecoverableSignature) -> core::cmp::Ordering { + let this = self.serialize(); + let that = other.serialize(); + this.cmp(&that) + } +} + +#[cfg(not(fuzzing))] +impl PartialEq for RecoverableSignature { + fn eq(&self, other: &Self) -> bool { + self.cmp(other) == core::cmp::Ordering::Equal + } +} + +#[cfg(not(fuzzing))] +impl Eq for RecoverableSignature {} + +#[cfg(not(fuzzing))] +impl core::hash::Hash for RecoverableSignature { + fn hash(&self, state: &mut H) { + let ser = self.serialize(); + ser.hash(state); + } +} + extern "C" { #[cfg_attr(not(rust_secp_no_symbol_renaming), link_name = "rustsecp256k1_v0_6_1_ecdsa_recoverable_signature_parse_compact")] pub fn secp256k1_ecdsa_recoverable_signature_parse_compact(cx: *const Context, sig: *mut RecoverableSignature, diff --git a/src/ecdsa/mod.rs b/src/ecdsa/mod.rs index 6e3256f..d44126a 100644 --- a/src/ecdsa/mod.rs +++ b/src/ecdsa/mod.rs @@ -18,8 +18,9 @@ use crate::{ }; /// An ECDSA signature -#[derive(Copy, Clone, PartialEq, Eq, Hash)] +#[derive(Copy, Clone, PartialOrd, Ord, PartialEq, Eq, Hash)] pub struct Signature(pub(crate) ffi::Signature); +impl_fast_comparisons!(Signature); impl fmt::Debug for Signature { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { fmt::Display::fmt(self, f) } diff --git a/src/key.rs b/src/key.rs index 6b67c67..910385b 100644 --- a/src/key.rs +++ b/src/key.rs @@ -94,10 +94,10 @@ impl str::FromStr for SecretKey { /// ``` /// [`bincode`]: https://docs.rs/bincode /// [`cbor`]: https://docs.rs/cbor -#[derive(Copy, Clone, Debug)] -#[cfg_attr(fuzzing, derive(PartialOrd, Ord, PartialEq, Eq, Hash))] +#[derive(Copy, Clone, Debug, PartialOrd, Ord, PartialEq, Eq, Hash)] #[repr(transparent)] pub struct PublicKey(ffi::PublicKey); +impl_fast_comparisons!(PublicKey); impl fmt::LowerHex for PublicKey { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { @@ -718,43 +718,6 @@ impl<'de> serde::Deserialize<'de> for PublicKey { } } -#[cfg(not(fuzzing))] -impl PartialOrd for PublicKey { - fn partial_cmp(&self, other: &PublicKey) -> Option { - Some(self.cmp(other)) - } -} - -#[cfg(not(fuzzing))] -impl Ord for PublicKey { - fn cmp(&self, other: &PublicKey) -> core::cmp::Ordering { - let ret = unsafe { - ffi::secp256k1_ec_pubkey_cmp( - ffi::secp256k1_context_no_precomp, - self.as_c_ptr(), - other.as_c_ptr(), - ) - }; - ret.cmp(&0i32) - } -} - -#[cfg(not(fuzzing))] -impl PartialEq for PublicKey { - fn eq(&self, other: &Self) -> bool { self.cmp(other) == core::cmp::Ordering::Equal } -} - -#[cfg(not(fuzzing))] -impl Eq for PublicKey {} - -#[cfg(not(fuzzing))] -impl core::hash::Hash for PublicKey { - fn hash(&self, state: &mut H) { - let ser = self.serialize(); - ser.hash(state); - } -} - /// Opaque data structure that holds a keypair consisting of a secret and a public key. /// /// # Serde support @@ -779,9 +742,10 @@ impl core::hash::Hash for PublicKey { /// ``` /// [`bincode`]: https://docs.rs/bincode /// [`cbor`]: https://docs.rs/cbor -#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] +#[derive(Copy, Clone, PartialOrd, Ord, PartialEq, Eq, Hash)] pub struct KeyPair(ffi::KeyPair); impl_display_secret!(KeyPair); +impl_fast_comparisons!(KeyPair); impl KeyPair { /// Obtains a raw const pointer suitable for use with FFI functions. @@ -1091,8 +1055,9 @@ impl CPtr for KeyPair { /// ``` /// [`bincode`]: https://docs.rs/bincode /// [`cbor`]: https://docs.rs/cbor -#[derive(Copy, Clone, PartialEq, Eq, Debug, PartialOrd, Ord, Hash)] +#[derive(Copy, Clone, Debug, PartialOrd, Ord, PartialEq, Eq, Hash)] pub struct XOnlyPublicKey(ffi::XOnlyPublicKey); +impl_fast_comparisons!(XOnlyPublicKey); impl fmt::LowerHex for XOnlyPublicKey { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { diff --git a/src/macros.rs b/src/macros.rs index b457c02..11ce90e 100644 --- a/src/macros.rs +++ b/src/macros.rs @@ -28,6 +28,8 @@ macro_rules! impl_array_newtype { } } + // We cannot derive these traits because Rust 1.41.1 requires `std::array::LengthAtMost32`. + impl PartialEq for $thing { #[inline] fn eq(&self, other: &$thing) -> bool { @@ -123,3 +125,28 @@ macro_rules! write_err { } } } + +/// Implements fast unstable comparison methods for `$ty`. +macro_rules! impl_fast_comparisons { + ($ty:ident) => { + impl $ty { + /// Like `cmp::Cmp` but faster and with no guarantees across library versions. + /// + /// The `Cmp` implementation for FFI types is stable but slow because it first + /// serializes `self` and `other` before comparing them. This function provides a faster + /// comparison if you know that your types come from the same library version. + pub fn cmp_fast_unstable(&self, other: &Self) -> core::cmp::Ordering { + self.0.cmp_fast_unstable(&other.0) + } + + /// Like `cmp::Eq` but faster and with no guarantees across library versions. + /// + /// The `Eq` implementation for FFI types is stable but slow because it first serializes + /// `self` and `other` before comparing them. This function provides a faster equality + /// check if you know that your types come from the same library version. + pub fn eq_fast_unstable(&self, other: &Self) -> bool { + self.0.eq_fast_unstable(&other.0) + } + } + } +}