From 297891a97b10d023369c0c6ddb3d954e372e228c Mon Sep 17 00:00:00 2001 From: Martin Mirchev Date: Sun, 11 May 2025 15:53:15 +0800 Subject: [PATCH 1/3] Fix read overflow --- Packet++/src/RawPacket.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/Packet++/src/RawPacket.cpp b/Packet++/src/RawPacket.cpp index 4eb36b31a6..5f15200979 100644 --- a/Packet++/src/RawPacket.cpp +++ b/Packet++/src/RawPacket.cpp @@ -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) + { + PCPP_LOG_ERROR("RawPacket::insertData: dataToInsertLen causes overflow"); + return; + } + // memmove copies data as if there was an intermediate buffer in between - so it allows for copying processes on // overlapping src/dest ptrs if insertData is called with atIndex == m_RawDataLen, then no data is being moved. // The data of the raw packet is still extended by dataToInsertLen From 41b3812416d91ccf4f31376e8539c6215e14c1f0 Mon Sep 17 00:00:00 2001 From: Martin Mirchev Date: Sat, 24 May 2025 21:28:35 +0800 Subject: [PATCH 2/3] Convert loging to error which is handled in BGPLayer --- Packet++/src/BgpLayer.cpp | 20 ++++++++++++++------ Packet++/src/RawPacket.cpp | 4 ++-- 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/Packet++/src/BgpLayer.cpp b/Packet++/src/BgpLayer.cpp index b32ab8d010..f56c87eeec 100644 --- a/Packet++/src/BgpLayer.cpp +++ b/Packet++/src/BgpLayer.cpp @@ -744,13 +744,21 @@ namespace pcpp if (newNlriDataLen > curNlriDataLen) { - bool res = extendLayer(sizeof(bgp_common_header) + 2 * sizeof(uint16_t) + curWithdrawnRoutesDataLen + - curPathAttributesDataLen, - newNlriDataLen - curNlriDataLen); - if (!res) + try { - PCPP_LOG_ERROR("Couldn't extend BGP update layer to include the additional NLRI data"); - return res; + bool res = extendLayer(sizeof(bgp_common_header) + 2 * sizeof(uint16_t) + curWithdrawnRoutesDataLen + + curPathAttributesDataLen, + newNlriDataLen - curNlriDataLen); + if (!res) + { + PCPP_LOG_ERROR("Couldn't extend BGP update layer to include the additional NLRI data"); + return res; + } + } + catch (const std::length_error& e) + { + PCPP_LOG_ERROR("Failed to extend BGP update layer: " << e.what()); + return false; } } else if (newNlriDataLen < curNlriDataLen) diff --git a/Packet++/src/RawPacket.cpp b/Packet++/src/RawPacket.cpp index 5f15200979..b53752a663 100644 --- a/Packet++/src/RawPacket.cpp +++ b/Packet++/src/RawPacket.cpp @@ -139,8 +139,8 @@ namespace pcpp // Check for overflow in the new length if ((size_t)m_RawDataLen + dataToInsertLen < (size_t)m_RawDataLen) { - PCPP_LOG_ERROR("RawPacket::insertData: dataToInsertLen causes overflow"); - return; + throw std::length_error( + "RawPacket::insertData: dataToInsertLen causes overflow in the new length calculation"); } // memmove copies data as if there was an intermediate buffer in between - so it allows for copying processes on From 0f943317c775b34a2d5a4a6576eda01d992d295a Mon Sep 17 00:00:00 2001 From: Martin Mirchev Date: Sun, 15 Jun 2025 19:05:20 +0800 Subject: [PATCH 3/3] Rework to not reach exception case --- Packet++/src/BgpLayer.cpp | 30 +++++++++++++++++------------- Packet++/src/RawPacket.cpp | 2 +- 2 files changed, 18 insertions(+), 14 deletions(-) diff --git a/Packet++/src/BgpLayer.cpp b/Packet++/src/BgpLayer.cpp index f56c87eeec..993d6edb7e 100644 --- a/Packet++/src/BgpLayer.cpp +++ b/Packet++/src/BgpLayer.cpp @@ -744,21 +744,25 @@ namespace pcpp if (newNlriDataLen > curNlriDataLen) { - try - { - bool res = extendLayer(sizeof(bgp_common_header) + 2 * sizeof(uint16_t) + curWithdrawnRoutesDataLen + - curPathAttributesDataLen, - newNlriDataLen - curNlriDataLen); - if (!res) - { - PCPP_LOG_ERROR("Couldn't extend BGP update layer to include the additional NLRI data"); - return res; - } + + // offsetInLayer, numOfBytesToExtend + // int indexToInsertData = layer->m_Data + offsetInLayer - m_RawPacket->getRawData(); + auto bytesToExtend = newNlriDataLen - curNlriDataLen; + if(m_Data != nullptr + && m_Packet != nullptr + && static_cast(m_Packet->getRawPacket()->getRawDataLen()) + bytesToExtend < static_cast(m_Packet->getRawPacket()->getRawDataLen()) ) + { + PCPP_LOG_ERROR("Failed to extend BGP update layer, the new data length exceeds the raw packet's data length"); + return false; } - catch (const std::length_error& e) + + bool res = extendLayer(sizeof(bgp_common_header) + 2 * sizeof(uint16_t) + curWithdrawnRoutesDataLen + + curPathAttributesDataLen, + bytesToExtend); + if (!res) { - PCPP_LOG_ERROR("Failed to extend BGP update layer: " << e.what()); - return false; + PCPP_LOG_ERROR("Couldn't extend BGP update layer to include the additional NLRI data"); + return res; } } else if (newNlriDataLen < curNlriDataLen) diff --git a/Packet++/src/RawPacket.cpp b/Packet++/src/RawPacket.cpp index dd0ba39d5b..2eca8a547e 100644 --- a/Packet++/src/RawPacket.cpp +++ b/Packet++/src/RawPacket.cpp @@ -115,7 +115,7 @@ 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) + if (static_cast(m_RawDataLen) + dataToInsertLen < static_cast(m_RawDataLen)) { throw std::length_error( "RawPacket::insertData: dataToInsertLen causes overflow in the new length calculation");