Skip to content

Commit

Permalink
fix: use domain separation for wallet message signing (#5400)
Browse files Browse the repository at this point in the history
Description
---
Uses domain separation for wallet message Schnorr signatures to prevent
context misuse. Supersedes [PR
5394](#5394).

Motivation and Context
---
Wallets can sign and verify arbitrary messages using their secret keys.
Because such messages are signed using Schnorr signatures with a default
Fiat-Shamir challenge hash domain, it may be possible to replay them in
different contexts, with possibly dangerous consequences.

Another [pending PR](#5394)
suggests prepending fixed data to wallet messages, but this can still
lead to collisions.

Fortunately, the `tari-crypto` signature API provides a [handy type
alias](https://github.com/tari-project/tari-crypto/blob/8a0823ff690f35409b64ad85a064033706f17580/src/ristretto/ristretto_sig.rs#L116)
that makes domain separation straightforward. Using this, it is not
possible to produce signature collisions (up to the collision resistance
of the hash function itself). This is a safer and more idiomatic design.

How Has This Been Tested?
---
An existing test passes.

What process can a PR reviewer use to test or verify this change?
---
Confirm that the `tari-crypto` signature API is being used correctly,
and that the macro-derived domain separator is unique within the
codebase.

Breaking Changes
---
Existing signatures will fail to verify, but this is unlikely to be
problematic.
  • Loading branch information
AaronFeickert committed May 22, 2023
1 parent cbdca6f commit 7d71f8b
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 15 deletions.
3 changes: 3 additions & 0 deletions base_layer/common_types/src/types/mod.rs
Expand Up @@ -32,6 +32,7 @@ use tari_crypto::{
RistrettoComAndPubSig,
RistrettoPublicKey,
RistrettoSchnorr,
RistrettoSchnorrWithDomain,
RistrettoSecretKey,
},
};
Expand All @@ -43,6 +44,8 @@ pub use fixed_hash::{FixedHash, FixedHashSizeError};
/// Define the explicit Signature implementation for the Tari base layer. A different signature scheme can be
/// employed by redefining this type.
pub type Signature = RistrettoSchnorr;
/// Define a generic signature type using a hash domain.
pub type SignatureWithDomain<H> = RistrettoSchnorrWithDomain<H>;
/// Define the explicit Commitment Signature implementation for the Tari base layer.
pub type ComAndPubSignature = RistrettoComAndPubSig;

Expand Down
25 changes: 13 additions & 12 deletions base_layer/wallet/src/wallet.rs
Expand Up @@ -27,7 +27,7 @@ use tari_common::configuration::bootstrap::ApplicationType;
use tari_common_types::{
tari_address::TariAddress,
transaction::{ImportStatus, TxId},
types::{ComAndPubSignature, Commitment, PrivateKey, PublicKey, Signature},
types::{ComAndPubSignature, Commitment, PrivateKey, PublicKey, SignatureWithDomain},
};
use tari_comms::{
multiaddr::Multiaddr,
Expand All @@ -53,12 +53,7 @@ use tari_core::{
CryptoFactories,
},
};
use tari_crypto::{
hash::blake2::Blake256,
ristretto::{RistrettoPublicKey, RistrettoSchnorr, RistrettoSecretKey},
signatures::{SchnorrSignature, SchnorrSignatureError},
tari_utilities::hex::Hex,
};
use tari_crypto::{hash::blake2::Blake256, hash_domain, signatures::SchnorrSignatureError, tari_utilities::hex::Hex};
use tari_key_manager::{
cipher_seed::CipherSeed,
key_manager::KeyManager,
Expand Down Expand Up @@ -108,6 +103,12 @@ const LOG_TARGET: &str = "wallet";
/// The minimum buffer size for the wallet pubsub_connector channel
const WALLET_BUFFER_MIN_SIZE: usize = 300;

// Domain separator for signing arbitrary messages with a wallet secret key
hash_domain!(
WalletMessageSigningDomain,
"com.tari.tari_project.base_layer.wallet.message_signing"
);

/// A structure containing the config and services that a Wallet application will require. This struct will start up all
/// the services and provide the APIs that applications will use to interact with the services
#[derive(Clone)]
Expand Down Expand Up @@ -500,16 +501,16 @@ where

pub fn sign_message(
&mut self,
secret: &RistrettoSecretKey,
secret: &PrivateKey,
message: &str,
) -> Result<SchnorrSignature<RistrettoPublicKey, RistrettoSecretKey>, SchnorrSignatureError> {
RistrettoSchnorr::sign_message(secret, message.as_bytes())
) -> Result<SignatureWithDomain<WalletMessageSigningDomain>, SchnorrSignatureError> {
SignatureWithDomain::<WalletMessageSigningDomain>::sign_message(secret, message.as_bytes())
}

pub fn verify_message_signature(
&mut self,
public_key: &RistrettoPublicKey,
signature: &Signature,
public_key: &PublicKey,
signature: &SignatureWithDomain<WalletMessageSigningDomain>,
message: &str,
) -> bool {
signature.verify_message(public_key, message)
Expand Down
6 changes: 3 additions & 3 deletions base_layer/wallet_ffi/src/lib.rs
Expand Up @@ -91,7 +91,7 @@ use tari_common_types::{
emoji::emoji_set,
tari_address::{TariAddress, TariAddressError},
transaction::{TransactionDirection, TransactionStatus, TxId},
types::{ComAndPubSignature, Commitment, PublicKey, Signature},
types::{ComAndPubSignature, Commitment, PublicKey, SignatureWithDomain},
};
use tari_comms::{
multiaddr::Multiaddr,
Expand Down Expand Up @@ -161,7 +161,7 @@ use tari_wallet::{
},
},
utxo_scanner_service::{service::UtxoScannerService, RECOVERY_KEY},
wallet::{derive_comms_secret_key, read_or_create_master_seed},
wallet::{derive_comms_secret_key, read_or_create_master_seed, WalletMessageSigningDomain},
Wallet,
WalletConfig,
WalletSqlite,
Expand Down Expand Up @@ -6163,7 +6163,7 @@ pub unsafe extern "C" fn wallet_verify_message_signature(
let public_nonce = TariPublicKey::from_hex(key2);
match public_nonce {
Ok(pn) => {
let sig = Signature::new(pn, p);
let sig = SignatureWithDomain::<WalletMessageSigningDomain>::new(pn, p);
result = (*wallet).wallet.verify_message_signature(&*public_key, &sig, &message)
},
Err(e) => {
Expand Down

0 comments on commit 7d71f8b

Please sign in to comment.