-
Notifications
You must be signed in to change notification settings - Fork 721
Fix read overflow #1812
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.
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
base: dev
Are you sure you want to change the base?
Fix read overflow #1812
Changes from all commits
297891a
3ec7718
41b3812
b342d18
3d45189
0f94331
e1c57c9
677d69f
4891427
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ | |
|
||
#include "Logger.h" | ||
#include "BgpLayer.h" | ||
#include "Packet.h" | ||
#include "EndianPortable.h" | ||
#include "GeneralUtils.h" | ||
|
||
|
@@ -744,9 +745,25 @@ namespace pcpp | |
|
||
if (newNlriDataLen > curNlriDataLen) | ||
{ | ||
|
||
// offsetInLayer, numOfBytesToExtend | ||
// int indexToInsertData = layer->m_Data + offsetInLayer - m_RawPacket->getRawData(); | ||
Marti2203 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
auto bytesToExtend = newNlriDataLen - curNlriDataLen; | ||
|
||
if (m_Data != nullptr && m_Packet != nullptr) | ||
{ | ||
auto raw_len = static_cast<size_t>(m_Packet->getRawPacket()->getRawDataLen()); | ||
Marti2203 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (raw_len + bytesToExtend < raw_len) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How can this be true? 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As previously discussed, this can overflow when unsigned going back to a lower value. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right! Can you add a comment above this line to remind future us why we did it? 🙂 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure! 😄 It is an annoying edge-case |
||
{ | ||
PCPP_LOG_ERROR( | ||
"Failed to extend BGP update layer, the new data length exceeds the raw packet's data length"); | ||
return false; | ||
} | ||
} | ||
|
||
bool res = extendLayer(sizeof(bgp_common_header) + 2 * sizeof(uint16_t) + curWithdrawnRoutesDataLen + | ||
curPathAttributesDataLen, | ||
newNlriDataLen - curNlriDataLen); | ||
bytesToExtend); | ||
if (!res) | ||
{ | ||
PCPP_LOG_ERROR("Couldn't extend BGP update layer to include the additional NLRI data"); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add the packet data that caused this issue to our regression tests, the same way it's done in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, will do!