Fixes performance regression during Pcap file reading. (#2072)

* Fixes performance regression during pcap file reading due to extensive allocator pressure.

* Try fixes to BGP optional parameter lengths to fix fuzz.

* Fix length calculation.

* Simplify finding next IAC by using an iterator based search.

* Disable fail-fast on regressions as each fuzzer has different sanitizer.

* Disable CI Fuzz fail fast as each job provides different error reports.

* Try hack to pass fuzz.

* Add basic data length checks on constructing DCHP and Vxlan.

* Update checks to use canReinterpretAs.

* Try increasing the hack buffer...

* Fix DHCP getValueAsString relying on recordLen directly, even though getDataSize can potentially have a Type that does not contain the LV part of TLV.

Fix potential underflow bug on `getValueAs` when offset is larger than `getDataSize()`.

* Temp fix casts.

* Fix potential underflow.

* Remove temp buffer hack.

* Update bgp length guard check.

* Harden SSL extension parsing against buffer overruns.

* Clamp extension buffer to message length.

* Harden SSLServerName hostname extraction against buffer overruns.

* Add safety check to insertData.
Add possible safety check to DnsLayer addResource.

* Cleanup compile errors.

* Update docs.

* Lint

* Increase example test timeout

* REVERT: Increase example test timeout

* Revert deduplication of SSL Extension parsing procedure.

* Lint

* Revert getExtType.

* Add docs.

---------

Co-authored-by: seladb <pcapplusplus@gmail.com>

Author: Dimi1010

.github/workflows/build_and_test.yml

@@ -999,7 +999,7 @@ jobs:
     name: Run ${{ matrix.engine }}-${{ matrix.sanitizer }} fuzzer for regressions
     runs-on: ubuntu-latest
     strategy:
-      fail-fast: true
+      fail-fast: false
       matrix:
         sanitizer: [address, undefined, memory]
         engine: [libfuzzer]

.github/workflows/cifuzz.yml

@@ -16,7 +16,7 @@ jobs:
   Fuzzing:
     runs-on: ubuntu-latest
     strategy:
-      fail-fast: true
+      fail-fast: false
       matrix:
         sanitizer: [address, undefined, memory]
     steps:

Packet++/header/DhcpLayer.h

@@ -418,7 +418,9 @@ namespace pcpp
 		/// @return DHCP option data as string
 		std::string getValueAsString(int valueOffset = 0) const
 		{
-			if (m_Data == nullptr || m_Data->recordLen - valueOffset < 1)
+			// TODO: This will burn if valueOffset is negative.
+			// Should negative offsets even be allowed? Potentially change it to size_t?
+			if (m_Data == nullptr || getDataSize() < static_cast<size_t>(valueOffset) + 1)
 				return "";
 
 			return std::string(reinterpret_cast<const char*>(m_Data->recordValue) + valueOffset,
@@ -589,6 +591,11 @@ namespace pcpp
 		/// A destructor for this layer
 		~DhcpLayer() override = default;
 
+		static bool isDataValid(uint8_t const* data, size_t dataLen)
+		{
+			return canReinterpretAs<dhcp_header>(data, dataLen);
+		}
+
 		/// Get a pointer to the DHCP header. Notice this points directly to the data, so every change will change the
 		/// actual packet data
 		/// @return A pointer to the @ref dhcp_header

Packet++/header/SSLHandshake.h

@@ -1,6 +1,7 @@
 #pragma once
 
 #include <utility>
+#include <type_traits>
 #include "SSLCommon.h"
 #include "PointerVector.h"
 #include "Asn1Codec.h"
@@ -99,8 +100,26 @@ namespace pcpp
 	public:
 		/// C'tor for this class
 		/// @param[in] data The raw data for the extension
+		/// @deprecated This constructor is deprecated because it uses an unbound memory span. Use the constructor with
+		/// bounded span.
+		PCPP_DEPRECATED("Unbound memory span. Use the constructor with bounded span.")
 		explicit SSLExtension(uint8_t* data);
 
+		/// @brief Constructs an SSLExtension that interprets the provided data as the raw bytes of the extension.
+		///
+		/// The memory span defined by data and dataLen can be larger than the actual extension data.
+		/// If so, only the bytes corresponding to the extension header (type and length fields) + [length] bytes of
+		/// extension data will be parsed. The rest of the span will be ignored.
+		///
+		/// The above behaviour allows variable length extensions to be parsed, as SSL extensions are laid out
+		/// sequentially in memory on a payload.
+		///
+		/// @param[in] data The raw data for the extension.
+		/// @param[in] dataLen The length of the data in bytes.
+		/// @throws std::invalid_argument if data is nullptr or if dataLen is smaller than the size of the extension
+		/// header (type and length fields).
+		SSLExtension(uint8_t* data, size_t dataLen);
+
 		virtual ~SSLExtension() = default;
 
 		/// @return The type of the extension as enum
@@ -118,6 +137,33 @@ namespace pcpp
 		/// @return A pointer to the raw data of the extension
 		uint8_t* getData() const;
 
+		/// @brief A static method that tries to create an instance of a specific extension type.
+		///
+		/// The factory method handles potential buffer overflows by validating that the data length of the span
+		/// is sufficient to contain the extension header (type and length fields) and the extension data as specified
+		/// in the length field.
+		///
+		/// @tparam T The type of the extension to create. This type must be a class that inherits from SSLExtension.
+		/// @param data Pointer to the raw data of the extension.
+		/// @param dataLen Max length of the span to be parsed.
+		/// @return The extension instance if the data buffer is valid, nullptr otherwise.
+		template <typename T, typename std::enable_if_t<std::is_base_of<SSLExtension, T>::value, bool> = true>
+		static std::unique_ptr<T> tryCreateExtension(uint8_t* data, size_t dataLen)
+		{
+			if (data == nullptr || dataLen < sizeof(SSLExtensionStruct))
+			{
+				return nullptr;
+			}
+
+			auto* extStruct = reinterpret_cast<SSLExtensionStruct*>(data);
+			if (dataLen < sizeof(SSLExtensionStruct) + extStruct->getDataLength())
+			{
+				return nullptr;
+			}
+
+			return std::make_unique<T>(data, dataLen);
+		}
+
 	protected:
 		/// @struct SSLExtensionStruct
 		/// Represents the common fields of the extension
@@ -129,9 +175,13 @@ namespace pcpp
 			uint16_t extensionDataLength;
 			/// Extension data as raw (byte array)
 			uint8_t extensionData[];
+
+			/// @brief Gets the extension length in host byte order
+			uint16_t getDataLength() const;
 		};
 
 		uint8_t* m_RawData;
+		size_t m_RawDataLen;
 
 		SSLExtensionStruct* getExtensionStruct() const
 		{
@@ -145,10 +195,7 @@ namespace pcpp
 	class SSLServerNameIndicationExtension : public SSLExtension
 	{
 	public:
-		/// C'tor for this class
-		/// @param[in] data The raw data for the extension
-		explicit SSLServerNameIndicationExtension(uint8_t* data) : SSLExtension(data)
-		{}
+		using SSLExtension::SSLExtension;
 
 		/// @return The hostname written in the extension data
 		std::string getHostName() const;
@@ -160,10 +207,7 @@ namespace pcpp
 	class SSLSupportedVersionsExtension : public SSLExtension
 	{
 	public:
-		/// C'tor for this class
-		/// @param[in] data The raw data for the extension
-		explicit SSLSupportedVersionsExtension(uint8_t* data) : SSLExtension(data)
-		{}
+		using SSLExtension::SSLExtension;
 
 		/// @return The list of supported versions mentioned in the extension data
 		std::vector<SSLVersion> getSupportedVersions() const;
@@ -175,10 +219,7 @@ namespace pcpp
 	class TLSSupportedGroupsExtension : public SSLExtension
 	{
 	public:
-		/// C'tor for this class
-		/// @param[in] data The raw data for the extension
-		explicit TLSSupportedGroupsExtension(uint8_t* data) : SSLExtension(data)
-		{}
+		using SSLExtension::SSLExtension;
 
 		/// @return A vector of the supported groups (also known as "Elliptic Curves")
 		std::vector<uint16_t> getSupportedGroups() const;
@@ -190,10 +231,7 @@ namespace pcpp
 	class TLSECPointFormatExtension : public SSLExtension
 	{
 	public:
-		/// C'tor for this class
-		/// @param[in] data The raw data for the extension
-		explicit TLSECPointFormatExtension(uint8_t* data) : SSLExtension(data)
-		{}
+		using SSLExtension::SSLExtension;
 
 		/// @return A vector of the elliptic curves point formats
 		std::vector<uint8_t> getECPointFormatList() const;

Packet++/header/TLVData.h

@@ -160,7 +160,7 @@ namespace pcpp
 		/// @return The record data as type T
 		template <typename T> T getValueAs(size_t offset = 0) const
 		{
-			if (getDataSize() - offset < sizeof(T))
+			if (getDataSize() < sizeof(T) + offset)
 				return 0;
 
 			T result;

Packet++/header/VxlanLayer.h

@@ -90,6 +90,15 @@ namespace pcpp
 
 		~VxlanLayer() override = default;
 
+		/// A static method that validates the input data
+		/// @param[in] data The pointer to the beginning of a byte stream of an Vxlan layer
+		/// @param[in] dataLen The length of the byte stream
+		/// @return True if the data is valid and can represent an Vxlan layer
+		static bool isDataValid(uint8_t const* data, size_t dataLen)
+		{
+			return canReinterpretAs<vxlan_header>(data, dataLen);
+		}
+
 		/// Get a pointer to the VXLAN header. Notice this points directly to the data, so every change will change the
 		/// actual packet data
 		/// @return A pointer to the vxlan_header

Packet++/src/BgpLayer.cpp

@@ -263,8 +263,7 @@ namespace pcpp
 			return;
 		}
 
-		size_t optionalParamsLen = (size_t)be16toh(msgHdr->optionalParameterLength);
-
+		size_t optionalParamsLen = msgHdr->optionalParameterLength;
 		if (optionalParamsLen > getHeaderLen() - sizeof(bgp_open_message))
 		{
 			optionalParamsLen = getHeaderLen() - sizeof(bgp_open_message);
@@ -301,7 +300,18 @@ namespace pcpp
 		bgp_open_message* msgHdr = getOpenMsgHeader();
 		if (msgHdr != nullptr)
 		{
-			return (size_t)(msgHdr->optionalParameterLength);
+			auto optParamLen = static_cast<size_t>(msgHdr->optionalParameterLength);
+
+			constexpr size_t bgpOpenMsgHeaderSize = sizeof(bgp_open_message);
+			size_t headerLen = getHeaderLen();
+			if (headerLen < optParamLen + bgpOpenMsgHeaderSize)
+			{
+				PCPP_LOG_WARN("BGP Layer optional param length exceeds total BGP message. "
+				              "This packet might be malformed! Trimming to maximum allowed by BGP message length.");
+				optParamLen = headerLen >= bgpOpenMsgHeaderSize ? headerLen - bgpOpenMsgHeaderSize : 0;
+			}
+
+			return optParamLen;
 		}
 
 		return 0;

Packet++/src/DnsLayer.cpp

@@ -478,6 +478,15 @@ namespace pcpp
 		while (curResource != nullptr && curResource->getType() <= resType)
 		{
 			newResourceOffsetInLayer += curResource->getSize();
+
+			if (newResourceOffsetInLayer > m_DataLen)
+			{
+				// This possibly means that the DNS layer has been created from a malformed packet.
+				PCPP_LOG_ERROR("Couldn't add resource! DNS Layer is malformed and contains out of bounds resources.");
+				delete newResource;
+				return nullptr;
+			}
+
 			IDnsResource* nextResource = curResource->getNextResource();
 			if (nextResource == nullptr || nextResource->getType() > resType)
 				break;

Packet++/src/RawPacket.cpp

@@ -139,6 +139,12 @@ namespace pcpp
 
 	void RawPacket::insertData(int atIndex, const uint8_t* dataToInsert, size_t dataToInsertLen)
 	{
+		if (atIndex > m_RawDataLen)
+		{
+			throw std::out_of_range("Insert index is out of raw packet bound. Inserts can only happen in range [0, " +
+			                        std::to_string(m_RawDataLen) + ']');
+		}
+
 		// 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