Lack of range checks in chain operations processing

[mkalinin]

Problem

There is no range checks for shard and validator_index during processing of beacon chain operations. Operation data is an inbound network data and any possible violations in this data should be strictly verified before processing it.

IMO, it's better to check ranges explicitly rather than relay on client implementation or a standard behavior that takes place when out of bound index is met.

[JustinDrake]

Can you be more specific where range checks should be done? Are you referring for example to state.validator_registry[proposer_slashing.proposer_index]? I guess out-of-bounds accesses are considered invalid, and we could make that explicit in the spec.

[mkalinin]

Here is a list of potential out-of-bounds accesses that I have discovered:

Proposer slashings

• Let proposer = state.validator_registry[proposer_slashing.proposer_index]

Casper slashings

• Verify that verify_slashable_vote_data(state, slashable_vote_data_1).

In verify_slashable_vote_data there is a call aggregate_pubkey([state.validators[i].pubkey for i in vote_data.custody_bit_0_indices]) which uses validator indices from slashable_vote_data_1.

Attestations

• Verify that either attestation.data.latest_crosslink_root or attestation.data.shard_block_root equals state.latest_crosslinks[shard].shard_block_root.

Looks like shard is a part of attestation_data.

Exits

• Let validator = state.validator_registry[exit.validator_index].

I think, general strategy of avoiding any out-of-bounds accesses from inbound data would be to check all index-like numbers against their ranges.

[JustinDrake]

I believe all your examples are out-of-range list accesses:

• proposer = state.validator_registry[proposer_slashing.proposer_index]
• validator = state.validator_registry[exit.validator_index]
• state.validators[i].pubkey for i in vote_data.custody_bit_0_indices
• latest_crosslinks[shard]

Those are handled by Python which throws an IndexError: list index out of range error. Are you arguing we should, for example, check shard < len(latest_crosslinks) before doing latest_crosslinks[shard]?

[mkalinin]

I think that main concern is that these accesses are not spec'd. I think that a note about potential IndexError: list index out of range would, also, work.

[JustinDrake]

these accesses are not spec'd

See "code appearing in this style is to be interpreted as an algorithm defined in Python".

[mkalinin]

@JustinDrake would something like this work for you? mkalinin#1

[JustinDrake]

Feel free to submit a pull request for this clarification and I'll merge it in 👍