Add header validity check for group and MCSP msg (#11441)

project-chip · Feb 6, 2023 · 2301096 · 2301096
1 parent 522c126
commit 2301096
Show file tree

Hide file tree

Showing 3 changed files with 39 additions and 18 deletions.
diff --git a/src/transport/SessionManager.cpp b/src/transport/SessionManager.cpp
@@ -463,31 +463,32 @@ void SessionManager::SecureGroupMessageDispatch(const PacketHeader & packetHeade
 
     VerifyOrExit(!msg.IsNull(), ChipLogError(Inet, "Secure transport received NULL packet, discarding"));
 
-    // MCSP check
-    if (packetHeader.IsSecureSessionControlMsg())
+    // Check if Message Header is valid first
+    if (!(packetHeader.IsValidMCSPMsg() || packetHeader.IsValidGroupMsg()))
     {
-        if (packetHeader.GetDestinationNodeId().HasValue() && packetHeader.HasPrivacyFlag())
-        {
-            // TODO
-            // if (packetHeader.GetDestinationNodeId().Value() == ThisDeviceNodeID)
-            // {
-            //     MCSP processing..
-            // }
-        }
-        else
-        {
-            ChipLogError(Inet, "Invalid condition found in packet header");
-            ExitNow(err = CHIP_ERROR_INCORRECT_STATE);
-        }
+        ChipLogError(Inet, "Invalid condition found in packet header");
+        ExitNow(err = CHIP_ERROR_INCORRECT_STATE);
     }
 
-    // TODO: Handle Group message counter here spec 4.7.3
-    // spec 4.5.1.2 for msg counter
-
     // Trial decryption with GroupDataProvider. TODO: Implement the GroupDataProvider Class
     // VerifyOrExit(CHIP_NO_ERROR == groups->DecryptMessage(packetHeader, payloadHeader, msg),
     //     ChipLogError(Inet, "Secure transport received group message, but failed to decode it, discarding"));
 
+    // MCSP check
+    if (packetHeader.IsValidMCSPMsg())
+    {
+        // TODO
+        // if (packetHeader.GetDestinationNodeId().Value() == ThisDeviceNodeID)
+        // {
+        //     MCSP processing..
+        // }
+
+        ExitNow(err = CHIP_NO_ERROR);
+    }
+
+    // TODO: Handle Group message counter here spec 4.7.3
+    // spec 4.5.1.2 for msg counter
+
     if (isDuplicate == SessionManagerDelegate::DuplicateMessage::Yes && !payloadHeader.NeedsAck())
     {
         ChipLogDetail(Inet,

diff --git a/src/transport/raw/MessageHeader.h b/src/transport/raw/MessageHeader.h
@@ -196,6 +196,20 @@ class PacketHeader
         }
     }
 
+    bool IsValidGroupMsg() const
+    {
+        // Check is based on spec 4.11.2
+        return (IsGroupSession() && GetSourceNodeId().HasValue() && GetDestinationGroupId().HasValue() &&
+                !IsSecureSessionControlMsg() && HasPrivacyFlag());
+    }
+
+    bool IsValidMCSPMsg() const
+    {
+        // Check is based on spec 4.9.2.4
+        return (IsGroupSession() && GetSourceNodeId().HasValue() && GetDestinationNodeId().HasValue() &&
+                IsSecureSessionControlMsg() && HasPrivacyFlag());
+    }
+
     bool IsEncrypted() const { return !((mSessionId == kMsgUnicastSessionIdUnsecured) && IsUnicastSession()); }
 
     uint16_t MICTagLength() const { return (IsEncrypted()) ? chip::Crypto::CHIP_CRYPTO_AEAD_MIC_LENGTH_BYTES : 0; }

diff --git a/src/transport/raw/tests/TestMessageHeader.cpp b/src/transport/raw/tests/TestMessageHeader.cpp
@@ -149,6 +149,8 @@ void TestPacketHeaderEncodeDecode(nlTestSuite * inSuite, void * inContext)
 
     header.ClearDestinationNodeId();
     header.SetSessionType(Header::SessionType::kGroupSession);
+    header.SetFlags(Header::SecFlagValues::kPrivacyFlag);
+    header.SetSecureSessionControlMsg(false);
     NL_TEST_ASSERT(inSuite, header.Encode(buffer, &encodeLen) == CHIP_NO_ERROR);
 
     // change it to verify decoding
@@ -157,9 +159,12 @@ void TestPacketHeaderEncodeDecode(nlTestSuite * inSuite, void * inContext)
     NL_TEST_ASSERT(inSuite, header.GetMessageCounter() == 234);
     NL_TEST_ASSERT(inSuite, header.GetDestinationGroupId() == Optional<uint16_t>::Value((uint16_t) 45));
     NL_TEST_ASSERT(inSuite, header.GetSourceNodeId() == Optional<uint64_t>::Value(77ull));
+    NL_TEST_ASSERT(inSuite, !header.IsSecureSessionControlMsg());
+    NL_TEST_ASSERT(inSuite, header.IsValidGroupMsg());
 
     // Verify MCSP state
     header.ClearDestinationGroupId().SetDestinationNodeId(42).SetFlags(Header::SecFlagValues::kPrivacyFlag);
+    header.SetSecureSessionControlMsg(true);
     NL_TEST_ASSERT(inSuite, header.Encode(buffer, &encodeLen) == CHIP_NO_ERROR);
 
     // change it to verify decoding
@@ -168,6 +173,7 @@ void TestPacketHeaderEncodeDecode(nlTestSuite * inSuite, void * inContext)
     NL_TEST_ASSERT(inSuite, header.GetDestinationNodeId() == Optional<uint64_t>::Value(42ull));
     NL_TEST_ASSERT(inSuite, !header.GetDestinationGroupId().HasValue());
     NL_TEST_ASSERT(inSuite, header.HasPrivacyFlag());
+    NL_TEST_ASSERT(inSuite, header.IsValidMCSPMsg());
 }
 
 void TestPayloadHeaderEncodeDecode(nlTestSuite * inSuite, void * inContext)