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.

Sign up for GitHub

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

Crypto: check ECDSA signature length in verification #10

Merged
merged 5 commits into from
Sep 25, 2020
Merged

Crypto: check ECDSA signature length in verification #10

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
abdfa0b
add check for signature length
Sep 24, 2020
5bd67de
add tests for invalid signature length
Sep 24, 2020
31aba75
include larger signature in the invalid length test
Sep 24, 2020
7c27630
do not return an error when a a signature length is invalid
Sep 25, 2020
3adaf3c
Merge branch 'master' into tarak/ecdsa-verify-sig-length
Kay-Zee Sep 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions crypto/bls.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,11 @@ 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, fmt.Errorf("signature length is %d, must be %d",
len(s), signatureLengthBLSBLS12381)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving this check here is picked from another WIP branch:
https://github.com/onflow/flow-go/blob/tarak/4482-multi-sig-verification-distinct-msg/crypto/bls.go#L116

Copy link
Member

Choose a reason for hiding this comment

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

This puts us in an odd position, because a signature of length 0 is an invalid signature (i.e. caused by the sig, not by other parts of the func), but returning the error here actually breaks the assumption that invalid signatures simply return false, nil and the error here is taken as a fatal error. I've addressed this in the deployed branch of testnet, but i'm not sure if that's what we want

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about this today when I made the change in ECDSA. The change in BLS here is only meant to match the change in ECDSA.

I think there is a fine line between checking the input format on one side, and verifying the signature against a key on the other. I'm also inclined not to return an error, although I think passing a signature with a wrong length generally happens when there is an implementation bug and it would be good to notify the developer about it, instead of returning false silently.
I'm happy not to error in this case if the error can cause trouble behind.

if kmac == nil {
return false, errors.New("VerifyBytes requires a Hasher")
}
Expand All @@ -113,6 +118,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)

Expand Down Expand Up @@ -328,9 +334,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]),
Expand Down
8 changes: 7 additions & 1 deletion crypto/ecdsa.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,9 +99,15 @@ 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())
Copy link
Member

Choose a reason for hiding this comment

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

Nice fix 👍
Would be great to have a test case to reproduce and confirm it works.


if len(sig) != 2*Nlen {
return false, fmt.Errorf("signature length is %d, must be %d",
len(sig), 2*Nlen)
}

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
Expand Down
9 changes: 9 additions & 0 deletions crypto/sign_test_utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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++ {
Expand Down Expand Up @@ -51,6 +54,12 @@ 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(len(s))
result, err = pk.Verify(s[:invalidLen], input, halg)
assert.Error(t, err)
assert.False(t, result, fmt.Sprintf(
"Verification should fail:\n signature:%s\n with invalid length %d", s[:invalidLen], invalidLen))
}
}

Expand Down