From 8d60abd6ca0cb97dbb736d4801548916870efff5 Mon Sep 17 00:00:00 2001 From: Preston Van Loon Date: Mon, 9 Dec 2024 14:00:30 -0600 Subject: [PATCH 1/2] Check non-nil validator before accessing withdrawal credentials --- beacon-chain/core/blocks/exports_test.go | 2 +- beacon-chain/core/blocks/withdrawals.go | 5 ++- beacon-chain/core/blocks/withdrawals_test.go | 35 ++++++++++++++++++++ 3 files changed, 40 insertions(+), 2 deletions(-) diff --git a/beacon-chain/core/blocks/exports_test.go b/beacon-chain/core/blocks/exports_test.go index 3508883b03bd..b3e1374b42e3 100644 --- a/beacon-chain/core/blocks/exports_test.go +++ b/beacon-chain/core/blocks/exports_test.go @@ -1,5 +1,5 @@ package blocks var ProcessBLSToExecutionChange = processBLSToExecutionChange - +var ErrInvalidBLSPrefix = errInvalidBLSPrefix var VerifyBlobCommitmentCount = verifyBlobCommitmentCount diff --git a/beacon-chain/core/blocks/withdrawals.go b/beacon-chain/core/blocks/withdrawals.go index b7806503c9d6..f51e916d6c1b 100644 --- a/beacon-chain/core/blocks/withdrawals.go +++ b/beacon-chain/core/blocks/withdrawals.go @@ -100,8 +100,11 @@ func ValidateBLSToExecutionChange(st state.ReadOnlyBeaconState, signed *ethpb.Si if err != nil { return nil, err } + if val == nil { + return nil, errors.Wrap(errInvalidWithdrawalCredentials, "validator is nil") // This should not be possible. + } cred := val.WithdrawalCredentials - if cred[0] != params.BeaconConfig().BLSWithdrawalPrefixByte { + if len(cred) < 2 || cred[0] != params.BeaconConfig().BLSWithdrawalPrefixByte { return nil, errInvalidBLSPrefix } diff --git a/beacon-chain/core/blocks/withdrawals_test.go b/beacon-chain/core/blocks/withdrawals_test.go index 7d7dc5dc08f6..72e4bf7659a2 100644 --- a/beacon-chain/core/blocks/withdrawals_test.go +++ b/beacon-chain/core/blocks/withdrawals_test.go @@ -113,7 +113,42 @@ func TestProcessBLSToExecutionChange(t *testing.T) { require.NoError(t, err) require.DeepEqual(t, digest[:], val.WithdrawalCredentials) }) + t.Run("nil validator does not panic", func(t *testing.T) { + priv, err := bls.RandKey() + require.NoError(t, err) + pubkey := priv.PublicKey().Marshal() + message := ðpb.BLSToExecutionChange{ + ToExecutionAddress: []byte{0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f, 0x10, 0x11, 0x12, 0x13}, + ValidatorIndex: 0, + FromBlsPubkey: pubkey, + } + + registry := []*ethpb.Validator{ + nil, + } + st, err := state_native.InitializeFromProtoPhase0(ðpb.BeaconState{ + Validators: registry, + Fork: ðpb.Fork{ + CurrentVersion: params.BeaconConfig().GenesisForkVersion, + PreviousVersion: params.BeaconConfig().GenesisForkVersion, + }, + Slot: params.BeaconConfig().SlotsPerEpoch * 5, + }) + require.NoError(t, err) + + signature, err := signing.ComputeDomainAndSign(st, time.CurrentEpoch(st), message, params.BeaconConfig().DomainBLSToExecutionChange, priv) + require.NoError(t, err) + + signed := ðpb.SignedBLSToExecutionChange{ + Message: message, + Signature: signature, + } + _, err = blocks.ValidateBLSToExecutionChange(st, signed) + // The state should return an empty validator, even when the validator object in the registry is + // nil. This error should return when the withdrawal credentials are invalid or too short. + require.ErrorIs(t, err, blocks.ErrInvalidBLSPrefix) + }) t.Run("non-existent validator", func(t *testing.T) { priv, err := bls.RandKey() require.NoError(t, err) From 30d78ec5371699afcccf7d914eaa5ca0e0c7c9ed Mon Sep 17 00:00:00 2001 From: Preston Van Loon Date: Mon, 9 Dec 2024 15:00:37 -0600 Subject: [PATCH 2/2] Updated changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 03eae5766d80..8f5ec24b65b5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -112,6 +112,7 @@ The format is based on Keep a Changelog, and this project adheres to Semantic Ve - P2P: Avoid infinite loop when looking for peers in small networks. - Fixed another rollback bug due to a context deadline. - Fix checkpoint sync bug on holesky. [pr](https://github.com/prysmaticlabs/prysm/pull/14689) +- Added check to prevent nil pointer deference or out of bounds array access when validating the BLSToExecutionChange on an impossibly nil validator. ### Security