[CASESession] Follow up to refactoring Sigma Parsing functions PR (#37425)

* Make ConstructTBSData outparam a MutableByteSpan instead of a Pointer + Len

* Adding a msgR2Decrypted struct member to improve clarity

* Clarifying Logs when Assigining Peer SessionID

* Removing TODO on validating signature before validation credentials

* changing Logs to ScopedNodeID instead of just NodeID

Author: Alami-Amine

src/protocols/secure_channel/CASESession.cpp

@@ -1054,7 +1054,7 @@ CASESession::NextStep CASESession::HandleSigma1(System::PacketBufferHandle && ms
 
     ReturnErrorVariantOnFailure(NextStep, ParseSigma1(tlvReader, parsedSigma1));
 
-    ChipLogDetail(SecureChannel, "Peer assigned session key ID %d", parsedSigma1.initiatorSessionId);
+    ChipLogDetail(SecureChannel, "Peer (Initiator) assigned session ID %d", parsedSigma1.initiatorSessionId);
     SetPeerSessionId(parsedSigma1.initiatorSessionId);
 
     // Set the Session parameters provided in the Sigma1 message
@@ -1222,23 +1222,23 @@ CHIP_ERROR CASESession::PrepareSigma2(EncodeSigma2Inputs & outSigma2Data)
     ReturnErrorOnFailure(DeriveSigmaKey(saltSpan, ByteSpan(kKDFSR2Info), sr2k));
 
     // Construct Sigma2 TBS Data
-    size_t msgR2SignedLen = EstimateStructOverhead(kMaxCHIPCertLength,     // responderNoc
-                                                   kMaxCHIPCertLength,     // responderICAC
-                                                   kP256_PublicKey_Length, // responderEphPubKey
-                                                   kP256_PublicKey_Length  // InitiatorEphPubKey
-    );
-
     P256ECDSASignature tbsData2Signature;
     {
+        size_t msgR2SignedLen = EstimateStructOverhead(kMaxCHIPCertLength,     // responderNoc
+                                                       kMaxCHIPCertLength,     // responderICAC
+                                                       kP256_PublicKey_Length, // responderEphPubKey
+                                                       kP256_PublicKey_Length  // InitiatorEphPubKey
+        );
+
         chip::Platform::ScopedMemoryBuffer<uint8_t> msgR2Signed;
         VerifyOrReturnError(msgR2Signed.Alloc(msgR2SignedLen), CHIP_ERROR_NO_MEMORY);
+        MutableByteSpan msgR2SignedSpan{ msgR2Signed.Get(), msgR2SignedLen };
 
         ReturnErrorOnFailure(ConstructTBSData(nocCert, icaCert, ByteSpan(mEphemeralKey->Pubkey(), mEphemeralKey->Pubkey().Length()),
-                                              ByteSpan(mRemotePubKey, mRemotePubKey.Length()), msgR2Signed.Get(), msgR2SignedLen));
+                                              ByteSpan(mRemotePubKey, mRemotePubKey.Length()), msgR2SignedSpan));
 
         // Generate a Signature
-        ReturnErrorOnFailure(
-            mFabricsTable->SignWithOpKeypair(mFabricIndex, ByteSpan{ msgR2Signed.Get(), msgR2SignedLen }, tbsData2Signature));
+        ReturnErrorOnFailure(mFabricsTable->SignWithOpKeypair(mFabricIndex, msgR2SignedSpan, tbsData2Signature));
     }
     // Construct Sigma2 TBE Data
     size_t msgR2SignedEncLen = EstimateStructOverhead(nocCert.size(),                             // responderNoc
@@ -1381,7 +1381,8 @@ CHIP_ERROR CASESession::HandleSigma2Resume(System::PacketBufferHandle && msg)
             GetRemoteSessionParameters());
     }
 
-    ChipLogDetail(SecureChannel, "Peer assigned session ID %d", parsedSigma2Resume.responderSessionId);
+    ChipLogDetail(SecureChannel, "Peer " ChipLogFormatScopedNodeId " assigned session ID %d", ChipLogValueScopedNodeId(GetPeer()),
+                  parsedSigma2Resume.responderSessionId);
     SetPeerSessionId(parsedSigma2Resume.responderSessionId);
 
     if (mSessionResumptionStorage != nullptr)
