[crypto] systematize verify_batch tests and bug fixes (MystenLabs/nar…

…whal#761)
Mrolcraft · Aug 16, 2022 · e653384 · e653384
1 parent c7451bc
commit e653384
Show file tree

Hide file tree

Showing 10 changed files with 225 additions and 209 deletions.
diff --git a/narwhal/crypto/benches/crypto.rs b/narwhal/crypto/benches/crypto.rs
@@ -113,21 +113,21 @@ mod ed25519_benches {
                 BenchmarkId::new("Ed25519 batch verification", *size),
                 &(msg.clone(), ed_public_keys, ed_signatures),
                 |b, i| {
-                    b.iter(|| VerifyingKey::verify_batch(&i.0, &i.1[..], &i.2[..]));
+                    b.iter(|| VerifyingKey::verify_batch_empty_fail(&i.0, &i.1[..], &i.2[..]));
                 },
             );
             c.bench_with_input(
                 BenchmarkId::new("BLS12377 batch verification", *size),
                 &(msg.clone(), bls_public_keys, bls_signatures),
                 |b, i| {
-                    b.iter(|| VerifyingKey::verify_batch(&i.0, &i.1[..], &i.2[..]));
+                    b.iter(|| VerifyingKey::verify_batch_empty_fail(&i.0, &i.1[..], &i.2[..]));
                 },
             );
             c.bench_with_input(
                 BenchmarkId::new("BLS12381 batch verification", *size),
                 &(msg, blst_public_keys, blst_signatures),
                 |b, i| {
-                    b.iter(|| VerifyingKey::verify_batch(&i.0, &i.1[..], &i.2[..]));
+                    b.iter(|| VerifyingKey::verify_batch_empty_fail(&i.0, &i.1[..], &i.2[..]));
                 },
             );
         }

diff --git a/narwhal/crypto/src/bls12377/mod.rs b/narwhal/crypto/src/bls12377/mod.rs
@@ -20,6 +20,7 @@ use ark_ff::{
 };
 use base64ct::{Base64, Encoding};
 use celo_bls::{hash_to_curve::try_and_increment, PublicKey};
+use eyre::eyre;
 use once_cell::sync::OnceCell;
 use serde::{de, Deserialize, Serialize};
 use serde_with::serde_as;
