From edd387d04eb99dae713f055ac521e6cb3cb85111 Mon Sep 17 00:00:00 2001 From: Jonathan Hui Date: Wed, 11 Mar 2026 14:03:42 -0500 Subject: [PATCH] [coap] enhance CoAP option parsing validation and robustness (#12670) This commit improves the robustness of CoAP option parsing by adding rigorous validation checks to prevent potential overflows and null pointer dereferences. Summary of changes: 1. In 'ReadExtendedOptionField()', added an overflow check when calculating extended lengths for 2-byte extensions. It now returns 'kErrorParse' if the value would exceed the 16-bit range. 2. In 'ReadBlockOptionValues()', added a check to ensure the block option exists before accessing it. This prevents a crash when 'GetOption()' returns null. 3. In 'ReadBlockOptionValues()', added length validation to ensure the option value does not exceed the local buffer size (5 bytes) before copying. 4. Added a new unit test 'test_coap_overflow' to verify these validation checks and ensure they correctly handle malformed or missing options. --- src/core/coap/coap_message.cpp | 6 +- tests/unit/CMakeLists.txt | 1 + tests/unit/test_coap_overflow.cpp | 134 ++++++++++++++++++++++++++++++ 3 files changed, 140 insertions(+), 1 deletion(-) create mode 100644 tests/unit/test_coap_overflow.cpp diff --git a/src/core/coap/coap_message.cpp b/src/core/coap/coap_message.cpp index 863592b78..b56a0a5e1 100644 --- a/src/core/coap/coap_message.cpp +++ b/src/core/coap/coap_message.cpp @@ -33,6 +33,7 @@ #include "coap_message.hpp" +#include "common/numeric_limits.hpp" #include "instance/instance.hpp" namespace ot { @@ -485,6 +486,8 @@ Error Message::ReadBlockOptionValues(uint16_t aBlockOptionNumber, BlockInfo &aIn } SuccessOrExit(error = iterator.Init(*this, aBlockOptionNumber)); + VerifyOrExit(!iterator.IsDone(), error = kErrorNotFound); + VerifyOrExit(iterator.GetOption()->GetLength() <= sizeof(buf), error = kErrorParse); SuccessOrExit(error = iterator.ReadOptionValue(buf)); switch (iterator.GetOption()->GetLength()) @@ -878,7 +881,8 @@ Error Option::Iterator::ReadExtendedOptionField(uint16_t &aValue) SuccessOrExit(error = Read(sizeof(uint16_t), &value16)); value16 = BigEndian::HostSwap16(value16); - aValue = value16 + Message::kOption2ByteExtensionOffset; + VerifyOrExit(CanAddSafely(value16, Message::kOption2ByteExtensionOffset), error = kErrorParse); + aValue = value16 + Message::kOption2ByteExtensionOffset; } else { diff --git a/tests/unit/CMakeLists.txt b/tests/unit/CMakeLists.txt index f613f5c1f..fbd1da0b5 100644 --- a/tests/unit/CMakeLists.txt +++ b/tests/unit/CMakeLists.txt @@ -207,6 +207,7 @@ ot_unit_test(child) ot_unit_test(child_table) ot_unit_test(cmd_line_parser) ot_unit_test(coap_message) +ot_unit_test(coap_overflow) ot_unit_test(crc) ot_unit_test(data) ot_unit_test(dataset) diff --git a/tests/unit/test_coap_overflow.cpp b/tests/unit/test_coap_overflow.cpp new file mode 100644 index 000000000..83a43726c --- /dev/null +++ b/tests/unit/test_coap_overflow.cpp @@ -0,0 +1,134 @@ +/* + * Copyright (c) 2026, The OpenThread Authors. + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are met: + * 1. Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * 2. Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in the + * documentation and/or other materials provided with the distribution. + * 3. Neither the name of the copyright holder nor the + * names of its contributors may be used to endorse or promote products + * derived from this software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE + * POSSIBILITY OF SUCH DAMAGE. + */ + +#include "test_platform.h" +#include "test_util.hpp" +#include "instance/instance.hpp" + +namespace ot { + +void TestCoapOverflow(void) +{ + Instance *instance; + Coap::Message *message; + uint8_t payload[] = {0x0e, 0xff, 0x18}; // Delta 0, Length 14 (extended 2 bytes), 0xff18 + // value16 = 0xff18 = 65304. + // aValue = 65304 + 269 = 65573. + // Truncated to uint16_t: 37. + + printf("TestCoapOverflow()\n"); + + instance = static_cast(testInitInstance()); + VerifyOrQuit(instance != nullptr); + + message = AsCoapMessagePtr(instance->Get().Allocate(Message::kTypeOther)); + VerifyOrQuit(message != nullptr); + + SuccessOrQuit(message->Init(Coap::kTypeNonConfirmable, Coap::kCodePut)); + + // Add some padding bytes to allow the "37" bytes read to not fail due to message end. + uint8_t padding[100]; + memset(padding, 0, sizeof(padding)); + SuccessOrQuit(message->AppendBytes(payload, sizeof(payload))); + SuccessOrQuit(message->AppendBytes(padding, sizeof(padding))); + + Coap::Option::Iterator iterator; + Error error = iterator.Init(*message); + + // After fix, this should return kErrorParse. + VerifyOrQuit(error == kErrorParse, "Fix failed: iterator.Init() did not return kErrorParse for truncated length!"); + + message->Free(); + testFreeInstance(instance); +} + +#if OPENTHREAD_CONFIG_COAP_BLOCKWISE_TRANSFER_ENABLE +void TestReadBlockOptionValuesInvalidLength(void) +{ + Instance *instance; + Coap::Message *message; + + // Header for Block2 (23) with an invalid length of 6. + // A well-formed Block Option's value is at most 3 bytes long. + // A malformed message could have a longer length. The check in + // `ReadBlockOptionValues` should reject this before attempting to + // copy the value into a small local buffer, preventing an overflow. + // + // Delta 23 = 13 + 10. + // Byte 0: 0xd6 (Delta 13, Length 6) + // Byte 1: 0x0a (Delta extension) + // Bytes 2-7: Option value (6 bytes) + + uint8_t payload[] = {0xd6, 0x0a, 0, 0, 0, 0, 0, 0}; + uint8_t padding[100]; + memset(padding, 0xaa, sizeof(padding)); // Fill with 0xaa to see it in overflow + + printf("TestReadBlockOptionValuesInvalidLength()\n"); + + instance = static_cast(testInitInstance()); + VerifyOrQuit(instance != nullptr); + + message = AsCoapMessagePtr(instance->Get().Allocate(Message::kTypeOther)); + VerifyOrQuit(message != nullptr); + + SuccessOrQuit(message->Init(Coap::kTypeNonConfirmable, Coap::kCodePut)); + SuccessOrQuit(message->AppendBytes(payload, sizeof(payload))); + SuccessOrQuit(message->AppendBytes(padding, sizeof(padding))); + + Coap::BlockInfo blockInfo; + Error error; + // After fix, this should return kErrorParse and NOT crash. + error = message->ReadBlockOptionValues(Coap::kOptionBlock2, blockInfo); + VerifyOrQuit(error == kErrorParse, + "Fix failed: ReadBlockOptionValues did not return kErrorParse for oversized option!"); + + message->Free(); + + message = AsCoapMessagePtr(instance->Get().Allocate(Message::kTypeOther)); + VerifyOrQuit(message != nullptr); + SuccessOrQuit(message->Init(Coap::kTypeNonConfirmable, Coap::kCodePut)); + + error = message->ReadBlockOptionValues(Coap::kOptionBlock2, blockInfo); + VerifyOrQuit(error == kErrorNotFound, "ReadBlockOptionValues did not return kErrorNotFound for missing option!"); + + message->Free(); + testFreeInstance(instance); +} +#endif + +} // namespace ot + +int main(void) +{ + ot::TestCoapOverflow(); +#if OPENTHREAD_CONFIG_COAP_BLOCKWISE_TRANSFER_ENABLE + ot::TestReadBlockOptionValuesInvalidLength(); +#endif + printf("All tests passed\n"); + return 0; +}