Skip to content

Add boundary checks to prevent memory safety issues in BgpLayer::getHeaderLen #1954

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from
Open

Add boundary checks to prevent memory safety issues in BgpLayer::getHeaderLen #1954

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7a836de
Add boundary checks to prevent out-of-bounds access in BgpLayer::getH…
danasana Sep 10, 2025
8e73f57
Add poc files to regression_samples
danasana Sep 10, 2025
913e63c
Move common validation before branch point to avoid redundancy
danasana Sep 10, 2025
ae1ed33
Change C-style casts to static_cast
danasana Sep 10, 2025
4c839c7
Remove mistakenly added files
danasana Sep 10, 2025
936c316
Add check to ensure offsetInLayer is not negative in Layer::extend/sh…
danasana Sep 10, 2025
f27a0a6
Fix pre-commit hook failures
danasana Sep 11, 2025
075bb45
Merge branch 'dev' into bugfix/bgp-headerlen-memory-safety
Dimi1010 Sep 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 59 additions & 12 deletions Packet++/src/Layer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,14 +61,20 @@ namespace pcpp
return false;
}

if (m_Packet == nullptr)
if (offsetInLayer < 0)
{
PCPP_LOG_ERROR("Requested offset is negative");
return false;
}

if (static_cast<size_t>(offsetInLayer) > m_DataLen)
{
if ((size_t)offsetInLayer > m_DataLen)
{
PCPP_LOG_ERROR("Requested offset is larger than data length");
return false;
}
PCPP_LOG_ERROR("Requested offset is larger than data length");
return false;
}

if (m_Packet == nullptr)
{
uint8_t* newData = new uint8_t[m_DataLen + numOfBytesToExtend];
memcpy(newData, m_Data, offsetInLayer);
memcpy(newData + offsetInLayer + numOfBytesToExtend, m_Data + offsetInLayer, m_DataLen - offsetInLayer);
Expand All @@ -78,6 +84,19 @@ namespace pcpp
return true;
}

if (m_Data - m_Packet->m_RawPacket->getRawData() + static_cast<ptrdiff_t>(offsetInLayer) >
static_cast<ptrdiff_t>(m_Packet->m_RawPacket->getRawDataLen()))
{
PCPP_LOG_ERROR("Requested offset is larger than total packet length");
return false;
}

if (m_NextLayer != nullptr && static_cast<ptrdiff_t>(offsetInLayer) > m_NextLayer->m_Data - m_Data)
{
PCPP_LOG_ERROR("Requested offset exceeds current layer's boundary");
return false;
}
Comment on lines +87 to +98
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these checks needed? All we need to check is that offsetInLayer is within the layer's boundary, which means it is smaller than or equal to m_DataLen. Why do we need to compare it to the whole packet or involve m_NextLayer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Length-related errors introduced during layer shortening can result in the same sanitizer issues when the layer is later extended

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand... can you elaborate?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to say that problems with m_DataLen not matching the actual layer length and the length from the bgp_common_header, as in the examples below, can lead to problems with exceeding the boundaries of the layer or packet during extension.


return m_Packet->extendLayer(this, offsetInLayer, numOfBytesToExtend);
}

Expand All @@ -89,14 +108,20 @@ namespace pcpp
return false;
}

if (m_Packet == nullptr)
if (offsetInLayer < 0)
{
PCPP_LOG_ERROR("Requested offset is negative");
return false;
}

if (static_cast<size_t>(offsetInLayer) >= m_DataLen)
{
if ((size_t)offsetInLayer >= m_DataLen)
{
PCPP_LOG_ERROR("Requested offset is larger than data length");
return false;
}
PCPP_LOG_ERROR("Requested offset is larger than data length");
return false;
}

