From 7ca21a38f27b1040384178661a72a0d2b426e9e3 Mon Sep 17 00:00:00 2001 From: Abtin Keshavarzian Date: Wed, 26 Jun 2024 18:37:01 -0700 Subject: [PATCH] [tlv] ensure handling of extended TLVs when iterating over sub-TLVs (#10439) This commit adds the `Tlv::ParseAndSkipTlv()` static method, which parses a TLV (regular or extended) in a message at a given offset. It validates that the TLV is fully contained within the message and updates the offset to skip over the entire parsed TLV. This helper method is used in various modules where manual iteration over a sequence of TLVs is performed, specifically to skip over extended TLVs. The following methods are updated to utilize this new method and perform additional TLV checks: - `DiscoverScanner::HandleDiscoveryResponse()` - `MleRouter::HandleDiscoveryRequest()` - `LinkMetrics::SubJect::AppendReport()` - `LinkMetrics::Subject::HandleManagementRequest()` - `LinkMetrics::Initiator::HandleReport()` --- src/core/common/tlvs.cpp | 18 ++++++++++++++++++ src/core/common/tlvs.hpp | 15 +++++++++++++++ src/core/thread/discover_scanner.cpp | 13 +++++++++++-- src/core/thread/link_metrics.cpp | 27 ++++++++++++++++++++++++++- src/core/thread/mle_router.cpp | 13 +++++++++++-- 5 files changed, 81 insertions(+), 5 deletions(-) diff --git a/src/core/common/tlvs.cpp b/src/core/common/tlvs.cpp index 04756d24a..2848e7f0c 100644 --- a/src/core/common/tlvs.cpp +++ b/src/core/common/tlvs.cpp @@ -56,6 +56,24 @@ const uint8_t *Tlv::GetValue(void) const Error Tlv::AppendTo(Message &aMessage) const { return aMessage.AppendBytes(this, static_cast(GetSize())); } +Error Tlv::ParseAndSkipTlv(const Message &aMessage, uint16_t &aOffset) +{ + Error error; + ParsedInfo info; + + SuccessOrExit(error = info.ParseFrom(aMessage, aOffset)); + + // `ParseFrom()` has already validated that the entire TLV is + // present within `aMessage`. This ensures that `aOffset + mSize` + // is less than `aMessage.GetLength()`, and therefore we cannot + // have an overflow here. + + aOffset += info.mSize; + +exit: + return error; +} + Error Tlv::FindTlv(const Message &aMessage, uint8_t aType, uint16_t aMaxSize, Tlv &aTlv) { uint16_t offset; diff --git a/src/core/common/tlvs.hpp b/src/core/common/tlvs.hpp index 1acc80694..7baf529a3 100644 --- a/src/core/common/tlvs.hpp +++ b/src/core/common/tlvs.hpp @@ -244,6 +244,21 @@ public: //------------------------------------------------------------------------------------------------------------------ // Static methods for reading/finding/appending TLVs in a `Message`. + /** + * Parses a TLV in a message at a given offset, validating that it is fully contained within the message and then + * updating the offset to skip over the entire parsed TLV. + * + * Can be used independent of whether the read TLV (from the message) is an Extended TLV or not. + * + * @param[in] aMessage The message to read from. + * @param[in,out] aOffset The offset to read from. On success, it is updated to point after the parsed TLV. + * + * @retval kErrorNone Successfully parsed a TLV from @p aMessage. @p aOffset is updated. + * @retval kErrorParse The TLV was not well-formed or was not fully contained in @p aMessage. + * + */ + static Error ParseAndSkipTlv(const Message &aMessage, uint16_t &aOffset); + /** * Reads a TLV's value in a message at a given offset expecting a minimum length for the value. * diff --git a/src/core/thread/discover_scanner.cpp b/src/core/thread/discover_scanner.cpp index e264a7be8..03e2ca4eb 100644 --- a/src/core/thread/discover_scanner.cpp +++ b/src/core/thread/discover_scanner.cpp @@ -335,12 +335,21 @@ void DiscoverScanner::HandleDiscoveryResponse(Mle::RxInfo &aRxInfo) const // Process MeshCoP TLVs while (offset < end) { - IgnoreError(aRxInfo.mMessage.Read(offset, meshcopTlv)); + SuccessOrExit(error = aRxInfo.mMessage.Read(offset, meshcopTlv)); + + if (meshcopTlv.IsExtended()) + { + SuccessOrExit(error = Tlv::ParseAndSkipTlv(aRxInfo.mMessage, offset)); + VerifyOrExit(offset <= end, error = kErrorParse); + continue; + } + + VerifyOrExit(meshcopTlv.GetSize() + offset <= aRxInfo.mMessage.GetLength(), error = kErrorParse); switch (meshcopTlv.GetType()) { case MeshCoP::Tlv::kDiscoveryResponse: - IgnoreError(aRxInfo.mMessage.Read(offset, discoveryResponse)); + SuccessOrExit(error = aRxInfo.mMessage.Read(offset, discoveryResponse)); VerifyOrExit(discoveryResponse.IsValid(), error = kErrorParse); result.mVersion = discoveryResponse.GetVersion(); result.mIsNative = discoveryResponse.IsNativeCommissioner(); diff --git a/src/core/thread/link_metrics.cpp b/src/core/thread/link_metrics.cpp index a8936f371..840d28e34 100644 --- a/src/core/thread/link_metrics.cpp +++ b/src/core/thread/link_metrics.cpp @@ -146,7 +146,14 @@ void Initiator::HandleReport(const Message &aMessage, uint16_t aOffset, uint16_t { SuccessOrExit(error = aMessage.Read(offset, tlv)); - VerifyOrExit(offset + sizeof(Tlv) + tlv.GetLength() <= endOffset, error = kErrorParse); + if (tlv.IsExtended()) + { + SuccessOrExit(error = Tlv::ParseAndSkipTlv(aMessage, offset)); + VerifyOrExit(offset <= endOffset, error = kErrorParse); + continue; + } + + VerifyOrExit(tlv.GetSize() + offset <= endOffset, error = kErrorParse); // The report must contain either: // - One or more Report Sub-TLVs (in case of success), or @@ -320,6 +327,15 @@ Error Initiator::HandleManagementResponse(const Message &aMessage, const Ip6::Ad SuccessOrExit(error = aMessage.Read(offset, tlv)); + if (tlv.IsExtended()) + { + SuccessOrExit(error = Tlv::ParseAndSkipTlv(aMessage, offset)); + VerifyOrExit(offset <= endOffset, error = kErrorParse); + continue; + } + + VerifyOrExit(tlv.GetSize() + offset <= endOffset, error = kErrorParse); + switch (tlv.GetType()) { case StatusSubTlv::kType: @@ -440,6 +456,15 @@ Error Subject::AppendReport(Message &aMessage, const Message &aRequestMessage, N { SuccessOrExit(error = aRequestMessage.Read(offset, tlv)); + if (tlv.IsExtended()) + { + SuccessOrExit(error = Tlv::ParseAndSkipTlv(aMessage, offset)); + VerifyOrExit(offset <= endOffset, error = kErrorParse); + continue; + } + + VerifyOrExit(tlv.GetSize() + offset <= endOffset, error = kErrorParse); + switch (tlv.GetType()) { case SubTlv::kQueryId: diff --git a/src/core/thread/mle_router.cpp b/src/core/thread/mle_router.cpp index 542cb370b..e03712345 100644 --- a/src/core/thread/mle_router.cpp +++ b/src/core/thread/mle_router.cpp @@ -2637,12 +2637,21 @@ void MleRouter::HandleDiscoveryRequest(RxInfo &aRxInfo) while (offset < end) { - IgnoreError(aRxInfo.mMessage.Read(offset, meshcopTlv)); + SuccessOrExit(error = aRxInfo.mMessage.Read(offset, meshcopTlv)); + + if (meshcopTlv.IsExtended()) + { + SuccessOrExit(error = Tlv::ParseAndSkipTlv(aRxInfo.mMessage, offset)); + VerifyOrExit(offset <= end, error = kErrorParse); + continue; + } + + VerifyOrExit(meshcopTlv.GetSize() + offset <= aRxInfo.mMessage.GetLength(), error = kErrorParse); switch (meshcopTlv.GetType()) { case MeshCoP::Tlv::kDiscoveryRequest: - IgnoreError(aRxInfo.mMessage.Read(offset, discoveryRequestTlv)); + SuccessOrExit(error = aRxInfo.mMessage.Read(offset, discoveryRequestTlv)); VerifyOrExit(discoveryRequestTlv.IsValid(), error = kErrorParse); break;