Security/rtr oob and dos fixes - #308
Merged
Merged
Conversation
Security review focused on out-of-bounds reads in the TLV-based protocol parsers surfaced several issues, fixed here: - rtr: validate error_text_len against the PDU before slicing in ErrorReport parsing (was a confirmed OOB slice panic reachable from a malicious/compromised RTR cache). - rtr: reject PDUs larger than the protocol maximum (65,535 octets, the ASPA PDU bound per RFC 8210-bis §5.12) before allocating in read_rtr_pdu, preventing a ~4 GiB allocation from a crafted header. - mrt/bgp4mp: use ip_size=0 for LinkState (read_address consumes 0 bytes) and saturating_sub so a truncated record fails the existing remaining() check instead of underflowing. - flowspec/nlri: reject prefix_len > 128 before copying into the fixed 16-byte buffer (latent OOB copy panic; not yet on the dispatch path). - bmp/openbmp: consume the exact declared admin-id length instead of silently clamping to 255, avoiding field desync. - utils: check has_n_remaining before Vec::with_capacity in read_asns and use checked_mul to avoid 32-bit overflow. Adds regression tests for the RTR and flowspec fixes. Assisted-by: Claude Code
Covers both parse_rtr_pdu (slice-based) and read_rtr_pdu (reader-based) so the OOB-slice and unbounded-allocation classes fixed in the prior commit are exercised in CI. RTR was previously the only parser without a fuzz target. Assisted-by: Claude Code
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #308 +/- ##
==========================================
+ Coverage 90.03% 90.07% +0.04%
==========================================
Files 91 91
Lines 18758 19136 +378
==========================================
+ Hits 16888 17236 +348
- Misses 1870 1900 +30 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens several parsers against out-of-bounds reads and memory-exhaustion conditions triggered by crafted length fields, and adds regression tests/fuzzing to guard against panics in those paths.
Changes:
- Add an RTR PDU length cap in the reader-based RTR parser and add regression tests for oversized length / error-text length handling.
- Prevent allocation/underflow issues in various length-driven parsing helpers (ASN list reading, BGP4MP LinkState payload sizing, OpenBMP admin-id length consumption).
- Add an RPKI RTR fuzz target and document how to run it.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/fuzz/README.md | Documents running the new RTR fuzzer. |
| tests/fuzz/fuzz_targets/fuzz_rtr.rs | Adds a fuzz target covering slice-based and reader-based RTR parsing. |
| tests/fuzz/Cargo.toml | Registers the new fuzz_rtr target. |
| src/parser/utils.rs | Avoids pre-allocation/overflow in read_asns by checking needed bytes first. |
| src/parser/rpki/rtr.rs | Adds max PDU length guard, adds ErrorReport length validation + regression tests. |
| src/parser/mrt/messages/bgp4mp.rs | Fixes LinkState payload length computation and prevents underflow via saturating subtracts. |
| src/parser/bmp/openbmp.rs | Consumes declared OpenBMP admin-id length instead of clamping. |
| src/models/bgp/flowspec/nlri.rs | Rejects oversized prefix lengths to prevent IPv6 buffer over-copy; adds regression test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+441
to
+445
| // Validate the declared error text actually fits within the PDU | ||
| // before slicing. `length` is a wire-controlled u32, and | ||
| // `error_text_len` is independent of it, so an oversized value | ||
| // would otherwise index past the buffer and panic. | ||
| if error_text_offset + error_text_len > length_usize { |
…e PDU cap for Error Reports - Compare wire-controlled lengths by subtraction instead of addition in the ErrorReport encap/text bounds checks, so they cannot overflow usize on 32-bit targets; saturate the reported 'expected' value instead of truncating it. - Cap read_rtr_pdu allocations at RTR_MAX_ERROR_REPORT_LEN (~128 KiB): an Error Report carrying a copy of a maximum-size PDU plus diagnostic text can legitimately exceed the 65,535-octet non-error PDU maximum. - Add a regression test for a >64 KiB but valid Error Report. Assisted-by: Claude Code
digizeph
added a commit
that referenced
this pull request
Jul 27, 2026
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.
Very much AI assisted changes. Review and fixes by Opus 4.8. The issues are known weak spots in TLV parsers, and the changes look logical (I do not know LinkState/BMP well enough to judge those though).