fix(grey): bound count in decode_guarantee to prevent OOM on malformed gossip#754
Open
nianchen1231-netizen wants to merge 1 commit into
Open
Conversation
Contributor
Genesis ReviewComparison targets:
How to reviewPost a comment with the following format (rank from best to worst): Use the short commit hashes above and To meta-review another reviewer's comment, react with 👍 or 👎. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix(grey): bound count in decode_guarantee to prevent OOM on malformed gossip
Problem
grey::guarantor::decode_guaranteereads a 16-bit credential count fromuntrusted gossip bytes and pre-allocates a
Vecof that size before theper-iteration bounds check runs:
Each credential entry is
(u16, Ed25519Signature)= 66 bytes on the wire,so a peer broadcasting a 38-byte message with
cred_count = 0xFFFFforcesevery receiver subscribed to the
guaranteesgossipsub topic to allocate~4.1 MiB before the loop's bounds check rejects the message and the
Vecis dropped. The decode runs before any signature verification — anyone
on the network can do this.
This is the same class of bug fixed by:
decode_state_kvsdecode_preimage_info_timeslotsReachability
handle_received_guarantee(same file, line 326) is the gossipsub handlerfor the
guaranteestopic. Decode happens before any auth check, so theattacker is the network — no validator key required.
Fix
Bound
cred_countby the bytes remaining indatabefore any allocation:Each credential is exactly 66 bytes on the wire (2-byte validator index +
64-byte Ed25519 signature, per the format docstring on
decode_guarantee),so any
cred_countlarger than(remaining_bytes) / 66is necessarilymalformed and can be rejected before allocation.
Reproducer (peak-allocation measurement)
A standalone reproducer that vendors the decode logic with and without the
fix, run with
cargo run --releaseon macOS (Apple Silicon):Per-call peak allocation: 4.12 MiB → 0 bytes for the same input that
both implementations correctly reject. Reproducer source available on
request.
Tests
cargo test -p grey decode_guarantee --release:The existing
decode_guarantee_never_panicsproptest generates randombytes 0..2048 in length, but uniform-random sampling effectively never
lands on the adversarial shape (a 38-byte input with
cred_count = 0xFFFF).The new
decode_guarantee_rejects_oversized_cred_counttest exercisesthat shape directly as a regression guard.
Future work (not in this PR)
A libfuzzer target
fuzz_guarantee_decodeis staged locally; it requiresadding
greyas a path dependency ingrey/fuzz/Cargo.toml. Happy tofold it into this PR or split it out — let me know preference.
Checklist
cargo fmt --check -p greycargo clippy -p grey --all-targets -- -D warningscargo test -p grey decode_guarantee --release