erigontech / erigontech/erigon

cl/rpc: response fork digest is never validated against the requested slot range

Open
#22,807 1 comment 0 reactions 1 assignee Claimed by @domiwei View on GitHub
Caplin tech debt reduction
Dominant language
Go
Stars
3.6k
Forks
1.5k
Avg merge
1d 16h
Merged PRs (30d)
455

Description

## Problem

For a libp2p req/resp **response**, the SSZ decode version is chosen entirely from the peer-supplied fork-context digest. `parseResponseData` (`cl/rpc/rpc.go`) checks only that the digest is non-zero and resolvable through `StateVersionByForkDigest`; nothing compares it against the request, the requested slot range, or the local clock.

Two things make that load-bearing:

- `NewEthereumClock` (`cl/utils/eth_clock/ethereum_clock.go`) registers **every** configured fork digest, with no activation filter, including forks scheduled at `FAR_FUTURE_EPOCH`. `configForkSchedule` (`cl/clparams/config.go`) adds Gloas unconditionally, and mainnet sets `GloasForkEpoch: math.MaxUint64`. So a peer can select the Gloas decoder on mainnet today.
- SSZ schemas diverge across the Gloas boundary in ways that leave fields nil. Gloas removed `ExecutionPayload`, `BlobKzgCommitments` and `ExecutionRequests` from `BeaconBody`, and `SignedBlockHeader`, `KzgCommitments` and `KzgCommitmentsInclusionProof` from `DataColumnSidecar`.

Any consumer that re-derives the fork from the object's own slot, rather than from the `Version()` it was decoded with, can therefore be steered into the branch that reads a field the decoded schema never populated. `solid.ListSSZ` has no nil-receiver guard, so those reads are nil dereferences rather than recoverable errors.

Two such sinks have been found and fixed per-site in #22797: the PeerDAS column downloader in `cl/das/peer_das.go`, and `OnBlock` in `cl/phase1/forkchoice/on_block.go`. Both fixes assert slot-fork and schema-fork agreement locally. This issue is about closing the class rather than continuing to patch instances — the next consumer added has no structural reason to get it right.

## Further sinks

Three more consumers make the same double inference, and none of them is fixed. None is reachable from the `DataColumnSidecarsByRoot` PoC, and none would be caught by a check in `parseResponseData` — which is itself part of the argument for closing the class rather than hardening the response path alone.

**`historical_states_reader.go` reads a payload header off a slot-derived version.** `ReadHistoricalState` sets the state version with `ret.SetVersion(slotData.Version)`, and `ReadSlotData` (`cl/persistence/state/state_accessors.go`) derives that as `GetCurrentStateVersion(slot / SlotsPerEpoch)`. The block alongside it comes from `blockReader.ReadBlockBySlot` and carries its own stored schema. The `ret.Version() < GloasVersion` branch then dereferences `block.Block.Body.ExecutionPayload.PayloadHeader()` with no nil check. This is now the only instance of the class with no guard whatsoever; it needs the archive reader and a canonical block, so it is not reachable from the network path.

**`httpConsensusVersion` defaults to Gloas.** In `cl/phase1/network/beacon_downloader.go` it maps the `Eth-Consensus-Version` response header to a `StateVersion` and falls through to `return clparams.GloasVersion` for anything it does not recognise, an absent header included. Both `fetchBlocksFromBeaconAPI` and `fetchBlockFromBeaconAPIByRoot` (`cl/phase1/network/backward_beacon_downloader.go`) decode with whatever it returns, so a checkpoint-sync endpoint that omits or misspells the header has its blocks decoded with the Gloas schema on a pre-Gloas network. Same mismatch, over HTTP rather than libp2p. An unparseable header should be an error rather than the newest fork.

**`processFuluMessage` writes to column storage unchecked.** `cl/phase1/network/services/data_column_sidecar_service.go` dispatches on `msg.Version()`, which is right, but the Fulu branch never compares the fork implied by the header slot against that schema before `WriteColumnSidecars`. `processGloasMessage` does bind the sidecar to a block already in fork choice (`if slot != block.Block.Slot`); the Fulu branch has no equivalent. Gossip decodes with the local clock rather than a peer-supplied digest, so this is a fork-boundary window rather than something a peer selects at will, but it is the last unchecked write into the column store.

## Possible directions

**Refusing digests for forks that are not yet activated is the strongest single lever.** On mainnet `GloasForkEpoch` is `FAR_FUTURE_EPOCH`, so a digest for Gloas resolving to `GloasVersion` is by definition unusable — no slot maps to that fork. Rejecting it in `parseResponseData` would make the Gloas decoder unselectable and close this class outright, rather than sink by sink. The per-site checks in #22797 would then be defence in depth for post-Gloas networks instead of the primary defence.

The check has to key on the fork's activation against the object's slot, not against the wall-clock epoch: historical responses from before the current fork must still decode.

**A slot-range cross-check does not generalise, and is the weaker option.** It works for `BeaconBlocksByRange` and `DataColumnSidecarsByRange`, where the request carries slots, but not for the by-root protocols — `DataColumnSidecarsByRoot` and `BeaconBlocksByRoot` carry block roots and no slot information at all, so `parseResponseData` has nothing to compare a digest against. Worth doing where it applies (`capAtForkBoundary` in `cl/phase1/network/beacon_downloader.go` already caps *requests* at fork boundaries and the response is never checked against that), but it cannot be the class-level fix.

**Making the types enforce it is complementary.** Version-aware accessors on `DataColumnSidecar` and `BeaconBody` that return an error rather than exposing a field the active schema leaves unset would stop a caller reaching one by accident, whichever fork is active. `DataColumnSidecar` already has three separate places deriving a slot from `Version()` with three different nil policies (`cl/das/peer_das.go`, and `WriteColumnSidecars` / `ReadColumnSidecarByColumnIndex` in `cl/persistence/blob_storage/data_column_db.go`), so an accessor there would consolidate existing duplication as well.

## Note

`cl/sentinel/communication/ssz_snappy/encoding.go` carries a TODO to assert that the digest matches the expected configuration, but `DecodeAndRead` and `DecodeListSSZ` there have no non-test callers. The live response path is `parseResponseData`, so that is where a central check belongs.

Inbound **request** decoding is not affected: handlers use `DecodeAndReadNoForkDigest` with a locally derived version.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.