diff --git a/src/core/coap/coap_message.cpp b/src/core/coap/coap_message.cpp index e5d7475e8..b2fb0120c 100644 --- a/src/core/coap/coap_message.cpp +++ b/src/core/coap/coap_message.cpp @@ -822,6 +822,7 @@ Error Option::Iterator::Advance(void) optionDelta = (headerByte & Message::kOptionDeltaMask) >> Message::kOptionDeltaOffset; SuccessOrExit(error = ReadExtendedOptionField(optionDelta)); + VerifyOrExit(CanAddSafely(mOption.mNumber, optionDelta), error = kErrorParse); optionLength = (headerByte & Message::kOptionLengthMask) >> Message::kOptionLengthOffset; SuccessOrExit(error = ReadExtendedOptionField(optionLength)); diff --git a/tests/unit/test_coap_overflow.cpp b/tests/unit/test_coap_overflow.cpp index 83a43726c..50116dce3 100644 --- a/tests/unit/test_coap_overflow.cpp +++ b/tests/unit/test_coap_overflow.cpp @@ -67,6 +67,88 @@ void TestCoapOverflow(void) testFreeInstance(instance); } +void TestCoapOptionNumberOverflow(void) +{ + Instance *instance; + Coap::Message *message; + + // Two options with deltas 65535 then 12. Each delta is individually + // valid, but their cumulative sum (65547) exceeds the 16-bit option + // number and would wrap to 11 (Uri-Path), producing a non-monotonic + // option sequence that RFC 7252 forbids. + // + // Byte 0xe0 : option 1, delta 14 (2-byte extension), length 0. + // Bytes 0xfe 0xf2 : extended delta 0xfef2 + 269 = 65535. + // Byte 0xc0 : option 2, delta 12, length 0. + + static constexpr uint8_t kOptions[] = {0xe0, 0xfe, 0xf2, 0xc0}; + + printf("TestCoapOptionNumberOverflow()\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(kOptions, sizeof(kOptions))); + + Coap::Option::Iterator iterator; + + SuccessOrQuit(iterator.Init(*message)); + VerifyOrQuit(!iterator.IsDone()); + VerifyOrQuit(iterator.GetOption()->GetNumber() == 65535); + + // The second option would wrap the running option number and must be rejected. + VerifyOrQuit(iterator.Advance() == kErrorParse, "Fix failed: cumulative option number overflow was not rejected!"); + VerifyOrQuit(iterator.IsDone()); + VerifyOrQuit(iterator.GetOption() == nullptr); + + message->Free(); + testFreeInstance(instance); +} + +void TestCoapOptionNumberMaxRepeated(void) +{ + Instance *instance; + Coap::Message *message; + + // Option 1: delta 65535 (0xe0, 0xfe, 0xf2), length 0. + // Option 2: delta 0 (0x00), length 0. + // Verifies that repeated options with delta 0 at max option number 65535 are accepted. + + static constexpr uint8_t kOptions[] = {0xe0, 0xfe, 0xf2, 0x00}; + + printf("TestCoapOptionNumberMaxRepeated()\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(kOptions, sizeof(kOptions))); + + Coap::Option::Iterator iterator; + + SuccessOrQuit(iterator.Init(*message)); + VerifyOrQuit(!iterator.IsDone()); + VerifyOrQuit(iterator.GetOption()->GetNumber() == 65535); + + SuccessOrQuit(iterator.Advance()); + VerifyOrQuit(!iterator.IsDone()); + VerifyOrQuit(iterator.GetOption()->GetNumber() == 65535); + + SuccessOrQuit(iterator.Advance()); + VerifyOrQuit(iterator.IsDone()); + VerifyOrQuit(iterator.GetOption() == nullptr); + + message->Free(); + testFreeInstance(instance); +} + #if OPENTHREAD_CONFIG_COAP_BLOCKWISE_TRANSFER_ENABLE void TestReadBlockOptionValuesInvalidLength(void) { @@ -126,6 +208,8 @@ void TestReadBlockOptionValuesInvalidLength(void) int main(void) { ot::TestCoapOverflow(); + ot::TestCoapOptionNumberOverflow(); + ot::TestCoapOptionNumberMaxRepeated(); #if OPENTHREAD_CONFIG_COAP_BLOCKWISE_TRANSFER_ENABLE ot::TestReadBlockOptionValuesInvalidLength(); #endif