Fix read overflow #1812
Fix read overflow #1812
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #1812 +/- ##
==========================================
+ Coverage 82.97% 82.98% +0.01%
==========================================
Files 285 285
Lines 48968 49023 +55
Branches 10346 10586 +240
==========================================
+ Hits 40630 40682 +52
- Misses 7177 7189 +12
+ Partials 1161 1152 -9
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:
|
Packet++/src/RawPacket.cpp
Outdated
| if ((size_t)m_RawDataLen + dataToInsertLen < (size_t)m_RawDataLen) | ||
| { | ||
| PCPP_LOG_ERROR("RawPacket::insertData: dataToInsertLen causes overflow"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Sounds good. I will also cover the nitpick and send it tonight :)
Packet++/src/RawPacket.cpp
Outdated
| @@ -136,6 +136,13 @@ namespace pcpp | |||
|
|
|||
| void RawPacket::insertData(int atIndex, const uint8_t* dataToInsert, size_t dataToInsertLen) | |||
| { | |||
| // Check for overflow in the new length | |||
| if ((size_t)m_RawDataLen + dataToInsertLen < (size_t)m_RawDataLen) | |||
There was a problem hiding this comment.
nit: maybe use static_cast instead of C-style casting?
Packet++/src/RawPacket.cpp
Outdated
| if ((size_t)m_RawDataLen + dataToInsertLen < (size_t)m_RawDataLen) | ||
| { | ||
| PCPP_LOG_ERROR("RawPacket::insertData: dataToInsertLen causes overflow"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
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
|
Pushed an update. Feedback is appreciated! |
@Marti2203 CI fails, can you please take a look? |
Fixes the following issue detected by OSS-Fuzz:
This fix was generated by CodeRover-S, an LLM agent for fixing security vulnerabilities. More details can be found at: