[bug/fuzzing] "panic: runtime error: nil pointer dereference" when processing ProposerSlashing

[pventuzelo]

🐞 Bug Report

Description

During fuzzing with beaconfuzz, I found the following bug:

what: panic: runtime error: invalid memory address or nil pointer dereference
where: in prysm
how: triggered during ProposerSlashing processing.

The bug is happening mainly in VerifyProposerSlashing function.

Here is some supposition:

prysm/beacon-chain/core/blocks/block_operations.go

Lines 426 to 432 in a0bf8cb

	proposer, err := beaconState.ValidatorAtIndexReadOnly(slashing.Header_1.Header.ProposerIndex)
	if err != nil {
	return err
	}
	if !helpers.IsSlashableValidatorUsingTrie(proposer, helpers.SlotToEpoch(beaconState.Slot())) {
	return fmt.Errorf("validator with key %#x is not slashable", proposer.PublicKey())
	}

• line 426: proposer will be nil after the call to ValidatorAtIndexReadOnly
• line 430: IsSlashableValidatorUsingTrie is called with proposer

prysm/beacon-chain/core/helpers/validators.go

Lines 50 to 51 in 9a11574

	func IsSlashableValidatorUsingTrie(val *stateTrie.ReadOnlyValidator, epoch uint64) bool {
	return checkValidatorSlashable(val.ActivationEpoch(), val.WithdrawableEpoch(), val.Slashed(), epoch)

• line 51: IsSlashableValidatorUsingTrie will execute val.Slashed() (val == proposer)

prysm/beacon-chain/state/getters.go

Lines 76 to 79 in 6a9112b

	// Slashed returns the read only validator is slashed.
	func (v *ReadOnlyValidator) Slashed() bool {
	return v.validator.Slashed
	}

• line 78: Slashed try to dereference v.validator without having verify that v and v.validator are not nil

🔥 Error

Download: panic_nil_deref_prysm_proposer.zip

./prysm_FuzzProposerSlashing2.libfuzzer panic_nil_deref_prysm_proposer.ssz
INFO: Seed: 1287398888
./prysm_FuzzProposerSlashing2.libfuzzer: Running 1 inputs 1 time(s) each.
Running: panic_nil_deref_prysm_proposer.ssz

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x38 pc=0x11d0360]

goroutine 17 [running, locked to thread]:
github.com/prysmaticlabs/prysm/beacon-chain/state.(*ReadOnlyValidator).Slashed(...)
	github.com/prysmaticlabs/prysm/beacon-chain/state/getters.go:78
github.com/prysmaticlabs/prysm/beacon-chain/core/helpers.IsSlashableValidatorUsingTrie(...)
	github.com/prysmaticlabs/prysm/beacon-chain/core/helpers/validators.go:51
github.com/prysmaticlabs/prysm/beacon-chain/core/blocks.VerifyProposerSlashing(0x10c000802f50, 0x10c0000edd50, 0x16, 0x10c000038fe0)
	github.com/prysmaticlabs/prysm/beacon-chain/core/blocks/block_operations.go:430 +0x400
github.com/prysmaticlabs/prysm/beacon-chain/core/blocks.ProcessProposerSlashings(0x18344c0, 0x10c000138030, 0x10c000802f50, 0x10c0000edd80, 0x0, 0x0, 0x0)
	github.com/prysmaticlabs/prysm/beacon-chain/core/blocks/block_operations.go:396 +0x102
github.com/prysmaticlabs/prysm.FuzzProposerSlashing(0x603000001900, 0x20, 0x20, 0x0)
	github.com/prysmaticlabs/prysm/fuzz.go:223 +0x1c5
main.LLVMFuzzerTestOneInput(...)
	command-line-arguments/main.132452089.go:21
main._cgoexpwrap_b2568cfea483_LLVMFuzzerTestOneInput(0x603000001900, 0x20, 0x5ed8ef0e)
	_cgo_gotypes.go:53 +0x56

🔬 Minimal Reproduction

I can only reproduce the bug running my fuzzer with the previous crashing ssz file.
Maybe you will succeed to reproduce with your fuzzers as well.

Fuzzing function:

func FuzzProposerSlashing(b []byte) int {
	params.UseMainnetConfig()
	data := &ethpb.ProposerSlashing{}
	if err := data.UnmarshalSSZ(b); err != nil {
		return 0
	}
	// get a valid beaconstate
	st := getbeaconstate()
	s, err := stateTrie.InitializeFromProto(st)
	if err != nil {
		// should never happen
		panic("stateTrie InitializeFromProto")
	}
	// process the container
	post, err := blocks.ProcessProposerSlashings(context.Background(), s, &ethpb.BeaconBlockBody{ProposerSlashings: []*ethpb.ProposerSlashing{data}})
	if err != nil {
		return 0
	}
	if post == nil {
		return 0
	}
	return 1
}

I have nevertheless fix the bug (with the following patch) and the fuzzer is not crashing anymore.

Patch

A simple fix consist to check if v and v.validator are nil like in WithdrawableEpoch() and ExitEpoch() functions.

// Slashed returns the read only validator is slashed.
func (v *ReadOnlyValidator) Slashed() bool {
	if v == nil || v.validator == nil {
		return false
	}
	return v.validator.Slashed
}

instead of:

prysm/beacon-chain/state/getters.go

Lines 77 to 79 in 6a9112b

	func (v *ReadOnlyValidator) Slashed() bool {
	return v.validator.Slashed
	}

🌍 Your Environment

Operating System:

OS: Ubuntu 18.04
Go: Go 1.14

What version of Prysm are you running? (Which release)

master
commit: d152b48

[farazdagi]

@pventuzelo thank you for such a detailed report.
As you already have a patch, will you be willing to open a PR, resolving the issue?

[pventuzelo]

Hi @farazdagi,

Thanks for the proposal but I prefer to let you choose how to fix this since it is dependent of the business logic.

My fix was just for testing to verify that the nil pointer deref was coming from this function. It's not taking care if this function should actually return false in that case, or if you should verify v and v.validator in the caller function.