fix(l1): cap snappy-decompressed length before allocating

[ElFantasma]

Motivation

The RLPx codec caps the compressed frame size (MAX_MESSAGE_SIZE = 0xFFFFFF), but snappy_decompress places no bound on the decompressed length. A snappy stream begins with a peer-controlled varint of the decompressed length, and snap's decompress_vec allocates a buffer of that size before validating the body — so a tiny frame can declare a very large length (up to ~4 GiB) and force a correspondingly large allocation. go-ethereum bounds snappy.DecodedLen against the message-size cap before allocating; ethrex did not.

Description

snappy_decompress now reads the declared decompressed length via snap::raw::decompress_len and rejects it if it exceeds MAX_SNAPPY_DECOMPRESSED_LEN (0xFFFFFF, matching the compressed-frame cap and geth's maxUint24 bound) before allocating. Normal messages are unaffected.

Adds unit tests: a crafted oversized declared-length header is rejected up front, and a normal round-trip still succeeds.

Checklist

• cargo test -p ethrex-p2p (snappy tests) and cargo clippy -p ethrex-p2p pass.

[github-actions]

⚠️ Known Issues — intentionally skipped tests

Source: docs/known_issues.md

Stateless blockchain EF-tests (zkevm bundle) skipped under Amsterdam v6.1.0

make -C tooling/ef_tests/blockchain test-stateless runs against the
tests-zkevm@v0.4.1 fixture bundle — currently the only published zkevm test
release. Those fixtures were filled against an older glamsterdam devnet and
re-execute every case under the for_amsterdam fork, so they lag the
glamsterdam-devnet v6.1.0 gas accounting this client now implements
(EIP-8037 / EIP-8038 / EIP-2780 / EIP-7976 / EIP-7981 …). Re-executing them
yields ~2790/2864 stale-gas failures ("Transaction execution unexpectedly
failed"), spread pervasively across every fork and through the EIP-8025 proof
suite, so there is no clean passing subset to keep.

Until a v6.1.0-aligned zkevm bundle is published, the entire bundle is skipped
for the stateless run via the fork_Amsterdam entry in the stateless-only
EXTRA_SKIPS (tooling/ef_tests/blockchain/tests/all.rs) — every test in this
Amsterdam-only bundle carries the [fork_Amsterdam-…] parametrization in its
test key. The skip is #[cfg(feature = "stateless")], so it does not touch
the non-stateless test-levm run. Coverage of these EIPs is retained by
test-levm, the engine EF-tests, and the state EF-tests, all of which execute
against the live v6.1.0 fixtures and pass.

Re-enable by removing the "fork_Amsterdam" skip once .fixtures_url_zkevm
points at a zkevm bundle filled for glamsterdam-devnet v6.1.0.

[github-actions]

🤖 Kimi Code Review

This is a solid security fix addressing a DoS vulnerability in RLPx snappy decompression. The implementation correctly prevents memory exhaustion attacks where a malicious peer declares an oversized decompressed length in a small compressed frame.

Summary of changes:

• Security: Adds a pre-allocation check using decompress_len to validate the declared decompressed size against the 16 MiB RLPx frame limit (0xFFFFFF) before decompress_vec allocates memory.
• Testing: Includes targeted tests for both the rejection of oversized declarations and normal roundtrip functionality.

Minor suggestions:

1. Test assertion fragility (line 117-120): The test checks for substring matches "exceed" or "too large" in the error message. This is slightly fragile if the underlying snap crate changes its error messages. Consider asserting against the specific error variant if RLPDecodeError supports it, or at least document that this relies on the current error format:

   // Consider: assert!(matches!(err, RLPDecodeError::InvalidCompression(_)));
   // or verify the specific constant message format

2. LEB128 comment (line 108): The manual LEB128 encoding in the test is correct but could use a brief comment explaining the | 0x80 continuation bit logic for future readers:

   // Encode 100 MiB as LEB128 varint (continuation bit 0x80 set on all but last byte)

3. Inclusive bound check: The code correctly uses > (not >=) for the limit check, allowing the exact maximum size (16,777,215 bytes), which aligns with the RLPx spec where MAX_MESSAGE_SIZE is inclusive.

Verdict: LGTM. The fix correctly addresses the allocation-of-doom vulnerability (CVE-2023-XXX style attack vector) and maintains backward compatibility for valid frames.

---

Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt

[github-actions]

🤖 Codex Code Review

Findings

No findings.

The change in utils.rs looks correct: it bounds the peer-controlled Snappy decoded length before decompress_vec allocates, which closes the decompression-bomb/forced-allocation path without changing normal message decoding. I also checked the only special-case caller, p2p.rs, and the DisconnectMessage compressed-or-raw fallback does not reintroduce the memory issue because the fallback path only reuses the received frame bytes.

I could not run the new tests locally: cargo test failed in this environment because rustup attempted to write under /home/runner/.rustup, which is read-only here.

---

Automated review by OpenAI Codex · gpt-5.4 · custom prompt

[github-actions]

Lines of code report

Total lines added: 39
Total lines removed: 0
Total lines changed: 39

Detailed view

+--------------------------------------------+-------+------+
| File                                       | Lines | Diff |
+--------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/rlpx/utils.rs | 104   | +39  |
+--------------------------------------------+-------+------+

[greptile-apps]

Greptile Summary

This PR caps the declared decompressed length in snappy_decompress before allocating, closing a denial-of-service vector where a malicious peer could send a tiny compressed frame with a 4 GiB declared length to force a huge heap allocation. The fix mirrors go-ethereum's maxUint24 guard.

• Adds decompress_len check against MAX_SNAPPY_DECOMPRESSED_LEN (0xFFFFFF) before calling decompress_vec, so no allocation happens for over-limit declared lengths.
• Adds two unit tests: one crafts an oversized varint header and verifies rejection, and one validates a normal round-trip still works.

Confidence Score: 5/5

Safe to merge — the guard is placed correctly before any allocation, the constant matches go-ethereum's bound, and both the rejection path and the normal round-trip are tested.

The change is a single, focused guard in one function. The decompress_len call is correctly sequenced before decompress_vec, the > comparison matches go-ethereum's decodedLen > maxUint24 semantics exactly, and the error variant's Display impl ensures the test assertion on 'exceed' fires against the right message. No existing callers are affected.

No files require special attention.

Important Files Changed

Filename	Overview
crates/networking/p2p/rlpx/utils.rs	Adds a declared-length guard to snappy_decompress using snap::raw::decompress_len before allocation, with a matching constant and two unit tests covering the rejection and round-trip cases.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[snappy_decompress called\nwith peer msg_data] --> B[decompress_len reads\nvarint from header]
    B --> C{declared_len >\nMAX_SNAPPY_DECOMPRESSED_LEN\n0xFFFFFF}
    C -->|Yes – reject before alloc| D[Return Err\nInvalidCompression\n'exceeds maximum']
    C -->|No – safe size| E[SnappyDecoder::new\ndecompress_vec allocates\ndeclared_len buffer]
    E --> F{decompress\nsucceeded?}
    F -->|Yes| G[Return Ok Vec of\ndecompressed bytes]
    F -->|No| H[Return Err\nInvalidCompression]

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[snappy_decompress called\nwith peer msg_data] --> B[decompress_len reads\nvarint from header]
    B --> C{declared_len >\nMAX_SNAPPY_DECOMPRESSED_LEN\n0xFFFFFF}
    C -->|Yes – reject before alloc| D[Return Err\nInvalidCompression\n'exceeds maximum']
    C -->|No – safe size| E[SnappyDecoder::new\ndecompress_vec allocates\ndeclared_len buffer]
    E --> F{decompress\nsucceeded?}
    F -->|Yes| G[Return Ok Vec of\ndecompressed bytes]
    F -->|No| H[Return Err\nInvalidCompression]

Loading

_{Reviews (1): Last reviewed commit: "fix(l1): cap snappy-decompressed length ..." | Re-trigger Greptile}

[github-actions]

🤖 Claude Code Review

snap 1.1.1 does export decompress_len under snap::raw, so the import is valid. This is a solid, well-scoped fix. Here's my review:

Review: fix(l1): cap snappy-decompressed length before allocating

Summary: This closes a real DoS vector — snap::raw::Decoder::decompress_vec allocates a buffer sized from the peer-controlled declared length before validating the compressed body, so a tiny malicious frame could force a ~4 GiB allocation. The fix calls decompress_len first and rejects anything over 0xFFFFFF (matching both the existing MAX_MESSAGE_SIZE frame cap in codec.rs and geth's maxUint24 bound in rlpx.go), which is the same mitigation upstream go-ethereum applies. Good catch and correct fix location — it's the single choke point used by every RLPx message decoder (eth/*, snap/*, l2/messages.rs, p2p.rs), so all callers get the protection for free.

Correctness:

• The 16 MiB bound intentionally mirrors the compressed-frame cap, which matches the real devp2p/geth behavior (actualSize > maxUint24 check in p2p/rlpx/rlpx.go), so this shouldn't reject legitimately-sized protocol messages.
• The fallback-decompression path in p2p.rs:316 (DisconnectMessage::decode, which treats decompression failure as "message wasn't compressed") is unaffected in behavior — an oversized declared length just becomes another reason to fall back to raw bytes, which is safe.
• decompress_len only parses the leading varint and doesn't allocate, so the ordering (check-then-allocate) is correct.
• The LEB128 varint construction in the test is correct and does exercise a value above the cap.

Minor / non-blocking:

• MAX_MESSAGE_SIZE is redefined as a second constant (MAX_SNAPPY_DECOMPRESSED_LEN in utils.rs:78) rather than reusing the one in connection/codec.rs:26. It's currently private (const, not pub(crate)), so duplication was likely the path of least resistance — but it does mean the two values could silently drift if one is ever changed. Consider making the original pub(crate) and importing it instead, or moving both to a shared constants module.
• Reusing RLPDecodeError::InvalidCompression for the new "declared length exceeds cap" case is reasonable given it's the closest existing variant, but it does mean callers/logs can't distinguish "corrupt snappy stream" from "oversized declared length" (e.g., for peer-scoring/banning heuristics that might want to treat the latter more severely). Not a blocker — just worth knowing if peer reputation logic is ever built on error variants.
• The doc comments on snappy_decompress and the test module repeat the same explanation about allocate-before-validate three times (function doc, inline comment, test doc). Not wrong, just slightly redundant — could be trimmed to one place.

Overall: correct, well-targeted security fix with good test coverage (oversized-header rejection + a round-trip regression check). No consensus, gas-accounting, or RLP-encoding implications since this sits purely in the transport/compression layer.

---

Automated review by Claude (Anthropic) · sonnet · custom prompt

[ElFantasma]

Thanks — all non-blocking; notes on each:

• Constant duplication (MAX_SNAPPY_DECOMPRESSED_LEN vs MAX_MESSAGE_SIZE): kept as a separate named constant deliberately — it's a conceptually distinct limit (decompressed size vs the compressed-frame cap in codec.rs) that happens to share geth's maxUint24 value; the doc comment records that linkage. Reusing the private codec const would couple two independent limits across modules. Can hoist both into a shared constants module if the team prefers a single source of truth.
• 16 MiB doesn't reject legit messages: confirmed — geth likewise bounds snappy.DecodedLen at maxUint24, so a compliant peer never sends a >16 MiB decompressed RLPx message.
• Test assertion / LEB128: the substring match is intentional (the snap crate's error text isn't a stable API and there's no dedicated variant to match); the round-trip test is the positive control.
• Error variant: reused InvalidCompression as the closest existing variant; if peer-scoring ever needs to distinguish "oversized declared length" from "corrupt stream," a dedicated variant can be split out then.