Fix read overflow #1812

Marti2203 · 2025-05-11T07:58:46Z

Fixes the following issue detected by OSS-Fuzz:

https://issues.oss-fuzz.com/issues/390004170

This fix was generated by CodeRover-S, an LLM agent for fixing security vulnerabilities. More details can be found at:

codecov · 2025-05-11T20:36:34Z

Codecov Report

❌ Patch coverage is 54.54545% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.48%. Comparing base (0a6241a) to head (4891427).

Files with missing lines	Patch %	Lines
Packet++/src/BgpLayer.cpp	62.50%	1 Missing and 2 partials ⚠️
Packet++/src/RawPacket.cpp	33.33%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #1812      +/-   ##
==========================================
- Coverage   83.48%   83.48%   -0.01%     
==========================================
  Files         311      311              
  Lines       54885    54890       +5     
  Branches    12218    12057     -161     
==========================================
+ Hits        45822    45825       +3     
+ Misses       7847     7846       -1     
- Partials     1216     1219       +3

Flag	Coverage Δ
alpine320	`75.94% <25.00%> (-0.02%)`	⬇️
fedora42	`76.07% <25.00%> (-0.04%)`	⬇️
macos-13	`81.63% <30.00%> (-0.02%)`	⬇️
macos-14	`81.63% <30.00%> (-0.02%)`	⬇️
macos-15	`81.64% <30.00%> (-0.02%)`	⬇️
mingw32	`70.32% <25.00%> (-0.03%)`	⬇️
mingw64	`70.31% <25.00%> (+0.10%)`	⬆️
npcap	`?`
rhel94	`75.79% <22.22%> (-0.04%)`	⬇️
ubuntu2004	`60.25% <33.33%> (+0.01%)`	⬆️
ubuntu2004-zstd	`60.35% <33.33%> (+0.01%)`	⬆️
ubuntu2204	`75.73% <22.22%> (-0.04%)`	⬇️
ubuntu2204-icpx	`60.71% <30.00%> (-0.02%)`	⬇️
ubuntu2404	`75.97% <25.00%> (-0.02%)`	⬇️
ubuntu2404-arm64	`75.61% <25.00%> (-0.02%)`	⬇️
unittest	`83.48% <54.54%> (-0.01%)`	⬇️
windows-2022	`85.47% <62.50%> (+0.14%)`	⬆️
windows-2025	`85.50% <62.50%> (+0.10%)`	⬆️
winpcap	`85.50% <62.50%> (-0.10%)`	⬇️
xdp	`53.56% <22.22%> (-0.01%)`	⬇️

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.

Dimi1010 · 2025-05-11T20:38:58Z

Packet++/src/RawPacket.cpp

+		if ((size_t)m_RawDataLen + dataToInsertLen < (size_t)m_RawDataLen)
+		{
+			PCPP_LOG_ERROR("RawPacket::insertData: dataToInsertLen causes overflow");
+			return;
+		}


Can we throw a std::length_error instead of just returning with a error message? The error message will not indicate the error condition to the caller.

Will throwing an exception prevent the application from crashing? 🤔

This method was called in BgpLayer so we should probably think how to prevent it from being called with malformed data

You want a crash if overflow happens. Or at least you want an exception thrown to indicate the failure of the operation, and if the caller doesn't handle it up the call stack, then to crash. It isn't really possible to recover from the situation as the system has reached the limit of how much data can be stored inside a packet.

Silently continuing with just an error message in the log is worse as the program will assume the operation went fine and produce corrupted data down the line. This is exactly the type of improbable situation the exception mechanism was made to handle.

I totally agree, however I don't think throwing an exception will resolve the OSS-Fuzz issue. We should probably throw an exception and fix the issue that caused it in BgpLayer

Hi, just to confirm - I will update the patch to throw an std::length_error . Would that be accepted by both of you? @Dimi1010 @seladb

@seladb @Marti2203

Generally, I find exceptions are fine as long as they aren't used to dictate common control flow. The current mainstream compilers use an architecture that has a "zero cost" try-catch blocks if the exception isn't thrown. The tradeoff is a significant overhead if an exception is thrown, so you only want them thrown for rare events.

The current usage seems ok to me? How often are overflows expected to happen?

In most cases, I wouldn't expect an exception to be thrown, however:

In applications that use PcapPlusPlus with an arbitrary payload (either to test malformed packet handling or because they generate a random payload), an exception can be thrown and we would like to avoid the cost of it