@@ -1517,12 +1518,15 @@ CHIP_ERROR CASESession::HandleSigma2(System::PacketBufferHandle && msg)
                                          nullptr, 0, parsedSigma2.msgR2MIC.data(), parsedSigma2.msgR2MIC.size(), sr2k.KeyHandle(),
                                          kTBEData2_Nonce, kTBEDataNonceLength, parsedSigma2.msgR2EncryptedPayload.data()));
 
+    parsedSigma2.msgR2Decrypted = std::move(parsedSigma2.msgR2Encrypted);
+    size_t msgR2DecryptedLength = parsedSigma2.msgR2EncryptedPayload.size();
+
     ContiguousBufferTLVReader decryptedDataTlvReader;
-    decryptedDataTlvReader.Init(parsedSigma2.msgR2EncryptedPayload.data(), parsedSigma2.msgR2EncryptedPayload.size());
+    decryptedDataTlvReader.Init(parsedSigma2.msgR2Decrypted.Get(), msgR2DecryptedLength);
     ParsedSigma2TBEData parsedSigma2TBEData;
     ReturnErrorOnFailure(ParseSigma2TBEData(decryptedDataTlvReader, parsedSigma2TBEData));
 
-    // Validate responder identity located in msgR2Encrypted
+    // Validate responder identity located in msgR2Decrypted
     // Constructing responder identity
     P256PublicKey responderPublicKey;
     {
@@ -1540,7 +1544,7 @@ CHIP_ERROR CASESession::HandleSigma2(System::PacketBufferHandle && msg)
         VerifyOrReturnError(mPeerNodeId == responderNodeId, CHIP_ERROR_INVALID_CASE_PARAMETER);
     }
 
