Make DecodeMRPParametersIfPresent() and ParseSigma1() methods static + fix for TLV reading corner case (#36956)

* Making DecodeMRPParametersIfPresent self-contained and static and renaming it

* Making ParseSigma1 static and not dependent on class state

* Making SetRemoteSessionParameters protected

* Fix for the following Corner case: if the received TLV-encoded SessionParameters struct has extra TLV elements that were added to the spec, Yet the TLV struct is not
terminated with an EndOfContainer Element.
In this Case, we should make sure that ExitContainer so that an Error is triggered.

* Adding Unit Test for DecodeSessionParametersIfPresent()

* Fix CI failure related to using aggregate initializer with C-String

* Integrating Comments

Author: Alami-Amine

src/protocols/secure_channel/CASESession.cpp

@@ -1097,9 +1097,10 @@ CASESession::NextStep CASESession::HandleSigma1(System::PacketBufferHandle && ms
     ChipLogDetail(SecureChannel, "Peer assigned session key ID %d", parsedSigma1.initiatorSessionId);
     SetPeerSessionId(parsedSigma1.initiatorSessionId);
 
-    // Set the MRP parameters provided in the Sigma1 message
-    if (parsedSigma1.initiatorMrpParamsPresent)
+    // Set the Session parameters provided in the Sigma1 message
+    if (parsedSigma1.initiatorSessionParamStructPresent)
     {
+        SetRemoteSessionParameters(parsedSigma1.initiatorSessionParams);
         mExchangeCtxt.Value()->GetSessionHandle()->AsUnauthenticatedSession()->SetRemoteSessionParameters(
             GetRemoteSessionParameters());
     }
@@ -1436,7 +1437,7 @@ CHIP_ERROR CASESession::HandleSigma2Resume(System::PacketBufferHandle && msg)
 
     if (tlvReader.Next() != CHIP_END_OF_TLV)
     {
-        SuccessOrExit(err = DecodeMRPParametersIfPresent(TLV::ContextTag(4), tlvReader));
+        SuccessOrExit(err = DecodeSessionParametersIfPresent(TLV::ContextTag(4), tlvReader, mRemoteSessionParams));
         mExchangeCtxt.Value()->GetSessionHandle()->AsUnauthenticatedSession()->SetRemoteSessionParameters(
             GetRemoteSessionParameters());
     }
@@ -1638,7 +1639,8 @@ CHIP_ERROR CASESession::HandleSigma2(System::PacketBufferHandle && msg)
     // Retrieve responderMRPParams if present
     if (tlvReader.Next() != CHIP_END_OF_TLV)
     {
-        SuccessOrExit(err = DecodeMRPParametersIfPresent(AsTlvContextTag(Sigma2Tags::kResponderSessionParams), tlvReader));
+        SuccessOrExit(err = DecodeSessionParametersIfPresent(AsTlvContextTag(Sigma2Tags::kResponderSessionParams), tlvReader,
+                                                             mRemoteSessionParams));
         mExchangeCtxt.Value()->GetSessionHandle()->AsUnauthenticatedSession()->SetRemoteSessionParameters(
             GetRemoteSessionParameters());
     }
@@ -2324,8 +2326,9 @@ CHIP_ERROR CASESession::ParseSigma1(TLV::ContiguousBufferTLVReader & tlvReader,
     CHIP_ERROR err = tlvReader.Next();
     if (err == CHIP_NO_ERROR && tlvReader.GetTag() == AsTlvContextTag(Sigma1Tags::kInitiatorSessionParams))
     {
-        ReturnErrorOnFailure(DecodeMRPParametersIfPresent(AsTlvContextTag(Sigma1Tags::kInitiatorSessionParams), tlvReader));
-        outParsedSigma1.initiatorMrpParamsPresent = true;
+        ReturnErrorOnFailure(DecodeSessionParametersIfPresent(AsTlvContextTag(Sigma1Tags::kInitiatorSessionParams), tlvReader,
+                                                              outParsedSigma1.initiatorSessionParams));
+        outParsedSigma1.initiatorSessionParamStructPresent = true;
 
         err = tlvReader.Next();
     }

src/protocols/secure_channel/CASESession.h

@@ -222,7 +222,8 @@ class DLL_EXPORT CASESession : public Messaging::UnsolicitedMessageHandler,
     struct ParsedSigma1 : Sigma1Param
     {
         ByteSpan initiatorEphPubKey;
-        bool initiatorMrpParamsPresent = false;
+        bool initiatorSessionParamStructPresent = false;
+        SessionParameters initiatorSessionParams;
     };
 
     struct EncodeSigma2Inputs
@@ -274,8 +275,10 @@ class DLL_EXPORT CASESession : public Messaging::UnsolicitedMessageHandler,
      * On success, either the sessionResumptionRequested field will be set to true
      * and the resumptionID and initiatorResumeMIC fields will be set to
      * valid values, or the sessionResumptionRequested field will be set to false.
+     *
+     *  @note Calls to this function must always be made with a newly created and fresh ParsedSigma1 parameter.
      */
-    CHIP_ERROR ParseSigma1(TLV::ContiguousBufferTLVReader & tlvReader, ParsedSigma1 & parsedMessage);
+    static CHIP_ERROR ParseSigma1(TLV::ContiguousBufferTLVReader & tlvReader, ParsedSigma1 & parsedMessage);
 
     /**
      * @brief  Encodes a Sigma2 message into TLV format and allocates a buffer for it, which is owned by the PacketBufferHandle

src/protocols/secure_channel/PASESession.cpp

@@ -372,7 +372,7 @@ CHIP_ERROR PASESession::HandlePBKDFParamRequest(System::PacketBufferHandle && ms
 
     if (tlvReader.Next() != CHIP_END_OF_TLV)
     {
-        SuccessOrExit(err = DecodeMRPParametersIfPresent(TLV::ContextTag(5), tlvReader));
+        SuccessOrExit(err = DecodeSessionParametersIfPresent(TLV::ContextTag(5), tlvReader, mRemoteSessionParams));
         mExchangeCtxt.Value()->GetSessionHandle()->AsUnauthenticatedSession()->SetRemoteSessionParameters(
             GetRemoteSessionParameters());
     }
@@ -493,7 +493,7 @@ CHIP_ERROR PASESession::HandlePBKDFParamResponse(System::PacketBufferHandle && m
     {
         if (tlvReader.Next() != CHIP_END_OF_TLV)
         {
-            SuccessOrExit(err = DecodeMRPParametersIfPresent(TLV::ContextTag(5), tlvReader));
+            SuccessOrExit(err = DecodeSessionParametersIfPresent(TLV::ContextTag(5), tlvReader, mRemoteSessionParams));
             mExchangeCtxt.Value()->GetSessionHandle()->AsUnauthenticatedSession()->SetRemoteSessionParameters(
                 GetRemoteSessionParameters());
         }
@@ -519,7 +519,7 @@ CHIP_ERROR PASESession::HandlePBKDFParamResponse(System::PacketBufferHandle && m
 
         if (tlvReader.Next() != CHIP_END_OF_TLV)
         {
-            SuccessOrExit(err = DecodeMRPParametersIfPresent(TLV::ContextTag(5), tlvReader));
+            SuccessOrExit(err = DecodeSessionParametersIfPresent(TLV::ContextTag(5), tlvReader, mRemoteSessionParams));
             mExchangeCtxt.Value()->GetSessionHandle()->AsUnauthenticatedSession()->SetRemoteSessionParameters(
                 GetRemoteSessionParameters());
         }

src/protocols/secure_channel/PairingSession.cpp

@@ -144,7 +144,8 @@ CHIP_ERROR PairingSession::EncodeSessionParameters(TLV::Tag tag, const ReliableM
     return tlvWriter.EndContainer(mrpParamsContainer);
 }
 
-CHIP_ERROR PairingSession::DecodeMRPParametersIfPresent(TLV::Tag expectedTag, TLV::ContiguousBufferTLVReader & tlvReader)
+CHIP_ERROR PairingSession::DecodeSessionParametersIfPresent(TLV::Tag expectedTag, TLV::ContiguousBufferTLVReader & tlvReader,
+                                                            SessionParameters & outSessionParameters)
 {
     CHIP_ERROR err = CHIP_NO_ERROR;
 
@@ -167,7 +168,7 @@ CHIP_ERROR PairingSession::DecodeMRPParametersIfPresent(TLV::Tag expectedTag, TL
     {
         uint32_t idleRetransTimeout;
         ReturnErrorOnFailure(tlvReader.Get(idleRetransTimeout));
-        mRemoteSessionParams.SetMRPIdleRetransTimeout(System::Clock::Milliseconds32(idleRetransTimeout));
+        outSessionParameters.SetMRPIdleRetransTimeout(System::Clock::Milliseconds32(idleRetransTimeout));
 
         // The next element is optional. If it's not present, return CHIP_NO_ERROR.
         SuccessOrExit(err = tlvReader.Next());
@@ -177,7 +178,7 @@ CHIP_ERROR PairingSession::DecodeMRPParametersIfPresent(TLV::Tag expectedTag, TL
     {
         uint32_t activeRetransTimeout;
         ReturnErrorOnFailure(tlvReader.Get(activeRetransTimeout));
-        mRemoteSessionParams.SetMRPActiveRetransTimeout(System::Clock::Milliseconds32(activeRetransTimeout));
+        outSessionParameters.SetMRPActiveRetransTimeout(System::Clock::Milliseconds32(activeRetransTimeout));
 
         // The next element is optional. If it's not present, return CHIP_NO_ERROR.
         SuccessOrExit(err = tlvReader.Next());
@@ -187,7 +188,7 @@ CHIP_ERROR PairingSession::DecodeMRPParametersIfPresent(TLV::Tag expectedTag, TL
     {
         uint16_t activeThresholdTime;
         ReturnErrorOnFailure(tlvReader.Get(activeThresholdTime));
-        mRemoteSessionParams.SetMRPActiveThresholdTime(System::Clock::Milliseconds16(activeThresholdTime));
+        outSessionParameters.SetMRPActiveThresholdTime(System::Clock::Milliseconds16(activeThresholdTime));
 
         // The next element is optional. If it's not present, return CHIP_NO_ERROR.
         SuccessOrExit(err = tlvReader.Next());
@@ -197,7 +198,7 @@ CHIP_ERROR PairingSession::DecodeMRPParametersIfPresent(TLV::Tag expectedTag, TL
     {
         uint16_t dataModelRevision;
         ReturnErrorOnFailure(tlvReader.Get(dataModelRevision));
-        mRemoteSessionParams.SetDataModelRevision(dataModelRevision);
+        outSessionParameters.SetDataModelRevision(dataModelRevision);
 
         // The next element is optional. If it's not present, return CHIP_NO_ERROR.
         SuccessOrExit(err = tlvReader.Next());
@@ -207,7 +208,7 @@ CHIP_ERROR PairingSession::DecodeMRPParametersIfPresent(TLV::Tag expectedTag, TL
     {
         uint16_t interactionModelRevision;
         ReturnErrorOnFailure(tlvReader.Get(interactionModelRevision));
-        mRemoteSessionParams.SetInteractionModelRevision(interactionModelRevision);
+        outSessionParameters.SetInteractionModelRevision(interactionModelRevision);
 
         // The next element is optional. If it's not present, return CHIP_NO_ERROR.
         SuccessOrExit(err = tlvReader.Next());
@@ -217,7 +218,7 @@ CHIP_ERROR PairingSession::DecodeMRPParametersIfPresent(TLV::Tag expectedTag, TL
     {
         uint32_t specificationVersion;
         ReturnErrorOnFailure(tlvReader.Get(specificationVersion));
-        mRemoteSessionParams.SetSpecificationVersion(specificationVersion);
+        outSessionParameters.SetSpecificationVersion(specificationVersion);
 
         // The next element is optional. If it's not present, return CHIP_NO_ERROR.
         SuccessOrExit(err = tlvReader.Next());
@@ -227,15 +228,15 @@ CHIP_ERROR PairingSession::DecodeMRPParametersIfPresent(TLV::Tag expectedTag, TL
     {
         uint16_t maxPathsPerInvoke;
         ReturnErrorOnFailure(tlvReader.Get(maxPathsPerInvoke));
-        mRemoteSessionParams.SetMaxPathsPerInvoke(maxPathsPerInvoke);
+        outSessionParameters.SetMaxPathsPerInvoke(maxPathsPerInvoke);
 
         // The next element is optional. If it's not present, return CHIP_NO_ERROR.
         SuccessOrExit(err = tlvReader.Next());
     }
 
     // Future proofing - Don't error out if there are other tags
 exit:
-    if (err == CHIP_END_OF_TLV)
+    if (err == CHIP_END_OF_TLV || err == CHIP_NO_ERROR)
     {
         return tlvReader.ExitContainer(containerType);
     }

src/protocols/secure_channel/PairingSession.h

@@ -129,6 +129,9 @@ class DLL_EXPORT PairingSession : public SessionDelegate
     void DiscardExchange(); // Clear our reference to our exchange context pointer so that it can close itself at some later time.
 
     void SetPeerSessionId(uint16_t id) { mPeerSessionId.SetValue(id); }
+
+    void SetRemoteSessionParameters(const SessionParameters & sessionParams) { mRemoteSessionParams = sessionParams; }
+
     virtual void OnSuccessStatusReport() {}
 
     // Handle a failure StatusReport message from the server.  protocolData will
@@ -207,16 +210,17 @@ class DLL_EXPORT PairingSession : public SessionDelegate
     }
 
     /**
-     * Try to decode the current element (pointed by the TLV reader) as MRP parameters.
-     * If the MRP parameters are found, mRemoteSessionParams is updated with the devoded values.
+     * Try to decode the current element (pointed by the TLV reader) as Session parameters (which include MRP parameters).
+     * If the Session parameters are found, outparam sessionParameters is updated with the decoded values.
      *
-     * MRP parameters are optional. So, if the TLV reader is not pointing to the MRP parameters,
+     * Session parameters are optional. So, if the TLV reader is not pointing to the Session parameters,
      * the function is a noop.
      *
      * If the parameters are present, but TLV reader fails to correctly parse it, the function will
      * return the corresponding error.
      */
-    CHIP_ERROR DecodeMRPParametersIfPresent(TLV::Tag expectedTag, TLV::ContiguousBufferTLVReader & tlvReader);
+    static CHIP_ERROR DecodeSessionParametersIfPresent(TLV::Tag expectedTag, TLV::ContiguousBufferTLVReader & tlvReader,
+                                                       SessionParameters & sessionParameters);
 
     bool IsSessionEstablishmentInProgress();
 

src/protocols/secure_channel/tests/TestCASESession.cpp

@@ -760,10 +760,9 @@ static CHIP_ERROR EncodeSigma1Helper(MutableByteSpan & buf)
                                                                                                                                    \
         TLV::ContiguousBufferTLVReader reader;                                                                                     \
         reader.Init(buf);                                                                                                          \
-        CASESessionAccess session;                                                                                                 \
         CASESessionAccess::ParsedSigma1 parsedSigma1;                                                                              \
                                                                                                                                    \
-        EXPECT_EQ(session.ParseSigma1(reader, parsedSigma1) == CHIP_NO_ERROR, params::kExpectSuccess);                             \
+        EXPECT_EQ(CASESessionAccess::ParseSigma1(reader, parsedSigma1) == CHIP_NO_ERROR, params::kExpectSuccess);                  \
         if (params::kExpectSuccess)                                                                                                \
         {                                                                                                                          \
             EXPECT_EQ(parsedSigma1.sessionResumptionRequested,                                                                     \
@@ -936,10 +935,9 @@ TEST_F(TestCASESession, EncodeSigma1Test)
         System::PacketBufferTLVReader tlvReader;
         tlvReader.Init(std::move(msg1));
 
-        CASESessionAccess session;
         CASESessionAccess::ParsedSigma1 parsedMessage;
 
-        EXPECT_EQ(CHIP_NO_ERROR, session.ParseSigma1(tlvReader, parsedMessage));
+        EXPECT_EQ(CHIP_NO_ERROR, CASESessionAccess::ParseSigma1(tlvReader, parsedMessage));
 
         // compare parsed values with original values
         EXPECT_TRUE(parsedMessage.initiatorRandom.data_equal(encodeParams.initiatorRandom));
@@ -970,10 +968,9 @@ TEST_F(TestCASESession, EncodeSigma1Test)
         System::PacketBufferTLVReader tlvReader;
         tlvReader.Init(std::move(msg2));
 
-        CASESessionAccess session;
         CASESessionAccess::ParsedSigma1 parsedMessage;
 
-        EXPECT_EQ(CHIP_NO_ERROR, session.ParseSigma1(tlvReader, parsedMessage));
+        EXPECT_EQ(CHIP_NO_ERROR, CASESessionAccess::ParseSigma1(tlvReader, parsedMessage));
 
         // RoundTrip
         EXPECT_TRUE(parsedMessage.initiatorRandom.data_equal(encodeParams.initiatorRandom));
@@ -984,7 +981,7 @@ TEST_F(TestCASESession, EncodeSigma1Test)
 
         EXPECT_TRUE(parsedMessage.resumptionId.data_equal(encodeParams.resumptionId));
         EXPECT_TRUE(parsedMessage.initiatorResumeMIC.data_equal(encodeParams.initiatorResumeMIC));
-        EXPECT_TRUE(parsedMessage.initiatorMrpParamsPresent);
+        EXPECT_TRUE(parsedMessage.sessionResumptionRequested);
     }
     // Release EphemeralKeyPair
     gDeviceOperationalKeystore.ReleaseEphemeralKeypair(ephemeralKey);