Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat!: improve signature api #145

Merged
merged 11 commits into from
Nov 9, 2022
2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ categories = ["cryptography"]
homepage = "https://tari.com"
readme = "README.md"
license = "BSD-3-Clause"
version = "0.15.7"
version = "0.16.0"
edition = "2018"

[dependencies]
Expand Down
10 changes: 4 additions & 6 deletions benches/signatures.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ fn native_keypair(c: &mut Criterion) {
struct SigningData {
k: RistrettoSecretKey,
p: RistrettoPublicKey,
r: RistrettoSecretKey,
m: RistrettoSecretKey,
}

Expand All @@ -39,17 +38,16 @@ fn gen_keypair() -> SigningData {
let mut msg = [0u8; 32];
rng.fill_bytes(&mut msg);
let (k, p) = RistrettoPublicKey::random_keypair(&mut rng);
let r = RistrettoSecretKey::random(&mut rng);
let m = RistrettoSecretKey::from_bytes(&msg).unwrap();
CjS77 marked this conversation as resolved.
Show resolved Hide resolved
SigningData { k, p, r, m }
SigningData { k, p, m }
}

fn sign_message(c: &mut Criterion) {
c.bench_function("Create RistrettoSchnorr", move |b| {
b.iter_batched(
gen_keypair,
|d| {
let _sig = RistrettoSchnorr::sign(d.k, d.r, &d.m.to_vec()).unwrap();
let _sig = RistrettoSchnorr::sign_message(&d.k, d.m.to_vec()).unwrap();
},
BatchSize::SmallInput,
);
Expand All @@ -63,10 +61,10 @@ fn verify_message(c: &mut Criterion) {
b.iter_batched(
|| {
let d = gen_keypair();
let s = RistrettoSchnorr::sign(d.k.clone(), d.r.clone(), &d.m.to_vec()).unwrap();
let s = RistrettoSchnorr::sign_message(&d.k, d.m.to_vec()).unwrap();
(d, s)
},
|(d, s)| assert!(s.verify(&d.p, &d.m)),
|(d, s)| assert!(s.verify_message(&d.p, d.m.as_bytes())),
BatchSize::SmallInput,
);
});
Expand Down
15 changes: 7 additions & 8 deletions src/ffi/keys.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ use crate::{
RistrettoSchnorr,
RistrettoSecretKey,
},
signatures::SchnorrSignature,
};

pub const KEY_LENGTH: usize = 32;
Expand Down Expand Up @@ -70,13 +71,15 @@ pub unsafe extern "C" fn sign(
Ok(k) => k,
_ => return INVALID_SECRET_KEY_SER,
};
let pubkey = RistrettoPublicKey::from_secret_key(&k);
let r = RistrettoSecretKey::random(&mut OsRng);
let pub_r = RistrettoPublicKey::from_secret_key(&r);
let msg = match CStr::from_ptr(msg).to_str() {
Ok(s) => s,
_ => return STR_CONV_ERR,
};
let challenge = Blake256::digest(msg.as_bytes());
let sig = match RistrettoSchnorr::sign(k, r, &challenge) {
let e = SchnorrSignature::construct_domain_separated_challenge::<_, Blake256>(&pub_r, &pubkey, msg.as_bytes());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docstring for this function on line 55 (can't make a review comment on that line directly) should be updated to be consistent (it was wrong before, too):

  • The caller provides the private key and public nonce, not the private key and challenge
  • The caller must never reuse a nonce with the same private key (and shouldn't reuse it in any other circumstance, since in that case something else has probably gone horribly wrong)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The caller does not provide a nonce. The nonce argument is mutated to receive the public nonce value. I updated the docs to point this out and changed the argument name to be more specific.

let sig = match RistrettoSchnorr::sign_raw(&k, r, e.as_ref()) {
Ok(sig) => sig,
_ => return SIGNING_ERROR,
};
Expand Down Expand Up @@ -123,13 +126,9 @@ pub unsafe extern "C" fn verify(
Ok(s) => s,
_ => return false,
};

let sig = RistrettoSchnorr::new(r_pub, sig);
let challenge = Blake256::digest(msg.as_bytes());
let challenge = match RistrettoSecretKey::from_bytes(challenge.as_slice()) {
Ok(e) => e,
_ => return false,
};
sig.verify(&pk, &challenge)
sig.verify_message(&pk, msg.as_bytes())
CjS77 marked this conversation as resolved.
Show resolved Hide resolved
}

/// Generate a Pedersen commitment (C) using the provided value and spending key (a, x).
Expand Down
1 change: 1 addition & 0 deletions src/hashing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -520,6 +520,7 @@ pub trait DerivedKeyDomain: DomainSeparation {
#[macro_export]
macro_rules! hash_domain {
($name:ident, $domain:expr, $version: expr) => {
/// A hashing domain instance
#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
pub struct $name;

Expand Down
76 changes: 54 additions & 22 deletions src/ristretto/ristretto_sig.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,8 @@ use crate::{
///
/// #[allow(non_snake_case)]
/// let (k, P) = get_keypair();
/// let (r, R) = get_keypair();
/// let e = Blake256::digest(b"Small Gods");
/// let sig = RistrettoSchnorr::sign(k, r, &e);
/// let msg = "Small Gods";
/// let sig = RistrettoSchnorr::sign_message(&k, &msg);
/// ```
///
/// # Verifying signatures
Expand All @@ -72,34 +71,32 @@ use crate::{
/// # use tari_utilities::ByteArray;
/// # use digest::Digest;
///
/// # #[allow(non_snake_case)]
/// let P = RistrettoPublicKey::from_hex(
/// "74896a30c89186b8194e25f8c1382f8d3081c5a182fb8f8a6d34f27fbefbfc70",
/// )
/// .unwrap();
/// let R = RistrettoPublicKey::from_hex(
/// "fa14cb581ce5717248444721242e6b195a482d503a853dea4acb513074d8d803",
/// )
/// .unwrap();
/// let s = RistrettoSecretKey::from_hex(
/// let msg = "Maskerade";
/// let k = RistrettoSecretKey::from_hex(
/// "bd0b253a619310340a4fa2de54cdd212eac7d088ee1dc47e305c3f6cbd020908",
/// )
/// .unwrap();
/// let sig = RistrettoSchnorr::new(R, s);
/// let e = Blake256::digest(b"Maskerade");
/// assert!(sig.verify_challenge(&P, &e));
/// # #[allow(non_snake_case)]
/// let P = RistrettoPublicKey::from_secret_key(&k);
/// let sig: SchnorrSignature<RistrettoPublicKey, RistrettoSecretKey> =
/// SchnorrSignature::sign_message(&k, msg).unwrap();
/// assert!(sig.verify_message(&P, msg));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docstring above is incorrect. It should not state that the implementer provides the challenge along with the signature, as we now take care of challenge generation internally.

/// ```
pub type RistrettoSchnorr = SchnorrSignature<RistrettoPublicKey, RistrettoSecretKey>;

#[cfg(test)]
mod test {
use digest::Digest;
use tari_utilities::{hex::from_hex, ByteArray};
use tari_utilities::{
hex::{from_hex, to_hex, Hex},
ByteArray,
};

use crate::{
hash::blake2::Blake256,
keys::{PublicKey, SecretKey},
ristretto::{RistrettoPublicKey, RistrettoSchnorr, RistrettoSecretKey},
signatures::SchnorrSignature,
};

#[test]
Expand All @@ -112,18 +109,19 @@ mod test {
/// Create a signature, and then verify it. Also checks that some invalid signatures fail to verify
#[test]
#[allow(non_snake_case)]
fn sign_and_verify_message() {
fn raw_sign_and_verify_challenge() {
let mut rng = rand::thread_rng();
let (k, P) = RistrettoPublicKey::random_keypair(&mut rng);
let (r, R) = RistrettoPublicKey::random_keypair(&mut rng);
// Use sign raw, and bind the nonce and public key manually
let e = Blake256::new()
.chain(P.as_bytes())
.chain(R.as_bytes())
.chain(b"Small Gods")
.finalize();
let e_key = RistrettoSecretKey::from_bytes(&e).unwrap();
let s = &r + &e_key * &k;
let sig = RistrettoSchnorr::sign(k, r, &e).unwrap();
let sig = RistrettoSchnorr::sign_raw(&k, r, &e).unwrap();
let R_calc = sig.get_public_nonce();
assert_eq!(R, *R_calc);
assert_eq!(sig.get_signature(), &s);
Expand Down Expand Up @@ -155,9 +153,9 @@ mod test {
.chain(b"Moving Pictures")
.finalize();
// Calculate Alice's signature
let s1 = RistrettoSchnorr::sign(k1, r1, &e).unwrap();
let s1 = RistrettoSchnorr::sign_raw(&k1, r1, &e).unwrap();
// Calculate Bob's signature
let s2 = RistrettoSchnorr::sign(k2, r2, &e).unwrap();
let s2 = RistrettoSchnorr::sign_raw(&k2, r2, &e).unwrap();
// Now add the two signatures together
let s_agg = &s1 + &s2;
// Check that the multi-sig verifies
Expand All @@ -167,11 +165,45 @@ mod test {
/// Ristretto scalars have a max value 2^255. This test checks that hashed messages above this value can still be
/// signed as a result of applying modulo arithmetic on the challenge value
#[test]
#[allow(non_snake_case)]
fn challenge_from_invalid_scalar() {
let mut rng = rand::thread_rng();
let m = from_hex("ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff").unwrap();
let k = RistrettoSecretKey::random(&mut rng);
let r = RistrettoSecretKey::random(&mut rng);
assert!(RistrettoSchnorr::sign(k, r, &m).is_ok());
assert!(RistrettoSchnorr::sign_raw(&k, r, &m).is_ok());
}

#[test]
#[allow(non_snake_case)]
fn domain_separated_challenge() {
let P =
RistrettoPublicKey::from_hex("74896a30c89186b8194e25f8c1382f8d3081c5a182fb8f8a6d34f27fbefbfc70").unwrap();
let R =
RistrettoPublicKey::from_hex("fa14cb581ce5717248444721242e6b195a482d503a853dea4acb513074d8d803").unwrap();
let msg = "Moving Pictures";
let hash = SchnorrSignature::construct_domain_separated_challenge::<_, Blake256>(&R, &P, msg);
let naiive = Blake256::new()
.chain(R.as_bytes())
.chain(P.as_bytes())
CjS77 marked this conversation as resolved.
Show resolved Hide resolved
.chain(msg)
.finalize()
.to_vec();
assert_ne!(hash.as_ref(), naiive.as_bytes());
assert_eq!(
to_hex(hash.as_ref()),
"d8f6b29b641113c91175b8d44f265ff1167d58d5aa5ee03e6f1f521505b09d80"
);
}

#[test]
#[allow(non_snake_case)]
fn sign_and_verify_message() {
let mut rng = rand::thread_rng();
let (k, P) = RistrettoPublicKey::random_keypair(&mut rng);
let sig = RistrettoSchnorr::sign_message(&k, "Queues are things that happen to other people").unwrap();
assert!(sig.verify_message(&P, "Queues are things that happen to other people"));
assert!(!sig.verify_message(&P, "Qs are things that happen to other people"));
assert!(!sig.verify_message(&(&P + &P), "Queues are things that happen to other people"));
}
}
6 changes: 5 additions & 1 deletion src/ristretto/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@ pub struct SignatureSet {
/// # Panics
///
/// The function panics if it cannot generate a suitable signature
#[deprecated(
since = "0.16.0",
note = "Use SchnorrSignature::sign_message instead. This method will be removed in v1.0.0"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though this file notes that its functions are useful for testing, still recommend that the deprecation message (and the docstring) specifically indicate that this method is unsafe, and that signatures generated from it should not be accepted by a verifier without good reason. Even though it binds the public nonce to the challenge, it does not bind the public key. An attacker can forge signatures with arbitrary messages against public keys for which it does not possess the secret keys (though it cannot choose these keys in advance).

)]
pub fn sign<D: Digest>(
private_key: &RistrettoSecretKey,
message: &[u8],
Expand All @@ -41,7 +45,7 @@ pub fn sign<D: Digest>(
.finalize()
.to_vec();
let e = RistrettoSecretKey::from_bytes(&message).map_err(|_| SchnorrSignatureError::InvalidChallenge)?;
let s = RistrettoSchnorr::sign(private_key.clone(), nonce.clone(), e.as_bytes())?;
let s = RistrettoSchnorr::sign_raw(private_key, nonce.clone(), e.as_bytes())?;
Ok(SignatureSet {
nonce,
public_nonce,
Expand Down
Loading