cluster: reject fixed size fields of the wrong length

cluster/cluster_test.go

@@ -166,9 +166,19 @@ func TestEncode(t *testing.T) {
 			require.NoError(t, err)
 			require.Equal(t, definition, definition2)
 
+			// Signatures aren't covered by the lock hash. Pad the 32 byte values drawn from r to real lengths
+			// using a separate random source, so r's sequence (and thereby the golden lock hashes) is unchanged.
+			sigR := rand.New(rand.NewSource(2))
+			padSig := func(b []byte, length int) []byte {
+				pad := make([]byte, length-len(b))
+				_, _ = sigR.Read(pad)
+
+				return append(b, pad...)
+			}
+
 			lock := cluster.Lock{
 				Definition:         definition,
-				SignatureAggregate: testutil.RandomBytes32Seed(r),
+				SignatureAggregate: padSig(testutil.RandomBytes32Seed(r), 96),
 				Validators: []cluster.DistValidator{
 					{
 						PubKey: testutil.RandomBytes48Seed(r),
@@ -189,8 +199,8 @@ func TestEncode(t *testing.T) {
 					},
 				},
 				NodeSignatures: [][]byte{
-					testutil.RandomBytes32Seed(r),
-					testutil.RandomBytes32Seed(r),
+					padSig(testutil.RandomBytes32Seed(r), 65),
+					padSig(testutil.RandomBytes32Seed(r), 65),
 				},
 			}
 
@@ -304,6 +314,33 @@ func TestExamples(t *testing.T) {
 	}
 }
 
+// TestTestdataStrictParsing ensures all golden testdata definitions and locks of all versions
+// still parse and verify hashes with strict fixed size field length checks.
+func TestTestdataStrictParsing(t *testing.T) {
+	files, err := filepath.Glob("testdata/cluster_*.json")
+	require.NoError(t, err)
+	require.NotEmpty(t, files)
+
+	for _, file := range files {
+		t.Run(filepath.Base(file), func(t *testing.T) {
+			b, err := os.ReadFile(file)
+			require.NoError(t, err)
+
+			if strings.HasPrefix(filepath.Base(file), "cluster_lock") {
+				var lock cluster.Lock
+				require.NoError(t, json.Unmarshal(b, &lock))
+				require.NoError(t, lock.VerifyHashes())
+
+				return
+			}
+
+			var def cluster.Definition
+			require.NoError(t, json.Unmarshal(b, &def))
+			require.NoError(t, def.VerifyHashes())
+		})
+	}
+}
+
 func TestDefinitionPeers(t *testing.T) {
 	seed := 5
 	random := rand.New(rand.NewSource(int64(seed)))

cluster/definition.go

@@ -575,8 +575,72 @@
 	return nil
 }
 
+// verifyDefinitionFieldLengths returns an error if any fixed size field of the definition has an invalid length.
+// Empty values are allowed since they represent absent values (e.g. unsigned operators), see checkBytesN.
+// Presence of required values is checked by VerifyHashes and VerifySignatures.
+func verifyDefinitionFieldLengths(d Definition) error {
+	if err := checkBytesN(d.ForkVersion, sszLenForkVersion, "fork_version"); err != nil {
+		return err
+	}
+
+	if err := checkBytesN(d.ConfigHash, sszLenHash, "config_hash"); err != nil {
+		return err
+	}
+
+	if err := checkBytesN(d.DefinitionHash, sszLenHash, "definition_hash"); err != nil {
+		return err
+	}
+
+	if err := checkAddressLength(d.Creator.Address, "creator_address"); err != nil {
+		return err
+	}
+
+	if err := validateSignatureLength(d.Version, d.Creator.ConfigSignature, "creator_config_signature"); err != nil {
+		return err
+	}
+
+	for i, o := range d.Operators {
+		if err := checkAddressLength(o.Address, "operator_address"); err != nil {
+			return errors.Wrap(err, "invalid operator", z.Int("operator_index", i))
+		}
+
+		if err := validateSignatureLength(d.Version, o.ConfigSignature, "operator_config_signature"); err != nil {
+			return errors.Wrap(err, "invalid operator", z.Int("operator_index", i))
+		}
+
+		if err := validateSignatureLength(d.Version, o.ENRSignature, "operator_enr_signature"); err != nil {
+			return errors.Wrap(err, "invalid operator", z.Int("operator_index", i))
+		}
+	}
+
+	for i, v := range d.ValidatorAddresses {
+		if err := checkAddressLength(v.FeeRecipientAddress, "fee_recipient_address"); err != nil {
+			return errors.Wrap(err, "invalid validator addresses", z.Int("validator_index", i))
+		}
+
+		if err := checkAddressLength(v.WithdrawalAddress, "withdrawal_address"); err != nil {
+			return errors.Wrap(err, "invalid validator addresses", z.Int("validator_index", i))
+		}
+	}
+
+	return nil
+}
+
+// checkAddressLength returns an error if the address is neither empty nor a 20 byte hex address.
+func checkAddressLength(addr string, field string) error {
+	if _, err := from0xHex(addr, addressLen); err != nil {
+		return errors.Wrap(err, "invalid address", z.Str("field", field))
+	}
+
+	return nil
+}
+
 // VerifyHashes returns an error if hashes populated from json object doesn't matches actual hashes.
 func (d Definition) VerifyHashes() error {
+	if err := verifyDefinitionFieldLengths(d); err != nil {
+		return err
+	}
+
 	configHash, err := hashDefinition(d, true)
 	if err != nil {
 		return errors.Wrap(err, "config hash")

cluster/helpers.go

@@ -277,17 +277,47 @@ func putK1SigList(h sszutils.HashWalker, sig []byte, maxAmountSigs int, field st
 	return nil
 }
 
+// checkBytesN returns an error if b is neither empty nor exactly n bytes long.
+// Empty is allowed since absent values (e.g. unsigned operators, missing deposit data or
+// builder registrations) are hashed as zeros. Any other length is rejected since ssz
+// would otherwise pad it, resulting in the same hash as a different (padded) value.
+func checkBytesN(b []byte, n int, field string) error {
+	if len(b) != 0 && len(b) != n {
+		return errors.New("invalid fixed size bytes length",
+			z.Str("field", field),
+			z.Int("expect", n),
+			z.Int("actual", len(b)),
+		)
+	}
+
+	return nil
+}
+
 // putBytesN appends b as a ssz fixed size byte array of length n.
-func putBytesN(h sszutils.HashWalker, b []byte, n int) error {
-	if len(b) > n {
-		return errors.New("bytes too long", z.Int("n", n), z.Int("l", len(b)))
+// b must be either empty (hashed as zeros) or exactly n bytes long.
+func putBytesN(h sszutils.HashWalker, b []byte, n int, field string) error {
+	if err := checkBytesN(b, n, field); err != nil {
+		return err
 	}
 
 	h.PutBytes(leftPad(b, n))
 
 	return nil
 }
 
+// putBytesRaw appends b as is (not padded) after checking it is either empty or exactly n bytes long.
+// It is used by older hashing versions that appended fixed size fields without padding,
+// where an empty value must still append nothing for backwards compatible hashes.
+func putBytesRaw(h sszutils.HashWalker, b []byte, n int, field string) error {
+	if err := checkBytesN(b, n, field); err != nil {
+		return err
+	}
+
+	h.PutBytes(b)
+
+	return nil
+}
+
 // putHexBytes20 appends a 20 byte fixed size byte ssz array from the 0xhex address.
 func putHexBytes20(h sszutils.HashWalker, addr string) error {
 	b, err := from0xHex(addr, addressLen)
@@ -328,7 +358,7 @@ func from0xHex(s string, length int) ([]byte, error) {
 	if err != nil {
 		return nil, errors.Wrap(err, "decode hex")
 	} else if len(b) != length {
-		return nil, errors.Wrap(err, "invalid hex length", z.Int("expect", length), z.Int("actual", len(b)))
+		return nil, errors.New("invalid hex length", z.Int("expect", length), z.Int("actual", len(b)))
 	}
 
 	return b, nil

cluster/helpers_internal_test.go

@@ -149,22 +149,11 @@ func TestPutBytesN(t *testing.T) {
 			b:    []byte{0x01, 0x02, 0x03, 0x04},
 			n:    4,
 		},
-		{
-			name: "short_n32",
-			b:    []byte{0xab, 0xcd},
-			n:    32,
-		},
 		{
 			name: "full_n4",
 			b:    []byte{0xde, 0xad, 0xbe, 0xef},
 			n:    4,
 		},
-		{
-			name:  "after_uint64_1",
-			setup: func(hh sszutils.HashWalker) { hh.PutUint64(1) },
-			b:     []byte{0xab, 0xcd},
-			n:     32,
-		},
 		{
 			name:  "after_bytes20",
 			setup: func(hh sszutils.HashWalker) { hh.PutBytes(make([]byte, 20)) },
@@ -177,7 +166,7 @@ func TestPutBytesN(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			fn := func(hh sszutils.HashWalker) error { return putBytesN(hh, tt.b, tt.n) }
+			fn := func(hh sszutils.HashWalker) error { return putBytesN(hh, tt.b, tt.n, "test") }
 
 			var got string
 			if tt.setup != nil {
@@ -193,6 +182,31 @@ func TestPutBytesN(t *testing.T) {
 	testutil.RequireGoldenJSON(t, roots)
 }
 
+// TestPutBytesNInvalidLength ensures values that are neither empty nor exactly n bytes are rejected
+// instead of being padded, since padding would result in the same hash as the padded value.
+func TestPutBytesNInvalidLength(t *testing.T) {
+	tests := []struct {
+		name string
+		b    []byte
+		n    int
+	}{
+		{name: "short_n32", b: []byte{0xab, 0xcd}, n: 32},
+		{name: "address_as_creds", b: bytes.Repeat([]byte{0x11}, 20), n: 32},
+		{name: "long_n4", b: []byte{0x01, 0x02, 0x03, 0x04, 0x05}, n: 4},
+		{name: "short_pubkey", b: make([]byte, 47), n: 48},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			hh := hasher.DefaultHasherPool.Get()
+			defer hasher.DefaultHasherPool.Put(hh)
+
+			require.ErrorContains(t, putBytesN(hh, tt.b, tt.n, "test"), "invalid fixed size bytes length")
+			require.ErrorContains(t, putBytesRaw(hh, tt.b, tt.n, "test"), "invalid fixed size bytes length")
+		})
+	}
+}
+
 func TestPutHexBytes20(t *testing.T) {
 	tests := []struct {
 		name  string

cluster/lock.go

@@ -118,6 +118,63 @@ func (l *Lock) UnmarshalJSON(data []byte) error {
 	return nil
 }
 
+// verifyLockFieldLengths returns an error if any fixed size field included in the lock hash has an invalid length.
+// Empty values are allowed since they represent absent values (e.g. missing deposit data), see checkBytesN.
+// Presence of required values is checked by VerifyHashes and VerifySignatures.
+// The definition fields are checked by Definition.VerifyHashes.
+//
+// Note that SignatureAggregate and NodeSignatures are not included in the lock hash
+// and their lengths are checked by VerifySignatures.
+func verifyLockFieldLengths(l Lock) error {
+	if err := checkBytesN(l.LockHash, sszLenHash, "lock_hash"); err != nil {
+		return err
+	}
+
+	for i, val := range l.Validators {
+		if err := verifyDistValidatorFieldLengths(val); err != nil {
+			return errors.Wrap(err, "invalid distributed validator", z.Int("dv_index", i))
+		}
+	}
+
+	return nil
+}
+
+// verifyDistValidatorFieldLengths returns an error if any fixed size field of the distributed validator has an invalid length.
+func verifyDistValidatorFieldLengths(val DistValidator) error {
+	type fixedField struct {
+		b     []byte
+		n     int
+		field string
+	}
+
+	fields := []fixedField{
+		{val.PubKey, sszLenPubKey, "distributed_public_key"},
+		{val.BuilderRegistration.Message.FeeRecipient, addressLen, "builder_registration_fee_recipient"},
+		{val.BuilderRegistration.Message.PubKey, sszLenPubKey, "builder_registration_pubkey"},
+		{val.BuilderRegistration.Signature, sszLenBLSSig, "builder_registration_signature"},
+	}
+
+	for _, share := range val.PubShares {
+		fields = append(fields, fixedField{share, sszLenPubKey, "public_shares"})
+	}
+
+	for _, dd := range val.PartialDepositData {
+		fields = append(fields,
+			fixedField{dd.PubKey, sszLenPubKey, "deposit_data_pubkey"},
+			fixedField{dd.WithdrawalCredentials, sszLenWithdrawCreds, "deposit_data_withdrawal_credentials"},
+			fixedField{dd.Signature, sszLenBLSSig, "deposit_data_signature"},
+		)
+	}
+
+	for _, f := range fields {
+		if err := checkBytesN(f.b, f.n, f.field); err != nil {
+			return err
+		}
+	}
+
+	return nil
+}
+
 // SetLockHash returns a copy of the lock with the lock hash populated.
 func (l Lock) SetLockHash() (Lock, error) {
 	lockHash, err := hashLock(l)
@@ -136,6 +193,10 @@ func (l Lock) VerifyHashes() error {
 		return errors.Wrap(err, "invalid definition")
 	}
 
+	if err := verifyLockFieldLengths(l); err != nil {
+		return err
+	}
+
 	if len(l.Validators) != l.NumValidators {
 		return errors.New("lock validators count mismatch",
 			z.Int("definition_num_validators", l.NumValidators),
@@ -170,6 +231,10 @@ func (l Lock) VerifySignatures(eth1 eth1wrap.EthClientRunner) error {
 		return errors.New("empty lock aggregate signature")
 	}
 
+	if len(l.SignatureAggregate) != sszLenBLSSig {
+		return errors.New("invalid lock aggregate signature length", z.Int("length", len(l.SignatureAggregate)))
+	}
+
 	sig, err := tblsconv.SignatureFromBytes(l.SignatureAggregate)
 	if err != nil {
 		return err
@@ -249,6 +314,10 @@ func (l Lock) verifyNodeSignatures() error {
 
 	// Verify the node signatures
 	for idx := range len(l.Operators) {
+		if len(l.NodeSignatures[idx]) != sszLenK1Sig {
+			return errors.New("invalid node signature length", z.Int("peer_index", idx), z.Int("length", len(l.NodeSignatures[idx])))
+		}
+
 		record, err := enr.Parse(l.Operators[idx].ENR)
 		if err != nil {
 			return errors.Wrap(err, "operator ENR")