From 9f451a2b325718579cd1a1a398264a1a410dd1a3 Mon Sep 17 00:00:00 2001
From: Abtin Keshavarzian <abtink@google.com>
Date: Wed, 10 Aug 2022 10:28:32 -0700
Subject: [PATCH] [mle] fix processing of CSL TLVs (#8004)

This commit contains fixes and enhancements related to processing
of CSL Channel TLV and CSL Clock Accuracy TLV in MLE messages:

- Updates/adds `IsValid()` method to check if TLV is well-formed.
  In particular, we check the TLV length to be at least the
  expecte length (but can be larger) to allow for future changes
  to the TLV format (adding new fields while remaining backward
  compatible).

- Ensure to verify that the read TLV is valid before using its
  content.

 - Fix processing of Accuracy TLV in `HandleChildUpdateResponse()`
   (where we could use incorrect values if TLV was not present).
---
 src/core/thread/mle.cpp        | 27 ++++++++++++++++-----------
 src/core/thread/mle_router.cpp |  9 ++++++++-
 src/core/thread/mle_tlvs.hpp   | 11 ++++++++++-
 3 files changed, 34 insertions(+), 13 deletions(-)

diff --git a/src/core/thread/mle.cpp b/src/core/thread/mle.cpp
index 413da2d5f..94268e3c1 100644
--- a/src/core/thread/mle.cpp
+++ b/src/core/thread/mle.cpp
@@ -3127,7 +3127,11 @@ void Mle::HandleParentResponse(RxInfo &aRxInfo)
 
 #if OPENTHREAD_CONFIG_MAC_CSL_RECEIVER_ENABLE
     // CSL Accuracy
-    if (Tlv::FindTlv(aRxInfo.mMessage, clockAccuracy) != kErrorNone)
+    if (Tlv::FindTlv(aRxInfo.mMessage, clockAccuracy) == kErrorNone)
+    {
+        VerifyOrExit(clockAccuracy.IsValid(), error = kErrorParse);
+    }
+    else
     {
         clockAccuracy.SetCslClockAccuracy(kCslWorstCrystalPpm);
         clockAccuracy.SetCslUncertainty(kCslWorstUncertainty);
@@ -3625,8 +3629,9 @@ void Mle::HandleChildUpdateResponse(RxInfo &aRxInfo)
 
 #if OPENTHREAD_CONFIG_MAC_CSL_RECEIVER_ENABLE
         // CSL Accuracy
-        if (Tlv::FindTlv(aRxInfo.mMessage, clockAccuracy) != kErrorNone)
+        if (Tlv::FindTlv(aRxInfo.mMessage, clockAccuracy) == kErrorNone)
         {
+            VerifyOrExit(clockAccuracy.IsValid(), error = kErrorParse);
             Get<Mac::Mac>().SetCslParentClockAccuracy(clockAccuracy.GetCslClockAccuracy());
             Get<Mac::Mac>().SetCslParentUncertainty(clockAccuracy.GetCslUncertainty());
         }
@@ -4808,27 +4813,27 @@ exit:
 
 Error Mle::TxMessage::AppendCslTimeoutTlv(void)
 {
-    OT_ASSERT(Get<Mac::Mac>().IsCslEnabled());
-    return Tlv::Append<CslTimeoutTlv>(*this,
-                                      Get<Mle>().mCslTimeout == 0 ? Get<Mle>().mTimeout : Get<Mle>().mCslTimeout);
+    uint32_t timeout = Get<Mle>().GetCslTimeout();
+
+    if (timeout == 0)
+    {
+        timeout = Get<Mle>().GetTimeout();
+    }
+
+    return Tlv::Append<CslTimeoutTlv>(*this, timeout);
 }
 #endif // OPENTHREAD_CONFIG_MAC_CSL_RECEIVER_ENABLE
 
 #if OPENTHREAD_CONFIG_MAC_CSL_TRANSMITTER_ENABLE
 Error Mle::TxMessage::AppendCslClockAccuracyTlv(void)
 {
-    Error               error = kErrorNone;
     CslClockAccuracyTlv cslClockAccuracy;
 
     cslClockAccuracy.Init();
-
     cslClockAccuracy.SetCslClockAccuracy(Get<Radio>().GetCslAccuracy());
     cslClockAccuracy.SetCslUncertainty(Get<Radio>().GetCslUncertainty());
 
-    SuccessOrExit(error = Append(cslClockAccuracy));
-
-exit:
-    return error;
+    return Append(cslClockAccuracy);
 }
 #endif
 
diff --git a/src/core/thread/mle_router.cpp b/src/core/thread/mle_router.cpp
index 06433ce32..ad1a42f6e 100644
--- a/src/core/thread/mle_router.cpp
+++ b/src/core/thread/mle_router.cpp
@@ -2647,15 +2647,22 @@ void MleRouter::HandleChildUpdateRequest(RxInfo &aRxInfo)
         CslChannelTlv cslChannel;
         uint32_t      cslTimeout;
 
-        if (Tlv::Find<CslTimeoutTlv>(aRxInfo.mMessage, cslTimeout) == kErrorNone)
+        switch (Tlv::Find<CslTimeoutTlv>(aRxInfo.mMessage, cslTimeout))
         {
+        case kErrorNone:
             child->SetCslTimeout(cslTimeout);
             // MUST include CSL accuracy TLV when request includes CSL timeout
             tlvs[tlvslength++] = Tlv::kCslClockAccuracy;
+            break;
+        case kErrorNotFound:
+            break;
+        default:
+            ExitNow(error = kErrorNone);
         }
 
         if (Tlv::FindTlv(aRxInfo.mMessage, cslChannel) == kErrorNone)
         {
+            VerifyOrExit(cslChannel.IsValid(), error = kErrorParse);
             child->SetCslChannel(static_cast<uint8_t>(cslChannel.GetChannel()));
         }
         else
diff --git a/src/core/thread/mle_tlvs.hpp b/src/core/thread/mle_tlvs.hpp
index 4b1dd6725..3e8e6ec51 100644
--- a/src/core/thread/mle_tlvs.hpp
+++ b/src/core/thread/mle_tlvs.hpp
@@ -1230,7 +1230,7 @@ public:
      * @retval FALSE  If the TLV does not appear to be well-formed.
      *
      */
-    bool IsValid(void) const { return GetLength() == sizeof(*this) - sizeof(Tlv); }
+    bool IsValid(void) const { return GetLength() >= sizeof(*this) - sizeof(Tlv); }
 
     /**
      * This method returns the Channel Page value.
@@ -1290,6 +1290,15 @@ public:
         SetLength(sizeof(*this) - sizeof(Tlv));
     }
 
+    /**
+     * This method indicates whether or not the TLV appears to be well-formed.
+     *
+     * @retval TRUE   If the TLV appears to be well-formed.
+     * @retval FALSE  If the TLV does not appear to be well-formed.
+     *
+     */
+    bool IsValid(void) const { return GetLength() >= sizeof(*this) - sizeof(Tlv); }
+
     /**
      * This method returns the CSL Clock Accuracy value.
      *