feat: add constant-time trait bounds (#219)

Currently, the only implementation of the `SecretKey` and `PublicKey`
traits is for Ristretto, where both
[scalars](https://github.com/dalek-cryptography/curve25519-dalek/blob/ba737a379071191158bacfa6d138f6249b12fc09/curve25519-dalek/src/scalar.rs#L296-L300)
and [group
elements](https://github.com/dalek-cryptography/curve25519-dalek/blob/ba737a379071191158bacfa6d138f6249b12fc09/curve25519-dalek/src/ristretto.rs#L822-L826)
use constant-time equality in their underlying `PartialEq`
implementations, and which support the `ConstantTimeEq` trait.

This PR does what it can to encourage the use of constant-time equality
for keys by doing a few things.

First, it requires that any types implementing `SecretKey` or
`PublicKey` also implement `ConstantTimeEq`. Unfortunately, this doesn't
guarantee that their `PartialEq` implementation defaults to this, and it
doesn't appear possible to enforce this at the trait level.

It also sets a good example by manually implementing `PartialEq` on the
Ristretto key types to use their `ConstantTimeEq` implementations. This
isn't strictly necessary, but hopefully helps to indicate best practice.
It also implements `ConstantTimeEq` directly as required by the new
trait bounds.

Finally, it implements `ConstantTimeEq` for `DiffieHellmanSharedSecret`
using the new trait bound, and removes a redundant `Zeroize` trait
bound.

Note that this doesn't actually change the current implementations'
behavior, and therefore incurs no performance hit.

Closes #139.

Cargo.toml

@@ -24,6 +24,7 @@ rand_core = { version = "0.6" , default-features = false}
 serde = { version = "1.0", optional = true }
 sha3 = { version = "0.10", default-features = false  }
 snafu = { version = "0.7", default-features = false}
+subtle = { version = "2.5.0", default-features = false }
 zeroize = {version = "1" , default-features = false}
 
 [dev-dependencies]
@@ -48,6 +49,7 @@ std = [
     "serde?/std",
     "sha3/std",
     "snafu/std",
+    "subtle/std",
     "tari_utilities/std",
     "zeroize/std",
 ]
@@ -61,4 +63,4 @@ crate-type = ["lib", "cdylib"]
 [[bench]]
 name = "benches"
 path = "benches/mod.rs"
-harness = false
+harness = false

src/dhke.rs

@@ -11,18 +11,19 @@
 
 use core::ops::Mul;
 
+use subtle::{Choice, ConstantTimeEq};
 use zeroize::{Zeroize, ZeroizeOnDrop};
 
 use crate::keys::PublicKey;
 
 /// The result of a Diffie-Hellman key exchange
-#[derive(Zeroize, ZeroizeOnDrop)]
+#[derive(PartialEq, Eq, Zeroize, ZeroizeOnDrop)]
 pub struct DiffieHellmanSharedSecret<P>(P)
-where P: Zeroize;
+where P: PublicKey;
 
 impl<P> DiffieHellmanSharedSecret<P>
 where
-    P: PublicKey + Zeroize,
+    P: PublicKey,
     for<'a> &'a <P as PublicKey>::K: Mul<&'a P, Output = P>,
 {
     /// Perform a Diffie-Hellman key exchange
@@ -36,6 +37,14 @@ where
     }
 }
 
+impl<P> ConstantTimeEq for DiffieHellmanSharedSecret<P>
+where P: PublicKey
+{
+    fn ct_eq(&self, other: &DiffieHellmanSharedSecret<P>) -> Choice {
+        self.0.ct_eq(&other.0)
+    }
+}
+
 #[cfg(test)]
 mod test {
     use rand_core::OsRng;

src/keys.rs

@@ -9,6 +9,7 @@
 use core::ops::Add;
 
 use rand_core::{CryptoRng, RngCore};
+use subtle::ConstantTimeEq;
 use tari_utilities::{ByteArray, ByteArrayError};
 use zeroize::{Zeroize, ZeroizeOnDrop};
 
@@ -27,7 +28,7 @@ use zeroize::{Zeroize, ZeroizeOnDrop};
 /// let p = RistrettoPublicKey::from_secret_key(&k);
 /// ```
 pub trait SecretKey:
-    ByteArray + Clone + PartialEq + Eq + Add<Output = Self> + Default + Zeroize + ZeroizeOnDrop
+    ByteArray + Clone + ConstantTimeEq + PartialEq + Eq + Add<Output = Self> + Default + Zeroize + ZeroizeOnDrop
 {
     /// The length of the byte encoding of a key, in bytes
     const KEY_LEN: usize;
@@ -54,7 +55,9 @@ pub trait SecretKey:
 /// implementations need to implement this trait for them to be used in Tari.
 ///
 /// See [SecretKey](trait.SecretKey.html) for an example.
-pub trait PublicKey: ByteArray + Add<Output = Self> + Clone + PartialOrd + Ord + Default + Zeroize {
+pub trait PublicKey:
+    ByteArray + ConstantTimeEq + PartialEq + Eq + Add<Output = Self> + Clone + PartialOrd + Ord + Default + Zeroize
+{
     /// The length of the byte encoding of a key, in bytes
     const KEY_LEN: usize;
 

src/ristretto/ristretto_keys.rs

@@ -21,6 +21,7 @@ use curve25519_dalek::{
 use digest::{consts::U64, Digest};
 use once_cell::sync::OnceCell;
 use rand_core::{CryptoRng, RngCore};
+use subtle::ConstantTimeEq;
 use tari_utilities::{hex::Hex, ByteArray, ByteArrayError, Hashable};
 use zeroize::{Zeroize, ZeroizeOnDrop, Zeroizing};
 
@@ -51,7 +52,7 @@ use crate::{
 /// let _k2 = RistrettoSecretKey::from_hex(&"100000002000000030000000040000000");
 /// let _k3 = RistrettoSecretKey::random(&mut rng);
 /// ```
-#[derive(Eq, Clone, Default, Zeroize, ZeroizeOnDrop)]
+#[derive(Clone, Default, Zeroize, ZeroizeOnDrop)]
 pub struct RistrettoSecretKey(pub(crate) Scalar);
 
 #[cfg(feature = "borsh")]
@@ -132,12 +133,20 @@ impl Hash for RistrettoSecretKey {
     }
 }
 
+impl ConstantTimeEq for RistrettoSecretKey {
+    fn ct_eq(&self, other: &Self) -> subtle::Choice {
+        self.0.ct_eq(&other.0)
+    }
+}
+
 impl PartialEq for RistrettoSecretKey {
     fn eq(&self, other: &Self) -> bool {
-        self.0.eq(&other.0)
+        self.ct_eq(other).into()
     }
 }
 
+impl Eq for RistrettoSecretKey {}
+
 //----------------------------------   RistrettoSecretKey Debug --------------------------------------------//
 impl fmt::Debug for RistrettoSecretKey {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
@@ -423,6 +432,12 @@ impl fmt::Display for RistrettoPublicKey {
     }
 }
 
+impl ConstantTimeEq for RistrettoPublicKey {
+    fn ct_eq(&self, other: &Self) -> subtle::Choice {
+        self.point.ct_eq(&other.point)
+    }
+}
+
 impl RistrettoPublicKey {
     // Formats a 64 char hex string to a given width.
     // If w >= 64, we pad the result.
@@ -471,9 +486,7 @@ impl fmt::Debug for RistrettoPublicKey {
 
 impl PartialEq for RistrettoPublicKey {
     fn eq(&self, other: &RistrettoPublicKey) -> bool {
-        // Although this is slower than `self.compressed == other.compressed`, expanded point comparison is an equal
-        // time comparison
-        self.point == other.point
+        self.ct_eq(other).into()
     }
 }