From 9638ab40a51eb203fb93f4b3e630474953602995 Mon Sep 17 00:00:00 2001 From: Alex Xiong Date: Sun, 16 Oct 2022 03:04:03 +0800 Subject: [PATCH] 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 | 24 +++-------- src/batch.rs | 2 +- src/errors.rs | 3 ++ src/keypair.rs | 75 +++++++++++++++++++++++------------ src/lib.rs | 17 ++++---- src/secret.rs | 50 ++++++++++------------- tests/ed25519.rs | 57 ++++++-------------------- 7 files changed, 99 insertions(+), 129 deletions(-) diff --git a/benches/ed25519_benchmarks.rs b/benches/ed25519_benchmarks.rs index 45dce35..125e718 100644 --- a/benches/ed25519_benchmarks.rs +++ b/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 = (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 = keypairs.iter().map(|key| key.sign(&msg)).collect(); - let public_keys: Vec = keypairs.iter().map(|key| key.public).collect(); + let signatures: Vec = + keypairs.iter().map(|key| key.sign(&msg)).collect(); + let public_keys: Vec = + 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, diff --git a/src/batch.rs b/src/batch.rs index 3a4b8e9..cb28188 100644 --- a/src/batch.rs +++ b/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 = keypairs.iter().map(|key| key.sign(&msg)).collect(); -/// let public_keys: Vec = keypairs.iter().map(|key| key.public).collect(); +/// let public_keys: Vec = keypairs.iter().map(|key| key.public_key()).collect(); /// /// let result = verify_batch(&messages[..], &signatures[..], &public_keys[..]); /// assert!(result.is_ok()); diff --git a/src/errors.rs b/src/errors.rs index b66fae0..d4e8201 100644 --- a/src/errors.rs +++ b/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"), } } } diff --git a/src/keypair.rs b/src/keypair.rs index 55af2df..bcbb6e2 100644 --- a/src/keypair.rs +++ b/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 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 { + pub fn from_bytes(bytes: &[u8]) -> Result { 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, { - 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) } } diff --git a/src/lib.rs b/src/lib.rs index c8ee87d..6e8933b 100644 --- a/src/lib.rs +++ b/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,6 +126,7 @@ //! # 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> { @@ -134,8 +134,8 @@ //! # 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 = 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 = serialize(&public_key).unwrap(); //! # let encoded_signature: Vec = 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))] diff --git a/src/secret.rs b/src/secret.rs index 15c0cd4..f8b9da8 100644 --- a/src/secret.rs +++ b/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 { + pub(crate) fn from_bytes(bytes: &[u8]) -> Result { 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(&self, serializer: S) -> Result - where - S: Serializer, - { - let bytes = &self.to_bytes()[..]; - SerdeBytes::new(bytes).serialize(serializer) - } -} - -#[cfg(feature = "serde")] -impl<'d> Deserialize<'d> for ExpandedSecretKey { - fn deserialize(deserializer: D) -> Result - where - D: Deserializer<'d>, - { - let bytes = ::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); + } } diff --git a/tests/ed25519.rs b/tests/ed25519.rs index 0a403be..24740d8 100644 --- a/tests/ed25519.rs +++ b/tests/ed25519.rs @@ -29,7 +29,6 @@ use sha2::Sha512; #[cfg(test)] mod vectors { use curve25519_dalek::{edwards::EdwardsPoint, scalar::Scalar}; - use ed25519::signature::Signature as _; use sha2::{digest::Digest, Sha512}; use std::convert::TryFrom; @@ -69,8 +68,10 @@ mod vectors { let sig_bytes: Vec = FromHex::from_hex(&parts[3]).unwrap(); let secret: SecretKey = SecretKey::from_bytes(&sec_bytes[..SECRET_KEY_LENGTH]).unwrap(); - let public: PublicKey = PublicKey::from_bytes(&pub_bytes[..PUBLIC_KEY_LENGTH]).unwrap(); - let keypair: Keypair = Keypair{ secret: secret, public: public }; + let expected_public: PublicKey = + PublicKey::from_bytes(&pub_bytes[..PUBLIC_KEY_LENGTH]).unwrap(); + let keypair: Keypair = Keypair::from(secret); + assert_eq!(expected_public, keypair.public_key()); // The signatures in the test vectors also include the message // at the end, but we just want R and S. @@ -97,8 +98,10 @@ mod vectors { let sig_bytes: Vec = FromHex::from_hex(signature).unwrap(); let secret: SecretKey = SecretKey::from_bytes(&sec_bytes[..SECRET_KEY_LENGTH]).unwrap(); - let public: PublicKey = PublicKey::from_bytes(&pub_bytes[..PUBLIC_KEY_LENGTH]).unwrap(); - let keypair: Keypair = Keypair{ secret: secret, public: public }; + let expected_public: PublicKey = + PublicKey::from_bytes(&pub_bytes[..PUBLIC_KEY_LENGTH]).unwrap(); + let keypair: Keypair = Keypair::from(secret); + assert_eq!(expected_public, keypair.public_key()); let sig1: Signature = Signature::from_bytes(&sig_bytes[..]).unwrap(); let mut prehash_for_signing: Sha512 = Sha512::default(); @@ -280,17 +283,6 @@ mod integrations { assert!(result.is_ok()); } - - #[test] - fn pubkey_from_secret_and_expanded_secret() { - let mut csprng = 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); - } } #[serde(crate = "serde_crate")] @@ -401,28 +393,6 @@ mod serialisation { } } - #[test] - fn serialize_deserialize_expanded_secret_key_bincode() { - let expanded_secret_key = ExpandedSecretKey::from(&SecretKey::from_bytes(&SECRET_KEY_BYTES).unwrap()); - let encoded_expanded_secret_key: Vec = bincode::serialize(&expanded_secret_key).unwrap(); - let decoded_expanded_secret_key: ExpandedSecretKey = bincode::deserialize(&encoded_expanded_secret_key).unwrap(); - - for i in 0..EXPANDED_SECRET_KEY_LENGTH { - assert_eq!(expanded_secret_key.to_bytes()[i], decoded_expanded_secret_key.to_bytes()[i]); - } - } - - #[test] - fn serialize_deserialize_expanded_secret_key_json() { - let expanded_secret_key = ExpandedSecretKey::from(&SecretKey::from_bytes(&SECRET_KEY_BYTES).unwrap()); - let encoded_expanded_secret_key = serde_json::to_string(&expanded_secret_key).unwrap(); - let decoded_expanded_secret_key: ExpandedSecretKey = serde_json::from_str(&encoded_expanded_secret_key).unwrap(); - - for i in 0..EXPANDED_SECRET_KEY_LENGTH { - assert_eq!(expanded_secret_key.to_bytes()[i], decoded_expanded_secret_key.to_bytes()[i]); - } - } - #[test] fn serialize_deserialize_keypair_bincode() { let keypair = Keypair::from_bytes(&KEYPAIR_BYTES).unwrap(); @@ -471,13 +441,10 @@ mod serialisation { #[test] fn serialize_secret_key_size() { let secret_key: SecretKey = SecretKey::from_bytes(&SECRET_KEY_BYTES).unwrap(); - assert_eq!(bincode::serialized_size(&secret_key).unwrap() as usize, BINCODE_INT_LENGTH + SECRET_KEY_LENGTH); - } - - #[test] - fn serialize_expanded_secret_key_size() { - let expanded_secret_key = ExpandedSecretKey::from(&SecretKey::from_bytes(&SECRET_KEY_BYTES).unwrap()); - assert_eq!(bincode::serialized_size(&expanded_secret_key).unwrap() as usize, BINCODE_INT_LENGTH + EXPANDED_SECRET_KEY_LENGTH); + assert_eq!( + bincode::serialized_size(&secret_key).unwrap() as usize, + BINCODE_INT_LENGTH + SECRET_KEY_LENGTH + ); } #[test]