Skip to content

Commit

Permalink
[Security]: Make ExpandedSecretKey::sign_with_pubkey unsafe
Browse files Browse the repository at this point in the history
  • Loading branch information
satoshiotomakan committed Dec 6, 2024
1 parent 08e496a commit d71c775
Show file tree
Hide file tree
Showing 4 changed files with 24 additions and 8 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,10 @@ 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())
unsafe {
self.private()
.sign_with_public_key(self.public(), message.as_slice())
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,13 @@ impl<H: Hasher512> ExtendedPrivateKey<H> {
}

/// `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<H>,
message: &[u8],
Expand All @@ -61,7 +67,7 @@ impl<H: Hasher512> SigningKeyTrait for ExtendedPrivateKey<H> {
type Signature = Signature;

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

Expand Down
6 changes: 4 additions & 2 deletions rust/tw_keypair/src/ed25519/private.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,10 @@ impl<H: Hasher512> PrivateKey<H> {
public: &PublicKey<H>,
message: &[u8],
) -> KeyPairResult<Signature> {
self.expanded_key
.sign_with_pubkey(public.to_bytes(), message)
unsafe {
self.expanded_key
.sign_with_pubkey(public.to_bytes(), message)
}
}
}

Expand Down
10 changes: 8 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
///
/// # 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],
Expand Down Expand Up @@ -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);
}
Expand Down

0 comments on commit d71c775

Please sign in to comment.