if (m_Packet == nullptr)
{
uint8_t* newData = new uint8_t[m_DataLen - numOfBytesToShorten];
memcpy(newData, m_Data, offsetInLayer);
memcpy(newData + offsetInLayer, m_Data + offsetInLayer + numOfBytesToShorten,
Expand All @@ -107,6 +132,28 @@ namespace pcpp
return true;
}

if (static_cast<size_t>(offsetInLayer) + numOfBytesToShorten > m_DataLen)
{
PCPP_LOG_ERROR("Requested number of bytes to shorten is larger than data length");
return false;
}

if (m_Data - m_Packet->m_RawPacket->getRawData() + static_cast<ptrdiff_t>(offsetInLayer) +
static_cast<ptrdiff_t>(numOfBytesToShorten) >
static_cast<ptrdiff_t>(m_Packet->m_RawPacket->getRawDataLen()))
{
PCPP_LOG_ERROR("Requested number of bytes to shorten is larger than total packet length");
return false;
}

if (m_NextLayer != nullptr &&
static_cast<ptrdiff_t>(offsetInLayer) + static_cast<ptrdiff_t>(numOfBytesToShorten) >
m_NextLayer->m_Data - m_Data)
{
PCPP_LOG_ERROR("Requested number of bytes to shorten exceeds current layer's boundary");
return false;
}
Comment on lines +141 to +155
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I analyzed several malformed packets that triggered sanitizer errors, and identified some of the root causes.

When iterating over layers during recalculation after shortening a layer, the pointer to the next layer is computed as dataPtr (current layer pointer) + headerLen. For BGP, headerLen corresponds either to the length field of the bgp_common_header structure or to the actual layer length m_DataLen — whichever is smaller. In malformed packets, the header length may not match the actual layer length, or may contain arbitrary values (possibly due to previous incorrect shortening or extension), which can lead to several issues

  • numOfBytesToShorten is subtracted from the m_DataLen of all layers preceding the shortened one. If numOfBytesToShorten is greater than m_DataLen, this causes m_DataLen to wrap around, resulting in an extremely large value. In such cases, when calculating headerLen, the header field is selected, which may be invalid, leading to out-of-bounds memory access.

  • In the shortened layer, numOfBytesToShorten is first subtracted from m_DataLen, then again from headerLen. BgpLayer::getHeaderLen() returns new m_DataLen as the smallest, which results in numOfBytesToShorten being subtracted twice, potentially producing a negative value.

I believe headerLen calculation could be moved before updating the current layer’s m_DataLen. However, this would not resolve the issue described in the next point.

  • Malformed packets may also contain nested BGP layers, which is inherently invalid and can trigger the errors described earlier.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • numOfBytesToShorten is subtracted from the m_DataLen of all layers preceding the shortened one. If numOfBytesToShorten is greater than m_DataLen

How can numOfBytesToShorten be greater than m_DataLen? 🤔

  • In the shortened layer, numOfBytesToShorten is first subtracted from m_DataLen, then again from headerLen. BgpLayer::getHeaderLen() returns new m_DataLen as the smallest, which results in numOfBytesToShorten being subtracted twice, potentially producing a negative value.

I believe this is only relevant to the changes in Packet.cpp, right?

  • Malformed packets may also contain nested BGP layers, which is inherently invalid and can trigger the errors described earlier.

By nested BPG layers, do you mean a BGP layer that comes after another BGP layer? If yes, this is actually a valid scenario that we support:

PcapPlusPlus/Packet++/src/BgpLayer.cpp

Line 80 in 32384c2

void BgpLayer::parseNextLayer()

What's the issue in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can numOfBytesToShorten be greater than m_DataLen? 🤔

I'll try to explain with an example. The diagrams show the packet layout in memory. Each dot represents the start of a layer, layer addresses are represented by the last two bytes, and for some points I've calculated the actual offset from the beginning of the packet for clarity.
image

Let's consider a packet from the file *d94f8b from regression_samples before shortening that caused sanitizer error.
The fragment that should be removed is highlighted in bold (atIndex = 148, numOfBytesToShorten = 46).

The first thing that stands out is that it gets to the next layer. Simply adding a check like (size_t)offsetInLayer + numOfBytesToShorten >= m_DataLen won't be sufficient in this case, because m_DataLen of BGP layers doesn't match the actual layer size in memory:
102 - 66 = 36, but DataLen = 95
183 - 125 = 58, but DataLen = 73

However, the error doesn't occur because of this. After removing the fragment, m_DataLen of previous layers is recalculated according to the condition:

if (!passedExtendedLayer)
    curLayer->m_DataLen -= numOfBytesToShorten;

Since the layer at address afe6 has length of 23, subtracting 46 causes an overflow. The headerLen is taken from the bgp_common_header structure (which is 907 in this case), dataPtr points beyond the packet boundaries, and subsequent call to BgpLayer::getHeaderLen() results in a heap-buffer-overflow error.

By nested BPG layers, do you mean a BGP layer that comes after another BGP layer?

I mean BGP layers where one layer is inside another, i.e. in the BGP payload there is another BGP layer.
For example, in file *d6be97 the following situation occurs:

image

I believe this is only relevant to the changes in Packet.cpp, right?

If you mean moving the calculation of headerLen, then yes, this applies to changes in Packet.cpp.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the thorough and detailed explanation, much appreciated! 🙏

I have 2 follow-up questions:

  1. Since the fixes are within extendLayer() and shortenLayer() I assume you found them when trying to edit packets, is that correct? As far as I know, our regression tests don't edit packets, so the files you added to Tests/Fuzzers/RegressionTests/regression_samples don't actually test the scenario being fixed
  2. Is there any way to make these fixes inside BgpLayer or do they have to be Layer and Packet?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. This PR addresses bug fix [Bug]: Heap-Use-After-Free and Heap-Buffer-Overflow in BgpLayer::getHeaderLen() #1864, and the files added to regression_samples are poc files that triggered sanitizer errors. I can write additional tests if needed

I apologize, our fuzz tests do modify packets:

PcapPlusPlus/Tests/Fuzzers/FuzzTarget.cpp

Line 66 in 32384c2

readParsedPacket(parsedPacket, layer);

So adding these files should be ok.

BTW, did you have a chance to run these files before the fix and they failed?

2. As for the BGP layer, I believe the following checks could be implemented

What I meant is fix BgpOpenMessageLayer::setOptionalParameters to disallow calling shortenLayer and extendLayer if the packet data is invalid. Would it fix the issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, did you have a chance to run these files before the fix and they failed?

Yes, each of these files triggered a sanitizer error (heap-buffer-overflow or heap-use-after-free) when calling pcpp::BgpLayer::getHeaderLen() on the problematic layer.

What I meant is fix BgpOpenMessageLayer::setOptionalParameters to disallow calling shortenLayer and extendLayer if the packet data is invalid.

The checks could be moved into the BGP layer. To do that, they’d need to be extracted into a separate helper method, since shortenLayer/extendLayer can be called from:

  • BgpOpenMessageLayer::setOptionalParameters
  • BgpUpdateMessageLayer::setWithdrawnRoutes
  • BgpUpdateMessageLayer::setPathAttributes
  • BgpUpdateMessageLayer::setNetworkLayerReachabilityInfo
  • BgpNotificationMessageLayer::setNotificationData

Also, shortenLayer/extendLayer are used by other protocols as well, so similar issues could potentially appear there too.

At the very least, it makes sense to keep in shortenLayer a check to ensure the number of bytes being removed doesn’t exceed the layer’s length:

(size_t)offsetInLayer + numOfBytesToShorten > m_DataLen

And it would be better to move the headerLen calculation before updating the current layer m_DataLen, to avoid double subtraction.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR fixes the issue in setNetworkLayerReachabilityInfo 🙂

I think we can do the same for the rest of the setters in BPG layer. In general, we typically try to fix bugs where they occur, so lower-level methods such as shortenLayer and extendLayer don't receive "garbage data".

At the very least, it makes sense to keep in shortenLayer a check to ensure the number of bytes being removed doesn’t exceed the layer’s length

I guess we can do that, sure 👍
I'd only keep this check and remove the rest

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, we’re keeping the check for the number of bytes being removed in Layer.cpp, and moving the rest into the BGP layer?

Copy link
Owner

@seladb seladb Oct 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think we can keep this specific check, but fix the real issue in the setters of BGP layer


return m_Packet->shortenLayer(this, offsetInLayer, numOfBytesToShorten);
}

