From d71c775052efa2420182fe0efb23808aedcd7cca Mon Sep 17 00:00:00 2001 From: Satoshi Otomakan Date: Fri, 6 Dec 2024 16:20:07 +0100 Subject: [PATCH 1/2] [Security]: Make `ExpandedSecretKey::sign_with_pubkey` unsafe --- .../ed25519/modifications/cardano/extended_keypair.rs | 6 ++++-- .../ed25519/modifications/cardano/extended_private.rs | 10 ++++++++-- rust/tw_keypair/src/ed25519/private.rs | 6 ++++-- rust/tw_keypair/src/ed25519/secret.rs | 10 ++++++++-- 4 files changed, 24 insertions(+), 8 deletions(-) diff --git a/rust/tw_keypair/src/ed25519/modifications/cardano/extended_keypair.rs b/rust/tw_keypair/src/ed25519/modifications/cardano/extended_keypair.rs index f746f12d6d1..09f42fe03fb 100644 --- a/rust/tw_keypair/src/ed25519/modifications/cardano/extended_keypair.rs +++ b/rust/tw_keypair/src/ed25519/modifications/cardano/extended_keypair.rs @@ -35,8 +35,10 @@ impl SigningKeyTrait for ExtendedKeyPair { type Signature = Signature; fn sign(&self, message: Self::SigningMessage) -> KeyPairResult { - self.private() - .sign_with_public_key(self.public(), message.as_slice()) + unsafe { + self.private() + .sign_with_public_key(self.public(), message.as_slice()) + } } } diff --git a/rust/tw_keypair/src/ed25519/modifications/cardano/extended_private.rs b/rust/tw_keypair/src/ed25519/modifications/cardano/extended_private.rs index f2fac1a0687..1fe056621fd 100644 --- a/rust/tw_keypair/src/ed25519/modifications/cardano/extended_private.rs +++ b/rust/tw_keypair/src/ed25519/modifications/cardano/extended_private.rs @@ -45,7 +45,13 @@ impl ExtendedPrivateKey { } /// `ed25519` signing uses a public key associated with the private key. - pub(crate) fn sign_with_public_key( + /// + /// # Unsafe + /// + /// Ensure that the public key is always correctly paired with the private key, + /// preventing scenarios where an arbitrary public key could be introduced into the signing process. + /// Security report: https://github.com/trustwallet/wallet-core/security/advisories/GHSA-7g72-jxww-q9vq + pub(crate) unsafe fn sign_with_public_key( &self, public: &ExtendedPublicKey, message: &[u8], @@ -61,7 +67,7 @@ impl SigningKeyTrait for ExtendedPrivateKey { type Signature = Signature; fn sign(&self, message: Self::SigningMessage) -> KeyPairResult { - self.sign_with_public_key(&self.public(), message.as_slice()) + unsafe { self.sign_with_public_key(&self.public(), message.as_slice()) } } } diff --git a/rust/tw_keypair/src/ed25519/private.rs b/rust/tw_keypair/src/ed25519/private.rs index 984392393a6..2af319eb34f 100644 --- a/rust/tw_keypair/src/ed25519/private.rs +++ b/rust/tw_keypair/src/ed25519/private.rs @@ -43,8 +43,10 @@ impl PrivateKey { public: &PublicKey, message: &[u8], ) -> KeyPairResult { - self.expanded_key - .sign_with_pubkey(public.to_bytes(), message) + unsafe { + self.expanded_key + .sign_with_pubkey(public.to_bytes(), message) + } } } diff --git a/rust/tw_keypair/src/ed25519/secret.rs b/rust/tw_keypair/src/ed25519/secret.rs index 895f788b15d..f2e93fd0a45 100644 --- a/rust/tw_keypair/src/ed25519/secret.rs +++ b/rust/tw_keypair/src/ed25519/secret.rs @@ -69,8 +69,14 @@ impl ExpandedSecretKey { /// Signs a message with this `ExpandedSecretKey`. /// Source: https://github.com/dalek-cryptography/ed25519-dalek/blob/1.0.1/src/secret.rs#L389-L412 /// Ported: https://github.com/trustwallet/wallet-core/blob/423f0e34725f69c0a9d535e1a32534c99682edea/trezor-crypto/crypto/ed25519-donna/ed25519.c#L97-L130 + /// + /// # Unsafe + /// + /// Ensure that the public key is always correctly paired with the private key, + /// preventing scenarios where an arbitrary public key could be introduced into the signing process. + /// Security report: https://github.com/trustwallet/wallet-core/security/advisories/GHSA-7g72-jxww-q9vq #[allow(non_snake_case)] - pub(crate) fn sign_with_pubkey( + pub(crate) unsafe fn sign_with_pubkey( &self, pubkey: H256, message: &[u8], @@ -122,7 +128,7 @@ mod tests { let message = hex::decode("f0").unwrap(); // Anyway, the result signature has an expected value. - let sign = secret_key.sign_with_pubkey(public, &message).unwrap(); + let sign = unsafe { secret_key.sign_with_pubkey(public, &message).unwrap() }; let expected = H512::from("ed55bce14a845a275e7a3a7242420ed1eeaba79dc3141bebf42ca0d12169e209a6e56b6981a336f711ae3aaea8d063b72b0e79a8808311d08cb42cabfdd0450d"); assert_eq!(sign.to_bytes(), expected); } From aad7ee8101c712dd51489963905da6f98b3ecec0 Mon Sep 17 00:00:00 2001 From: Satoshi Otomakan Date: Fri, 6 Dec 2024 18:13:49 +0100 Subject: [PATCH 2/2] [Security]: Rename `ExpandedSecretKey::sign_with_pubkey` to `dangerous_sign_with_pubkey` --- rust/tw_keypair/src/ed25519/keypair.rs | 2 +- .../modifications/cardano/extended_keypair.rs | 5 +---- .../modifications/cardano/extended_private.rs | 21 +++---------------- rust/tw_keypair/src/ed25519/private.rs | 15 ++----------- rust/tw_keypair/src/ed25519/secret.rs | 8 ++++--- 5 files changed, 12 insertions(+), 39 deletions(-) diff --git a/rust/tw_keypair/src/ed25519/keypair.rs b/rust/tw_keypair/src/ed25519/keypair.rs index 3ac2ac1e1fa..636bbb0ad68 100644 --- a/rust/tw_keypair/src/ed25519/keypair.rs +++ b/rust/tw_keypair/src/ed25519/keypair.rs @@ -33,7 +33,7 @@ impl SigningKeyTrait for KeyPair { type Signature = Signature; fn sign(&self, message: Self::SigningMessage) -> KeyPairResult { - self.private().sign_with_public_key(self.public(), &message) + self.private().sign(message) } } diff --git a/rust/tw_keypair/src/ed25519/modifications/cardano/extended_keypair.rs b/rust/tw_keypair/src/ed25519/modifications/cardano/extended_keypair.rs index 09f42fe03fb..eef6895bde7 100644 --- a/rust/tw_keypair/src/ed25519/modifications/cardano/extended_keypair.rs +++ b/rust/tw_keypair/src/ed25519/modifications/cardano/extended_keypair.rs @@ -35,10 +35,7 @@ impl SigningKeyTrait for ExtendedKeyPair { type Signature = Signature; fn sign(&self, message: Self::SigningMessage) -> KeyPairResult { - unsafe { - self.private() - .sign_with_public_key(self.public(), message.as_slice()) - } + self.private().sign(message) } } diff --git a/rust/tw_keypair/src/ed25519/modifications/cardano/extended_private.rs b/rust/tw_keypair/src/ed25519/modifications/cardano/extended_private.rs index 1fe056621fd..6a2bd83124a 100644 --- a/rust/tw_keypair/src/ed25519/modifications/cardano/extended_private.rs +++ b/rust/tw_keypair/src/ed25519/modifications/cardano/extended_private.rs @@ -43,23 +43,6 @@ impl ExtendedPrivateKey { ExtendedPublicKey::new(key, second_key) } - - /// `ed25519` signing uses a public key associated with the private key. - /// - /// # Unsafe - /// - /// Ensure that the public key is always correctly paired with the private key, - /// preventing scenarios where an arbitrary public key could be introduced into the signing process. - /// Security report: https://github.com/trustwallet/wallet-core/security/advisories/GHSA-7g72-jxww-q9vq - pub(crate) unsafe fn sign_with_public_key( - &self, - public: &ExtendedPublicKey, - message: &[u8], - ) -> KeyPairResult { - self.key - .expanded_key - .sign_with_pubkey(public.key_for_signing(), message) - } } impl SigningKeyTrait for ExtendedPrivateKey { @@ -67,7 +50,9 @@ impl SigningKeyTrait for ExtendedPrivateKey { type Signature = Signature; fn sign(&self, message: Self::SigningMessage) -> KeyPairResult { - unsafe { self.sign_with_public_key(&self.public(), message.as_slice()) } + self.key + .expanded_key + .dangerous_sign_with_pubkey(self.public().key_for_signing(), message.as_slice()) } } diff --git a/rust/tw_keypair/src/ed25519/private.rs b/rust/tw_keypair/src/ed25519/private.rs index 2af319eb34f..ce0c2ef056f 100644 --- a/rust/tw_keypair/src/ed25519/private.rs +++ b/rust/tw_keypair/src/ed25519/private.rs @@ -36,18 +36,6 @@ impl PrivateKey { pub fn public(&self) -> PublicKey { PublicKey::with_expanded_secret(&self.expanded_key) } - - /// `ed25519` signing uses a public key associated with the private key. - pub(crate) fn sign_with_public_key( - &self, - public: &PublicKey, - message: &[u8], - ) -> KeyPairResult { - unsafe { - self.expanded_key - .sign_with_pubkey(public.to_bytes(), message) - } - } } impl SigningKeyTrait for PrivateKey { @@ -55,7 +43,8 @@ impl SigningKeyTrait for PrivateKey { type Signature = Signature; fn sign(&self, message: Self::SigningMessage) -> KeyPairResult { - self.sign_with_public_key(&self.public(), &message) + self.expanded_key + .dangerous_sign_with_pubkey(self.public().to_bytes(), &message) } } diff --git a/rust/tw_keypair/src/ed25519/secret.rs b/rust/tw_keypair/src/ed25519/secret.rs index f2e93fd0a45..5265b3a06d9 100644 --- a/rust/tw_keypair/src/ed25519/secret.rs +++ b/rust/tw_keypair/src/ed25519/secret.rs @@ -70,13 +70,13 @@ impl ExpandedSecretKey { /// Source: https://github.com/dalek-cryptography/ed25519-dalek/blob/1.0.1/src/secret.rs#L389-L412 /// Ported: https://github.com/trustwallet/wallet-core/blob/423f0e34725f69c0a9d535e1a32534c99682edea/trezor-crypto/crypto/ed25519-donna/ed25519.c#L97-L130 /// - /// # Unsafe + /// # Important /// /// Ensure that the public key is always correctly paired with the private key, /// preventing scenarios where an arbitrary public key could be introduced into the signing process. /// Security report: https://github.com/trustwallet/wallet-core/security/advisories/GHSA-7g72-jxww-q9vq #[allow(non_snake_case)] - pub(crate) unsafe fn sign_with_pubkey( + pub(crate) fn dangerous_sign_with_pubkey( &self, pubkey: H256, message: &[u8], @@ -128,7 +128,9 @@ mod tests { let message = hex::decode("f0").unwrap(); // Anyway, the result signature has an expected value. - let sign = unsafe { secret_key.sign_with_pubkey(public, &message).unwrap() }; + let sign = secret_key + .dangerous_sign_with_pubkey(public, &message) + .unwrap(); let expected = H512::from("ed55bce14a845a275e7a3a7242420ed1eeaba79dc3141bebf42ca0d12169e209a6e56b6981a336f711ae3aaea8d063b72b0e79a8808311d08cb42cabfdd0450d"); assert_eq!(sign.to_bytes(), expected); }