Conversation
…d packets GREv0Layer::isDataValid() and GREv1Layer::isDataValid() only checked the data length against the fixed base header size (sizeof(gre_basic_header) for v0, sizeof(gre1_header) for v1), regardless of which optional fields (checksum/routing, key, sequence number, ack sequence number) the packet's bit flags claim are present. GreLayer::getFieldValue() computes each optional field's offset purely from those bit flags with no bounds check against the actual buffer length, so a truncated GRE packet with an optional bit set but not enough trailing bytes causes getChecksum()/getKey()/getSequenceNumber()/ getAcknowledgmentNum()/getOffset() to read past the end of the buffer. This is reachable via ordinary packet parsing: IPv4Layer::parseNextLayer() dispatches protocol-47 payloads into tryConstructNextLayerWithFallback<GREv0Layer/GREv1Layer, PayloadLayer>, which only gates construction on isDataValid(), so a short/truncated capture (or a crafted packet) is enough to trigger the out-of-bounds read once a caller reads one of the optional fields. Fix both isDataValid() implementations to sum the required length from the same optional-field bits that GreLayer::getHeaderLen() already uses, so truncated packets are rejected up front (falling back to PayloadLayer) instead of allowing a GreLayer to be constructed over a buffer too short for the fields it claims to contain.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #2189 +/- ##
==========================================
- Coverage 82.66% 82.66% -0.01%
==========================================
Files 332 332
Lines 60155 60169 +14
Branches 12768 12547 -221
==========================================
+ Hits 49727 49736 +9
- Misses 9019 9023 +4
- Partials 1409 1410 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
seladb
reviewed
Aug 2, 2026
| // end of the buffer | ||
| auto* header = reinterpret_cast<const gre_basic_header*>(data); | ||
| size_t requiredLen = sizeof(gre_basic_header); | ||
| if (header->checksumBit == 1 || header->routingBit == 1) |
Owner
There was a problem hiding this comment.
nit: I think the checks with == 1 could just be: if header->checksumBit || header->routingBit etc.
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
2 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.
Problem
GREv0Layer::isDataValid()andGREv1Layer::isDataValid()only check thedata length against the fixed base header size (
sizeof(gre_basic_header)for v0,
sizeof(gre1_header)for v1), regardless of which optional fields(checksum/routing, key, sequence number, ack sequence number) the packet's
own bit flags claim are present.
GreLayer::getFieldValue()computes each optional field's offset purelyfrom those bit flags, with no check against the actual buffer length. So a
truncated GRE packet that has an optional bit set but not enough trailing
bytes causes
getChecksum()/getKey()/getSequenceNumber()/getAcknowledgmentNum()/getOffset()to read past the end of thebuffer.
This is reachable through ordinary packet parsing, not just direct API
misuse:
IPv4Layer::parseNextLayer()dispatches protocol-47 payloads intotryConstructNextLayerWithFallback<GREv0Layer/GREv1Layer, PayloadLayer>,which gates construction solely on
isDataValid(). A short/truncatedcapture (or a crafted packet) is enough to get a
GreLayerconstructedover a buffer too small for the fields it claims to contain, and the next
call to one of the getters above reads out of bounds.
I verified this empirically (VirtualAlloc guard page + AddVectoredExceptionHandler,
since ASan isn't available in this toolchain) by building the real,
unmodified
Packet++library and:GREv0Layer::isDataValid()accepts a 4-byte buffer withchecksumBitset, and that the subsequentgetChecksum()/getKey()call reads past the end of the allocation
GREv1Layer::isDataValid()accepts an 8-byte buffer withkeyBitandsequenceNumBitboth set even though the sequence-numberfield it computes sits one byte past the 8-byte allocation
IPv4Layer::parseNextLayer()→tryConstructNextLayerWithFallbackchain constructs an (unsafe)
GREv0Layerinstead of falling back toPayloadLayerFix
Both
isDataValid()implementations now sum the required buffer lengthfrom the same optional-field bits that
GreLayer::getHeaderLen()alreadyuses, so a truncated packet is correctly rejected (falling back to
PayloadLayerduring parsing) instead of allowing aGreLayerto beconstructed over an undersized buffer.
After the fix, the same harness confirms:
GREv0Layer::isDataValid()now rejects the 4-byte/checksumBit-set caseGREv1Layer::isDataValid()now rejects the 8-byte/keyBit+sequenceNumBit-set caseGREv0Layer(falls back toPayloadLayer)present at its correct offset) still parse correctly and all getters
still return the correct values, so this doesn't regress the normal
parsing path
Only
Packet++/header/GreLayer.his touched; no behavior changes forcorrectly-sized packets.