Improve batch verification API (#10)

* Improve batch verification API

Incorporates feedback from @FiloSottile.

- [x] fix the order of arguments in `BatchVerifier.Add` to match `Verify`;
- [x] made `Add` infallible, deferring failures to the batch verification;
- [x] rename `BatchVerifier.VerifyBatch` to `BatchVerifier.Verify`;
- [x] make `Verify` fail on an empty batch;
- [x] make `Verify` idempotent.

Closes #9

* Avoid panics on wrong-length signatures

* Remove markdown from doc strings

batch.go

@@ -8,61 +8,74 @@ import (
 	"filippo.io/edwards25519"
 )
 
-// BatchVerifier holds entries of public keys, signature and a scalar which are used for batch verification.
+// BatchVerifier accumulates batch entries with Add, before performing batch verification with Verify.
 type BatchVerifier struct {
-	entries []ks
+	entries []entry
 }
 
-// ks represents the public key, signature and scalar which the caller wants to batch verify
-type ks struct {
+// entry represents a batch entry with the public key, signature and scalar which the caller wants to verify
+type entry struct {
 	pubkey    ed25519.PublicKey
 	signature []byte
 	k         *edwards25519.Scalar
 }
 
-// NewBatchVerifier creates a Verifier that entries of signatures, keys and messages
-// can be added to for verification
+// NewBatchVerifier creates an empty BatchVerifier.
 func NewBatchVerifier() BatchVerifier {
 	return BatchVerifier{
-		entries: []ks{},
+		entries: []entry{},
 	}
 }
 
-// Add adds a (public key, signature, message) triple to the current batch.
-func (v *BatchVerifier) Add(publicKey ed25519.PublicKey, sig, message []byte) bool {
-	if l := len(publicKey); l != ed25519.PublicKeySize {
-		return false
-	}
+// Add adds a (public key, message, sig) triple to the current batch.
+func (v *BatchVerifier) Add(publicKey ed25519.PublicKey, message, sig []byte) {
+	// Compute the challenge scalar for this entry upfront, so that we don't
+	// introduce a dependency on the lifetime of the message array. This doesn't
+	// matter so much for Go, which has garbage collection, but did matter for
+	// the Rust implementation this was ported from, but not keeping buffers
+	// alive for longer than they have to is nice to do anyways.
 
-	if len(sig) != ed25519.SignatureSize || sig[63]&224 != 0 {
-		return false
+	h := sha512.New()
+
+	// R_bytes is the first 32 bytes of the signature, but because the signature
+	// is passed as a variable-length array it could be too short. In that case
+	// we'll fail in Verify, so just avoid a panic here.
+	n := 32
+	if len(sig) < n {
+		n = len(sig)
 	}
+	h.Write(sig[:n])
 
-	h := sha512.New()
-	h.Write(sig[:32])
 	h.Write(publicKey)
 	h.Write(message)
 	var digest [64]byte
 	h.Sum(digest[:0])
 
 	k := new(edwards25519.Scalar).SetUniformBytes(digest[:])
 
-	ksS := ks{
+	e := entry{
 		pubkey:    publicKey,
 		signature: sig,
 		k:         k,
 	}
 
-	v.entries = append(v.entries, ksS)
-
-	return true
+	v.entries = append(v.entries, e)
 }
 
-// Verify checks all entries in the current batch, returning `true` if
-// *all* entries are valid and `false` if *any one* entry is invalid.
+// Verify checks all entries in the current batch, returning true if all entries
+// are valid and false if any one entry is invalid.
 //
-// If a failure arises it is unknown which entry failed, the caller must verify each entry individually.
-func (v *BatchVerifier) VerifyBatch() bool {
+// If a failure arises it is unknown which entry failed, the caller must verify
+// each entry individually.
+//
+// Calling Verify on an empty batch returns false.
+func (v *BatchVerifier) Verify() bool {
+	vl := len(v.entries)
+	// Abort early on an empty batch, which probably indicates a bug
+	if vl == 0 {
+		return false
+	}
+
 	// The batch verification equation is
 	//
 	// [-sum(z_i * s_i)]B + sum([z_i]R_i) + sum([z_i * k_i]A_i) = 0.
@@ -72,7 +85,6 @@ func (v *BatchVerifier) VerifyBatch() bool {
 	// - s_i is the signature's s value;
 	// - k_i is the hash of the message and other data;
 	// - z_i is a random 128-bit Scalar.
-	vl := len(v.entries)
 	svals := make([]edwards25519.Scalar, 1+vl+vl)
 	scalars := make([]*edwards25519.Scalar, 1+vl+vl)
 
@@ -96,6 +108,12 @@ func (v *BatchVerifier) VerifyBatch() bool {
 
 	B.Set(edwards25519.NewGeneratorPoint())
 	for i, entry := range v.entries {
+		// Check that the signature is exactly 64 bytes upfront,
+		// so that we can slice it later without potential panics
+		if len(entry.signature) != 64 {
+			return false
+		}
+
 		if _, err := Rs[i].SetBytes(entry.signature[:32]); err != nil {
 			return false
 		}
@@ -121,9 +139,6 @@ func (v *BatchVerifier) VerifyBatch() bool {
 	}
 	Bcoeff.Negate(Bcoeff) // this term is subtracted in the summation
 
-	// purge BatchVerifier for reuse
-	v.entries = []ks{}
-
 	check := new(edwards25519.Point).VarTimeMultiScalarMult(scalars, points)
 	check.MultByCofactor(check)
 	return check.Equal(edwards25519.NewIdentityPoint()) == 1

batch_test.go

@@ -12,31 +12,53 @@ func TestBatch(t *testing.T) {
 	v := NewBatchVerifier()
 	populateBatchVerifier(t, &v)
 
-	if !v.VerifyBatch() {
+	if !v.Verify() {
 		t.Error("failed batch verification")
 	}
+}
+
+func TestBatchFailsOnShortSig(t *testing.T) {
+	v := NewBatchVerifier()
+	pub, _, _ := ed25519.GenerateKey(nil)
+	v.Add(pub, []byte("message"), []byte{})
+	if v.Verify() {
+		t.Error("batch verification should fail due to short signature")
+	}
+}
 
-	// corrput a key to check batch verification fails
+func TestBatchFailsOnCorruptKey(t *testing.T) {
+	v := NewBatchVerifier()
 	populateBatchVerifier(t, &v)
 	v.entries[1].pubkey[1] ^= 1
-	if v.VerifyBatch() {
+	if v.Verify() {
 		t.Error("batch verification should fail due to corrupt key")
 	}
+}
+
+func TestBatchFailsOnCorruptSignature(t *testing.T) {
+	v := NewBatchVerifier()
 
-	// corrput a signature to check batch verification fails
 	populateBatchVerifier(t, &v)
+	// corrupt the R value of one of the signatures
 	v.entries[4].signature[1] ^= 1
-	if v.VerifyBatch() {
-		t.Error("batch verification should fail due to corrupt key")
+	if v.Verify() {
+		t.Error("batch verification should fail due to corrupt signature")
 	}
 
 	populateBatchVerifier(t, &v)
 	// negate a scalar to check batch verification fails
 	v.entries[1].k.Negate(edwards25519.NewScalar())
-	if v.VerifyBatch() {
-		t.Error("batch verification should fail due to corrupt key")
+	if v.Verify() {
+		t.Error("batch verification should fail due to corrupt signature")
 	}
+}
 
+func TestEmptyBatchFails(t *testing.T) {
+	v := NewBatchVerifier()
+
+	if v.Verify() {
+		t.Error("batch verification should fail on an empty batch")
+	}
 }
 
 func BenchmarkVerifyBatch(b *testing.B) {
@@ -51,7 +73,7 @@ func BenchmarkVerifyBatch(b *testing.B) {
 			}
 			// NOTE: dividing by n so that metrics are per-signature
 			for i := 0; i < b.N/n; i++ {
-				if !v.VerifyBatch() {
+				if !v.Verify() {
 					b.Fatal("signature set failed batch verification")
 				}
 			}
@@ -61,6 +83,7 @@ func BenchmarkVerifyBatch(b *testing.B) {
 
 // populateBatchVerifier populates a verifier with multiple entries
 func populateBatchVerifier(t *testing.T, v *BatchVerifier) {
+	*v = NewBatchVerifier()
 	for i := 0; i <= 38; i++ {
 
 		pub, priv, _ := ed25519.GenerateKey(nil)
@@ -74,8 +97,6 @@ func populateBatchVerifier(t *testing.T, v *BatchVerifier) {
 
 		sig := ed25519.Sign(priv, msg)
 
-		if !v.Add(pub, sig, msg) {
-			t.Error("unable to add s k m")
-		}
+		v.Add(pub, msg, sig)
 	}
 }