Fix heap-buffer-overflow in BgpOpenMessageLayer::getOptionalParameters() (#1865)#2143
Merged
seladb merged 9 commits intoMay 27, 2026
Merged
Conversation
alacrity-aya
force-pushed
the
fix/bgp-open-optional-params-heap-overflow
branch
from
13b1eb8 to
c939086
Compare
…s() (seladb#1865) Multiple out-of-bounds read vulnerabilities existed when parsing malformed BGP OPEN messages with truncated or crafted optional parameters: - dataPtr[0]/dataPtr[1] (type/length fields) could be read past the buffer when fewer than 2 bytes remained in the optional parameters region - The individual parameter length check did not account for the 2-byte type+length header, allowing totalLen to exceed remaining bytes - No bounds check on getHeaderLen() vs sizeof(bgp_open_message) in getOptionalParameters(), getOptionalParametersLength(), and setOptionalParameters() Fixes: - Add early return when headerLen < sizeof(bgp_open_message) in all three methods - Check remaining < optParamHdrSize before reading type and length bytes - Fix totalLen check to include the 2-byte parameter header - Use std::min for cleaner clamping of optionalParamsLen - Zero-initialize optional_parameter default constructor via std::memset Tests: - Add BgpOpenMalformedOptionalParamsTest covering: - BGP OPEN with header shorter than bgp_open_message struct - Truncated optional parameter header (< 2 bytes remaining) - Individual parameter length exceeding available buffer - Partial parse: valid first param + truncated second param - optional_parameter default constructor zero-initialization
alacrity-aya
force-pushed
the
fix/bgp-open-optional-params-heap-overflow
branch
from
c939086 to
3d2cd21
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #2143 +/- ##
==========================================
+ Coverage 82.70% 82.72% +0.02%
==========================================
Files 332 332
Lines 59753 59800 +47
Branches 12288 12262 -26
==========================================
+ Hits 49418 49469 +51
- Misses 8932 8966 +34
+ Partials 1403 1365 -38
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
Author
|
The original PoC from #1865 triggers a separate heap-buffer-overflow in For more details, see TelnetLayer Heap-Buffer-Overflow |
Dimi1010
approved these changes
May 21, 2026
Dimi1010
left a comment
Dimi1010
left a comment
Collaborator
There was a problem hiding this comment.
LGTM. One minor comment.
Comment on lines
+158
to
+160
| { | ||
| std::memset(this, 0, sizeof(*this)); | ||
| } |
Collaborator
There was a problem hiding this comment.
nit: Maybe we can do it with default member initialization instead of memset?
Contributor
Author
There was a problem hiding this comment.
I'll update the code when I have some free time tomorrow
Contributor
Author
|
Updated as suggested, PTAL. @Dimi1010 |
seladb
reviewed
May 25, 2026
- Add static isDataValid() to BgpOpenMessageLayer, mirroring the pattern used by BgpUpdateMessageLayer. Rejects OPEN messages whose length field is smaller than sizeof(bgp_open_message). - Use isDataValid() in parseBgpLayer() so malformed OPEN messages are rejected at parse time instead of producing a half-formed layer. - Replace duplicated headerLen < sizeof(bgp_open_message) guards in getOptionalParameters(), getOptionalParametersLength(), and setOptionalParameters() with isDataValid(). - Add Doxygen comment to optional_parameter default constructor. - Add explanatory comment for (std::min) parentheses. - Update BgpOpenMalformedOptionalParamsTest: Test 1 now expects nullptr (layer rejected at parse time); use auto* for layer locals.
seladb
reviewed
May 26, 2026
Add pcap versions of newly added malformed BGP OPEN .dat fixtures so they can be inspected directly in Wireshark.
- Remove redundant OPEN layer validity guards in optional-parameter helpers - Keep optional-parameter parsing bounds checks unchanged - Keep <algorithm> include in BgpLayer.cpp and remove unnecessary header include
The early return for optionalParameterLength == 0 is unnecessary because the subsequent while loop naturally handles this case (loop condition byteCount < optionalParamsLen will be false when optionalParamsLen is 0).
seladb
approved these changes
May 27, 2026
seladb
left a comment
seladb
left a comment
Owner
There was a problem hiding this comment.
Please see 3 nit comments, otherwise LGTM
- Add braces around early return in isDataValid - Move standard library include after project includes - Replace individual malformed BGP OPEN pcap fixtures with one combined pcap
Owner
|
Thank you @alacrity-aya for working on this, much appreciated! 🙏 |
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
3 participants
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 heap-buffer-overflow in BgpOpenMessageLayer::getOptionalParameters()
Fixes #1865
Problem
Multiple heap-buffer-overflow vulnerabilities were detected via fuzzing in BgpOpenMessageLayer::getOptionalParameters() in BgpLayer.cpp. When parsing malformed or truncated BGP OPEN messages, the following out-of-bounds reads could occur:
dataPtr[0]/dataPtr[1]read OOB — The loop entered wheneverbyteCount < optionalParamsLen, but never verified that at least 2 bytes remained to safely read thetypeandlengthfields.Incorrect bounds check for individual parameter length — The old check
op.length > optionalParamsLen - byteCountdid not account for the 2-bytetype+lengthheader itself, so a parameter withop.length = 0could still causetotalLen = 2to pushdataPtrpast the end of the buffer.No guard on
headerLenvssizeof(bgp_open_message)— If the BGP message was too short to hold a full bgp_open_message struct, arithmetic likeheaderLen - sizeof(bgp_open_message)would underflow.The same lack of header-size guard also existed in getOptionalParametersLength() and setOptionalParameters().
Additionally, the optional_parameter default constructor had a
FIXMEnote indicating it did not actually zero-initialize its fields.Root Cause
Fix
headerLen < sizeof(bgp_open_message). Inside the loop, checkremaining < optParamHdrSize(2 bytes) before readingtype/length. Replace the length check withtotalLen > remainingwheretotalLen = optParamHdrSize + op.length.headerLen < sizeof(bgp_open_message). Refactored from nestedifto early-return style.falsewhenheaderLen < sizeof(bgp_open_message).std::memset(this, 0, sizeof(*this)), resolving the existingFIXME.Tests
Added
BgpOpenMalformedOptionalParamsTestin Tests/Packet++Test/Tests/BgpTests.cpp with 5 sub-cases, each backed by a dedicated.datfixture file:headerLen(25) <sizeof(bgp_open_message)(29)optionalParameterLength= 5, only 1 byte of data availableoptionalParameterLength= 5, first param claimslength= 16 (OOB)