From 968dbb2a04d7fc2722fb6056cf6ee107218d14ab Mon Sep 17 00:00:00 2001 From: Abtin Keshavarzian Date: Wed, 20 Aug 2025 22:22:39 -0700 Subject: [PATCH] [message] check for potential integer overflows (#11834) This commit adds checks to prevent potential integer overflow issues within the `Message` class. Previously, calculations involving message offset and length, such as `offset + length`, assumed the caller would provide values within a safe range. However, in some edge cases where larger values are given, this addition could wrap around. This could lead to incorrect behavior, potential memory corruption, or assertion failures. To address this, this change introduces a new generic utility function, `CanAddSafely()`, to detect unsigned integer addition overflows. This check is now applied in the following `Message` methods to validate lengths and offsets before performing arithmetic: - `AppendBytes()`: Returns an error if `offset + length` overflows. - `AppendBytesFromMessage()`: Returns an error on overflow. - `GetFirstChunk()`: Safely clamps the read length to the available message length. - `WriteBytes()`: Asserts if `offset + length` overflows. Unit tests for the new `CanAddSafely()` utility are included, covering `uint8_t` and `uint16_t` cases. --- src/core/common/message.cpp | 23 ++++++++++++++++------- src/core/common/num_utils.hpp | 20 ++++++++++++++++++++ tests/unit/test_serial_number.cpp | 25 +++++++++++++++++++++++++ 3 files changed, 61 insertions(+), 7 deletions(-) diff --git a/src/core/common/message.cpp b/src/core/common/message.cpp index ce4002ce7..3c5320de9 100644 --- a/src/core/common/message.cpp +++ b/src/core/common/message.cpp @@ -263,12 +263,14 @@ void Message::Free(void) Error Message::SetLength(uint16_t aLength) { - Error error = kErrorNone; - uint16_t totalLengthRequest = GetReserved() + aLength; + Error error; + uint16_t size; - VerifyOrExit(totalLengthRequest >= GetReserved(), error = kErrorInvalidArgs); + VerifyOrExit(CanAddSafely(GetReserved(), aLength), error = kErrorNoBufs); + + size = GetReserved() + aLength; + SuccessOrExit(error = ResizeMessage(size)); - SuccessOrExit(error = ResizeMessage(totalLengthRequest)); GetMetadata().mLength = aLength; // Correct the offset in case shorter length is set. @@ -368,10 +370,12 @@ void Message::InvokeTxCallback(Error aError) Error Message::AppendBytes(const void *aBuf, uint16_t aLength) { - Error error = kErrorNone; + Error error; uint16_t oldLength = GetLength(); - SuccessOrExit(error = SetLength(GetLength() + aLength)); + VerifyOrExit(CanAddSafely(oldLength, aLength), error = kErrorNoBufs); + + SuccessOrExit(error = SetLength(oldLength + aLength)); WriteBytes(oldLength, aBuf, aLength); exit: @@ -389,7 +393,11 @@ Error Message::AppendBytesFromMessage(const Message &aMessage, uint16_t aOffset, uint16_t writeOffset = GetLength(); Chunk chunk; + VerifyOrExit(CanAddSafely(aOffset, aLength), error = kErrorInvalidArgs); + VerifyOrExit(aMessage.GetLength() >= aOffset + aLength, error = kErrorParse); + + VerifyOrExit(CanAddSafely(GetLength(), aLength), error = kErrorNoBufs); SuccessOrExit(error = SetLength(GetLength() + aLength)); aMessage.GetFirstChunk(aOffset, aLength, chunk); @@ -531,7 +539,7 @@ void Message::GetFirstChunk(uint16_t aOffset, uint16_t &aLength, Chunk &aChunk) VerifyOrExit(aOffset < GetLength(), aChunk.SetLength(0)); - if (aOffset + aLength >= GetLength()) + if (!CanAddSafely(aOffset, aLength) || (aOffset + aLength >= GetLength())) { aLength = GetLength() - aOffset; } @@ -700,6 +708,7 @@ void Message::WriteBytes(uint16_t aOffset, const void *aBuf, uint16_t aLength) const uint8_t *bufPtr = reinterpret_cast(aBuf); MutableChunk chunk; + OT_ASSERT(CanAddSafely(aOffset, aLength)); OT_ASSERT(aOffset + aLength <= GetLength()); GetFirstChunk(aOffset, aLength, chunk); diff --git a/src/core/common/num_utils.hpp b/src/core/common/num_utils.hpp index 70c0fcf62..b89ec3096 100644 --- a/src/core/common/num_utils.hpp +++ b/src/core/common/num_utils.hpp @@ -145,6 +145,26 @@ template int8_t ClampToInt8(IntType aValue) static_cast(NumericLimits::kMax))); } +/** + * Indicates whether or not the addition of two unsigned integers will result in an overflow. + * + * @tparam UintType The value type (MUST be `uint8_t`, `uint16_t`, `uint32_t`, or `uint64_t`). + * + * @param[in] aFirstValue The first operand in the addition. + * @param[in] aSecondValue The second operand in the addition. + * + * @retval TRUE If the addition can be performed safely and does not cause an overflow. + * @retval FALSE If the addition will result in an overflow. + */ +template bool CanAddSafely(UintType aFirstValue, UintType aSecondValue) +{ + static_assert(TypeTraits::IsUint::kValue, "UintType must be an unsigned int (8, 16, 32, or 64 bit len)"); + + UintType sum = static_cast(aFirstValue + aSecondValue); + + return (sum >= aFirstValue); +} + /** * This template function checks whether a given value is in a given closed range [min, max]. * diff --git a/tests/unit/test_serial_number.cpp b/tests/unit/test_serial_number.cpp index 8e388ddb8..8e3213892 100644 --- a/tests/unit/test_serial_number.cpp +++ b/tests/unit/test_serial_number.cpp @@ -110,6 +110,31 @@ void TestNumUtils(void) u32 = 0xfff0000; VerifyOrQuit(ClampToUint16(u32) == 0xffff); + VerifyOrQuit(CanAddSafely(0, 0)); + VerifyOrQuit(CanAddSafely(100, 0)); + VerifyOrQuit(CanAddSafely(0, 100)); + VerifyOrQuit(CanAddSafely(200, 55)); + VerifyOrQuit(CanAddSafely(56, 199)); + VerifyOrQuit(CanAddSafely(127, 127)); + + VerifyOrQuit(!CanAddSafely(200, 56)); + VerifyOrQuit(!CanAddSafely(100, 156)); + VerifyOrQuit(!CanAddSafely(1, 255)); + VerifyOrQuit(!CanAddSafely(255, 1)); + VerifyOrQuit(!CanAddSafely(255, 255)); + VerifyOrQuit(!CanAddSafely(128, 128)); + + VerifyOrQuit(CanAddSafely(0, 0)); + VerifyOrQuit(CanAddSafely(0xffff, 0)); + VerifyOrQuit(CanAddSafely(0, 0xffff)); + VerifyOrQuit(CanAddSafely(0xff00, 0xff)); + VerifyOrQuit(CanAddSafely(0xfff, 0xf000)); + + VerifyOrQuit(!CanAddSafely(0xffff, 1)); + VerifyOrQuit(!CanAddSafely(1, 0xffff)); + VerifyOrQuit(!CanAddSafely(65000, 65000)); + VerifyOrQuit(!CanAddSafely(32768, 32768)); + VerifyOrQuit(IsValueInRange(5, 5, 10)); VerifyOrQuit(IsValueInRange(7, 5, 10)); VerifyOrQuit(IsValueInRange(10, 5, 10));