From e82531507b7bd1efe6c70ebf4741d5c05c05c6ab Mon Sep 17 00:00:00 2001
From: Jonathan Hui <jonhui@google.com>
Date: Tue, 12 Mar 2019 10:47:12 -0700
Subject: [PATCH] [meshcop] make length checks more strict on ChannelMaskTlv
 parsing (#3672)

---
 src/core/meshcop/meshcop_tlvs.cpp | 48 +++++++++++--------------------
 src/core/meshcop/meshcop_tlvs.hpp | 25 +++++++++-------
 2 files changed, 31 insertions(+), 42 deletions(-)

diff --git a/src/core/meshcop/meshcop_tlvs.cpp b/src/core/meshcop/meshcop_tlvs.cpp
index f95654baf..6fc0afe86 100644
--- a/src/core/meshcop/meshcop_tlvs.cpp
+++ b/src/core/meshcop/meshcop_tlvs.cpp
@@ -33,6 +33,8 @@
 
 #include "meshcop_tlvs.hpp"
 
+#include "common/debug.hpp"
+
 namespace ot {
 namespace MeshCoP {
 
@@ -148,20 +150,6 @@ void ChannelTlv::SetChannel(uint16_t aChannel)
     mChannel = HostSwap16(aChannel);
 }
 
-ChannelMaskEntryBase *ChannelMaskEntryBase::GetNext(const Tlv *aChannelMaskBaseTlv)
-{
-    return const_cast<ChannelMaskEntryBase *>(
-        static_cast<const ChannelMaskEntryBase *>(this)->GetNext(aChannelMaskBaseTlv));
-}
-
-const ChannelMaskEntryBase *ChannelMaskEntryBase::GetNext(const Tlv *aChannelMaskBaseTlv) const
-{
-    const uint8_t *entry = reinterpret_cast<const uint8_t *>(this) + GetEntrySize();
-    const uint8_t *end   = aChannelMaskBaseTlv->GetValue() + aChannelMaskBaseTlv->GetLength();
-
-    return (entry < end) ? reinterpret_cast<const ChannelMaskEntryBase *>(entry) : NULL;
-}
-
 const ChannelMaskEntryBase *ChannelMaskBaseTlv::GetFirstEntry(void) const
 {
     const ChannelMaskEntryBase *entry = NULL;
@@ -180,33 +168,31 @@ ChannelMaskEntryBase *ChannelMaskBaseTlv::GetFirstEntry(void)
     return const_cast<ChannelMaskEntryBase *>(static_cast<const ChannelMaskBaseTlv *>(this)->GetFirstEntry());
 }
 
-otError ChannelMaskTlv::SetChannelMask(uint32_t aChannelMask)
+void ChannelMaskTlv::SetChannelMask(uint32_t aChannelMask)
 {
-    otError           error  = OT_ERROR_NONE;
     uint8_t           length = 0;
     ChannelMaskEntry *entry;
 
-    VerifyOrExit((entry = static_cast<ChannelMaskEntry *>(GetFirstEntry())) != NULL, error = OT_ERROR_NO_BUFS);
+    entry = static_cast<ChannelMaskEntry *>(GetFirstEntry());
 
 #if OPENTHREAD_CONFIG_RADIO_915MHZ_OQPSK_SUPPORT
     if (aChannelMask & OT_RADIO_915MHZ_OQPSK_CHANNEL_MASK)
     {
+        assert(entry != NULL);
         entry->Init();
         entry->SetChannelPage(OT_RADIO_CHANNEL_PAGE_2);
         entry->SetMask(aChannelMask & OT_RADIO_915MHZ_OQPSK_CHANNEL_MASK);
 
         length += sizeof(MeshCoP::ChannelMaskEntry);
 
-#if OPENTHREAD_CONFIG_RADIO_2P4GHZ_OQPSK_SUPPORT
-        VerifyOrExit((entry = static_cast<MeshCoP::ChannelMaskEntry *>(entry->GetNext(this))) != NULL,
-                     error = OT_ERROR_NO_BUFS);
-#endif
+        entry = static_cast<MeshCoP::ChannelMaskEntry *>(entry->GetNext());
     }
 #endif
 
 #if OPENTHREAD_CONFIG_RADIO_2P4GHZ_OQPSK_SUPPORT
     if (aChannelMask & OT_RADIO_2P4GHZ_OQPSK_CHANNEL_MASK)
     {
+        assert(entry != NULL);
         entry->Init();
         entry->SetChannelPage(OT_RADIO_CHANNEL_PAGE_0);
         entry->SetMask(aChannelMask & OT_RADIO_2P4GHZ_OQPSK_CHANNEL_MASK);
@@ -216,33 +202,31 @@ otError ChannelMaskTlv::SetChannelMask(uint32_t aChannelMask)
 #endif
 
     SetLength(length);
-
-exit:
-    return error;
 }
 
 uint32_t ChannelMaskTlv::GetChannelMask(void) const
 {
     uint32_t                mask = 0;
-    const ChannelMaskEntry *entry;
+    const ChannelMaskEntry *cur  = static_cast<const ChannelMaskEntry *>(GetFirstEntry());
+    const ChannelMaskEntry *end  = reinterpret_cast<const ChannelMaskEntry *>(GetValue() + GetLength());
 
-    VerifyOrExit((entry = static_cast<const ChannelMaskEntry *>(GetFirstEntry())) != NULL);
-    while (entry != NULL)
+    for (; cur < end; cur = static_cast<const ChannelMaskEntry *>(cur->GetNext()))
     {
+        VerifyOrExit((cur + 1) <= end && cur->GetNext() <= end);
+
 #if OPENTHREAD_CONFIG_RADIO_915MHZ_OQPSK_SUPPORT
-        if (entry->GetChannelPage() == OT_RADIO_CHANNEL_PAGE_2)
+        if (cur->GetChannelPage() == OT_RADIO_CHANNEL_PAGE_2)
         {
-            mask |= entry->GetMask() & OT_RADIO_915MHZ_OQPSK_CHANNEL_MASK;
+            mask |= cur->GetMask() & OT_RADIO_915MHZ_OQPSK_CHANNEL_MASK;
         }
 #endif
 
 #if OPENTHREAD_CONFIG_RADIO_2P4GHZ_OQPSK_SUPPORT
-        if (entry->GetChannelPage() == OT_RADIO_CHANNEL_PAGE_0)
+        if (cur->GetChannelPage() == OT_RADIO_CHANNEL_PAGE_0)
         {
-            mask |= entry->GetMask() & OT_RADIO_2P4GHZ_OQPSK_CHANNEL_MASK;
+            mask |= cur->GetMask() & OT_RADIO_2P4GHZ_OQPSK_CHANNEL_MASK;
         }
 #endif
-        entry = static_cast<const ChannelMaskEntry *>(entry->GetNext(this));
     }
 
 exit:
diff --git a/src/core/meshcop/meshcop_tlvs.hpp b/src/core/meshcop/meshcop_tlvs.hpp
index 3f341bddc..2bf1a551b 100644
--- a/src/core/meshcop/meshcop_tlvs.hpp
+++ b/src/core/meshcop/meshcop_tlvs.hpp
@@ -1373,6 +1373,9 @@ private:
     uint32_t mDelayTimer;
 } OT_TOOL_PACKED_END;
 
+// forward declare ChannelMaskTlv
+class ChannelMaskTlv;
+
 /**
  * This class implements Channel Mask Entry generation and parsing.
  *
@@ -1460,22 +1463,24 @@ public:
     /**
      * This method gets the next Channel Mask Entry in a Channel Mask TLV.
      *
-     * @param[in] aChannelMaskBaseTlv  A pointer to the Channel Mask TLV to which this entry belongs.
-     *
-     * @returns A pointer to next Channel Mask Entry or NULL if none found.
+     * @returns A pointer to next Channel Mask Entry.
      *
      */
-    const ChannelMaskEntryBase *GetNext(const Tlv *aChannelMaskBaseTlv) const;
+    const ChannelMaskEntryBase *GetNext(void) const
+    {
+        return reinterpret_cast<const ChannelMaskEntryBase *>(reinterpret_cast<const uint8_t *>(this) + GetEntrySize());
+    }
 
     /**
      * This method gets the next Channel Mask Entry in a Channel Mask TLV.
      *
-     * @param[in] aChannelMaskBaseTlv  A pointer to the Channel Mask TLV to which this entry belongs.
-     *
-     * @returns A pointer to next Channel Mask Entry or NULL if not found.
+     * @returns A pointer to next Channel Mask Entry.
      *
      */
-    ChannelMaskEntryBase *GetNext(const Tlv *aChannelMaskBaseTlv);
+    ChannelMaskEntryBase *GetNext(void)
+    {
+        return const_cast<ChannelMaskEntryBase *>(static_cast<const ChannelMaskEntryBase *>(this)->GetNext());
+    }
 
 private:
     uint8_t mChannelPage;
@@ -1598,9 +1603,9 @@ public:
      * @param[in]  aMask  The Channel Mask value.
      *
      * @retval OT_ERROR_NONE       Successfully set the Channel Mask Entry.
-     * @retval OT_ERROR_NO_BUFS    Insufficient available buffers to store channel mask.
+     *
      */
-    otError SetChannelMask(uint32_t aChannelMask);
+    void SetChannelMask(uint32_t aChannelMask);
 
     /**
      * This method returns the Channel Mask value as a `uint32_t` bit mask.