Guard against out-of-bounds indexing on attacker-controlled attestation data#228
Guard against out-of-bounds indexing on attacker-controlled attestation data#228pablodeymo merged 3 commits intomainfrom
Conversation
…ttestation processing Three defensive fixes in process_attestations / try_finalize: 1. aggregation_bits OOB (finding #6): A malicious attestation could carry aggregation_bits longer than the validator set, causing votes[validator_id] to panic on out-of-bounds access. Now filter bits >= validator_count before indexing. 2. root_to_slot missing key (finding #7): try_finalize used direct HashMap index root_to_slot[root] which panics if a justification root is absent from historical_block_hashes (e.g. carried over from a prior finalization window). Replaced with .get() — missing roots are conservatively retained. 3. justifications[root] (finding #8): Reviewed and confirmed safe — the roots come from justifications.keys() so the key is always present. No change needed.
🤖 Kimi Code ReviewSecurity & Correctness Review1. Out-of-bounds panic prevention (lines 287-295)The fix for 2. Missing root handling (lines 421-428)The change from direct indexing to 3. Minor optimization opportunityIn the attestation processing loop (lines 290-298), consider using for (validator_id, voted) in attestation
.aggregation_bits
.iter()
.zip(0..validator_count)
.filter(|(voted, _)| **voted)
{
votes[validator_id] = true;
}However, the current fix is safer as it handles potentially malformed bitfields more defensively. 4. Consensus correctnessBoth changes are consensus-preserving:
SummaryThe PR correctly fixes two potential panic conditions that could be triggered by malicious network participants. The changes are minimal, focused, and maintain consensus correctness. Automated review by Kimi (Moonshot AI) · custom prompt |
🤖 Codex Code Review
No critical security or memory-safety regressions found in this diff; both changes are defensive and remove panic paths. I could not run tests in this environment due rustup temp-dir permission errors ( Automated review by OpenAI Codex · custom prompt |
🤖 Claude Code ReviewHere is my review of PR #228: PR Review: Guard against OOB panics on attacker-controlled attestation dataThe PR is a focused security patch for two crash vulnerabilities. The analysis and fixes are correct. A few points follow. Fix 1 —
|
…cate The bounds guard is now at the point of access rather than in the iterator filter. get_mut returns None for out-of-bounds indices, making the safety invariant self-contained in a single expression.
Greptile SummaryThis PR addresses three security-audit findings (6, 7, 8) in
Key concern: the conservative Confidence Score: 3/5
|
| Filename | Overview |
|---|---|
| crates/blockchain/state_transition/src/lib.rs | Two OOB panic guards added: (1) aggregation_bits filter correctly bounds-checks validator index against validator_count; (2) root_to_slot lookup switched to .get().is_none_or(...) to avoid missing-key panic, but the None branch conservatively retains stale entries that are already past the finalization boundary and will never be pruned, risking indefinite accumulation in state.justifications_roots. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[process_attestations] --> B{ZeroHashInJustificationRoots?}
B -- yes --> C[Return Err]
B -- no --> D[Build root_to_slot from\nhistorical_block_hashes\n after latest_finalized.slot]
D --> E[For each attestation]
E --> F{is_valid_vote?}
F -- no --> E
F -- yes --> G[entry target.root in justifications]
G --> H[Filter aggregation_bits\n *voted AND id < validator_count\n Fix 1: OOB guard]
H --> I[votes validator_id = true]
I --> J{3 × vote_count\n ≥ 2 × validator_count?}
J -- no --> E
J -- yes --> K[latest_justified = target\nremove target.root from justifications]
K --> L[try_finalize]
L --> M{Gap between source\nand target justifiable?}
M -- yes --> N[metrics::inc finalizations error\nreturn]
M -- no --> O[Advance latest_finalized = source\nshift justified_slots window]
O --> P[justifications.retain\nroot_to_slot.get root .is_none_or\n Fix 2: missing-key guard]
P --> Q{root in root_to_slot?}
Q -- None --> R[Conservatively retain\n stale entry never pruned]
Q -- Some slot > finalized --> S[Retain]
Q -- Some slot ≤ finalized --> T[Prune]
L --> E
E --> U[serialize_justifications\nstate.justifications_roots updated]
Prompt To Fix All 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.Last reviewed commit: 364eaa8
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| 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) |
There was a problem hiding this 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:
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.
| if let Some(vote) = votes.get_mut(validator_id) { | ||
| *vote = true; | ||
| } |
There was a problem hiding this comment.
We need to check what the spec does in this case
| root_to_slot | ||
| .get(root) | ||
| .is_none_or(|&slot| slot > state.latest_finalized.slot) |
…behavior Reject entire attestations when aggregation_bits exceeds validator count (matching spec crash, Zeam reject, Lantern reject) instead of silently skipping individual OOB bits. Prune justification roots missing from root_to_slot (their slot is at or below finalized_slot) instead of conservatively retaining them forever.
|
@greptile-apps wdyt? |
|
The approach looks solid. Both fixes are clean and well-reasoned: Fix 1 (aggregation_bits): The pre-loop length check is the right move. Simple, catches the issue early, and aligns with Zeam/Lantern's reject behavior. Fix 2 (justification roots): Using One consideration: The warnings could get noisy if someone floods malicious attestations. Not a blocker, but worth thinking about whether you want any rate limiting on these log messages or if you're comfortable with the spam during an active attack. Could also consider downgrading to The cross-client comparison table is really helpful context. Nice work on the thorough analysis. |
| .entry(target.root) | ||
| .or_insert_with(|| std::iter::repeat_n(false, validator_count).collect()); | ||
| // Reject attestations with aggregation_bits longer than the validator set. | ||
| // The spec would crash (IndexError) on OOB access; Zeam and Lantern reject. |
There was a problem hiding this comment.
An exception in the spec means a rejection. That's why it uses assert
2 participants
Motivation
Security audit findings #6, #7, #8: three sites in
process_attestations/try_finalizeuse direct indexing on data structures whose keys or indices come from network-controlled attestations. A malicious peer can craft attestations that trigger panics, crashing the node.Description
Fix 1 — Reject attestations with OOB
aggregation_bits(finding #6)File:
crates/blockchain/state_transition/src/lib.rsProblem:
aggregation_bitsis aBitList<ValidatorRegistryLimit>(max 4096 bits) deserialized from the network.votesis aVec<bool>of lengthvalidator_count(actual validators in state). If a malicious attestation carriesaggregation_bitswith bits set beyondvalidator_count, direct indexing panics.Spec and cross-client behavior:
justifications[target.root][validator_id]— crashes on OOB (IndexError)return -1Bitlist.Get()returns false for OOB (silent skip)Fix: Pre-loop length check that rejects the entire attestation when
aggregation_bits.len() > validator_count, matching the spec (crash = implicit reject) and the majority of other clients (Zeam, Lantern). Direct indexingvotes[validator_id] = trueis safe after the bounds check.Fix 2 — Prune justification roots missing from
root_to_slot(finding #7)File:
crates/blockchain/state_transition/src/lib.rs(intry_finalize)Problem:
root_to_slotis built fromhistorical_block_hashesfor slots after finalization. Butjustificationscan contain roots carried over from a previous finalization window that no longer appear inhistorical_block_hashes. Direct HashMap index panics on missing key.Spec and cross-client behavior:
assert all(root in root_to_slot for root in justifications)— crashes (invariant violation)The spec treats missing roots as an invariant violation. A root absent from
root_to_slotmeans its slot is at or belowfinalized_slot(already finalized). Conservatively retaining such roots (previous approach withis_none_or) would cause them to accumulate forever — they never reappear inroot_to_slot, andis_valid_voterejects new votes for them.Fix: Prune missing roots (return
falseinretain), matching the spec's filter intent:root_to_slot[root] > finalized_slotwould befalsefor such roots. A warning is logged when this happens.Finding #8 —
justifications[root](no change needed)File:
crates/blockchain/state_transition/src/lib.rs(inserialize_justifications)Reviewed and confirmed safe:
justification_rootsis built fromjustifications.keys(), so every root used as index is guaranteed to be present in the map. No change needed.How to Test
make fmt make lint cargo test --workspace --releaseAll 120 tests pass unchanged — valid attestations always have
aggregation_bits.len() <= validator_countand all justification roots are inroot_to_slot, so these guards only trigger on malformed/adversarial data not present in test fixtures.