-    // Construct msgR2Signed and validate the signature in msgR2Encrypted.
+    // Construct msgR2Signed and validate the signature in msgR2Decrypted.
     size_t msgR2SignedLen = EstimateStructOverhead(parsedSigma2TBEData.responderNOC.size(),  // resonderNOC
                                                    parsedSigma2TBEData.responderICAC.size(), // responderICAC
                                                    kP256_PublicKey_Length,                   // responderEphPubKey
@@ -1549,16 +1553,18 @@ CHIP_ERROR CASESession::HandleSigma2(System::PacketBufferHandle && msg)
 
     chip::Platform::ScopedMemoryBuffer<uint8_t> msgR2Signed;
     VerifyOrReturnError(msgR2Signed.Alloc(msgR2SignedLen), CHIP_ERROR_NO_MEMORY);
+    MutableByteSpan msgR2SignedSpan{ msgR2Signed.Get(), msgR2SignedLen };
 
-    ReturnErrorOnFailure(ConstructTBSData(
-        parsedSigma2TBEData.responderNOC, parsedSigma2TBEData.responderICAC, ByteSpan(mRemotePubKey, mRemotePubKey.Length()),
-        ByteSpan(mEphemeralKey->Pubkey(), mEphemeralKey->Pubkey().Length()), msgR2Signed.Get(), msgR2SignedLen));
+    ReturnErrorOnFailure(ConstructTBSData(parsedSigma2TBEData.responderNOC, parsedSigma2TBEData.responderICAC,
+                                          ByteSpan(mRemotePubKey, mRemotePubKey.Length()),
+                                          ByteSpan(mEphemeralKey->Pubkey(), mEphemeralKey->Pubkey().Length()), msgR2SignedSpan));
 
     // Validate signature
-    ReturnErrorOnFailure(
-        responderPublicKey.ECDSA_validate_msg_signature(msgR2Signed.Get(), msgR2SignedLen, parsedSigma2TBEData.tbsData2Signature));
+    ReturnErrorOnFailure(responderPublicKey.ECDSA_validate_msg_signature(msgR2SignedSpan.data(), msgR2SignedSpan.size(),
+                                                                         parsedSigma2TBEData.tbsData2Signature));
 
-    ChipLogDetail(SecureChannel, "Peer assigned session ID %d", parsedSigma2.responderSessionId);
+    ChipLogDetail(SecureChannel, "Peer " ChipLogFormatScopedNodeId " assigned session ID %d", ChipLogValueScopedNodeId(GetPeer()),
+                  parsedSigma2.responderSessionId);
     SetPeerSessionId(parsedSigma2.responderSessionId);
 
     std::copy(parsedSigma2TBEData.resumptionId.begin(), parsedSigma2TBEData.resumptionId.end(), mNewResumptionId.begin());
@@ -1728,14 +1734,18 @@ CHIP_ERROR CASESession::SendSigma3a()
         ReturnErrorOnFailure(mFabricsTable->FetchNOCCert(mFabricIndex, data.nocCert));
 
         // Prepare Sigma3 TBS Data Blob
-        data.msg_r3_signed_len =
-            EstimateStructOverhead(data.icaCert.size(), data.nocCert.size(), kP256_PublicKey_Length, kP256_PublicKey_Length);
+        size_t msgR3SignedLen = EstimateStructOverhead(data.nocCert.size(),    // initiatorNOC
+                                                       data.icaCert.size(),    // initiatorICAC
+                                                       kP256_PublicKey_Length, // initiatorEphPubKey
+                                                       kP256_PublicKey_Length  // responderEphPubKey
+        );
 
-        VerifyOrReturnError(data.msg_R3_Signed.Alloc(data.msg_r3_signed_len), CHIP_ERROR_NO_MEMORY);
+        VerifyOrReturnError(data.msgR3Signed.Alloc(msgR3SignedLen), CHIP_ERROR_NO_MEMORY);
+        data.msgR3SignedSpan = MutableByteSpan{ data.msgR3Signed.Get(), msgR3SignedLen };
 
-        ReturnErrorOnFailure(
-            ConstructTBSData(data.nocCert, data.icaCert, ByteSpan(mEphemeralKey->Pubkey(), mEphemeralKey->Pubkey().Length()),
-                             ByteSpan(mRemotePubKey, mRemotePubKey.Length()), data.msg_R3_Signed.Get(), data.msg_r3_signed_len));
+        ReturnErrorOnFailure(ConstructTBSData(data.nocCert, data.icaCert,
+                                              ByteSpan(mEphemeralKey->Pubkey(), mEphemeralKey->Pubkey().Length()),
+                                              ByteSpan(mRemotePubKey, mRemotePubKey.Length()), data.msgR3SignedSpan));
 
         if (data.keystore != nullptr)
         {
@@ -1759,14 +1769,12 @@ CHIP_ERROR CASESession::SendSigma3b(SendSigma3Data & data, bool & cancel)
     if (data.keystore != nullptr)
     {
         // Recommended case: delegate to operational keystore
-        ReturnErrorOnFailure(data.keystore->SignWithOpKeypair(
-            data.fabricIndex, ByteSpan{ data.msg_R3_Signed.Get(), data.msg_r3_signed_len }, data.tbsData3Signature));
+        ReturnErrorOnFailure(data.keystore->SignWithOpKeypair(data.fabricIndex, data.msgR3SignedSpan, data.tbsData3Signature));
     }
     else
     {
         // Legacy case: delegate to fabric table fabric info
-        ReturnErrorOnFailure(data.fabricTable->SignWithOpKeypair(
-            data.fabricIndex, ByteSpan{ data.msg_R3_Signed.Get(), data.msg_r3_signed_len }, data.tbsData3Signature));
+        ReturnErrorOnFailure(data.fabricTable->SignWithOpKeypair(data.fabricIndex, data.msgR3SignedSpan, data.tbsData3Signature));
     }
 
     // Prepare Sigma3 TBE Data Blob
@@ -1950,17 +1958,18 @@ CHIP_ERROR CASESession::HandleSigma3a(System::PacketBufferHandle && msg)
         SuccessOrExit(err = ParseSigma3TBEData(decryptedDataTlvReader, data));
 
         // Step 3 - Construct Sigma3 TBS Data
-        data.msgR3SignedLen = TLV::EstimateStructOverhead(data.initiatorNOC.size(),  // initiatorNOC
-                                                          data.initiatorICAC.size(), // initiatorICAC
-                                                          kP256_PublicKey_Length,    // initiatorEphPubKey
-                                                          kP256_PublicKey_Length     // responderEphPubKey
+        size_t msgR3SignedLen = TLV::EstimateStructOverhead(data.initiatorNOC.size(),  // initiatorNOC
+                                                            data.initiatorICAC.size(), // initiatorICAC
+                                                            kP256_PublicKey_Length,    // initiatorEphPubKey
+                                                            kP256_PublicKey_Length     // responderEphPubKey
         );
 
-        VerifyOrExit(data.msgR3Signed.Alloc(data.msgR3SignedLen), err = CHIP_ERROR_NO_MEMORY);
+        VerifyOrExit(data.msgR3Signed.Alloc(msgR3SignedLen), err = CHIP_ERROR_NO_MEMORY);
+        data.msgR3SignedSpan = MutableByteSpan{ data.msgR3Signed.Get(), msgR3SignedLen };
 
         SuccessOrExit(err = ConstructTBSData(data.initiatorNOC, data.initiatorICAC, ByteSpan(mRemotePubKey, mRemotePubKey.Length()),
                                              ByteSpan(mEphemeralKey->Pubkey(), mEphemeralKey->Pubkey().Length()),
-                                             data.msgR3Signed.Get(), data.msgR3SignedLen));
+                                             data.msgR3SignedSpan));
 
         // Prepare for Step 4/5
         {
@@ -1977,9 +1986,9 @@ CHIP_ERROR CASESession::HandleSigma3a(System::PacketBufferHandle && msg)
 
             // initiatorNOC and initiatorICAC are spans into msgR3Encrypted
             // which is going away, so to save memory, redirect them to their
-            // copies in msg_R3_signed, which is staying around
+            // copies in msgR3Signed, which is staying around
             TLV::ContiguousBufferTLVReader signedDataTlvReader;
-            signedDataTlvReader.Init(data.msgR3Signed.Get(), data.msgR3SignedLen);
+            signedDataTlvReader.Init(data.msgR3SignedSpan);
             SuccessOrExit(err = signedDataTlvReader.Next(containerType, AnonymousTag()));
             SuccessOrExit(err = signedDataTlvReader.EnterContainer(containerType));
 
@@ -2089,14 +2098,9 @@ CHIP_ERROR CASESession::HandleSigma3b(HandleSigma3Data & data, bool & cancel)
                                                         unused, initiatorFabricId, data.initiatorNodeId, initiatorPublicKey));
     VerifyOrReturnError(data.fabricId == initiatorFabricId, CHIP_ERROR_INVALID_CASE_PARAMETER);
 
