From a3372c97c8fbcc0b7b6a98b6f140c465acf97739 Mon Sep 17 00:00:00 2001 From: Sergei Boiko <127754187+satoshiotomakan@users.noreply.github.com> Date: Wed, 11 Dec 2024 18:24:39 +0700 Subject: [PATCH] [Security]: Rename `ExpandedSecretKey::sign_with_pubkey` to `dangerous_sign_with_pubkey` (#4154) * [Security]: Make `ExpandedSecretKey::sign_with_pubkey` unsafe * [Security]: Rename `ExpandedSecretKey::sign_with_pubkey` to `dangerous_sign_with_pubkey` --- rust/tw_keypair/src/ed25519/keypair.rs | 2 +- .../modifications/cardano/extended_keypair.rs | 3 +-- .../modifications/cardano/extended_private.rs | 15 +++------------ rust/tw_keypair/src/ed25519/private.rs | 13 ++----------- rust/tw_keypair/src/ed25519/secret.rs | 12 ++++++++++-- 5 files changed, 17 insertions(+), 28 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 f746f12d6d1..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,8 +35,7 @@ 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()) + 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 f2fac1a0687..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,17 +43,6 @@ impl ExtendedPrivateKey { ExtendedPublicKey::new(key, second_key) } - - /// `ed25519` signing uses a public key associated with the private key. - pub(crate) 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 { @@ -61,7 +50,9 @@ impl SigningKeyTrait for ExtendedPrivateKey { type Signature = Signature; fn sign(&self, message: Self::SigningMessage) -> KeyPairResult { - 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 984392393a6..ce0c2ef056f 100644 --- a/rust/tw_keypair/src/ed25519/private.rs +++ b/rust/tw_keypair/src/ed25519/private.rs @@ -36,16 +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 { - self.expanded_key - .sign_with_pubkey(public.to_bytes(), message) - } } impl SigningKeyTrait for PrivateKey { @@ -53,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 895f788b15d..5265b3a06d9 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 + /// + /// # 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) fn sign_with_pubkey( + pub(crate) fn dangerous_sign_with_pubkey( &self, pubkey: H256, message: &[u8], @@ -122,7 +128,9 @@ 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 = secret_key + .dangerous_sign_with_pubkey(public, &message) + .unwrap(); let expected = H512::from("ed55bce14a845a275e7a3a7242420ed1eeaba79dc3141bebf42ca0d12169e209a6e56b6981a336f711ae3aaea8d063b72b0e79a8808311d08cb42cabfdd0450d"); assert_eq!(sign.to_bytes(), expected); }