Made ExpandedSecretKey private to avoid signing key oracle (#205)

This fix eliminates a scenario where a user misuses the `ExpandedSecretKey` API
in a way that leaks the user's secret key. In short, if a user sends
`ExpandedSecretKey::sign(sk, msg, pk1)` followed by
`ExpandedSecretKey::sign(sk, msg, pk2)`, where `pk1 != pk2`, a passive
adversary [can easily][0] derive `sk`. To mitigate this, we remove the API
entirely.

[0]: https://github.com/MystenLabs/ed25519-unsafe-libs

benches/ed25519_benchmarks.rs

@@ -16,7 +16,7 @@ use criterion::Criterion;
 
 mod ed25519_benches {
     use super::*;
-    use ed25519_dalek::ExpandedSecretKey;
+    use ed25519_dalek::verify_batch;
     use ed25519_dalek::Keypair;
     use ed25519_dalek::PublicKey;
     use ed25519_dalek::Signature;
@@ -30,20 +30,7 @@ mod ed25519_benches {
         let keypair: Keypair = Keypair::generate(&mut csprng);
         let msg: &[u8] = b"";
 
-        c.bench_function("Ed25519 signing", move |b| {
-                         b.iter(| | keypair.sign(msg))
-        });
-    }
-
-    fn sign_expanded_key(c: &mut Criterion) {
-        let mut csprng: ThreadRng = thread_rng();
-        let keypair: Keypair = Keypair::generate(&mut csprng);
-        let expanded: ExpandedSecretKey = (&keypair.secret).into();
-        let msg: &[u8] = b"";
-
-        c.bench_function("Ed25519 signing with an expanded secret key", move |b| {
-                         b.iter(| | expanded.sign(msg, &keypair.public))
-        });
+        c.bench_function("Ed25519 signing", move |b| b.iter(|| keypair.sign(msg)));
     }
 
     fn verify(c: &mut Criterion) {
@@ -78,8 +65,10 @@ mod ed25519_benches {
                 let keypairs: Vec<Keypair> = (0..size).map(|_| Keypair::generate(&mut csprng)).collect();
                 let msg: &[u8] = b"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa";
                 let messages: Vec<&[u8]> = (0..size).map(|_| msg).collect();
-                let signatures:  Vec<Signature> = keypairs.iter().map(|key| key.sign(&msg)).collect();
-                let public_keys: Vec<PublicKey> = keypairs.iter().map(|key| key.public).collect();
+                let signatures: Vec<Signature> =
+                    keypairs.iter().map(|key| key.sign(&msg)).collect();
+                let public_keys: Vec<PublicKey> =
+                    keypairs.iter().map(|key| key.public_key()).collect();
 
                 b.iter(|| verify_batch(&messages[..], &signatures[..], &public_keys[..]));
             },
@@ -100,7 +89,6 @@ mod ed25519_benches {
         config = Criterion::default();
         targets =
             sign,
-            sign_expanded_key,
             verify,
             verify_strict,
             verify_batch_signatures,

src/batch.rs

@@ -209,7 +209,7 @@ fn zero_rng() -> ZeroRng {
 /// let msg: &[u8] = b"They're good dogs Brant";
 /// let messages: Vec<&[u8]> = (0..64).map(|_| msg).collect();
 /// let signatures:  Vec<Signature> = keypairs.iter().map(|key| key.sign(&msg)).collect();
-/// let public_keys: Vec<PublicKey> = keypairs.iter().map(|key| key.public).collect();
+/// let public_keys: Vec<PublicKey> = keypairs.iter().map(|key| key.public_key()).collect();
 ///
 /// let result = verify_batch(&messages[..], &signatures[..], &public_keys[..]);
 /// assert!(result.is_ok());

src/errors.rs

@@ -43,6 +43,8 @@ pub(crate) enum InternalError {
                       name_c: &'static str, length_c: usize, },
     /// An ed25519ph signature can only take up to 255 octets of context.
     PrehashedContextLengthError,
+    /// A mismatched (public, secret) key pair.
+    MismatchedKeypairError,
 }
 
 impl Display for InternalError {
@@ -63,6 +65,7 @@ impl Display for InternalError {
                               {} has length {}, {} has length {}.", na, la, nb, lb, nc, lc),
             InternalError::PrehashedContextLengthError
                 => write!(f, "An ed25519ph signature can only take up to 255 octets of context"),
+            InternalError::MismatchedKeypairError => write!(f, "Mismatched Keypair detected"),
         }
     }
 }

src/keypair.rs

@@ -17,7 +17,7 @@ use serde::de::Error as SerdeError;
 #[cfg(feature = "serde")]
 use serde::{Deserialize, Deserializer, Serialize, Serializer};
 #[cfg(feature = "serde")]
-use serde_bytes::{Bytes as SerdeBytes, ByteBuf as SerdeByteBuf};
+use serde_bytes::{ByteBuf as SerdeByteBuf, Bytes as SerdeBytes};
 
 pub use sha2::Sha512;
 
@@ -32,15 +32,34 @@ use crate::public::*;
 use crate::secret::*;
 
 /// An ed25519 keypair.
+// Invariant: `public` is always the public key of `secret`. This prevents the signing function
+// oracle attack described in https://github.com/MystenLabs/ed25519-unsafe-libs
 #[derive(Debug)]
 pub struct Keypair {
     /// The secret half of this keypair.
-    pub secret: SecretKey,
+    pub(crate) secret: SecretKey,
     /// The public half of this keypair.
-    pub public: PublicKey,
+    pub(crate) public: PublicKey,
+}
+
+impl From<SecretKey> for Keypair {
+    fn from(secret: SecretKey) -> Self {
+        let public = PublicKey::from(&secret);
+        Self { secret, public }
+    }
 }
 
 impl Keypair {
+    /// Get the secret key of this keypair.
+    pub fn secret_key(&self) -> SecretKey {
+        SecretKey(self.secret.0)
+    }
+
+    /// Get the public key of this keypair.
+    pub fn public_key(&self) -> PublicKey {
+        self.public
+    }
+
     /// Convert this keypair to bytes.
     ///
     /// # Returns
@@ -49,7 +68,8 @@ impl Keypair {
     /// `SECRET_KEY_LENGTH` of bytes is the `SecretKey`, and the next
     /// `PUBLIC_KEY_LENGTH` bytes is the `PublicKey` (the same as other
     /// libraries, such as [Adam Langley's ed25519 Golang
-    /// implementation](https://github.com/agl/ed25519/)).
+    /// implementation](https://github.com/agl/ed25519/)). It is guaranteed that
+    /// the encoded public key is the one derived from the encoded secret key.
     pub fn to_bytes(&self) -> [u8; KEYPAIR_LENGTH] {
         let mut bytes: [u8; KEYPAIR_LENGTH] = [0u8; KEYPAIR_LENGTH];
 
@@ -62,32 +82,31 @@ impl Keypair {
     ///
     /// # Inputs
     ///
-    /// * `bytes`: an `&[u8]` representing the scalar for the secret key, and a
-    ///   compressed Edwards-Y coordinate of a point on curve25519, both as bytes.
-    ///   (As obtained from `Keypair::to_bytes()`.)
-    ///
-    /// # Warning
-    ///
-    /// Absolutely no validation is done on the key.  If you give this function
-    /// bytes which do not represent a valid point, or which do not represent
-    /// corresponding parts of the key, then your `Keypair` will be broken and
-    /// it will be your fault.
+    /// * `bytes`: an `&[u8]` of length [`KEYPAIR_LENGTH`], representing the
+    ///   scalar for the secret key, and a compressed Edwards-Y coordinate of a
+    ///   point on curve25519, both as bytes. (As obtained from
+    ///   [`Keypair::to_bytes`].)
     ///
     /// # Returns
     ///
     /// A `Result` whose okay value is an EdDSA `Keypair` or whose error value
     /// is an `SignatureError` describing the error that occurred.
-    pub fn from_bytes<'a>(bytes: &'a [u8]) -> Result<Keypair, SignatureError> {
+    pub fn from_bytes(bytes: &[u8]) -> Result<Keypair, SignatureError> {
         if bytes.len() != KEYPAIR_LENGTH {
             return Err(InternalError::BytesLengthError {
                 name: "Keypair",
                 length: KEYPAIR_LENGTH,
-            }.into());
+            }
+            .into());
         }
         let secret = SecretKey::from_bytes(&bytes[..SECRET_KEY_LENGTH])?;
         let public = PublicKey::from_bytes(&bytes[SECRET_KEY_LENGTH..])?;
 
-        Ok(Keypair{ secret: secret, public: public })
+        if public != (&secret).into() {
+            return Err(InternalError::MismatchedKeypairError.into());
+        }
+
+        Ok(Keypair { secret, public })
     }
 
     /// Generate an ed25519 keypair.
@@ -131,7 +150,10 @@ impl Keypair {
         let sk: SecretKey = SecretKey::generate(csprng);
         let pk: PublicKey = (&sk).into();
 
-        Keypair{ public: pk, secret: sk }
+        Keypair {
+            public: pk,
+            secret: sk,
+        }
     }
 
     /// Sign a `prehashed_message` with this `Keypair` using the
@@ -244,16 +266,17 @@ impl Keypair {
     {
         let expanded: ExpandedSecretKey = (&self.secret).into(); // xxx thanks i hate this
 
-        expanded.sign_prehashed(prehashed_message, &self.public, context).into()
+        expanded
+            .sign_prehashed(prehashed_message, &self.public, context)
+            .into()
     }
 
     /// Verify a signature on a message with this keypair's public key.
     pub fn verify(
         &self,
         message: &[u8],
-        signature: &ed25519::Signature
-    ) -> Result<(), SignatureError>
-    {
+        signature: &ed25519::Signature,
+    ) -> Result<(), SignatureError> {
         self.public.verify(message, signature)
     }
 
@@ -303,7 +326,7 @@ impl Keypair {
     /// let mut prehashed_again: Sha512 = Sha512::default();
     /// prehashed_again.update(message);
     ///
-    /// let verified = keypair.public.verify_prehashed(prehashed_again, Some(context), &sig);
+    /// let verified = keypair.public_key().verify_prehashed(prehashed_again, Some(context), &sig);
     ///
     /// assert!(verified.is_ok());
     ///
@@ -329,7 +352,8 @@ impl Keypair {
     where
         D: Digest<OutputSize = U64>,
     {
-        self.public.verify_prehashed(prehashed_message, context, signature)
+        self.public
+            .verify_prehashed(prehashed_message, context, signature)
     }
 
     /// Strictly verify a signature on a message with this keypair's public key.
@@ -399,8 +423,7 @@ impl Keypair {
         &self,
         message: &[u8],
         signature: &ed25519::Signature,
-    ) -> Result<(), SignatureError>
-    {
+    ) -> Result<(), SignatureError> {
         self.public.verify_strict(message, signature)
     }
 }

src/lib.rs

@@ -87,7 +87,7 @@
 //! # let message: &[u8] = b"This is a test of the tsunami alert system.";
 //! # let signature: Signature = keypair.sign(message);
 //!
-//! let public_key: PublicKey = keypair.public;
+//! let public_key: PublicKey = keypair.public_key();
 //! assert!(public_key.verify(message, &signature).is_ok());
 //! # }
 //! ```
@@ -111,10 +111,9 @@
 //! # let keypair: Keypair = Keypair::generate(&mut csprng);
 //! # let message: &[u8] = b"This is a test of the tsunami alert system.";
 //! # let signature: Signature = keypair.sign(message);
-//! # let public_key: PublicKey = keypair.public;
 //!
-//! let public_key_bytes: [u8; PUBLIC_KEY_LENGTH] = public_key.to_bytes();
-//! let secret_key_bytes: [u8; SECRET_KEY_LENGTH] = keypair.secret.to_bytes();
+//! let public_key_bytes: [u8; PUBLIC_KEY_LENGTH] = keypair.public_key().to_bytes();
+//! let secret_key_bytes: [u8; SECRET_KEY_LENGTH] = keypair.secret_key().to_bytes();
 //! let keypair_bytes:    [u8; KEYPAIR_LENGTH]    = keypair.to_bytes();
 //! let signature_bytes:  [u8; SIGNATURE_LENGTH]  = signature.to_bytes();
 //! # }
@@ -127,15 +126,16 @@
 //! # extern crate ed25519_dalek;
 //! # use std::convert::TryFrom;
 //! # use rand::rngs::OsRng;
+//! # use std::convert::TryInto;
 //! # use ed25519_dalek::{Keypair, Signature, Signer, PublicKey, SecretKey, SignatureError};
 //! # use ed25519_dalek::{PUBLIC_KEY_LENGTH, SECRET_KEY_LENGTH, KEYPAIR_LENGTH, SIGNATURE_LENGTH};
 //! # fn do_test() -> Result<(SecretKey, PublicKey, Keypair, Signature), SignatureError> {
 //! # let mut csprng = OsRng{};
 //! # let keypair_orig: Keypair = Keypair::generate(&mut csprng);
 //! # let message: &[u8] = b"This is a test of the tsunami alert system.";
 //! # let signature_orig: Signature = keypair_orig.sign(message);
-//! # let public_key_bytes: [u8; PUBLIC_KEY_LENGTH] = keypair_orig.public.to_bytes();
-//! # let secret_key_bytes: [u8; SECRET_KEY_LENGTH] = keypair_orig.secret.to_bytes();
+//! # let public_key_bytes: [u8; PUBLIC_KEY_LENGTH] = keypair_orig.public_key().to_bytes();
+//! # let secret_key_bytes: [u8; SECRET_KEY_LENGTH] = keypair_orig.secret_key().to_bytes();
 //! # let keypair_bytes:    [u8; KEYPAIR_LENGTH]    = keypair_orig.to_bytes();
 //! # let signature_bytes:  [u8; SIGNATURE_LENGTH]  = signature_orig.to_bytes();
 //! #
@@ -181,7 +181,7 @@
 //! # let keypair: Keypair = Keypair::generate(&mut csprng);
 //! # let message: &[u8] = b"This is a test of the tsunami alert system.";
 //! # let signature: Signature = keypair.sign(message);
-//! # let public_key: PublicKey = keypair.public;
+//! # let public_key: PublicKey = keypair.public_key();
 //! # let verified: bool = public_key.verify(message, &signature).is_ok();
 //!
 //! let encoded_public_key: Vec<u8> = serialize(&public_key).unwrap();
@@ -213,7 +213,7 @@
 //! # let keypair: Keypair = Keypair::generate(&mut csprng);
 //! let message: &[u8] = b"This is a test of the tsunami alert system.";
 //! # let signature: Signature = keypair.sign(message);
-//! # let public_key: PublicKey = keypair.public;
+//! # let public_key: PublicKey = keypair.public_key();
 //! # let verified: bool = public_key.verify(message, &signature).is_ok();
 //! # let encoded_public_key: Vec<u8> = serialize(&public_key).unwrap();
 //! # let encoded_signature: Vec<u8> = serialize(&signature).unwrap();
@@ -234,7 +234,6 @@
 #![no_std]
 #![warn(future_incompatible)]
 #![deny(missing_docs)] // refuse to compile if documentation is missing
-
 #![cfg_attr(not(test), forbid(unsafe_code))]
 
 #[cfg(any(feature = "std", test))]