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

danasana · 2025-09-10T10:11:02Z

This PR fixes heap-use-after-free and heap-buffer-overflow issues in BgpLayer::getHeaderLen(), reported by AddressSanitizer when extending or shortening the BGP layer

Added boundary checks to ensure that requested offsets and lengths do not exceed:

the current layer length,
the total packet length,
the next layer boundary.

Added regression_samples

…eaderLen()

Dimi1010

I'm unsure why the CI has a failure on checkout on windows. :/

Packet++/src/Layer.cpp

Dimi1010 · 2025-09-10T11:36:19Z

I'm unsure why the CI has a failure on checkout on windows. :/

I think the error is that Windows can't contain : in the file names as that is reserved symbol.

…orten

codecov · 2025-09-10T14:51:42Z

Codecov Report

❌ Patch coverage is 52.63158% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.49%. Comparing base (098dd4b) to head (075bb45).

Files with missing lines	Patch %	Lines
Packet++/src/Layer.cpp	47.05%	18 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #1954      +/-   ##
==========================================
- Coverage   83.51%   83.49%   -0.02%     
==========================================
  Files         310      310              
  Lines       54884    54910      +26     
  Branches    12220    12223       +3     
==========================================
+ Hits        45834    45849      +15     
- Misses       7786     8197     +411     
+ Partials     1264      864     -400

Flag	Coverage Δ
alpine320	`75.91% <27.77%> (-0.05%)`	⬇️
fedora42	`76.06% <27.77%> (-0.05%)`	⬇️
macos-13	`81.61% <28.94%> (-0.04%)`	⬇️
macos-14	`81.61% <28.94%> (-0.04%)`	⬇️
macos-15	`81.62% <28.94%> (-0.03%)`	⬇️
mingw32	`70.22% <17.64%> (-0.09%)`	⬇️
mingw64	`70.20% <16.66%> (+0.01%)`	⬆️
npcap	`?`
rhel94	`75.76% <27.77%> (-0.05%)`	⬇️
ubuntu2004	`60.22% <27.77%> (-0.03%)`	⬇️
ubuntu2004-zstd	`60.32% <27.77%> (-0.03%)`	⬇️
ubuntu2204	`75.70% <27.77%> (-0.07%)`	⬇️
ubuntu2204-icpx	`60.75% <28.94%> (-0.04%)`	⬇️
ubuntu2404	`75.95% <27.77%> (-0.02%)`	⬇️
ubuntu2404-arm64	`75.58% <27.77%> (-0.05%)`	⬇️
unittest	`83.49% <52.63%> (-0.02%)`	⬇️
windows-2022	`85.45% <45.45%> (+0.10%)`	⬆️
windows-2025	`85.48% <45.45%> (+0.07%)`	⬆️
winpcap	`85.48% <45.45%> (-0.13%)`	⬇️
xdp	`53.54% <27.77%> (-0.03%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

seladb · 2025-09-18T08:51:47Z

Packet++/src/Layer.cpp

+		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;
+		}


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?

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

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

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.

seladb · 2025-09-18T08:52:09Z

Packet++/src/Layer.cpp

+		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;
+		}


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.

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?

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.

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:

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.

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

I have 2 follow-up questions:

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

Is there any way to make these fixes inside BgpLayer or do they have to be Layer and Packet?

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

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

Disallow nested BGP layers.

Do not shorten the length of the BGP layers that precede the layer that is actually being shortened.

Ensure the actual length matches the m_DataLen.

Update the length field in the header struct.

Disallow BGP layers shorter than the mandatory header length (Header + Mandatory Attributes).

However, I'm not 100% sure these measures will be sufficient. There might be some combination of shorten/extend that could still lead to memory access errors. Also, shorten/extend operations with other layers could cause the same type of issues.

seladb · 2025-09-18T08:54:26Z

Packet++/src/Packet.cpp

+			if (dataPtr > m_RawPacket->getRawData() + m_RawPacket->getRawDataLen())
+				break;


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

seladb · 2025-09-18T08:54:38Z

Packet++/src/Packet.cpp

+			if (dataPtr > m_RawPacket->getRawData() + m_RawPacket->getRawDataLen())
+				break;


danasana added 2 commits September 10, 2025 12:42

Add boundary checks to prevent out-of-bounds access in BgpLayer::getH…

7a836de

…eaderLen()

Add poc files to regression_samples

8e73f57

danasana requested a review from seladb as a code owner September 10, 2025 10:11

Dimi1010 reviewed Sep 10, 2025

View reviewed changes

danasana added 4 commits September 10, 2025 16:00

Move common validation before branch point to avoid redundancy

913e63c

Change C-style casts to static_cast

ae1ed33

Remove mistakenly added files

4c839c7

Add check to ensure offsetInLayer is not negative in Layer::extend/sh…

936c316

…orten

danasana and others added 2 commits September 11, 2025 11:08

Fix pre-commit hook failures

f27a0a6

Merge branch 'dev' into bugfix/bgp-headerlen-memory-safety

075bb45

Dimi1010 approved these changes Sep 15, 2025

View reviewed changes

seladb reviewed Sep 18, 2025

View reviewed changes

		if (dataPtr > m_RawPacket->getRawData() + m_RawPacket->getRawDataLen())
		break;

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

danasana commented Sep 10, 2025

Uh oh!

Dimi1010 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Dimi1010 commented Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Dimi1010 commented Sep 10, 2025 •

edited

Loading

codecov bot commented Sep 10, 2025 •

edited

Loading