From 168cffb0dd649b60b7a4b449120641dbd428e31b Mon Sep 17 00:00:00 2001 From: Nishant Das Date: Tue, 17 Nov 2020 12:12:23 +0800 Subject: [PATCH] Check Sub Group for Herumi and Fix Edge Cases (#7823) * check for herumi * clean up * fix tests * fix --- deps.bzl | 6 +++--- go.mod | 2 +- go.sum | 4 ++-- shared/bls/bls_test.go | 25 +++++++++++++++++++++++-- shared/bls/blst/public_key.go | 18 +++++------------- shared/bls/blst/secret_key.go | 3 +-- shared/bls/blst/secret_key_test.go | 3 ++- shared/bls/common/error.go | 7 +++++++ shared/bls/herumi/init.go | 3 +++ shared/bls/herumi/secret_key.go | 2 +- shared/bls/herumi/secret_key_test.go | 4 +++- shared/bls/spectest/sign_test.go | 3 ++- 12 files changed, 53 insertions(+), 27 deletions(-) diff --git a/deps.bzl b/deps.bzl index 68d9960ffc..c5b17aa15e 100644 --- a/deps.bzl +++ b/deps.bzl @@ -2463,11 +2463,11 @@ def prysm_deps(): http_archive( name = "com_github_supranational_blst", urls = [ - "https://github.com/supranational/blst/archive/03c38676b08bd0bdbb120b94464d9c692a509842.tar.gz", + "https://github.com/supranational/blst/archive/9b4b16fb42692370ba8a6ccfbca1803691225413.tar.gz", ], - strip_prefix = "blst-03c38676b08bd0bdbb120b94464d9c692a509842", + strip_prefix = "blst-9b4b16fb42692370ba8a6ccfbca1803691225413", build_file = "//third_party:blst/blst.BUILD", - sha256 = "390695dd5084228de3e9f06e6800c79a5f90cdef4129fa36aaa18b1d24730138", + sha256 = "4e03c7d673fdf9f8f2ddd3e64edc31001a86ad380a8e9266c197cff70856d054", ) go_repository( name = "com_github_syndtr_goleveldb", diff --git a/go.mod b/go.mod index 4ad1bad3df..4a4d4ef3a6 100644 --- a/go.mod +++ b/go.mod @@ -99,7 +99,7 @@ require ( github.com/sirupsen/logrus v1.6.0 github.com/status-im/keycard-go v0.0.0-20200402102358-957c09536969 // indirect github.com/stretchr/testify v1.6.1 - github.com/supranational/blst v0.2.1-0.20201025154037-03c38676b08b + github.com/supranational/blst v0.2.1-0.20201113213949-9b4b16fb4269 github.com/trailofbits/go-mutexasserts v0.0.0-20200708152505-19999e7d3cef github.com/tyler-smith/go-bip39 v1.0.2 github.com/urfave/cli/v2 v2.2.0 diff --git a/go.sum b/go.sum index cf6214b294..cc75c48cfd 100644 --- a/go.sum +++ b/go.sum @@ -1049,8 +1049,8 @@ github.com/stretchr/testify v1.5.1 h1:nOGnQDM7FYENwehXlg/kFVnos3rEvtKTjRvOWSzb6H github.com/stretchr/testify v1.5.1/go.mod h1:5W2xD1RspED5o8YsWQXVCued0rvSQ+mT+I5cxcmMvtA= github.com/stretchr/testify v1.6.1 h1:hDPOHmpOpP40lSULcqw7IrRb/u7w6RpDC9399XyoNd0= github.com/stretchr/testify v1.6.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= -github.com/supranational/blst v0.2.1-0.20201025154037-03c38676b08b h1:r+p8ymKeCTy9qi6do3EHUpdnijQgU+37kGXmM4yZKmo= -github.com/supranational/blst v0.2.1-0.20201025154037-03c38676b08b/go.mod h1:jZJtfjgudtNl4en1tzwPIV3KjUnQUvG3/j+w+fVonLw= +github.com/supranational/blst v0.2.1-0.20201113213949-9b4b16fb4269 h1:s2HYqf1hRkGl7iEdl3sEQBT96lBHXplHmRP9KFsjhsA= +github.com/supranational/blst v0.2.1-0.20201113213949-9b4b16fb4269/go.mod h1:jZJtfjgudtNl4en1tzwPIV3KjUnQUvG3/j+w+fVonLw= github.com/syndtr/goleveldb v1.0.0/go.mod h1:ZVVdQEZoIme9iO1Ch2Jdy24qqXrMMOU6lpPAyBWyWuQ= github.com/syndtr/goleveldb v1.0.1-0.20200815110645-5c35d600f0ca h1:Ld/zXl5t4+D69SiV4JoN7kkfvJdOWlPpfxrzxpLMoUk= github.com/syndtr/goleveldb v1.0.1-0.20200815110645-5c35d600f0ca/go.mod h1:u2MKkTVTVJWe5D1rCvame8WqhBd88EuIwODJZ1VHCPM= diff --git a/shared/bls/bls_test.go b/shared/bls/bls_test.go index ca6977305f..49bdf16377 100644 --- a/shared/bls/bls_test.go +++ b/shared/bls/bls_test.go @@ -23,9 +23,9 @@ func TestDisallowZeroSecretKeys(t *testing.T) { flags.EnableBlst = true reset := featureconfig.InitWithReset(flags) defer reset() - + // Blst does a zero check on the key during deserialization. _, err := SecretKeyFromBytes(common.ZeroSecretKey[:]) - require.Equal(t, common.ErrZeroKey, err) + require.Equal(t, common.ErrSecretUnmarshal, err) }) } @@ -49,3 +49,24 @@ func TestDisallowZeroPublicKeys(t *testing.T) { require.Equal(t, common.ErrInfinitePubKey, err) }) } + +func TestDisallowZeroPublicKeys_AggregatePubkeys(t *testing.T) { + flags := &featureconfig.Flags{} + + t.Run("herumi", func(t *testing.T) { + reset := featureconfig.InitWithReset(flags) + defer reset() + + _, err := AggregatePublicKeys([][]byte{common.InfinitePublicKey[:], common.InfinitePublicKey[:]}) + require.Equal(t, common.ErrInfinitePubKey, err) + }) + + t.Run("blst", func(t *testing.T) { + flags.EnableBlst = true + reset := featureconfig.InitWithReset(flags) + defer reset() + + _, err := AggregatePublicKeys([][]byte{common.InfinitePublicKey[:], common.InfinitePublicKey[:]}) + require.Equal(t, common.ErrInfinitePubKey, err) + }) +} diff --git a/shared/bls/blst/public_key.go b/shared/bls/blst/public_key.go index 64cecd0242..ea7a18606f 100644 --- a/shared/bls/blst/public_key.go +++ b/shared/bls/blst/public_key.go @@ -36,6 +36,7 @@ func PublicKeyFromBytes(pubKey []byte) (common.PublicKey, error) { if cv, ok := pubkeyCache.Get(string(pubKey)); ok { return cv.(*PublicKey).Copy(), nil } + // Subgroup check done when decompressing pubkey. p := new(blstPublicKey).Uncompress(pubKey) if p == nil { return nil, errors.New("could not unmarshal bytes into public key") @@ -57,20 +58,11 @@ func AggregatePublicKeys(pubs [][]byte) (common.PublicKey, error) { agg := new(blstAggregatePublicKey) mulP1 := make([]*blstPublicKey, 0, len(pubs)) for _, pubkey := range pubs { - if len(pubkey) != params.BeaconConfig().BLSPubkeyLength { - return nil, fmt.Errorf("public key must be %d bytes", params.BeaconConfig().BLSPubkeyLength) + pubKeyObj, err := PublicKeyFromBytes(pubkey) + if err != nil { + return nil, err } - if cv, ok := pubkeyCache.Get(string(pubkey)); ok { - mulP1 = append(mulP1, cv.(*PublicKey).Copy().(*PublicKey).p) - continue - } - p := new(blstPublicKey).Uncompress(pubkey) - if p == nil { - return nil, errors.New("could not unmarshal bytes into public key") - } - pubKeyObj := &PublicKey{p: p} - pubkeyCache.Set(string(pubkey), pubKeyObj.Copy(), 48) - mulP1 = append(mulP1, p) + mulP1 = append(mulP1, pubKeyObj.(*PublicKey).p) } agg.Aggregate(mulP1) return &PublicKey{p: agg.ToAffine()}, nil diff --git a/shared/bls/blst/secret_key.go b/shared/bls/blst/secret_key.go index a489e4ae47..0368a2f46b 100644 --- a/shared/bls/blst/secret_key.go +++ b/shared/bls/blst/secret_key.go @@ -6,7 +6,6 @@ package blst import ( "fmt" - "github.com/pkg/errors" "github.com/prysmaticlabs/prysm/shared/bls/common" "github.com/prysmaticlabs/prysm/shared/featureconfig" "github.com/prysmaticlabs/prysm/shared/params" @@ -42,7 +41,7 @@ func SecretKeyFromBytes(privKey []byte) (common.SecretKey, error) { } secKey := new(blst.SecretKey).Deserialize(privKey) if secKey == nil { - return nil, errors.New("could not unmarshal bytes into secret key") + return nil, common.ErrSecretUnmarshal } wrappedKey := &bls12SecretKey{p: secKey} if wrappedKey.IsZero() { diff --git a/shared/bls/blst/secret_key_test.go b/shared/bls/blst/secret_key_test.go index 34e8f48b66..721175c125 100644 --- a/shared/bls/blst/secret_key_test.go +++ b/shared/bls/blst/secret_key_test.go @@ -9,6 +9,7 @@ import ( "testing" "github.com/prysmaticlabs/prysm/shared/bls/blst" + "github.com/prysmaticlabs/prysm/shared/bls/common" "github.com/prysmaticlabs/prysm/shared/bytesutil" "github.com/prysmaticlabs/prysm/shared/testutil/assert" "github.com/prysmaticlabs/prysm/shared/testutil/require" @@ -54,7 +55,7 @@ func TestSecretKeyFromBytes(t *testing.T) { { name: "Bad", input: []byte{0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff}, - err: errors.New("could not unmarshal bytes into secret key"), + err: common.ErrSecretUnmarshal, }, { name: "Good", diff --git a/shared/bls/common/error.go b/shared/bls/common/error.go index 66dc62d29d..75b8d28f4b 100644 --- a/shared/bls/common/error.go +++ b/shared/bls/common/error.go @@ -5,5 +5,12 @@ import "errors" // ErrZeroKey describes an error due to a zero secret key. var ErrZeroKey = errors.New("received secret key is zero") +// ErrSecretUnmarshal describes an error which happens during unmarshalling +// a secret key. +var ErrSecretUnmarshal = errors.New("could not unmarshal bytes into secret key") + // ErrInfinitePubKey describes an error due to an infinite public key. var ErrInfinitePubKey = errors.New("received an infinite public key") + +// ErrInfiniteSignature describes an error due to an infinite signature. +var ErrInfiniteSignature = errors.New("received an infinite signature") diff --git a/shared/bls/herumi/init.go b/shared/bls/herumi/init.go index f6fb807d0b..67c7a70dfd 100644 --- a/shared/bls/herumi/init.go +++ b/shared/bls/herumi/init.go @@ -9,4 +9,7 @@ func init() { if err := bls.SetETHmode(bls.EthModeDraft07); err != nil { panic(err) } + // Check subgroup order for pubkeys and signatures. + bls.VerifyPublicKeyOrder(true) + bls.VerifySignatureOrder(true) } diff --git a/shared/bls/herumi/secret_key.go b/shared/bls/herumi/secret_key.go index 8384702858..1816238d86 100644 --- a/shared/bls/herumi/secret_key.go +++ b/shared/bls/herumi/secret_key.go @@ -33,7 +33,7 @@ func SecretKeyFromBytes(privKey []byte) (common.SecretKey, error) { secKey := &bls12.SecretKey{} err := secKey.Deserialize(privKey) if err != nil { - return nil, errors.Wrap(err, "could not unmarshal bytes into secret key") + return nil, common.ErrSecretUnmarshal } wrappedKey := &bls12SecretKey{p: secKey} if wrappedKey.IsZero() { diff --git a/shared/bls/herumi/secret_key_test.go b/shared/bls/herumi/secret_key_test.go index 6cdb276714..5fbb4a83a0 100644 --- a/shared/bls/herumi/secret_key_test.go +++ b/shared/bls/herumi/secret_key_test.go @@ -4,6 +4,8 @@ import ( "errors" "testing" + "github.com/prysmaticlabs/prysm/shared/bls/common" + "github.com/prysmaticlabs/prysm/shared/bls/herumi" "github.com/prysmaticlabs/prysm/shared/bytesutil" "github.com/prysmaticlabs/prysm/shared/testutil/assert" @@ -50,7 +52,7 @@ func TestSecretKeyFromBytes(t *testing.T) { { name: "Bad", input: []byte{0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff}, - err: errors.New("could not unmarshal bytes into secret key: err blsSecretKeyDeserialize ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff"), + err: common.ErrSecretUnmarshal, }, { name: "Good", diff --git a/shared/bls/spectest/sign_test.go b/shared/bls/spectest/sign_test.go index f3884cef3f..fbb487a040 100644 --- a/shared/bls/spectest/sign_test.go +++ b/shared/bls/spectest/sign_test.go @@ -42,7 +42,8 @@ func testSignMessageYaml(t *testing.T) { require.NoError(t, err) sk, err := bls.SecretKeyFromBytes(pkBytes) if err != nil { - if test.Output == "" && errors.Is(err, common.ErrZeroKey) { + if test.Output == "" && + (errors.Is(err, common.ErrZeroKey) || errors.Is(err, common.ErrSecretUnmarshal)) { return } t.Fatalf("cannot unmarshal secret key: %v", err)