Skip to content

Guard against out-of-bounds indexing on attacker-controlled attestation data #228

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
pablodeymo wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Guard against out-of-bounds indexing on attacker-controlled attestation data #228

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d19d3d4
Guard against out-of-bounds indexing on attacker-controlled data in a…
pablodeymo Mar 13, 2026
364eaa8
Use get_mut for aggregation_bits bounds check instead of filter predi…
pablodeymo Mar 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions crates/blockchain/state_transition/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,13 +284,17 @@ fn process_attestations(
.entry(target.root)
.or_insert_with(|| std::iter::repeat_n(false, validator_count).collect());
// Mark that each validator in this aggregation has voted for the target.
// Use get_mut to safely skip bits beyond the validator set — a malicious
// attestation could carry aggregation_bits longer than validator_count.
for (validator_id, _) in attestation
.aggregation_bits
.iter()
.enumerate()
.filter(|(_, voted)| *voted)
{
votes[validator_id] = true;
if let Some(vote) = votes.get_mut(validator_id) {
*vote = true;
}
Comment on lines +295 to +297
Copy link
Collaborator

@MegaRedHand MegaRedHand Mar 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to check what the spec does in this case

}

// Check whether the vote count crosses the supermajority threshold
Expand Down Expand Up @@ -416,10 +420,14 @@ fn try_finalize(
let delta = (state.latest_finalized.slot - old_finalized_slot) as usize;
justified_slots_ops::shift_window(&mut state.justified_slots, delta);

// Prune justifications whose roots only appear at now-finalized slots
// Prune justifications whose roots only appear at now-finalized slots.
// Use .get() instead of direct index — a root may be absent from root_to_slot
// if it was never in historical_block_hashes (e.g. carried over from a previous
// finalization window). Missing roots are conservatively retained.
justifications.retain(|root, _| {
let slot = root_to_slot[root];
slot > state.latest_finalized.slot
root_to_slot
.get(root)
Comment on lines 421 to +429
Copy link
Contributor

@greptile-apps greptile-apps bot Mar 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale justification entries retained forever, not pruned naturally

The comment states retained entries "will be pruned naturally once their slot finalizes", but these entries are in the None branch precisely because their slot is already at or below state.latest_finalized.slot — their slot has already been finalized. They will never appear in any future root_to_slot map (which always starts at latest_finalized.slot + 1), so the retain predicate will continue to return true for them indefinitely.

The only other removal path is justifications.remove(&target.root) at line 319, which fires only when a root reaches supermajority justification. For these stale roots that's prevented: is_slot_justified returns true for finalized-or-earlier slots (unwrap_or(true) in justified_slots_ops), so is_valid_vote rejects new attestations targeting them. They effectively can never be removed.

The practical consequence is that every such stale entry accumulates permanently in state.justifications_roots and state.justifications_validators across all subsequent blocks. While bounded by HistoricalRootsLimit / JustificationValidators capacity, a sustained adversarial workload that repeatedly triggers this path could grow state without a cleanup mechanism. The expect("justifications_roots limit exceeded") in serialize_justifications is then the only backstop, and it terminates the node rather than pruning gracefully.

Consider replacing the conservation approach with an explicit prune: if the root's slot is known-and-finalized, remove it; only retain truly unknown roots. That would make the comment accurate and eliminate the accumulation concern:

justifications.retain(|root, _| {
    match root_to_slot.get(root) {
        // Root's slot is known and still above the finalization boundary → keep
        Some(&slot) => slot > state.latest_finalized.slot,
        // Root not in root_to_slot: already finalized or unknown → prune
        None => false,
    }
});

Alternatively, update the comment to accurately describe the behaviour: these entries are permanently retained and will never be pruned by this path.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: crates/blockchain/state_transition/src/lib.rs
Line: 421-429

Comment:
**Stale justification entries retained forever, not pruned naturally**

The comment states retained entries *"will be pruned naturally once their slot finalizes"*, but these entries are in the `None` branch precisely because their slot is **already at or below** `state.latest_finalized.slot` — their slot has already been finalized. They will never appear in any future `root_to_slot` map (which always starts at `latest_finalized.slot + 1`), so the `retain` predicate will continue to return `true` for them indefinitely.

The only other removal path is `justifications.remove(&target.root)` at line 319, which fires only when a root reaches supermajority justification. For these stale roots that's prevented: `is_slot_justified` returns `true` for finalized-or-earlier slots (`unwrap_or(true)` in `justified_slots_ops`), so `is_valid_vote` rejects new attestations targeting them. They effectively can never be removed.

The practical consequence is that every such stale entry accumulates permanently in `state.justifications_roots` and `state.justifications_validators` across all subsequent blocks. While bounded by `HistoricalRootsLimit` / `JustificationValidators` capacity, a sustained adversarial workload that repeatedly triggers this path could grow state without a cleanup mechanism. The `expect("justifications_roots limit exceeded")` in `serialize_justifications` is then the only backstop, and it terminates the node rather than pruning gracefully.

Consider replacing the conservation approach with an explicit prune: if the root's slot is known-and-finalized, remove it; only retain truly unknown roots. That would make the comment accurate and eliminate the accumulation concern:

```rust
justifications.retain(|root, _| {
    match root_to_slot.get(root) {
        // Root's slot is known and still above the finalization boundary → keep
        Some(&slot) => slot > state.latest_finalized.slot,
        // Root not in root_to_slot: already finalized or unknown → prune
        None => false,
    }
});
```

Alternatively, update the comment to accurately describe the behaviour: these entries are *permanently* retained and will never be pruned by this path.

How can I resolve this? If you propose a fix, please make it concise.

.is_none_or(|&slot| slot > state.latest_finalized.slot)
Comment on lines +428 to +430
Copy link
Collaborator

@MegaRedHand MegaRedHand Mar 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

});
}

Expand Down
Loading