Do not validate signature on the attestation package

prysmaticlabs · May 16, 2023 · a64a6f9 · a64a6f9
1 parent a91831c
commit a64a6f9
Show file tree

Hide file tree

Showing 6 changed files with 92 additions and 6 deletions.
diff --git a/beacon-chain/operations/attestations/kv/aggregated.go b/beacon-chain/operations/attestations/kv/aggregated.go
@@ -55,7 +55,7 @@ func (c *AttCaches) aggregateUnaggregatedAttestations(ctx context.Context, unagg
 	for _, atts := range attsByDataRoot {
 		aggregated, err := attaggregation.AggregateDisjointOneBitAtts(atts)
 		if err != nil || aggregated == nil {
-			return errors.Wrap(err, "could not aggregate unnagregated attestations")
+			return errors.Wrap(err, "could not aggregate unaggregated attestations")
 		}
 		if helpers.IsAggregated(aggregated) {
 			if err := c.SaveAggregatedAttestations([]*ethpb.Attestation{aggregated}); err != nil {

diff --git a/beacon-chain/rpc/eth/beacon/pool_test.go b/beacon-chain/rpc/eth/beacon/pool_test.go
@@ -1183,7 +1183,7 @@ func TestServer_SubmitAttestations_InvalidAttestationGRPCHeader(t *testing.T) {
 	require.Equal(t, true, ok, "could not retrieve custom error metadata value")
 	assert.DeepEqual(
 		t,
-		[]string{"{\"failures\":[{\"index\":0,\"message\":\"Incorrect attestation signature: signature must be 96 bytes\"}]}"},
+		[]string{"{\"failures\":[{\"index\":0,\"message\":\"Incorrect attestation signature: could not create signature from byte slice: signature must be 96 bytes\"}]}"},
 		v,
 	)
 }

diff --git a/crypto/bls/bls.go b/crypto/bls/bls.go
@@ -24,6 +24,13 @@ func PublicKeyFromBytes(pubKey []byte) (PublicKey, error) {
 	return blst.PublicKeyFromBytes(pubKey)
 }
 
+// SignatureFromBytesNoValidation creates a BLS signature from a LittleEndian byte slice.
+// It does not check validity of the signature, use only when the byte slice has
+// already been verified
+func SignatureFromBytesNoValidation(sig []byte) (Signature, error) {
+	return blst.SignatureFromBytesNoValidation(sig)
+}
+
 // SignatureFromBytes creates a BLS signature from a LittleEndian byte slice.
 func SignatureFromBytes(sig []byte) (Signature, error) {
 	return blst.SignatureFromBytes(sig)

diff --git a/crypto/bls/blst/signature.go b/crypto/bls/blst/signature.go
@@ -24,15 +24,35 @@ type Signature struct {
 	s *blstSignature
 }
 
-// SignatureFromBytes creates a BLS signature from a LittleEndian byte slice.
-func SignatureFromBytes(sig []byte) (common.Signature, error) {
+// signatureFromBytesNoValidation creates a BLS signature from a LittleEndian
+// byte slice. It does not validate that the signature is in the BLS group
+func signatureFromBytesNoValidation(sig []byte) (*blstSignature, error) {
 	if len(sig) != fieldparams.BLSSignatureLength {
 		return nil, fmt.Errorf("signature must be %d bytes", fieldparams.BLSSignatureLength)
 	}
 	signature := new(blstSignature).Uncompress(sig)
 	if signature == nil {
 		return nil, errors.New("could not unmarshal bytes into signature")
 	}
+	return signature, nil
+}
+
+// SignatureFromBytesNoValidation creates a BLS signature from a LittleEndian
+// byte slice. It does not validate that the signature is in the BLS group
+func SignatureFromBytesNoValidation(sig []byte) (common.Signature, error) {
+	signature, err := signatureFromBytesNoValidation(sig)
+	if err != nil {
+		return nil, errors.Wrap(err, "could not create signature from byte slice")
+	}
+	return &Signature{s: signature}, nil
+}
+
+// SignatureFromBytes creates a BLS signature from a LittleEndian byte slice.
+func SignatureFromBytes(sig []byte) (common.Signature, error) {
+	signature, err := signatureFromBytesNoValidation(sig)
+	if err != nil {
+		return nil, errors.Wrap(err, "could not create signature from byte slice")
+	}
 	// Group check signature. Do not check for infinity since an aggregated signature
 	// could be infinite.
 	if !signature.SigValidate(false) {

diff --git a/crypto/bls/blst/signature_test.go b/crypto/bls/blst/signature_test.go
@@ -207,6 +207,11 @@ func TestSignatureFromBytes(t *testing.T) {
 			input: []byte{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00},
 			err:   errors.New("could not unmarshal bytes into signature"),
 		},
+		{
+			input: []byte{0xac, 0xb0, 0x12, 0x4c, 0x75, 0x74, 0xf2, 0x81, 0xa2, 0x93, 0xf4, 0x18, 0x5c, 0xad, 0x3c, 0xb2, 0x26, 0x81, 0xd5, 0x20, 0x91, 0x7c, 0xe4, 0x66, 0x65, 0x24, 0x3e, 0xac, 0xb0, 0x51, 0x00, 0x0d, 0x8b, 0xac, 0xf7, 0x5e, 0x14, 0x51, 0x87, 0x0c, 0xa6, 0xb3, 0xb9, 0xe6, 0xc9, 0xd4, 0x1a, 0x7b, 0x02, 0xea, 0xd2, 0x68, 0x5a, 0x84, 0x18, 0x8a, 0x4f, 0xaf, 0xd3, 0x82, 0x5d, 0xaf, 0x6a, 0x98, 0x96, 0x25, 0xd7, 0x19, 0xcc, 0xd2, 0xd8, 0x3a, 0x40, 0x10, 0x1f, 0x4a, 0x45, 0x3f, 0xca, 0x62, 0x87, 0x8c, 0x89, 0x0e, 0xca, 0x62, 0x23, 0x63, 0xf9, 0xdd, 0xb8, 0xf3, 0x67, 0xa9, 0x1e, 0x84},
+			name:  "Not in group",
+			err:   errors.New("signature not in group"),
+		},
 		{
 			name:  "Good",
 			input: []byte{0xab, 0xb0, 0x12, 0x4c, 0x75, 0x74, 0xf2, 0x81, 0xa2, 0x93, 0xf4, 0x18, 0x5c, 0xad, 0x3c, 0xb2, 0x26, 0x81, 0xd5, 0x20, 0x91, 0x7c, 0xe4, 0x66, 0x65, 0x24, 0x3e, 0xac, 0xb0, 0x51, 0x00, 0x0d, 0x8b, 0xac, 0xf7, 0x5e, 0x14, 0x51, 0x87, 0x0c, 0xa6, 0xb3, 0xb9, 0xe6, 0xc9, 0xd4, 0x1a, 0x7b, 0x02, 0xea, 0xd2, 0x68, 0x5a, 0x84, 0x18, 0x8a, 0x4f, 0xaf, 0xd3, 0x82, 0x5d, 0xaf, 0x6a, 0x98, 0x96, 0x25, 0xd7, 0x19, 0xcc, 0xd2, 0xd8, 0x3a, 0x40, 0x10, 0x1f, 0x4a, 0x45, 0x3f, 0xca, 0x62, 0x87, 0x8c, 0x89, 0x0e, 0xca, 0x62, 0x23, 0x63, 0xf9, 0xdd, 0xb8, 0xf3, 0x67, 0xa9, 0x1e, 0x84},
@@ -227,6 +232,60 @@ func TestSignatureFromBytes(t *testing.T) {
 	}
 }
 
+func TestSignatureFromBytesNoValidation(t *testing.T) {
+	tests := []struct {
+		name  string
+		input []byte
+		err   error
+	}{
+		{
+			name: "Nil",
+			err:  errors.New("signature must be 96 bytes"),
+		},
+		{
+			name:  "Empty",
+			input: []byte{},
+			err:   errors.New("signature must be 96 bytes"),
+		},
+		{
+			name:  "Short",
+			input: []byte{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00},
+			err:   errors.New("signature must be 96 bytes"),
+		},
+		{
+			name:  "Long",
+			input: []byte{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00},
+			err:   errors.New("signature must be 96 bytes"),
+		},
+		{
+			name:  "Bad",
+			input: []byte{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00},
+			err:   errors.New("could not unmarshal bytes into signature"),
+		},
+		{
+			name:  "Not in group",
+			input: []byte{0xac, 0xb0, 0x12, 0x4c, 0x75, 0x74, 0xf2, 0x81, 0xa2, 0x93, 0xf4, 0x18, 0x5c, 0xad, 0x3c, 0xb2, 0x26, 0x81, 0xd5, 0x20, 0x91, 0x7c, 0xe4, 0x66, 0x65, 0x24, 0x3e, 0xac, 0xb0, 0x51, 0x00, 0x0d, 0x8b, 0xac, 0xf7, 0x5e, 0x14, 0x51, 0x87, 0x0c, 0xa6, 0xb3, 0xb9, 0xe6, 0xc9, 0xd4, 0x1a, 0x7b, 0x02, 0xea, 0xd2, 0x68, 0x5a, 0x84, 0x18, 0x8a, 0x4f, 0xaf, 0xd3, 0x82, 0x5d, 0xaf, 0x6a, 0x98, 0x96, 0x25, 0xd7, 0x19, 0xcc, 0xd2, 0xd8, 0x3a, 0x40, 0x10, 0x1f, 0x4a, 0x45, 0x3f, 0xca, 0x62, 0x87, 0x8c, 0x89, 0x0e, 0xca, 0x62, 0x23, 0x63, 0xf9, 0xdd, 0xb8, 0xf3, 0x67, 0xa9, 0x1e, 0x84},
+		},
+		{
+			name:  "Good",
+			input: []byte{0xab, 0xb0, 0x12, 0x4c, 0x75, 0x74, 0xf2, 0x81, 0xa2, 0x93, 0xf4, 0x18, 0x5c, 0xad, 0x3c, 0xb2, 0x26, 0x81, 0xd5, 0x20, 0x91, 0x7c, 0xe4, 0x66, 0x65, 0x24, 0x3e, 0xac, 0xb0, 0x51, 0x00, 0x0d, 0x8b, 0xac, 0xf7, 0x5e, 0x14, 0x51, 0x87, 0x0c, 0xa6, 0xb3, 0xb9, 0xe6, 0xc9, 0xd4, 0x1a, 0x7b, 0x02, 0xea, 0xd2, 0x68, 0x5a, 0x84, 0x18, 0x8a, 0x4f, 0xaf, 0xd3, 0x82, 0x5d, 0xaf, 0x6a, 0x98, 0x96, 0x25, 0xd7, 0x19, 0xcc, 0xd2, 0xd8, 0x3a, 0x40, 0x10, 0x1f, 0x4a, 0x45, 0x3f, 0xca, 0x62, 0x87, 0x8c, 0x89, 0x0e, 0xca, 0x62, 0x23, 0x63, 0xf9, 0xdd, 0xb8, 0xf3, 0x67, 0xa9, 0x1e, 0x84},
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			res, err := SignatureFromBytesNoValidation(test.input)
+			if test.err != nil {
+				assert.NotEqual(t, nil, err, "No error returned")
+				assert.ErrorContains(t, test.err.Error(), err, "Unexpected error returned")
+			} else {
+				assert.NoError(t, err)
+				assert.DeepEqual(t, 0, bytes.Compare(res.Marshal(), test.input))
+			}
+		})
+	}
+}
+
 func TestMultipleSignatureFromBytes(t *testing.T) {
 	tests := []struct {
 		name  string

diff --git a/proto/prysm/v1alpha1/attestation/aggregation/attestations/attestations.go b/proto/prysm/v1alpha1/attestation/aggregation/attestations/attestations.go
@@ -15,7 +15,7 @@ type attList []*ethpb.Attestation
 // significantly more expensive than the inner logic of AggregateAttestations so they must be
 // substituted for benchmarks which analyze AggregateAttestations.
 var aggregateSignatures = bls.AggregateSignatures
-var signatureFromBytes = bls.SignatureFromBytes
+var signatureFromBytes = bls.SignatureFromBytesNoValidation
 
 var _ = logrus.WithField("prefix", "aggregation.attestations")
 
@@ -36,7 +36,7 @@ func Aggregate(atts []*ethpb.Attestation) ([]*ethpb.Attestation, error) {
 	return MaxCoverAttestationAggregation(atts)
 }
 
-// AggregateDisjointOneBitAtts aggregates unnagregated attestations with the
+// AggregateDisjointOneBitAtts aggregates unaggregated attestations with the
 // exact same attestation data.
 func AggregateDisjointOneBitAtts(atts []*ethpb.Attestation) (*ethpb.Attestation, error) {
 	if len(atts) == 0 {