@@ -265,9 +266,18 @@ impl VerifyingKey for BLS12377PublicKey {
     type Sig = BLS12377Signature;
     const LENGTH: usize = CELO_BLS_PUBLIC_KEY_LENGTH;
 
-    fn verify_batch(msg: &[u8], pks: &[Self], sigs: &[Self::Sig]) -> Result<(), signature::Error> {
-        if pks.len() != sigs.len() {
-            return Err(signature::Error::new());
+    fn verify_batch_empty_fail(
+        msg: &[u8],
+        pks: &[Self],
+        sigs: &[Self::Sig],
+    ) -> Result<(), eyre::Report> {
+        if sigs.is_empty() {
+            return Err(eyre!("Critical Error! This behavious can signal something dangerous, and that someone may be trying to bypass signature verification through providing empty batches."));
+        }
+        if sigs.len() != pks.len() {
+            return Err(eyre!(
+                "Mismatch between number of signatures and public keys provided"
+            ));
         }
         let mut batch = celo_bls::bls::Batch::new(msg, &[]);
         pks.iter()
@@ -276,7 +286,7 @@ impl VerifyingKey for BLS12377PublicKey {
         let hash_to_g1 = &*celo_bls::hash_to_curve::try_and_increment::COMPOSITE_HASH_TO_G1;
         batch
             .verify(hash_to_g1)
-            .map_err(|_| signature::Error::new())
+            .map_err(|_| eyre!("Signature verification failed"))
     }
 }
 

diff --git a/narwhal/crypto/src/bls12381.rs b/narwhal/crypto/src/bls12381.rs
@@ -19,6 +19,7 @@ use crate::{
     pubkey_bytes::PublicKeyBytes,
     serde_helpers::{keypair_decode_base64, BlsSignature},
 };
+use eyre::eyre;
 use serde::{
     de::{self},
     Deserialize, Serialize,
@@ -190,10 +191,19 @@ impl VerifyingKey for BLS12381PublicKey {
 
     const LENGTH: usize = BLS_PUBLIC_KEY_LENGTH;
 
-    fn verify_batch(msg: &[u8], pks: &[Self], sigs: &[Self::Sig]) -> Result<(), signature::Error> {
+    fn verify_batch_empty_fail(
+        msg: &[u8],
+        pks: &[Self],
+        sigs: &[Self::Sig],
+    ) -> Result<(), eyre::Report> {
         let num_sigs = sigs.len();
-        if pks.len() != num_sigs {
-            return Err(signature::Error::new());
+        if sigs.is_empty() {
+            return Err(eyre!("Critical Error! This behavious can signal something dangerous, and that someone may be trying to bypass signature verification through providing empty batches."));
+        }
+        if sigs.len() != pks.len() {
+            return Err(eyre!(
+                "Mismatch between number of signatures and public keys provided"
+            ));
         }
         let mut rands: Vec<blst_scalar> = Vec::with_capacity(num_sigs);
         let mut rng = OsRng;
@@ -228,7 +238,7 @@ impl VerifyingKey for BLS12381PublicKey {
         if result == BLST_ERROR::BLST_SUCCESS {
             Ok(())
         } else {
-            Err(signature::Error::new())
+            Err(eyre!("Verification failed!"))
         }
     }
 }

diff --git a/narwhal/crypto/src/ed25519.rs b/narwhal/crypto/src/ed25519.rs
@@ -2,6 +2,7 @@
 // SPDX-License-Identifier: Apache-2.0
 use base64ct::{Base64, Encoding};
 use ed25519_consensus::{batch, VerificationKeyBytes};
+use eyre::eyre;
 use once_cell::sync::OnceCell;
 use serde::{
     de::{self, MapAccess, SeqAccess, Visitor},
@@ -76,7 +77,20 @@ impl VerifyingKey for Ed25519PublicKey {
     type Sig = Ed25519Signature;
     const LENGTH: usize = ED25519_PUBLIC_KEY_LENGTH;
 
-    fn verify_batch(msg: &[u8], pks: &[Self], sigs: &[Self::Sig]) -> Result<(), signature::Error> {
+    fn verify_batch_empty_fail(
+        msg: &[u8],
+        pks: &[Self],
+        sigs: &[Self::Sig],
+    ) -> Result<(), eyre::Report> {
+        if sigs.is_empty() {
+            return Err(eyre!("Critical Error! This behavious can signal something dangerous, and that someone may be trying to bypass signature verification through providing empty batches."));
+        }
+        if sigs.len() != pks.len() {
+            return Err(eyre!(
+                "Mismatch between number of signatures and public keys provided"
+            ));
+        }
+
         let mut batch = batch::Verifier::new();
 
         for i in 0..sigs.len() {
@@ -85,7 +99,7 @@ impl VerifyingKey for Ed25519PublicKey {
         }
         batch
             .verify(&mut OsRng)
-            .map_err(|_| signature::Error::new())
+            .map_err(|_| eyre!("Signature verification failed"))
     }
 }
 

diff --git a/narwhal/crypto/src/tests/bls12377_tests.rs b/narwhal/crypto/src/tests/bls12377_tests.rs
@@ -80,8 +80,7 @@ fn verify_invalid_signature() {
     assert!(kp.public().verify(&digest.0, &signature).is_err());
 }
 
-#[test]
-fn verify_valid_batch() {
+fn signature_test_inputs() -> (Vec<u8>, Vec<BLS12377PublicKey>, Vec<BLS12377Signature>) {
     // Make signatures.
     let message: &[u8] = b"Hello, world!";
     let digest = message.digest();
@@ -94,95 +93,78 @@ fn verify_valid_batch() {
         })
         .unzip();
 
-    // Verify the batch.
-    let res = BLS12377PublicKey::verify_batch(&digest.0, &pubkeys, &signatures);
+    (digest.to_vec(), pubkeys, signatures)
+}
+
+#[test]
+fn verify_valid_batch() {
+    let (digest, pubkeys, signatures) = signature_test_inputs();
+
+    let res = BLS12377PublicKey::verify_batch_empty_fail(&digest[..], &pubkeys, &signatures);
     assert!(res.is_ok(), "{:?}", res);
 }
 
 #[test]
 fn verify_invalid_batch() {
-    // Make signatures.
-    let message: &[u8] = b"Hello, world!";
-    let digest = message.digest();
-    let (pubkeys, mut signatures): (Vec<BLS12377PublicKey>, Vec<BLS12377Signature>) = keys()
-        .into_iter()
-        .take(3)
-        .map(|kp| {
-            let sig = kp.sign(&digest.0);
-            (kp.public().clone(), sig)
-        })
-        .unzip();
-
+    let (digest, pubkeys, mut signatures) = signature_test_inputs();
     // mangle one signature
     signatures[0] = BLS12377Signature::default();
 
-    // Verify the batch.
-    let res = BLS12377PublicKey::verify_batch(&digest.0, &pubkeys, &signatures);
+    let res = BLS12377PublicKey::verify_batch_empty_fail(&digest[..], &pubkeys, &signatures);
     assert!(res.is_err(), "{:?}", res);
 }
 
 #[test]
-fn verify_valid_aggregate_signature() {
-    // Make signatures.
-    let message: &[u8] = b"Hello, world!";
-    let digest = message.digest();
-    let (pubkeys, signatures): (Vec<BLS12377PublicKey>, Vec<BLS12377Signature>) = keys()
-        .into_iter()
-        .take(3)
-        .map(|kp| {
-            let sig = kp.sign(&digest.0);
-            (kp.public().clone(), sig)
-        })
-        .unzip();
+fn verify_empty_batch() {
+    let (digest, _, _) = signature_test_inputs();
+
+    let res = BLS12377PublicKey::verify_batch_empty_fail(&digest[..], &[], &[]);
+    assert!(res.is_err(), "{:?}", res);
+}
 
+#[test]
+fn verify_batch_missing_public_keys() {
+    let (digest, pubkeys, signatures) = signature_test_inputs();
+
+    // missing leading public keys
+    let res = BLS12377PublicKey::verify_batch_empty_fail(&digest, &pubkeys[1..], &signatures);
+    assert!(res.is_err(), "{:?}", res);
+
+    // missing trailing public keys
+    let res = BLS12377PublicKey::verify_batch_empty_fail(
+        &digest,
+        &pubkeys[..pubkeys.len() - 1],
+        &signatures,
+    );
+    assert!(res.is_err(), "{:?}", res);
+}
+
+#[test]
+fn verify_valid_aggregate_signaature() {
+    let (digest, pubkeys, signatures) = signature_test_inputs();
     let aggregated_signature = BLS12377AggregateSignature::aggregate(signatures).unwrap();
 
-    // // Verify the batch.
-    let res = aggregated_signature.verify(&pubkeys[..], &digest.0);
+    let res = aggregated_signature.verify(&pubkeys[..], &digest);
     assert!(res.is_ok(), "{:?}", res);
 }
 
 #[test]
 fn verify_invalid_aggregate_signature_length_mismatch() {
-    // Make signatures.
-    let message: &[u8] = b"Hello, world!";
-    let digest = message.digest();
-    let (pubkeys, signatures): (Vec<BLS12377PublicKey>, Vec<BLS12377Signature>) = keys()
-        .into_iter()
-        .take(3)
-        .map(|kp| {
-            let sig = kp.sign(&digest.0);
-            (kp.public().clone(), sig)
-        })
-        .unzip();
-
+    let (digest, pubkeys, signatures) = signature_test_inputs();
     let aggregated_signature = BLS12377AggregateSignature::aggregate(signatures).unwrap();
 
-    // // Verify the batch.
-    let res = aggregated_signature.verify(&pubkeys[..2], &digest.0);
+    let res = aggregated_signature.verify(&pubkeys[..2], &digest);
     assert!(res.is_err(), "{:?}", res);
 }
 
 #[test]
 fn verify_invalid_aggregate_signature_public_key_switch() {
-    // Make signatures.
-    let message: &[u8] = b"Hello, world!";
-    let digest = message.digest();
-    let (mut pubkeys, signatures): (Vec<BLS12377PublicKey>, Vec<BLS12377Signature>) = keys()
-        .into_iter()
-        .take(3)
-        .map(|kp| {
-            let sig = kp.sign(&digest.0);
-            (kp.public().clone(), sig)
-        })
-        .unzip();
-
+    let (digest, mut pubkeys, signatures) = signature_test_inputs();
     let aggregated_signature = BLS12377AggregateSignature::aggregate(signatures).unwrap();
 
     pubkeys[0] = keys()[3].public().clone();
 
-    // // Verify the batch.
-    let res = aggregated_signature.verify(&pubkeys[..], &digest.0);
+    let res = aggregated_signature.verify(&pubkeys[..], &digest);
     assert!(res.is_err(), "{:?}", res);
 }