-    // TODO - Validate message signature prior to validating the received operational credentials.
-    //        The op cert check requires traversal of cert chain, that is a more expensive operation.
-    //        If message signature check fails, the cert chain check will be unnecessary, but with the
-    //        current flow of code, a malicious node can trigger a DoS style attack on the device.
-    //        The same change should be made in Sigma2 processing.
     // Step 7 - Validate Signature
-    ReturnErrorOnFailure(
-        initiatorPublicKey.ECDSA_validate_msg_signature(data.msgR3Signed.Get(), data.msgR3SignedLen, data.tbsData3Signature));
+    ReturnErrorOnFailure(initiatorPublicKey.ECDSA_validate_msg_signature(data.msgR3SignedSpan.data(), data.msgR3SignedSpan.size(),
+                                                                         data.tbsData3Signature));
 
     return CHIP_NO_ERROR;
 }
@@ -2241,12 +2245,12 @@ CHIP_ERROR CASESession::ValidateSigmaResumeMIC(const ByteSpan & resumeMIC, const
 }
 
 CHIP_ERROR CASESession::ConstructTBSData(const ByteSpan & senderNOC, const ByteSpan & senderICAC, const ByteSpan & senderPubKey,
-                                         const ByteSpan & receiverPubKey, uint8_t * tbsData, size_t & tbsDataLen)
+                                         const ByteSpan & receiverPubKey, MutableByteSpan & outTbsData)
 {
     TLVWriter tlvWriter;
     TLVType outerContainerType = kTLVType_NotSpecified;
 
-    tlvWriter.Init(tbsData, tbsDataLen);
+    tlvWriter.Init(outTbsData);
     ReturnErrorOnFailure(tlvWriter.StartContainer(AnonymousTag(), kTLVType_Structure, outerContainerType));
     ReturnErrorOnFailure(tlvWriter.Put(AsTlvContextTag(TBSDataTags::kSenderNOC), senderNOC));
     if (!senderICAC.empty())
@@ -2257,7 +2261,7 @@ CHIP_ERROR CASESession::ConstructTBSData(const ByteSpan & senderNOC, const ByteS
     ReturnErrorOnFailure(tlvWriter.Put(AsTlvContextTag(TBSDataTags::kReceiverPubKey), receiverPubKey));
     ReturnErrorOnFailure(tlvWriter.EndContainer(outerContainerType));
     ReturnErrorOnFailure(tlvWriter.Finalize());
-    tbsDataLen = static_cast<size_t>(tlvWriter.GetLengthWritten());
+    outTbsData.reduce_size(static_cast<size_t>(tlvWriter.GetLengthWritten()));
 
     return CHIP_NO_ERROR;
 }

src/protocols/secure_channel/CASESession.h

@@ -249,6 +249,7 @@ class DLL_EXPORT CASESession : public Messaging::UnsolicitedMessageHandler,
         ByteSpan responderEphPubKey;
 
         Platform::ScopedMemoryBufferWithSize<uint8_t> msgR2Encrypted;
+        Platform::ScopedMemoryBufferWithSize<uint8_t> msgR2Decrypted;
         // Below ByteSpans are Backed by: msgR2Encrypted buffer
         // Lifetime: Valid as long as msgR2Encrypted is not released
         MutableByteSpan msgR2EncryptedPayload;
@@ -260,8 +261,8 @@ class DLL_EXPORT CASESession : public Messaging::UnsolicitedMessageHandler,
 
     struct ParsedSigma2TBEData
     {
-        // Below ByteSpans are Backed by: msgR2Encrypted Buffer, member of ParsedSigma2 struct
-        // Lifetime: Valid for the lifetime of the instance of ParsedSigma2 that contains the msgR2Encrypted Buffer.
+        // Below ByteSpans are Backed by: msgR2Decrypted Buffer, member of ParsedSigma2 struct
+        // Lifetime: Valid for the lifetime of the instance of ParsedSigma2 that contains the msgR2Decrypted Buffer.
         ByteSpan responderNOC;
         ByteSpan responderICAC;
         ByteSpan resumptionId;
@@ -297,8 +298,8 @@ class DLL_EXPORT CASESession : public Messaging::UnsolicitedMessageHandler,
         const FabricTable * fabricTable;
         const Crypto::OperationalKeystore * keystore;
 
-        chip::Platform::ScopedMemoryBuffer<uint8_t> msg_R3_Signed;
-        size_t msg_r3_signed_len;
+        chip::Platform::ScopedMemoryBuffer<uint8_t> msgR3Signed;
+        MutableByteSpan msgR3SignedSpan;
 
         chip::Platform::ScopedMemoryBuffer<uint8_t> msg_R3_Encrypted;
         size_t msg_r3_encrypted_len;
@@ -315,7 +316,7 @@ class DLL_EXPORT CASESession : public Messaging::UnsolicitedMessageHandler,
     struct HandleSigma3Data
     {
         chip::Platform::ScopedMemoryBuffer<uint8_t> msgR3Signed;
-        size_t msgR3SignedLen;
+        MutableByteSpan msgR3SignedSpan;
 
         // Below ByteSpans are Backed by: msgR3Encrypted Buffer, local to the HandleSigma3a() method,
         // The Spans are later modified to point to the msgR3Signed member of this struct.
@@ -382,9 +383,9 @@ class DLL_EXPORT CASESession : public Messaging::UnsolicitedMessageHandler,
      * Parse a decrypted TBEData2Encrypted message. This function will return success only if the message passes schema checks.
      *
      * @param tlvReader a reference to the TLVReader that points to the decrypted TBEData2Encrypted buffer (i.e.
-     *                  msgR2Encrypted member of ParsedSigma2 struct)
+     *                  msgR2Decrypted member of ParsedSigma2 struct)
      * @param outParsedSigma2TBEData a reference to ParsedSigma2TBEData. All members of parsedMessage will stay valid as long
-     *                               as the msgR2Encrypted member of ParsedSigma2 is valid
+     *                               as the msgR2Decrypted member of ParsedSigma2 is valid
      *
      * @note Calls to this function must always be made with a newly created and fresh ParsedSigma2TBEData parameter.
      **/
@@ -513,7 +514,7 @@ class DLL_EXPORT CASESession : public Messaging::UnsolicitedMessageHandler,
     CHIP_ERROR ConstructSaltSigma2(const ByteSpan & rand, const Crypto::P256PublicKey & pubkey, const ByteSpan & ipk,
                                    MutableByteSpan & salt);
     CHIP_ERROR ConstructTBSData(const ByteSpan & senderNOC, const ByteSpan & senderICAC, const ByteSpan & senderPubKey,
-                                const ByteSpan & receiverPubKey, uint8_t * tbsData, size_t & tbsDataLen);
+                                const ByteSpan & receiverPubKey, MutableByteSpan & outTbsData);
     CHIP_ERROR ConstructSaltSigma3(const ByteSpan & ipk, MutableByteSpan & salt);
 
     CHIP_ERROR ConstructSigmaResumeKey(const ByteSpan & initiatorRandom, const ByteSpan & resumptionID, const ByteSpan & skInfo,