Merge pull request #10 from onflow/tarak/ecdsa-verify-sig-length

Crypto: check ECDSA signature length in verification

crypto/bls.go

@@ -105,6 +105,10 @@ func NewBLSKMAC(tag string) hash.Hasher {
 // If the hasher used is KMAC128, the hasher is only read.
 // The public key is only read by the function.
 func (pk *PubKeyBLSBLS12381) Verify(s Signature, data []byte, kmac hash.Hasher) (bool, error) {
+	if len(s) != signatureLengthBLSBLS12381 {
+		return false, nil
+	}
+
 	if kmac == nil {
 		return false, errors.New("VerifyBytes requires a Hasher")
 	}
@@ -113,6 +117,7 @@ func (pk *PubKeyBLSBLS12381) Verify(s Signature, data []byte, kmac hash.Hasher)
 		return false, fmt.Errorf("Hasher with at least %d output byte size is required, current size is %d",
 			opSwUInputLenBLSBLS12381, kmac.Size())
 	}
+
 	// hash the input to 128 bytes
 	h := kmac.ComputeHash(data)
 
@@ -328,9 +333,7 @@ func (a *blsBLS12381Algo) blsSign(sk *scalar, data []byte) Signature {
 
 // Checks the validity of a bls signature through the C layer
 func (a *blsBLS12381Algo) blsVerify(pk *pointG2, s Signature, data []byte) bool {
-	if len(s) != signatureLengthBLSBLS12381 {
-		return false
-	}
+
 	verif := C.bls_verify((*C.ep2_st)(pk),
 		(*C.uchar)(&s[0]),
 		(*C.uchar)(&data[0]),

crypto/bls_test.go

@@ -48,15 +48,16 @@ func TestBLSBLS12381Hasher(t *testing.T) {
 	// generate a key pair
 	seed := make([]byte, KeyGenSeedMinLenBLSBLS12381)
 	sk := randomSK(t, seed)
+	sig := make([]byte, SignatureLenBLSBLS12381)
 	// empty hasher
 	_, err := sk.Sign(seed, nil)
 	assert.Error(t, err)
-	_, err = sk.PublicKey().Verify(Signature{}, seed, nil)
+	_, err = sk.PublicKey().Verify(sig, seed, nil)
 	assert.Error(t, err)
 	// short size hasher
 	_, err = sk.Sign(seed, hash.NewSHA2_256())
 	assert.Error(t, err)
-	_, err = sk.PublicKey().Verify(Signature{}, seed, hash.NewSHA2_256())
+	_, err = sk.PublicKey().Verify(sig, seed, hash.NewSHA2_256())
 	assert.Error(t, err)
 }
 

crypto/ecdsa.go

@@ -99,9 +99,14 @@ func (sk *PrKeyECDSA) Sign(data []byte, alg hash.Hasher) (Signature, error) {
 
 // verifyHash implements ECDSA signature verification
 func (pk *PubKeyECDSA) verifyHash(sig Signature, h hash.Hash) (bool, error) {
+	Nlen := bitsToBytes((pk.alg.curve.Params().N).BitLen())
+
+	if len(sig) != 2*Nlen {
+		return false, nil
+	}
+
 	var r big.Int
 	var s big.Int
-	Nlen := bitsToBytes((pk.alg.curve.Params().N).BitLen())
 	r.SetBytes(sig[:Nlen])
 	s.SetBytes(sig[Nlen:])
 	return goecdsa.Verify(pk.goPubKey, h, &r, &s), nil

crypto/sign_test_utils.go

@@ -3,7 +3,9 @@ package crypto
 import (
 	"crypto/rand"
 	"fmt"
+	mrand "math/rand"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -18,6 +20,7 @@ func testGenSignVerify(t *testing.T, salg SigningAlgorithm, halg hash.Hasher) {
 	seedMinLength := 48
 	seed := make([]byte, seedMinLength)
 	input := make([]byte, 100)
+	mrand.Seed(time.Now().UnixNano())
 
 	loops := 50
 	for j := 0; j < loops; j++ {
@@ -51,6 +54,16 @@ func testGenSignVerify(t *testing.T, salg SigningAlgorithm, halg hash.Hasher) {
 		require.NoError(t, err)
 		assert.False(t, result, fmt.Sprintf(
 			"Verification should fail:\n signature:%s\n message:%x\n private key:%s", s, input, sk))
+		// test a wrong signature length
+		invalidLen := mrand.Intn(2 * len(s)) // try random invalid lengths
+		if invalidLen == len(s) {            // map to an invalid length
+			invalidLen = 0
+		}
+		invalidSig := make([]byte, invalidLen)
+		result, err = pk.Verify(invalidSig, input, halg)
+		require.NoError(t, err)
+		assert.False(t, result, fmt.Sprintf(
+			"Verification should fail:\n signature:%s\n with invalid length %d", invalidSig, invalidLen))
 	}
 }