Merge #13: Revert s overflow handling

df69673 halfagg: Note that invalid sigs can have valid aggsig and add test (Tim Ruffing)
6460917 Revert "halfagg: fail inc_agg if input scalars overflow" (Tim Ruffing)

Pull request description:

ACKs for top commit:
  jonasnick:
    ACK df69673

Tree-SHA512: 84e70bf39533bd48f917f7e692f1422be54c86aae2d854cdd661e473d1a6a2f194e2b43779981c86196633985b98b061ce20c4cb30ba8280e3f3669c71638664

Author: jonasnick

hacspec-halfagg/src/halfagg.rs

@@ -54,8 +54,7 @@ pub fn inc_aggregate(
         let (pk, msg) = pm_aggd[i];
         pmr[i] = (pk, msg, Bytes32::from_slice(aggsig, 32 * i, 32));
     }
-    let mut s = scalar_from_bytes_strict(Bytes32::from_seq(&aggsig.slice(32 * v, 32)))
-        .ok_or(Error::MalformedSignature)?;
+    let mut s = Scalar::from_byte_seq_be(&aggsig.slice(32 * v, 32));
 
     for i in v..v + u {
         let (pk, msg, sig) = pms_to_agg[i - v];
@@ -66,9 +65,7 @@ pub fn inc_aggregate(
         let z = scalar_from_bytes(hash_halfagg(
             &Seq::<(PublicKey, Message, Bytes32)>::from_slice(&pmr, 0, i + 1),
         ));
-        let si = scalar_from_bytes_strict(Bytes32::from_slice(&sig, 32, 32))
-            .ok_or(Error::MalformedSignature)?;
-        s = s + z * si;
+        s = s + z * Scalar::from_byte_seq_be(&Bytes32::from_slice(&sig, 32, 32));
     }
     let mut ret = Seq::<U8>::new(0);
     for i in 0..pmr.len() {

hacspec-halfagg/tests/tests.rs

@@ -1,4 +1,5 @@
 use hacspec_bip_340::*;
+use hacspec_dev::rand::*;
 use hacspec_halfagg::*;
 use hacspec_lib::*;
 
@@ -129,6 +130,60 @@ fn test_aggregate_verify() {
     }
 }
 
+/// Constructs two invalid signatures whose aggregate signature is valid
+#[test]
+fn test_aggregate_verify_strange() {
+    let mut pms_triples = Seq::<(PublicKey, Message, Signature)>::new(0);
+    for i in 0..2 {
+        let sk = [i as u8 + 1; 32];
+        let sk = SecretKey::from_public_array(sk);
+        let msg = [i as u8 + 2; 32];
+        let msg = Message::from_public_array(msg);
+        let aux_rand = [i as u8 + 3; 32];
+        let aux_rand = AuxRand::from_public_array(aux_rand);
+        let sig = sign(msg, sk, aux_rand).unwrap();
+        pms_triples = pms_triples.push(&(pubkey_gen(sk).unwrap(), msg, sig));
+    }
+    let aggsig = aggregate(&pms_triples).unwrap();
+    let pm_tuples = strip_sigs(&pms_triples);
+    assert!(verify_aggregate(&aggsig, &pm_tuples).is_ok());
+
+    // Compute z values like in IncAggegrate
+    let mut pmr = Seq::<(PublicKey, Message, Bytes32)>::new(0);
+    let mut z = Seq::new(0);
+    for i in 0..2 {
+        let (pk, msg, sig) = pms_triples[i];
+        pmr = pmr.push(&(pk, msg, Bytes32::from_slice(&sig, 0, 32)));
+        // TODO: The following line hashes i elements and therefore leads to
+        // quadratic runtime. Instead, we should cache the intermediate result
+        // and only hash the new element.
+        z = z.push(&scalar_from_bytes(hash_halfagg(
+            &Seq::<(PublicKey, Message, Bytes32)>::from_slice(&pmr, 0, i + 1),
+        )));
+    }
+
+    // Shift signatures appropriately
+    let sagg = scalar_from_bytes(Bytes32::from_seq(&aggsig.slice(32 * 2, 32)));
+    let s1: [u8; 32] = random_bytes();
+    let s1 = scalar_from_bytes(Bytes32::from_public_array(s1));
+    // Division is ordinary integer division, so use inv() explicitly
+    let s0 = (sagg - z[1] * s1) * (z[0] as Scalar).inv();
+
+    let (pk0, msg0, sig0): (PublicKey, Message, Signature) = pms_triples[0];
+    let (pk1, msg1, sig1): (PublicKey, Message, Signature) = pms_triples[1];
+    let sig0_invalid = sig0.update(32, &bytes_from_scalar(s0));
+    let sig1_invalid = sig1.update(32, &bytes_from_scalar(s1));
+    assert!(!verify(msg0, pk0, sig0_invalid).is_ok());
+    assert!(!verify(msg1, pk1, sig1_invalid).is_ok());
+
+    let mut pms_strange = Seq::<(PublicKey, Message, Signature)>::new(0);
+    pms_strange = pms_strange.push(&(pk0, msg0, sig0_invalid));
+    pms_strange = pms_strange.push(&(pk1, msg1, sig1_invalid));
+    let aggsig_strange = aggregate(&pms_strange).unwrap();
+    let pm_strange = strip_sigs(&pms_strange);
+    assert!(verify_aggregate(&aggsig_strange, &pm_strange).is_ok());
+}
+
 #[test]
 fn test_edge_cases() {
     let empty_pm = Seq::<(PublicKey, Message)>::new(0);

half-aggregation.mediawiki

@@ -115,7 +115,8 @@ The following conventions are used, with constants as defined for [https://www.s
 ==== Aggregate ====
 
 ''Aggregate'' takes an array of public key, message and signature triples and returns an aggregate signature.
-If for every triple ''(p, m, s)'' we have that ''Verify(p, m, s)'' (as defined in BIP 340) returns true, then the returned aggregate signature and the array of ''(p, m)'' tuples passes ''VerifyAggregate''.
+If every triple ''(p, m, s)'' is valid (i.e., ''Verify(p, m, s)'' as defined in BIP 340 returns true), then the returned aggregate signature and the array of ''(p, m)'' tuples passes ''VerifyAggregate''.
+(However, the inverse does not hold: given suitable valid triples, it is possible to construct an input array to ''Aggregate'' which contains invalid triples, but for which ''VerifyAggregate'' will accept the aggregate signature returned by ''Aggregate''. If this is undesired, input triples should be verified individually before passing them to ''Aggregate''.)
 
 Input:
 * ''pms<sub>0..u-1</sub>'': an array of ''u'' triples, where the first element of each triple is a 32-byte public key, the second element is a 32-byte message and the third element is a 64-byte BIP 340 signature
@@ -128,7 +129,8 @@ Input:
 
 ''IncAggregate'' takes an aggregate signature, an array of public key and message tuples corresponding to the aggregate signature, and an additional array of public key, message and signature triples.
 It aggregates the additional array of triples into the existing aggregate signature and returns the resulting new aggregate signature.
-In other words, if ''VerifyAggregate(aggsig, pm_aggd)'' passes and for every triple ''(p, m, s)'' in ''pms_to_agg'' we have that ''Verify(p, m, s)'' (as defined in BIP 340) returns true, then the returned aggregate signature along with the array of ''(p, m)'' tuples of ''pm_aggd'' and ''pms_to_agg'' passes ''VerifyAggregate''.
+In other words, if ''VerifyAggregate(aggsig, pm_aggd)'' passes and every triple ''(p, m, s)'' in ''pms_to_agg'' is valid (i.e., ''Verify(p, m, s)'' as defined in BIP 340 returns true), then the returned aggregate signature along with the array of ''(p, m)'' tuples of ''pm_aggd'' and ''pms_to_agg'' passes ''VerifyAggregate''.
+(However, the inverse does not hold: given a suitable valid aggregate signature and suitable valid triples, it is possible to construct inputs to ''IncAggregate'' which contain an invalid aggregate signature or invalid triples, but for which ''VerifyAggregate'' will accept the aggregate signature returned by ''IncAggregate''. If this is undesired, the input triples and the input aggregate signature should be verified individually before passing them to ''IncAggregate''.)
 
 Input:
 * ''aggsig'' : a byte array
@@ -144,10 +146,9 @@ Input:
 * For ''i = v .. v+u-1'':
 ** Let ''(pk<sub>i</sub>, m<sub>i</sub>, sig<sub>i</sub>) = pms_to_agg<sub>i-v</sub>''
 ** Let ''r<sub>i</sub> = sig<sub>i</sub>[0:32]''
-** Let ''s<sub>i</sub> = int(sig<sub>i</sub>[32:64])''; fail if ''s<sub>i</sub> &ge; n''
+** Let ''s<sub>i</sub> = int(sig<sub>i</sub>[32:64])''
 ** Let ''z<sub>i</sub> = int(hash<sub>HalfAgg/randomizer</sub>(r<sub>0</sub> || pk<sub>0</sub> || m<sub>0</sub> || ... || r<sub>i</sub> || pk<sub>i</sub> || m<sub>i</sub>)) mod n''
-* Let ''q = int(aggsig[(v⋅32:(v+1)⋅32]))''; fail if ''q &ge; n''
-* Let ''s = q + z<sub>v</sub>⋅s<sub>v</sub> + ... + z<sub>v+u-1</sub>⋅s<sub>v+u-1</sub> mod n''
+* Let ''s = int(aggsig[(v⋅32:(v+1)⋅32]) + z<sub>v</sub>⋅s<sub>v</sub> + ... + z<sub>v+u-1</sub>⋅s<sub>v+u-1</sub> mod n''
 * Return ''r<sub>0</sub> || ... || r<sub>v+u-1</sub> || bytes(s)''
 
 ==== VerifyAggregate ====