Handle potential error from newBlockRangeBatcher

[jtraglia]

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?

This is a really interesting bug that I'm surprised errcheck didn't identify. The error from newBlockRangeBatcher isn't checked. This could actually be exploited to crash Prysm with a nil pointer dereference.

prysm/beacon-chain/sync/rpc_blob_sidecars_by_range.go

Lines 93 to 97 in 1123df7

	batcher, err := newBlockRangeBatcher(rp, s.cfg.beaconDB, s.rateLimiter, s.cfg.chain.IsCanonical, ticker)
	var batch blockBatch
	wQuota := params.BeaconNetworkConfig().MaxRequestBlobSidecars
	for batch, ok = batcher.next(ctx, stream); ok; batch, ok = batcher.next(ctx, stream) {

Details

If an attacker sends a BlobSidecarsByRange request with the start slot greater than the current slot, it will return a rangeParams with size: 0.

prysm/beacon-chain/sync/rpc_blob_sidecars_by_range.go

Lines 149 to 153 in 1123df7

	// Peers may overshoot the current slot when in initial sync, so we don't want to penalize them by treating the
	// request as an error. So instead we return a set of params that acts as a noop.
	if rp.start > current {
	return rangeParams{start: current, end: current, size: 0}, nil
	}

When newBlockRangeBatcher gets this, it will return nil, someErr which doesn't get checked:

prysm/beacon-chain/sync/block_batcher.go

Lines 45 to 47 in 45a2746

	if rp.size == 0 {
	return nil, fmt.Errorf("invalid batch size of %d", rp.size)
	}

Then it will nil pointer dereference when batcher.next() is called:

prysm/beacon-chain/sync/rpc_blob_sidecars_by_range.go

Lines 93 to 97 in 1123df7

	batcher, err := newBlockRangeBatcher(rp, s.cfg.beaconDB, s.rateLimiter, s.cfg.chain.IsCanonical, ticker)
	var batch blockBatch
	wQuota := params.BeaconNetworkConfig().MaxRequestBlobSidecars
	for batch, ok = batcher.next(ctx, stream); ok; batch, ok = batcher.next(ctx, stream) {

This is not exploitable until the Deneb hardfork. The RPC handler is only registered at that epoch.

prysm/beacon-chain/sync/rpc.go

Lines 53 to 55 in 8aaab86

	if currEpoch >= params.BeaconConfig().DenebForkEpoch {
	s.registerRPCHandlersDeneb()
	}

[prestonvanloon]

Upstream issue kisielk/errcheck#7