Merge pull request from GHSA-wr2p-64gm-8x2c

This allowed users to create invalid Ed25519 Extended Secret Keys
with potentially cryptographically weak ECDSA Signatures.

However we still allow to have an _unsafe_ version to construct
a `SecretKeyExtended` from bytes without performing checks.
This is in order to allow a compatibility path without breaking
codes too much.

allow the direct conversion from XPrv from ed25519_bip32 crates to pallas-crypto's SecretKeyExtended without performing the bit tweaks check

While it is unsafe to call the SecretKeyExtended::from_bytes_unchecked
(unsafe in the cryptographic sense, not in the rust memory management
sense) we know this is going to be okay because the XPrv was already
safely created.

We previously removed the direct conversion of byte arrays into SecretKeyExtended

This has been replaced with a `TryFrom` and a `from_bytes() -> Result<Self>` function.
This allows us to perform the recovery of the wrapped private keys
without losing the security of performing the checks of the validity
of the Ed25519 Extended structure.

This should be safe to use and shouldn't make incompatibilities
because the Xprv was already checked for bit tweaks previously
in the flow.

add unsafe functions to leak the content of the SecretKey or SecretKeyExtended

Remove the From implementation to convert Secret Keys into Bytes

Instead prefer the explicit unsafe functions to leak the content of the keys

temporarily remove the public access of the as_bytes function

this is to prevent leaking the bytes of the private keys.

pallas-crypto/src/key/ed25519.rs

@@ -53,6 +53,15 @@ pub enum TryFromSignatureError {
     InvalidSize,
 }
 
