Fix telnet off by one error. (#1930)

* Fix telnet off by one error.

* Added additional bounds checking for dataPos.

* Improved bounds checking on telnet sequence parsing.

- Replaced `isCommandField` -> `isTelnetCommand`
- Replaced `isDataField` -> `isTelnetData`
- Refactored `getNextCommand` and `getNextDataField` with an improved loop.

* Lint

* Changed isTelnetData to return `false` on nullptr input.

* debug log fix

* Added error handling for invalid telnet sequence when searching for next field.

* Add error handling for toString().

* Added more error handling.

* Move dataPos to be entirely inside the `removeEscapeCharacters` branch.

* Added error handling.

* Refactored to remove exception throwing in relatively common error operation.

Telnet packets can commonly be incomplete due to being based on TCP stream protocol.
Parsing errors near the end of the stream are expected to be common, thus throwing exception for them is not recommended.

* Fixed switch unhandled value.

* Suppressed gcc unused function warning.

* Removed redundant else.

* Revert "Suppressed gcc unused function warning."

This reverts commit 47b35a5.

* Removed unused function.

* Replaced `dataPos` range assertion with assertion macro.

* Fix assert condition.

* Remove unnecessary local function.

Author: Dimi1010

Common++/CMakeLists.txt

@@ -14,6 +14,7 @@ add_library(
 
 set(
   public_headers
+  header/AssertionUtils.h
   header/DeprecationUtils.h
   header/GeneralUtils.h
   header/IpAddress.h

Common++/header/AssertionUtils.h

@@ -0,0 +1,48 @@
+#pragma once
+
+/// @file
+/// @brief This file contains internal assertion utilities used in PcapPlusPlus for debugging.
+
+#ifndef PCPP_ASSERT_USE_C_ASSERT
+#	define PCPP_ASSERT_USE_C_ASSERT 0
+#endif  // !PCPP_ASSERT_USE_C_ASSERT
+
+#include <stdexcept>
+#if PCPP_ASSERT_USE_C_ASSERT
+#	include <cassert>
+#else
+#	include <string>
+#	include <sstream>
+#endif  // PCPP_ASSERT_USE_C_ASSERT
+
+namespace pcpp
+{
+	namespace internal
+	{
+		/// @brief A custom assertion error class derived from std::logic_error to be used with PCPP_ASSERT.
+		class AssertionError : public std::logic_error
+		{
+		public:
+			using std::logic_error::logic_error;
+		};
+	}  // namespace internal
+}  // namespace pcpp
+
+#ifndef NDEBUG
+#	if PCPP_ASSERT_USE_C_ASSERT
+#		define PCPP_ASSERT(condition, message) assert((condition) && (message))
+#	else  // !PCPP_ASSERT_USE_C_ASSERT
+#		define PCPP_ASSERT(condition, message)                                                                        \
+			do                                                                                                         \
+			{                                                                                                          \
+				if (!(condition))                                                                                      \
+				{                                                                                                      \
+					std::stringstream ss;                                                                              \
+					ss << "[PCPP] Assertion failed on [" << __FILE__ << ":" << __LINE__ << "] with: " << (message);    \
+					throw pcpp::internal::AssertionError(ss.str());                                                    \
+				}                                                                                                      \
+			} while (false)
+#	endif  // PCPP_ASSERT_USE_C_ASSERT
+#else
+#	define PCPP_ASSERT(condition, message) (void)0;
+#endif  // !NDEBUG

Packet++/header/TelnetLayer.h

@@ -15,10 +15,6 @@ namespace pcpp
 		// Position iterator for next command
 		size_t lastPositionOffset;
 
-		// Checks if position is a data field
-		bool isDataField(uint8_t* pos) const;
-		// Checks if position is a command field
-		bool isCommandField(uint8_t* pos) const;
 		// Returns distance to next IAC
 		size_t distanceToNextIAC(uint8_t* startPos, size_t maxLength);
 		// Returns length of provided field

Packet++/src/TelnetLayer.cpp

@@ -3,22 +3,72 @@
 #include "TelnetLayer.h"
 #include "Logger.h"
 #include "GeneralUtils.h"
+#include "AssertionUtils.h"
 #include <cstring>
+#include <iterator>
+#include <algorithm>
 
 namespace pcpp
 {
-
-	bool TelnetLayer::isDataField(uint8_t* pos) const
+	namespace
 	{
-		// "FF FF" means data
-		return pos[0] != static_cast<int>(TelnetCommand::InterpretAsCommand) ||
-		       pos[1] == static_cast<int>(TelnetCommand::InterpretAsCommand);
-	}
+		/// @brief An enum representing the type of a Telnet sequence
+		enum class TelnetSequenceType
+		{
+			/// @brief An unknown sequence type. Usually means parsing error.
+			/// Commonly happens when an IAC symbol is found at the end of the buffer without a following byte.
+			Unknown,
+			/// @brief A telnet command sequence. Starts with IAC followed by a command code.
+			Command,
+			/// @brief A telnet data sequence. Either does not start with IAC or starts with IAC IAC.
+			UserData,
+		};
+
+		/// @brief Checks if a given sequence matches Telnet command or data pattern.
+		/// @param first Start of the sequence to check
+		/// @param maxCount Maximum number of bytes to check
+		/// @return The type of the Telnet sequence. Unknown if the sequence does not match either command or data
+		/// pattern or if parsing error occurs.
+		TelnetSequenceType getTelnetSequenceType(uint8_t const* first, size_t maxCount)
+		{
+			if (first == nullptr || maxCount == 0)
+			{
+				PCPP_LOG_DEBUG("Checking empty or null buffer for telnet sequence type");
+				return TelnetSequenceType::Unknown;
+			}
 
-	bool TelnetLayer::isCommandField(uint8_t* pos) const
-	{
-		return !isDataField(pos);
-	}
+			// If first byte is not "FF" it's data
+			if (*first != static_cast<int>(TelnetLayer::TelnetCommand::InterpretAsCommand))
+			{
+				return TelnetSequenceType::UserData;
+			}
+
+			// IAC must be followed by another octet
+			if (maxCount <= 1)
+			{
+				PCPP_LOG_DEBUG("Telnet Parse Error: IAC (FF) must always be followed by another octet");
+				return TelnetSequenceType::Unknown;
+			}
+
+			if (first[1] == static_cast<int>(TelnetLayer::TelnetCommand::InterpretAsCommand))
+			{
+				// "FF FF" means data continue
+				return TelnetSequenceType::UserData;
+			}
+
+			// "FF X" where X != "FF" means command
+			return TelnetSequenceType::Command;
+		}
+
+		/// @brief Checks if a given sequence matches Telnet command pattern.
+		/// @param first Start of the sequence to check
+		/// @param maxCount Maximum number of bytes to check
+		/// @return True if the buffer matches Telnet command pattern, false otherwise
+		bool isTelnetCommand(uint8_t const* first, size_t maxCount)
+		{
+			return getTelnetSequenceType(first, maxCount) == TelnetSequenceType::Command;
+		}
+	}  // namespace
 
 	size_t TelnetLayer::distanceToNextIAC(uint8_t* startPos, size_t maxLength)
 	{
@@ -64,36 +114,65 @@ namespace pcpp
 
 	uint8_t* TelnetLayer::getNextDataField(uint8_t* pos, size_t len)
 	{
-		size_t offset = 0;
-		while (offset < len)
-		{
-			// Move to next field
-			size_t length = getFieldLen(pos, len - offset);
-			pos += length;
-			offset += length;
+		// This assumes `pos` points to the start of a valid field.
+		auto const endIt = pos + len;
 
-			if (isDataField(pos))
+		// Advance to the next field, as we are skipping the current one from the search.
+		pos += getFieldLen(pos, len);
+
+		while (pos < endIt)
+		{
+			// Check if the current field is data
+			switch (getTelnetSequenceType(pos, std::distance(pos, endIt)))
+			{
+			case TelnetSequenceType::Unknown:
+			{
+				PCPP_LOG_DEBUG("Telnet Parse Error: Unknown sequence found during data field search.");
+				return nullptr;
+			}
+			case TelnetSequenceType::UserData:
 				return pos;
+			default:
+				break;  // continue searching
+			}
+
+			// If not data, move to next field
+			pos += getFieldLen(pos, std::distance(pos, endIt));
 		}
 
+		// If we got here, no data field has been found before the end of the buffer
 		return nullptr;
 	}
 
 	uint8_t* TelnetLayer::getNextCommandField(uint8_t* pos, size_t len)
 	{
-		size_t offset = 0;
-		while (offset < len)
-		{
-			// Move to next field
-			size_t length = getFieldLen(pos, len - offset);
-			pos += length;
-			offset += length;
+		// This assumes `pos` points to the start of a valid field.
+		auto const endIt = pos + len;
+
+		// Advance to the next field, as we are skipping the current one from the search.
+		pos += getFieldLen(pos, len);
 
-			if ((static_cast<size_t>(pos - m_Data) <= (m_DataLen - 2)) &&
-			    isCommandField(pos))  // Need at least 2 bytes for command
+		while (pos < endIt)
+		{
+			// Check if the current field is command
+			switch (getTelnetSequenceType(pos, std::distance(pos, endIt)))
+			{
+			case TelnetSequenceType::Unknown:
+			{
+				PCPP_LOG_DEBUG("Telnet Parse Error: Unknown sequence found during command field search.");
+				return nullptr;
+			}
+			case TelnetSequenceType::Command:
 				return pos;
+			default:
+				break;  // continue searching
+			}
+
+			// If not command, move to next field
+			pos += getFieldLen(pos, std::distance(pos, endIt));
 		}
 
+		// If we got here, no command field has been found before the end of the buffer
 		return nullptr;
 	}
 
@@ -117,36 +196,59 @@ namespace pcpp
 
 	std::string TelnetLayer::getDataAsString(bool removeEscapeCharacters)
 	{
-		uint8_t* dataPos = nullptr;
-		if (isDataField(m_Data))
-			dataPos = m_Data;
-		else
-			dataPos = getNextDataField(m_Data, m_DataLen);
-
-		if (!dataPos)
+		if (m_Data == nullptr)
 		{
-			PCPP_LOG_DEBUG("Packet does not have a data field");
-			return std::string();
+			PCPP_LOG_DEBUG("Layer does not have data");
+			return {};
 		}
 
 		// Convert to string
 		if (removeEscapeCharacters)
 		{
-			std::stringstream ss;
-			for (size_t idx = 0; idx < m_DataLen - (dataPos - m_Data) + 1; ++idx)
+			uint8_t* dataPos = nullptr;
+			switch (getTelnetSequenceType(m_Data, m_DataLen))
+			{
+			case TelnetSequenceType::Unknown:
+			{
+				PCPP_LOG_DEBUG("Telnet Parse Error: Unknown sequence found during data string extraction.");
+				return {};
+			}
+			case TelnetSequenceType::UserData:
+				dataPos = m_Data;
+				break;
+			case TelnetSequenceType::Command:
+				dataPos = getNextDataField(m_Data, m_DataLen);
+				break;
+			default:
+				throw std::logic_error("Unsupported sequence type");
+			}
+
+			if (!dataPos)
 			{
-				if (int(dataPos[idx]) < 127 && int(dataPos[idx]) > 31)  // From SPACE to ~
-					ss << dataPos[idx];
+				PCPP_LOG_DEBUG("Packet does not have a data field");
+				return std::string();
 			}
-			return ss.str();
+
+			PCPP_ASSERT(dataPos >= m_Data && dataPos < (m_Data + m_DataLen),
+			            "Data position is out of bounds, this should never happen!");
+
+			// End of range is corrected by the advance offset.
+			auto const* beginIt = dataPos;
+			auto const* endIt = dataPos + m_DataLen - std::distance(m_Data, dataPos);
+
+			std::string result;
+			std::copy_if(beginIt, endIt, std::back_inserter(result), [](char ch) -> bool {
+				return ch > 31 && ch < 127;  // From SPACE to ~
+			});
+			return result;
 		}
 		return std::string((char*)m_Data, m_DataLen);
 	}
 
 	size_t TelnetLayer::getTotalNumberOfCommands()
 	{
 		size_t ctr = 0;
-		if (isCommandField(m_Data))
+		if (isTelnetCommand(m_Data, m_DataLen))
 			++ctr;
 
 		uint8_t* pos = m_Data;
@@ -167,7 +269,7 @@ namespace pcpp
 			return 0;
 
 		size_t ctr = 0;
-		if (isCommandField(m_Data) && m_Data[1] == static_cast<int>(command))
+		if (isTelnetCommand(m_Data, m_DataLen) && m_Data[1] == static_cast<int>(command))
 			++ctr;
 
 		uint8_t* pos = m_Data;
@@ -185,7 +287,7 @@ namespace pcpp
 	TelnetLayer::TelnetCommand TelnetLayer::getFirstCommand()
 	{
 		// If starts with command
-		if (isCommandField(m_Data))
+		if (isTelnetCommand(m_Data, m_DataLen))
 			return static_cast<TelnetCommand>(m_Data[1]);
 
 		// Check is there any command
@@ -200,7 +302,7 @@ namespace pcpp
 		if (lastPositionOffset == SIZE_MAX)
 		{
 			lastPositionOffset = 0;
-			if (isCommandField(m_Data))
+			if (isTelnetCommand(m_Data, m_DataLen))
 				return static_cast<TelnetLayer::TelnetCommand>(m_Data[1]);
 		}
 
@@ -231,7 +333,7 @@ namespace pcpp
 			return TelnetOption::TelnetOptionNoOption;
 		}
 
-		if (isCommandField(m_Data) && m_Data[1] == static_cast<int>(command))
+		if (isTelnetCommand(m_Data, m_DataLen) && m_Data[1] == static_cast<int>(command))
 			return static_cast<TelnetOption>(getSubCommand(m_Data, getFieldLen(m_Data, m_DataLen)));
 
 		uint8_t* pos = m_Data;
@@ -271,7 +373,7 @@ namespace pcpp
 			return nullptr;
 		}
 
-		if (isCommandField(m_Data) && m_Data[1] == static_cast<int>(command))
+		if (isTelnetCommand(m_Data, m_DataLen) && m_Data[1] == static_cast<int>(command))
 		{
 			size_t lenBuffer = getFieldLen(m_Data, m_DataLen);
 			uint8_t* posBuffer = getCommandData(m_Data, lenBuffer);
@@ -473,9 +575,18 @@ namespace pcpp
 
 	std::string TelnetLayer::toString() const
 	{
-		if (isDataField(m_Data))
+		// TODO: Perhaps print the entire sequence of commands and data?
+		switch (getTelnetSequenceType(m_Data, m_DataLen))
+		{
+		case TelnetSequenceType::Unknown:
+			return "Telnet Unknown";
+		case TelnetSequenceType::Command:
+			return "Telnet Control";
+		case TelnetSequenceType::UserData:
 			return "Telnet Data";
-		return "Telnet Control";
+		default:
+			throw std::logic_error("Unsupported sequence type");
+		}
 	}
 
 }  // namespace pcpp