From 4a0c7fca6a3a93e66e6ba87a376db3bf04fa9bbd Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Tue, 22 Nov 2022 09:39:27 +1100 Subject: [PATCH 1/4] Do not use impl_array_newtype for SecretKey In preparation for changing the logic of comparison trait (Ord, Eq) implementations on the `SecretKey` copy all the code out of `impl_array_newtype` and implement it directly in `key.rs`. Refactor only, no logic changes (although I removed a few unneeded references). --- src/key.rs | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 64 insertions(+), 3 deletions(-) diff --git a/src/key.rs b/src/key.rs index 3cf6111..d480e00 100644 --- a/src/key.rs +++ b/src/key.rs @@ -17,7 +17,7 @@ //! use core::convert::TryFrom; -use core::ops::BitXor; +use core::ops::{self, BitXor}; use core::{fmt, ptr, str}; #[cfg(feature = "serde")] @@ -28,7 +28,7 @@ use crate::ffi::{self, CPtr}; #[cfg(all(feature = "global-context", feature = "rand-std"))] use crate::schnorr; use crate::Error::{self, InvalidPublicKey, InvalidPublicKeySum, InvalidSecretKey}; -use crate::{constants, from_hex, impl_array_newtype, Scalar, Secp256k1, Signing, Verification}; +use crate::{constants, from_hex, Scalar, Secp256k1, Signing, Verification}; #[cfg(feature = "global-context")] use crate::{ecdsa, Message, SECP256K1}; #[cfg(feature = "bitcoin-hashes")] @@ -58,9 +58,70 @@ use crate::{hashes, ThirtyTwoByteHash}; /// [`cbor`]: https://docs.rs/cbor #[derive(Copy, Clone)] pub struct SecretKey([u8; constants::SECRET_KEY_SIZE]); -impl_array_newtype!(SecretKey, u8, constants::SECRET_KEY_SIZE); impl_display_secret!(SecretKey); +impl PartialEq for SecretKey { + #[inline] + fn eq(&self, other: &Self) -> bool { + self[..] == other[..] + } +} + +impl Eq for SecretKey {} + +impl core::hash::Hash for SecretKey { + fn hash(&self, state: &mut H) { + self[..].hash(state) + } +} + +impl PartialOrd for SecretKey { + #[inline] + fn partial_cmp(&self, other: &SecretKey) -> Option { + self[..].partial_cmp(&other[..]) + } +} + +impl Ord for SecretKey { + #[inline] + fn cmp(&self, other: &SecretKey) -> core::cmp::Ordering { + self[..].cmp(&other[..]) + } +} + +impl AsRef<[u8; constants::SECRET_KEY_SIZE]> for SecretKey { + /// Gets a reference to the underlying array + #[inline] + fn as_ref(&self) -> &[u8; constants::SECRET_KEY_SIZE] { + let &SecretKey(ref dat) = self; + dat + } +} + +impl ops::Index for SecretKey +where + [u8]: ops::Index, +{ + type Output = <[u8] as ops::Index>::Output; + + #[inline] + fn index(&self, index: I) -> &Self::Output { &self.0[index] } +} + +impl ffi::CPtr for SecretKey { + type Target = u8; + + fn as_c_ptr(&self) -> *const Self::Target { + let &SecretKey(ref dat) = self; + dat.as_ptr() + } + + fn as_mut_c_ptr(&mut self) -> *mut Self::Target { + let &mut SecretKey(ref mut dat) = self; + dat.as_mut_ptr() + } +} + impl str::FromStr for SecretKey { type Err = Error; fn from_str(s: &str) -> Result { From 19039d92813d2b1b720ea920e06957df3a3f10b1 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Tue, 22 Nov 2022 09:58:19 +1100 Subject: [PATCH 2/4] Remove `Ord`/`Hash` traits from SecretKey The current trait implementations of `Ord` and `PartialOrd` for `SecretKey` leak data when doing the comparison i.e., they are not constant time. Since there is no real usecase for ordering secret keys remove the trait implementations all together. Remove `Hash` at the same time because it does not make sense to implement it if `Ord`/`PartialOrd` are not implemented. --- src/key.rs | 20 -------------------- 1 file changed, 20 deletions(-) diff --git a/src/key.rs b/src/key.rs index d480e00..9bbf8c7 100644 --- a/src/key.rs +++ b/src/key.rs @@ -69,26 +69,6 @@ impl PartialEq for SecretKey { impl Eq for SecretKey {} -impl core::hash::Hash for SecretKey { - fn hash(&self, state: &mut H) { - self[..].hash(state) - } -} - -impl PartialOrd for SecretKey { - #[inline] - fn partial_cmp(&self, other: &SecretKey) -> Option { - self[..].partial_cmp(&other[..]) - } -} - -impl Ord for SecretKey { - #[inline] - fn cmp(&self, other: &SecretKey) -> core::cmp::Ordering { - self[..].cmp(&other[..]) - } -} - impl AsRef<[u8; constants::SECRET_KEY_SIZE]> for SecretKey { /// Gets a reference to the underlying array #[inline] From 7cf3c6c8a4ba76c1a1165ff9f102d10f4af9a290 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Tue, 22 Nov 2022 10:09:59 +1100 Subject: [PATCH 3/4] Implement constant time comparison for SecretKey The current implementation of `PartialEq` leaks data because it is not constant time. Attempt to make the `PartialEq` implementation constant time. --- src/key.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/key.rs b/src/key.rs index 9bbf8c7..3979be6 100644 --- a/src/key.rs +++ b/src/key.rs @@ -61,9 +61,12 @@ pub struct SecretKey([u8; constants::SECRET_KEY_SIZE]); impl_display_secret!(SecretKey); impl PartialEq for SecretKey { + /// This implementation is designed to be constant time to help prevent side channel attacks. #[inline] fn eq(&self, other: &Self) -> bool { - self[..] == other[..] + let accum = self.0.iter().zip(&other.0) + .fold(0, |accum, (a, b)| accum | a ^ b); + unsafe { core::ptr::read_volatile(&accum) == 0 } } } From b9eefea09215bf011b670e94599d4c706ed81423 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Tue, 22 Nov 2022 10:20:03 +1100 Subject: [PATCH 4/4] Add documentation on side channel attacks to SecretKey We recently added a constant time `eq` implementation however library users can inadvertently bypass this protection if they use `AsRef`. To help prevent this add documentation to the `SecretKey` and also to the `AsRef` implementation. --- src/key.rs | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/src/key.rs b/src/key.rs index 3979be6..816d4be 100644 --- a/src/key.rs +++ b/src/key.rs @@ -36,6 +36,13 @@ use crate::{hashes, ThirtyTwoByteHash}; /// Secret 256-bit key used as `x` in an ECDSA signature. /// +/// # Side channel attacks +/// +/// We have attempted to reduce the side channel attack surface by implementing a constant time `eq` +/// method. For similar reasons we explicitly do not implement `PartialOrd`, `Ord`, or `Hash` on +/// `SecretKey`. If you really want to order secrets keys then you can use `AsRef` to get at the +/// underlying bytes and compare them - however this is almost certainly a bad idea. +/// /// # Serde support /// /// Implements de/serialization with the `serde` feature enabled. We treat the byte value as a tuple @@ -73,7 +80,14 @@ impl PartialEq for SecretKey { impl Eq for SecretKey {} impl AsRef<[u8; constants::SECRET_KEY_SIZE]> for SecretKey { - /// Gets a reference to the underlying array + /// Gets a reference to the underlying array. + /// + /// # Side channel attacks + /// + /// Using ordering functions (`PartialOrd`/`Ord`) on a reference to secret keys leaks data + /// because the implementations are not constant time. Doing so will make your code vulnerable + /// to side channel attacks. [`SecretKey::eq`] is implemented using a constant time algorithm, + /// please consider using it to do comparisons of secret keys. #[inline] fn as_ref(&self) -> &[u8; constants::SECRET_KEY_SIZE] { let &SecretKey(ref dat) = self;