+/// Error type used when retrieving a [`SecretKeyExtended`] via
+/// [`SecretKeyExtended::from_bytes`] or [`TryFrom`].
+///
+#[derive(Debug, Error)]
+pub enum TryFromSecretKeyExtendedError {
+    #[error("Invalid Ed25519 Extended Secret Key format")]
+    InvalidBitTweaks,
+}
+
 macro_rules! impl_size_zero {
     ($Type:ty, $Size:expr) => {
         impl $Type {
@@ -120,6 +129,41 @@ impl SecretKey {
 
         Signature(signature)
     }
+
+    /// convert the [`SecretKey`] into its compressed byte composition
+    ///
+    /// This function is marked unsafe because we wished to highlight the
+    /// importance of keeping the content of the secret key private.
+    /// However there are reasons that may be valid to _leak_ the private
+    /// key: to encrypt it and store securely (as used in `pallas-wallet`'s
+    /// wrapping method for safely storing the private keys).
+    ///
+    /// This function is on purpose an associated function in order to
+    /// force the explicite use of the type name (`SecretKey`) and the
+    /// associated function when calling it: `SecretKey::leak_into_bytes(key)`.
+    ///
+    /// # Safety
+    ///
+    /// This function is not safe because:
+    ///
+    /// * using it removes all the security measure we put in place
+    ///   to protect your private key: opaque [`Debug`] impl, zeroisation on [`Drop`], ...
+    /// * you will need to be careful not to leak the bytes
+    ///
+    /// # Example
+    ///
+    /// ```
+    /// # use pallas_crypto::key::ed25519::SecretKey;
+    /// #
+    /// let key: SecretKey = // ...
+    /// # [0; SecretKey::SIZE].into() ;
+    /// let _: [u8; SecretKey::SIZE] = unsafe { SecretKey::leak_into_bytes(key) };
+    /// ```
+    ///
+    #[inline]
+    pub unsafe fn leak_into_bytes(Self(bytes): Self) -> [u8; Self::SIZE] {
+        bytes
+    }
 }
 
 impl SecretKeyExtended {
@@ -152,6 +196,66 @@ impl SecretKeyExtended {
             && (self.0[31] & 0b1000_0000) == 0
     }
 
+    /// Retrieve a [`SecretKeyExtended`] from the given `bytes`` array.
+    ///
+    /// # error
+    ///
+    /// This function will check that the given bytes are valid for
+    /// an Ed25519 Extended Secret key. I.e. it will check that the
+    /// proper bits have been zeroed.
+    ///
+    /// # Example
+    ///
+    /// ```
+    /// # use pallas_crypto::key::ed25519::{SecretKeyExtended, TryFromSecretKeyExtendedError};
+    /// #
+    /// # fn test() -> Result<(), TryFromSecretKeyExtendedError> {
+    /// let bytes = // ...
+    /// # [0; 64] ;
+    /// let key = SecretKeyExtended::from_bytes(bytes)?;
+    /// # let _ = key; Ok(()) }
+    /// # assert!(matches!(test(), Err(TryFromSecretKeyExtendedError::InvalidBitTweaks)));
+    /// ```
+    ///
+    pub fn from_bytes(bytes: [u8; Self::SIZE]) -> Result<Self, TryFromSecretKeyExtendedError> {
+        let candidate = Self(bytes);
+        if candidate.check_structure() {
+            Ok(candidate)
+        } else {
+            Err(TryFromSecretKeyExtendedError::InvalidBitTweaks)
+        }
+    }
+
+    /// Retrieve a [`SecretKeyExtended`] from the given bytes
+    ///
+    /// **You should prefer [`SecretKeyExtended::from_bytes`] instead
+    /// as this function does not check that the bytes are correct
+    /// for Ed25519 Extended**
+    ///
+    /// # Safety
+    ///
+    /// This function creates a [`SecretKeyExtended`] without checking
+    /// the validity of the bytes (the bits tweaked or not).
+    ///
+    /// It will not panic but using the created key may result to undefined
+    /// behavior or may generate [`Signature`] that are not cryptographically
+    /// secure.
+    ///
+    /// # Example
+    ///
+    /// ```
+    /// # use pallas_crypto::key::ed25519::SecretKeyExtended;
+    /// #
+    /// let bytes = // ...
+    /// # [0; 64] ;
+    /// let key = unsafe { SecretKeyExtended::from_bytes_unchecked(bytes) };
+    /// # let _ = key;
+    /// ```
+    ///
+    pub unsafe fn from_bytes_unchecked(bytes: [u8; Self::SIZE]) -> Self {
+        Self(bytes)
+    }
+
     /// get the [`PublicKey`] associated to this key
     ///
     /// Unlike the [`SecretKeyExtended`], the [`PublicKey`] can be safely
@@ -173,6 +277,41 @@ impl SecretKeyExtended {
 
         Signature::from(signature)
     }
+
+    /// convert the [`SecretKeyExtended`] into its compressed byte composition
+    ///
+    /// This function is marked unsafe because we wished to highlight the
+    /// importance of keeping the content of the secret key private.
+    /// However there are reasons that may be valid to _leak_ the private
+    /// key: to encrypt it and store securely (as used in `pallas-wallet`'s
+    /// wrapping method for safely storing the private keys).
+    ///
+    /// This function is on purpose an associated function in order to
+    /// force the explicite use of the type name (`SecretKeyExtended`) and the
+    /// associated function when calling it: `SecretKeyExtended::leak_into_bytes(key)`.
+    ///
+    /// # Safety
+    ///
+    /// This function is not safe because:
+    ///
+    /// * using it removes all the security measure we put in place
+    ///   to protect your private key: opaque [`Debug`] impl, zeroisation on [`Drop`], ...
+    /// * you will need to be careful not to leak the bytes
+    ///
+    /// # Example
+    ///
+    /// ```
+    /// # use pallas_crypto::key::ed25519::SecretKeyExtended;
+    /// #
+    /// let key: SecretKeyExtended = // ...
+    /// # unsafe { SecretKeyExtended::from_bytes_unchecked([0; SecretKeyExtended::SIZE]) };
+    /// let _: [u8; SecretKeyExtended::SIZE] = unsafe { SecretKeyExtended::leak_into_bytes(key) };
+    /// ```
+    ///
+    #[inline]
+    pub unsafe fn leak_into_bytes(Self(bytes): Self) -> [u8; Self::SIZE] {
+        bytes
+    }
 }
 
 impl PublicKey {
@@ -317,21 +456,10 @@ impl From<[u8; Self::SIZE]> for SecretKey {
     }
 }
 
-impl From<SecretKey> for [u8; SecretKey::SIZE] {
-    fn from(sk: SecretKey) -> Self {
-        sk.0
-    }
-}
-
-impl From<[u8; Self::SIZE]> for SecretKeyExtended {
-    fn from(bytes: [u8; Self::SIZE]) -> Self {
-        Self(bytes)
-    }
-}
-
-impl From<SecretKeyExtended> for [u8; SecretKeyExtended::SIZE] {
-    fn from(ske: SecretKeyExtended) -> Self {
-        ske.0
+impl TryFrom<[u8; Self::SIZE]> for SecretKeyExtended {
+    type Error = TryFromSecretKeyExtendedError;
+    fn try_from(bytes: [u8; Self::SIZE]) -> Result<Self, Self::Error> {
+        Self::from_bytes(bytes)
     }
 }
 

pallas-wallet/src/hd.rs

@@ -3,7 +3,7 @@ use bip39::rand_core::{CryptoRng, RngCore};
 use bip39::{Language, Mnemonic};
 use cryptoxide::{hmac::Hmac, pbkdf2::pbkdf2, sha2::Sha512};
 use ed25519_bip32::{self, XPrv, XPub, XPRV_SIZE};
-use pallas_crypto::key::ed25519;
+use pallas_crypto::key::ed25519::{self, SecretKeyExtended};
 
 use crate::{Error, PrivateKey};
 
@@ -72,7 +72,11 @@ impl Bip32PrivateKey {
     }
 
     pub fn to_ed25519_private_key(&self) -> PrivateKey {
-        PrivateKey::Extended(self.0.extended_secret_key().into())
+        PrivateKey::Extended(unsafe {
+            // The use of unsafe is allowed here. The key is an Extended Secret Key
+            // already because it passed through the ed25519_bip32 crates checks
+            SecretKeyExtended::from_bytes_unchecked(self.0.extended_secret_key())
+        })
     }
 
     pub fn to_public(&self) -> Bip32PublicKey {

pallas-wallet/src/lib.rs

@@ -1,4 +1,6 @@
-use pallas_crypto::key::ed25519::{PublicKey, SecretKey, SecretKeyExtended, Signature};
+use pallas_crypto::key::ed25519::{
+    PublicKey, SecretKey, SecretKeyExtended, Signature, TryFromSecretKeyExtendedError,
+};
 use thiserror::Error;
 
 pub mod hd;
@@ -30,6 +32,10 @@ pub enum Error {
     /// Error when attempting to derive ed25519-bip32 key
     #[error("Error when attempting to derive ed25519-bip32 key: {0}")]
     DerivationError(ed25519_bip32::DerivationError),
+    /// Error that may occurs when trying to decrypt a private key
+    /// which is not valid.
+    #[error("Invalid Ed25519 Extended Secret Key: {0}")]
+    InvalidSecretKeyExtended(#[from] TryFromSecretKeyExtendedError),
 }
 
 /// A standard or extended Ed25519 secret key
@@ -64,14 +70,15 @@ impl PrivateKey {
         }
     }
 
-    pub fn as_bytes(&self) -> Vec<u8> {
+    pub(crate) fn as_bytes(&self) -> Vec<u8> {
         match self {
             Self::Normal(x) => {
-                let bytes: [u8; SecretKey::SIZE] = x.clone().into();
+                let bytes: [u8; SecretKey::SIZE] = unsafe { SecretKey::leak_into_bytes(x.clone()) };
                 bytes.to_vec()
             }
             Self::Extended(x) => {
-                let bytes: [u8; SecretKeyExtended::SIZE] = x.clone().into();
+                let bytes: [u8; SecretKeyExtended::SIZE] =
+                    unsafe { SecretKeyExtended::leak_into_bytes(x.clone()) };
                 bytes.to_vec()
             }
         }

pallas-wallet/src/wrapper.rs

@@ -113,7 +113,7 @@ pub fn decrypt_private_key(password: &String, data: Vec<u8>) -> Result<PrivateKe
             let mut plaintext = [0u8; SecretKeyExtended::SIZE];
 
             if chacha20.decrypt(ciphertext, &mut plaintext, tag) {
-                let secret_key: SecretKeyExtended = plaintext.into();
+                let secret_key = SecretKeyExtended::from_bytes(plaintext)?;
 
                 Ok(secret_key.into())
             } else {