From f42bf5f2b9db6f362379e613246e59cf308c5a22 Mon Sep 17 00:00:00 2001 From: Jonathan Hui Date: Sat, 22 Aug 2026 15:43:45 -0700 Subject: [PATCH] [ip6] fix out-of-bounds access and length corruption in Prefix (#13537) `Ip6::Prefix::SizeForLength()` returned uncapped byte sizes for prefix bit lengths greater than 128. This resulted in out-of-bounds memory access and state corruption: - In `Prefix::Tidy()`, `byteLength = GetBytesSize()` returned values greater than 16. For lengths 129-136, masking `m8[byteLength - 1]` overwrote `mLength` itself (offset 16 in packed `otIp6Prefix`), and for lengths >= 137, wrote beyond the struct. Furthermore, `(byteLength * 8 - mLength)` underflowed `uint8_t`, leading to undefined bit-shift behavior. - In `Prefix::ToString()`, `sizeInUint16` calculated from `GetBytesSize()` exceeded 8 words, causing `AppendHexWords()` to read past the 16-byte address. This commit resolves these issues by: - Capping `Prefix::SizeForLength()` to `Prefix::kMaxSize` (16 bytes). - Adding `VerifyOrExit(mLength < kMaxLength)` in `Prefix::Tidy()`. - Updating `PrefixTlv::IsValid()` to check `mPrefixLength <= Ip6::Prefix::kMaxLength`. - Adding unit test `TestIp6PrefixInvalidLengths()` in `test_ip_address.cpp`. --- src/core/net/ip6_address.cpp | 12 ++++- src/core/net/ip6_address.hpp | 3 +- src/core/thread/network_data_tlvs.hpp | 2 +- tests/unit/test_ip_address.cpp | 66 +++++++++++++++++++++++++++ 4 files changed, 79 insertions(+), 4 deletions(-) diff --git a/src/core/net/ip6_address.cpp b/src/core/net/ip6_address.cpp index 34da878f5..1f6e4dcf2 100644 --- a/src/core/net/ip6_address.cpp +++ b/src/core/net/ip6_address.cpp @@ -97,8 +97,13 @@ bool Prefix::IsCoveredBy(const NetworkPrefix &aNetworkPrefix) const void Prefix::Tidy(void) { - uint8_t byteLength = GetBytesSize(); - uint8_t lastByteBitMask = static_cast(~(static_cast(1 << (byteLength * 8 - mLength)) - 1)); + uint8_t byteLength; + uint8_t lastByteBitMask; + + VerifyOrExit(mLength < kMaxLength); + + byteLength = GetBytesSize(); + lastByteBitMask = static_cast(0xffU << (byteLength * 8 - mLength)); if (byteLength != 0) { @@ -109,6 +114,9 @@ void Prefix::Tidy(void) { mPrefix.mFields.m8[i] = 0; } + +exit: + return; } bool Prefix::operator==(const Prefix &aOther) const diff --git a/src/core/net/ip6_address.hpp b/src/core/net/ip6_address.hpp index 0f1a7e4af..4c746157b 100644 --- a/src/core/net/ip6_address.hpp +++ b/src/core/net/ip6_address.hpp @@ -43,6 +43,7 @@ #include "common/clearable.hpp" #include "common/encoding.hpp" #include "common/equatable.hpp" +#include "common/num_utils.hpp" #include "common/string.hpp" #include "mac/mac_types.hpp" @@ -275,7 +276,7 @@ public: * * @returns The size (in bytes) of the prefix. */ - static uint8_t SizeForLength(uint8_t aLength) { return BytesForBitSize(aLength); } + static uint8_t SizeForLength(uint8_t aLength) { return Min(BytesForBitSize(aLength), kMaxSize); } /** * Indicates whether or not a given prefix length is valid for use as a NAT64 prefix. diff --git a/src/core/thread/network_data_tlvs.hpp b/src/core/thread/network_data_tlvs.hpp index a9c8fea89..49cf7d184 100644 --- a/src/core/thread/network_data_tlvs.hpp +++ b/src/core/thread/network_data_tlvs.hpp @@ -620,7 +620,7 @@ public: { return ((GetLength() >= sizeof(*this) - sizeof(NetworkDataTlv)) && (GetLength() >= Ip6::Prefix::SizeForLength(mPrefixLength) + sizeof(*this) - sizeof(NetworkDataTlv)) && - (Ip6::Prefix::SizeForLength(mPrefixLength) <= sizeof(Ip6::Address))); + (mPrefixLength <= Ip6::Prefix::kMaxLength)); } /** diff --git a/tests/unit/test_ip_address.cpp b/tests/unit/test_ip_address.cpp index 1be067b0c..167fbca7d 100644 --- a/tests/unit/test_ip_address.cpp +++ b/tests/unit/test_ip_address.cpp @@ -929,6 +929,71 @@ void TestIp4Cidr(void) } } +void TestIp6PrefixInvalidLengths(void) +{ + static const uint8_t kInvalidLengths[] = {129, 130, 136, 137, 144, 200, 255}; + static const uint8_t kRawPrefix[OT_IP6_ADDRESS_SIZE] = { + 0x20, 0x01, 0x0d, 0xb8, 0x00, 0x01, 0x00, 0x02, 0x00, 0x03, 0x00, 0x04, 0x00, 0x05, 0x00, 0x06, + }; + static const char kExpectedPrefixString[] = "2001:db8:1:2:3:4:5:6"; + + OT_TOOL_PACKED_BEGIN + struct + { + Ip6::Prefix mPrefix; + uint8_t mCanary[16]; + } OT_TOOL_PACKED_END packedPrefix; + + printf("\nTestIp6PrefixInvalidLengths()\n"); + + for (uint16_t len = 0; len <= 255; len++) + { + uint8_t length = static_cast(len); + + if (length <= Ip6::Prefix::kMaxLength) + { + VerifyOrQuit(Ip6::Prefix::SizeForLength(length) == (length + 7) / 8); + } + else + { + VerifyOrQuit(Ip6::Prefix::SizeForLength(length) == Ip6::Prefix::kMaxSize); + } + } + + for (uint8_t invalidLen : kInvalidLengths) + { + char expectedString[Ip6::Prefix::kInfoStringSize]; + + memset(&packedPrefix, 0, sizeof(packedPrefix)); + memset(packedPrefix.mCanary, 0xaa, sizeof(packedPrefix.mCanary)); + + packedPrefix.mPrefix.InitFrom(kRawPrefix, invalidLen); + VerifyOrQuit(packedPrefix.mPrefix.GetLength() == invalidLen); + VerifyOrQuit(!packedPrefix.mPrefix.IsValid()); + VerifyOrQuit(packedPrefix.mPrefix.GetBytesSize() == Ip6::Prefix::kMaxSize); + + for (uint8_t b : packedPrefix.mCanary) + { + VerifyOrQuit(b == 0xaa, "Prefix::InitFrom corrupted memory beyond prefix"); + } + + packedPrefix.mPrefix.Tidy(); + + VerifyOrQuit(packedPrefix.mPrefix.GetLength() == invalidLen, "Prefix::Tidy mutated invalid length"); + VerifyOrQuit(memcmp(packedPrefix.mPrefix.GetBytes(), kRawPrefix, sizeof(kRawPrefix)) == 0, + "Prefix::Tidy corrupted prefix bytes"); + + for (uint8_t b : packedPrefix.mCanary) + { + VerifyOrQuit(b == 0xaa, "Prefix::Tidy wrote out of bounds"); + } + + snprintf(expectedString, sizeof(expectedString), "%s/%u", kExpectedPrefixString, invalidLen); + VerifyOrQuit(strcmp(packedPrefix.mPrefix.ToString().AsCString(), expectedString) == 0, + "Prefix::ToString() output mismatch for length > 128"); + } +} + } // namespace ot int main(void) @@ -939,6 +1004,7 @@ int main(void) ot::TestIp6PrefixFromString(); ot::TestIp6Prefix(); ot::TestIp6PrefixTidy(); + ot::TestIp6PrefixInvalidLengths(); ot::TestIp4MappedIp6Address(); ot::TestIp4Ip6Translation(); ot::TestIp4Cidr();