restrict ExpandedSK::sign visibility to avoid pk oracle

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))]

src/secret.rs

@@ -239,7 +239,7 @@ impl<'d> Deserialize<'d> for SecretKey {
 // same signature scheme, and which both fail in exactly the same way.  For a
 // better-designed, Schnorr-based signature scheme, see Trevor Perrin's work on
 // "generalised EdDSA" and "VXEdDSA".
-pub struct ExpandedSecretKey {
+pub(crate) struct ExpandedSecretKey {
     pub(crate) key: Scalar,
     pub(crate) nonce: [u8; 32],
 }
@@ -256,7 +256,7 @@ impl<'a> From<&'a SecretKey> for ExpandedSecretKey {
     ///
     /// # Examples
     ///
-    /// ```
+    /// ```ignore
     /// # extern crate rand;
     /// # extern crate sha2;
     /// # extern crate ed25519_dalek;
@@ -302,7 +302,7 @@ impl ExpandedSecretKey {
     ///
     /// # Examples
     ///
-    /// ```
+    /// ```ignore
     /// # extern crate rand;
     /// # extern crate sha2;
     /// # extern crate ed25519_dalek;
@@ -342,7 +342,7 @@ impl ExpandedSecretKey {
     ///
     /// # Examples
     ///
-    /// ```
+    /// ```ignore
     /// # extern crate rand;
     /// # extern crate sha2;
     /// # extern crate ed25519_dalek;
@@ -375,12 +375,13 @@ impl ExpandedSecretKey {
     /// # fn main() { }
     /// ```
     #[inline]
-    pub fn from_bytes(bytes: &[u8]) -> Result<ExpandedSecretKey, SignatureError> {
+    pub(crate) fn from_bytes(bytes: &[u8]) -> Result<ExpandedSecretKey, SignatureError> {
         if bytes.len() != EXPANDED_SECRET_KEY_LENGTH {
             return Err(InternalError::BytesLengthError {
                 name: "ExpandedSecretKey",
                 length: EXPANDED_SECRET_KEY_LENGTH,
-            }.into());
+            }
+            .into());
         }
         let mut lower: [u8; 32] = [0u8; 32];
         let mut upper: [u8; 32] = [0u8; 32];
@@ -396,7 +397,7 @@ impl ExpandedSecretKey {
 
     /// Sign a message with this `ExpandedSecretKey`.
     #[allow(non_snake_case)]
-    pub fn sign(&self, message: &[u8], public_key: &PublicKey) -> ed25519::Signature {
+    pub(crate) fn sign(&self, message: &[u8], public_key: &PublicKey) -> ed25519::Signature {
         let mut h: Sha512 = Sha512::new();
         let R: CompressedEdwardsY;
         let r: Scalar;
@@ -441,7 +442,7 @@ impl ExpandedSecretKey {
     ///
     /// [rfc8032]: https://tools.ietf.org/html/rfc8032#section-5.1
     #[allow(non_snake_case)]
-    pub fn sign_prehashed<'a, D>(
+    pub(crate) fn sign_prehashed<'a, D>(
         &self,
         prehashed_message: D,
         public_key: &PublicKey,
@@ -507,28 +508,6 @@ impl ExpandedSecretKey {
     }
 }
 
-#[cfg(feature = "serde")]
-impl Serialize for ExpandedSecretKey {
-    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
-    where
-        S: Serializer,
-    {
-        let bytes = &self.to_bytes()[..];
-        SerdeBytes::new(bytes).serialize(serializer)
-    }
-}
-
-#[cfg(feature = "serde")]
-impl<'d> Deserialize<'d> for ExpandedSecretKey {
-    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
-    where
-        D: Deserializer<'d>,
-    {
-        let bytes = <SerdeByteBuf>::deserialize(deserializer)?;
-        ExpandedSecretKey::from_bytes(bytes.as_ref()).map_err(SerdeError::custom)
-    }
-}
-
 #[cfg(test)]
 mod test {
     use super::*;
@@ -547,4 +526,15 @@ mod test {
 
         assert!(!memory.contains(&0x15));
     }
+
+    #[test]
+    fn pubkey_from_secret_and_expanded_secret() {
+        let mut csprng = rand::rngs::OsRng {};
+        let secret: SecretKey = SecretKey::generate(&mut csprng);
+        let expanded_secret: ExpandedSecretKey = (&secret).into();
+        let public_from_secret: PublicKey = (&secret).into(); // XXX eww
+        let public_from_expanded_secret: PublicKey = (&expanded_secret).into(); // XXX eww
+
+        assert!(public_from_secret == public_from_expanded_secret);
+    }
 }