Skip to content

Commit

Permalink
[Security]: Rename ExpandedSecretKey::sign_with_pubkey to `dangerou…
Browse files Browse the repository at this point in the history
…s_sign_with_pubkey` (#4154)

* [Security]: Make `ExpandedSecretKey::sign_with_pubkey` unsafe

* [Security]: Rename `ExpandedSecretKey::sign_with_pubkey` to `dangerous_sign_with_pubkey`
  • Loading branch information
satoshiotomakan authored Dec 11, 2024
1 parent 99b69d1 commit a3372c9
Show file tree
Hide file tree
Showing 5 changed files with 17 additions and 28 deletions.
2 changes: 1 addition & 1 deletion rust/tw_keypair/src/ed25519/keypair.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ impl<H: Hasher512> SigningKeyTrait for KeyPair<H> {
type Signature = Signature;

fn sign(&self, message: Self::SigningMessage) -> KeyPairResult<Self::Signature> {
self.private().sign_with_public_key(self.public(), &message)
self.private().sign(message)
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,7 @@ impl<H: Hasher512> SigningKeyTrait for ExtendedKeyPair<H> {
type Signature = Signature;

fn sign(&self, message: Self::SigningMessage) -> KeyPairResult<Self::Signature> {
self.private()
.sign_with_public_key(self.public(), message.as_slice())
self.private().sign(message)
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,25 +43,16 @@ impl<H: Hasher512> ExtendedPrivateKey<H> {

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<H>,
message: &[u8],
) -> KeyPairResult<Signature> {
self.key
.expanded_key
.sign_with_pubkey(public.key_for_signing(), message)
}
}

impl<H: Hasher512> SigningKeyTrait for ExtendedPrivateKey<H> {
type SigningMessage = Vec<u8>;
type Signature = Signature;

fn sign(&self, message: Self::SigningMessage) -> KeyPairResult<Self::Signature> {
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())
}
}

Expand Down
13 changes: 2 additions & 11 deletions rust/tw_keypair/src/ed25519/private.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,24 +36,15 @@ impl<H: Hasher512> PrivateKey<H> {
pub fn public(&self) -> PublicKey<H> {
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<H>,
message: &[u8],
) -> KeyPairResult<Signature> {
self.expanded_key
.sign_with_pubkey(public.to_bytes(), message)
}
}

impl<H: Hasher512> SigningKeyTrait for PrivateKey<H> {
type SigningMessage = Vec<u8>;
type Signature = Signature;

fn sign(&self, message: Self::SigningMessage) -> KeyPairResult<Self::Signature> {
self.sign_with_public_key(&self.public(), &message)
self.expanded_key
.dangerous_sign_with_pubkey(self.public().to_bytes(), &message)
}
}

Expand Down
12 changes: 10 additions & 2 deletions rust/tw_keypair/src/ed25519/secret.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,14 @@ impl<H: Hasher512> ExpandedSecretKey<H> {
/// 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],
Expand Down Expand Up @@ -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);
}
Expand Down

0 comments on commit a3372c9

Please sign in to comment.