Most (or all?) of our critical path code doesn't catch exceptions and try to handle edge cases so they don't end up with exceptions. I think we should do the same here for consistency

Hi, I have been working on other things and have been checking out this PR again. I agree with sela's points and will prepare a patch after a confirmation from your side -

NO exception will be thrown in RawPacket, only it is logged and an error is returned. This is to ensure new clients of the code do not lead to a crash and code stylistically aligns with the rest.

Separately, will add a check for the error in the BGPLayer as it is the only client in the project that uses it.

I think we can still throw an error in insertData() but I would avoid try..catch in BgpLayer and instead check the sizes before calling extendLayer(). @Marti2203 let me know what you think

Sounds good. I will also cover the nitpick and send it tonight :)

Packet++/src/RawPacket.cpp

seladb · 2025-06-12T03:33:04Z

Packet++/src/RawPacket.cpp

+		if ((size_t)m_RawDataLen + dataToInsertLen < (size_t)m_RawDataLen)
+		{
+			PCPP_LOG_ERROR("RawPacket::insertData: dataToInsertLen causes overflow");
+			return;
+		}


I think we can still throw an error in insertData() but I would avoid try..catch in BgpLayer and instead check the sizes before calling extendLayer(). @Marti2203 let me know what you think

Marti2203 · 2025-06-15T11:06:07Z

Pushed an update. Feedback is appreciated!

seladb · 2025-06-15T20:20:50Z

Pushed an update. Feedback is appreciated!

@Marti2203 CI fails, can you please take a look?

Marti2203 · 2025-09-22T09:13:09Z

Hi @seladb Apologies for the super late response, will take care of it today

Marti2203 · 2025-09-24T07:31:55Z

Thanks for the rerun, will check the results!

Marti2203 · 2025-09-24T16:15:18Z

@seladb I have added Packet.h to fix the forward declaration issue. Would be happy to discuss a different solution but I think that it should not be a problem

Packet++/src/BgpLayer.cpp

seladb · 2025-09-30T08:33:03Z

Packet++/src/BgpLayer.cpp

+			if (m_Data != nullptr && m_Packet != nullptr)
+			{
+				auto raw_len = static_cast<size_t>(m_Packet->getRawPacket()->getRawDataLen());
+				if (raw_len + bytesToExtend < raw_len)


How can this be true? 🤔
bytesToExtend is of type size_t, which can't be a negative number

As previously discussed, this can overflow when unsigned going back to a lower value.

You're right! Can you add a comment above this line to remind future us why we did it? 🙂

Sure! 😄 It is an annoying edge-case

seladb · 2025-09-30T08:35:17Z

Packet++/src/BgpLayer.cpp

Can you add the packet data that caused this issue to our regression tests, the same way it's done in this PR?

Sure, will do!

Fix read overflow

297891a

Marti2203 requested a review from seladb as a code owner May 11, 2025 07:58

Merge branch 'dev' into fix-390004170

3ec7718

Dimi1010 reviewed May 11, 2025

View reviewed changes

Marti2203 and others added 2 commits May 24, 2025 21:28

Convert loging to error which is handled in BGPLayer

41b3812

Merge branch 'dev' into fix-390004170

b342d18

seladb reviewed Jun 12, 2025

View reviewed changes

Marti2203 and others added 2 commits June 13, 2025 11:38

Merge branch 'dev' into fix-390004170

3d45189

Rework to not reach exception case

0f94331

Merge branch 'dev' into fix-390004170

e1c57c9

Marti2203 requested a review from seladb September 22, 2025 09:13

Add Packet.h header and refacor condition

677d69f

Fix fomatting

4891427

seladb reviewed Sep 30, 2025

View reviewed changes

seladb mentioned this pull request Oct 1, 2025

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

Open

Fix read overflow #1812

Are you sure you want to change the base?

Fix read overflow #1812

Uh oh!

Conversation

Marti2203 commented May 11, 2025

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

Choose a reason for hiding this comment

Uh oh!

Marti2203 commented Jun 15, 2025

Uh oh!

seladb commented Jun 15, 2025

Uh oh!

Marti2203 commented Sep 22, 2025

Uh oh!

Marti2203 commented Sep 24, 2025

Uh oh!

Marti2203 commented Sep 24, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

codecov bot commented May 11, 2025 •

edited

Loading

Dimi1010 May 11, 2025 •

edited

Loading

Dimi1010 May 12, 2025 •

edited

Loading

Marti2203 Sep 30, 2025 •

edited

Loading