Expand Down
5 changes: 5 additions & 0 deletions Packet++/src/Packet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -609,6 +609,9 @@ namespace pcpp
// assuming header length of the layer that requested to be extended hasn't been enlarged yet
size_t headerLen = curLayer->getHeaderLen() + (curLayer == layer ? numOfBytesToExtend : 0);
dataPtr += headerLen;

if (dataPtr > m_RawPacket->getRawData() + m_RawPacket->getRawDataLen())
break;
Comment on lines +613 to +614
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can such a thing happen? curLayer->getHeaderLen() should never exceed the packet data, unless we have a bug in one of the layers

}

return true;
Expand Down Expand Up @@ -660,6 +663,8 @@ namespace pcpp
// assuming header length of the layer that requested to be extended hasn't been enlarged yet
size_t headerLen = curLayer->getHeaderLen() - (curLayer == layer ? numOfBytesToShorten : 0);
dataPtr += headerLen;
if (dataPtr > m_RawPacket->getRawData() + m_RawPacket->getRawDataLen())
break;
Comment on lines +666 to +667
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

curLayer = curLayer->getNextLayer();
}

Expand Down
Binary file added ...Tests/regression_samples/047c56c3504ad04232497c903af06c0bc4b5c6f73927ad07cde79cfa28d94f8b
View file
Open in desktop
Binary file not shown.
Binary file added ...Tests/regression_samples/1eb005b8d62599b6561acf7e778f51603ccbc5ddf948c0b081617668bb56cee4
View file
Open in desktop
Binary file not shown.
Binary file added ...Tests/regression_samples/39d92632b6a92f1c682c052febfbc74b0561536687be3b7ce7b9508e8b9726d6
View file
Open in desktop
Binary file not shown.
Binary file added ...Tests/regression_samples/6f6fd63a9a6d8dd1a206eff00c915ed4b643034dad7e9986fd89a43244d6be97
View file
Open in desktop
Binary file not shown.
Binary file added ...Tests/regression_samples/6fda21d7ebaa0391434f17eecf25b1d7c58633f96b6906183c626cbdfc35b325
View file
Open in desktop
Binary file not shown.
Binary file added ...Tests/regression_samples/a7a658b6e51576eafe7990dbc35c5bfff991d914c9ead914e26e3bfc3e88221c
View file
Open in desktop
Binary file not shown.
Binary file added ...Tests/regression_samples/adb739b5829aacedbec6cf3969413997d23b6f2e3350ddf0b1cc3b4d1c335a95
View file
Open in desktop
Binary file not shown.
Binary file added ...Tests/regression_samples/e0c0f8fea6e931d9671384a66f4e1ab02d84e27a5f091e9c7b1fce89d9c18837
View file
Open in desktop
Binary file not shown.
Binary file added ...Tests/regression_samples/ec4b17a4e7b994548f03ae05d8d8924a515a236892959f3300e1c9e8a9c56ba7
View file
Open in desktop
Binary file not shown.
Loading