Fix crash on removal of accessing fabric (#17815)

* Add TestSelfFabricRemoval.yaml test * Fix crash on removal of accessing fabric Because of an access to prior fabric data that is now deleted, in SessionManager::PrepareMessage, while trying to reply to RemoveFabric, applications crash when RemoveFabric is done on the accessing fabric. This crash was awaiting full fix of #16748 to be fixed, but that issue is much bigger scope. We can actually fix the crash with a suggestion made by @turon (#16748 (comment)) to keep the *local node ID* in the SecureSession so that SessionManager does not try to look-back at the FabricTable whenever preparing a CASE message where the fabric may be gone. This is a root cause fix for that very crash, but does not address the other aspects of #16748 which relate to completely cleanly handling fabric removal edge cases. Issue #16748 Fixes #17579 Fixes #17680 Fixes #16729 This PR does the following: - Add local node ID to the SecureSession and fix all associated plumbing - Use the local node ID for nonce generation in PrepareMessage rather than looking-up the fabric table (which may no longer hold the fabric that has that prior node ID) - Improve CASE session establishment logging - Fix the tests needed - Fix bad comments in TestPairingSession tests Testing done: - Added a YAML test (TestSelfFabricRemoval.yaml) for this case - Validated it failed before code fixes with the previously seen crash. - Validated that it passes with the new fixes - Added necessary tests to TestPairingSession for new methods - Unit tests pass - Cert tests pass * Restyled by whitespace * Restyled by clang-format * Regen ZAP after comment * Address review comments * Restyled by clang-format * Reorder one argument used in test-only code * Restyled by clang-format Co-authored-by: Restyled.io <[email protected]>
project-chip · Aug 16, 2023 · c601c1e · c601c1e
1 parent fde9051
commit c601c1e
Show file tree

Hide file tree

Showing 23 changed files with 499 additions and 100 deletions.
diff --git a/examples/chip-tool-darwin/templates/tests/tests.js b/examples/chip-tool-darwin/templates/tests/tests.js
@@ -22,8 +22,7 @@ function getManualTests()
 
 // clang-format off
 
-function getTests()
-{
+function getTests() {
   const AccessControl = [
     'TestAccessControlCluster',
   ];
@@ -284,6 +283,7 @@ function getTests()
     'TestIdentifyCluster',
     'TestLogCommands',
     'TestOperationalCredentialsCluster',
+    'TestSelfFabricRemoval',
     'TestBinding',
   ];
 

diff --git a/examples/chip-tool/templates/tests/tests.js b/examples/chip-tool/templates/tests/tests.js
@@ -547,6 +547,7 @@ function getTests()
     'TestIdentifyCluster',
     'TestOperationalCredentialsCluster',
     'TestModeSelectCluster',
+    'TestSelfFabricRemoval',
     'TestSystemCommands',
     'TestBinding',
     'TestUserLabelCluster',

diff --git a/scripts/tools/zap_regen_yaml_tests.sh b/scripts/tools/zap_regen_yaml_tests.sh
@@ -22,5 +22,4 @@
 #      rather than all of zap like `./scripts/tools/zap_regen_all.py
 #
 
-./scripts/tools/zap/generate.py src/controller/data_model/controller-clusters.zap \
-    -o zzz_generated/chip-tool/zap-generated -t examples/chip-tool/templates/templates.json
+./scripts/tools/zap_regen_all.py --type tests
diff --git a/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp b/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp
@@ -468,7 +468,6 @@ bool emberAfOperationalCredentialsClusterRemoveFabricCallback(app::CommandHandle
     {
         SendNOCResponse(commandObj, commandPath, OperationalCertStatus::kSuccess, fabricBeingRemoved, CharSpan());
 
-        // Use a more direct getter for FabricIndex from commandObj
         chip::Messaging::ExchangeContext * ec = commandObj->GetExchangeContext();
         FabricIndex currentFabricIndex        = commandObj->GetAccessingFabricIndex();
         if (currentFabricIndex == fabricBeingRemoved)

diff --git a/src/app/tests/suites/TestSelfFabricRemoval.yaml b/src/app/tests/suites/TestSelfFabricRemoval.yaml
@@ -0,0 +1,55 @@
+# Copyright (c) 2022 Project CHIP Authors
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+name: Validate removal of the only fabric left does not crash
+
+config:
+    nodeId: 0x12344321
+    cluster: "Operational Credentials"
+    endpoint: 0
+
+tests:
+    - label: "Wait for the commissioned device to be retrieved"
+      cluster: "DelayCommands"
+      command: "WaitForCommissionee"
+      arguments:
+          values:
+              - name: "nodeId"
+                value: nodeId
+
+    - label: "Read number of commissioned fabrics"
+      command: "readAttribute"
+      attribute: "CommissionedFabrics"
+      response:
+          value: 1
+          constraints:
+              type: uint8
+
+    - label: "Read current fabric index"
+      command: "readAttribute"
+      attribute: "CurrentFabricIndex"
+      response:
+          saveAs: ourFabricIndex
+          constraints:
+              type: uint8
+              # 0 is not a valid value, but past that we have no idea what the
+              # other side will claim here.
+              minValue: 1
+
+    - label: "Remove single own fabric"
+      command: "RemoveFabric"
+      arguments:
+          values:
+              - name: "FabricIndex"
+                value: ourFabricIndex
diff --git a/src/darwin/Framework/CHIP/templates/tests/tests.js b/src/darwin/Framework/CHIP/templates/tests/tests.js
@@ -281,6 +281,7 @@ function getTests()
     'TestIdentifyCluster',
     'TestLogCommands',
     'TestOperationalCredentialsCluster',
+    // 'TestSelfFabricRemoval',  --> TODO: Integrate here when Darwin can live with current fabric going away
     'TestBinding',
     'TestUserLabelCluster',
   ];

diff --git a/src/protocols/secure_channel/CASESession.cpp b/src/protocols/secure_channel/CASESession.cpp
@@ -156,6 +156,10 @@ void CASESession::Clear()
     Crypto::ClearSecretData(mIPK);
 
     AbortExchange();
+
+    mLocalNodeId = kUndefinedNodeId;
+    mPeerNodeId  = kUndefinedNodeId;
+    mFabricInfo  = nullptr;
 }
 
 void CASESession::AbortExchange()
@@ -250,7 +254,11 @@ CHIP_ERROR CASESession::EstablishSession(SessionManager & sessionManager, Fabric
     mLocalMRPConfig           = mrpConfig;
 
     mExchangeCtxt->SetResponseTimeout(kSigma_Response_Timeout + mExchangeCtxt->GetSessionHandle()->GetAckTimeout());
-    mPeerNodeId = peerNodeId;
+    mPeerNodeId  = peerNodeId;
+    mLocalNodeId = fabric->GetNodeId();
+
+    ChipLogProgress(SecureChannel, "Initiating session on local FabricIndex %u from 0x" ChipLogFormatX64 " -> 0x" ChipLogFormatX64,
+                    static_cast<unsigned>(fabric->GetFabricIndex()), ChipLogValueX64(mLocalNodeId), ChipLogValueX64(mPeerNodeId));
 
     err = SendSigma1();
     SuccessOrExit(err);
@@ -343,9 +351,13 @@ CHIP_ERROR CASESession::RecoverInitiatorIpk()
     size_t ipkIndex = (ipkKeySet.num_keys_used > 1) ? ((ipkKeySet.num_keys_used - 1) - 1) : 0;
     memcpy(&mIPK[0], ipkKeySet.epoch_keys[ipkIndex].key, sizeof(mIPK));
 
+    // Leaving this logging code for debug, but this cannot be enabled at runtime
+    // since it leaks private security material.
+#if 0
     ChipLogProgress(SecureChannel, "RecoverInitiatorIpk: GroupDataProvider %p, Got IPK for FabricIndex %u", mGroupDataProvider,
                     static_cast<unsigned>(mFabricInfo->GetFabricIndex()));
     ChipLogByteSpan(SecureChannel, ByteSpan(mIPK));
+#endif
 
     return CHIP_NO_ERROR;
 }
@@ -497,7 +509,8 @@ CHIP_ERROR CASESession::FindLocalNodeFromDestionationId(const ByteSpan & destina
                 found = true;
                 MutableByteSpan ipkSpan(mIPK);
                 CopySpanToMutableSpan(candidateIpkSpan, ipkSpan);
-                mFabricInfo = &fabricInfo;
+                mFabricInfo  = &fabricInfo;
+                mLocalNodeId = nodeId;
                 break;
             }
         }
@@ -529,7 +542,8 @@ CHIP_ERROR CASESession::TryResumeSession(SessionResumptionStorage::ConstResumpti
     if (mFabricInfo == nullptr)
         return CHIP_ERROR_INTERNAL;
 
-    mPeerNodeId = node.GetNodeId();
+    mPeerNodeId  = node.GetNodeId();
+    mLocalNodeId = mFabricInfo->GetNodeId();
 
     return CHIP_NO_ERROR;
 }
@@ -578,11 +592,15 @@ CHIP_ERROR CASESession::HandleSigma1(System::PacketBufferHandle && msg)
         return CHIP_NO_ERROR;
     }
 
+    // Attempt to match the initiator's desired destination based on local fabric table.
     err = FindLocalNodeFromDestionationId(destinationIdentifier, initiatorRandom);
     if (err == CHIP_NO_ERROR)
     {
         ChipLogProgress(SecureChannel, "CASE matched destination ID: fabricIndex %u, NodeID 0x" ChipLogFormatX64,
-                        static_cast<unsigned>(mFabricInfo->GetFabricIndex()), ChipLogValueX64(mFabricInfo->GetNodeId()));
+                        static_cast<unsigned>(mFabricInfo->GetFabricIndex()), ChipLogValueX64(mLocalNodeId));
+
+        // Side-effect of FindLocalNodeFromDestionationId success was that mFabricInfo/mLocalNodeId are now
+        // set to the local fabric and associated NodeId that was targeted by the initiator.
     }
     else
     {

diff --git a/src/protocols/secure_channel/CASESession.h b/src/protocols/secure_channel/CASESession.h
@@ -64,6 +64,7 @@ class DLL_EXPORT CASESession : public Messaging::UnsolicitedMessageHandler,
 
     Transport::SecureSession::Type GetSecureSessionType() const override { return Transport::SecureSession::Type::kCASE; }
     ScopedNodeId GetPeer() const override { return ScopedNodeId(mPeerNodeId, GetFabricIndex()); }
+    ScopedNodeId GetLocalScopedNodeId() const override { return ScopedNodeId(mLocalNodeId, GetFabricIndex()); }
     CATValues GetPeerCATs() const override { return mPeerCATs; };
 
     /**
@@ -255,6 +256,7 @@ class DLL_EXPORT CASESession : public Messaging::UnsolicitedMessageHandler,
     FabricTable * mFabricsTable    = nullptr;
     const FabricInfo * mFabricInfo = nullptr;
     NodeId mPeerNodeId             = kUndefinedNodeId;
+    NodeId mLocalNodeId            = kUndefinedNodeId;
     CATValues mPeerCATs;
 
     SessionResumptionStorage::ResumptionIdStorage mResumeResumptionId; // ResumptionId which is used to resume this session

diff --git a/src/protocols/secure_channel/PASESession.h b/src/protocols/secure_channel/PASESession.h
@@ -78,6 +78,13 @@ class DLL_EXPORT PASESession : public Messaging::UnsolicitedMessageHandler,
     {
         return ScopedNodeId(NodeIdFromPAKEKeyId(kDefaultCommissioningPasscodeId), kUndefinedFabricIndex);
     }
+
+    ScopedNodeId GetLocalScopedNodeId() const override
+    {
+        // For PASE, source is always the undefined node ID
+        return ScopedNodeId();
+    }
+
     CATValues GetPeerCATs() const override { return CATValues(); };
 
     CHIP_ERROR OnUnsolicitedMessageReceived(const PayloadHeader & payloadHeader, ExchangeDelegate *& newDelegate) override;

diff --git a/src/transport/GroupSession.h b/src/transport/GroupSession.h
@@ -27,7 +27,7 @@ namespace Transport {
 class IncomingGroupSession : public Session
 {
 public:
-    IncomingGroupSession(GroupId group, FabricIndex fabricIndex, NodeId sourceNodeId) : mGroupId(group), mSourceNodeId(sourceNodeId)
+    IncomingGroupSession(GroupId group, FabricIndex fabricIndex, NodeId peerNodeId) : mGroupId(group), mPeerNodeId(peerNodeId)
     {
         SetFabricIndex(fabricIndex);
     }
@@ -38,7 +38,8 @@ class IncomingGroupSession : public Session
     const char * GetSessionTypeString() const override { return "incoming group"; };
 #endif
 
-    ScopedNodeId GetPeer() const override { return ScopedNodeId(mSourceNodeId, GetFabricIndex()); }
+    ScopedNodeId GetPeer() const override { return ScopedNodeId(mPeerNodeId, GetFabricIndex()); }
+    ScopedNodeId GetLocalScopedNodeId() const override { return ScopedNodeId(kUndefinedNodeId, GetFabricIndex()); }
 
     Access::SubjectDescriptor GetSubjectDescriptor() const override
     {
@@ -68,11 +69,9 @@ class IncomingGroupSession : public Session
 
     GroupId GetGroupId() const { return mGroupId; }
 
-    NodeId GetSourceNodeId() const { return mSourceNodeId; }
-
 private:
     const GroupId mGroupId;
-    const NodeId mSourceNodeId;
+    const NodeId mPeerNodeId;
 };
 
 class OutgoingGroupSession : public Session
@@ -86,7 +85,10 @@ class OutgoingGroupSession : public Session
     const char * GetSessionTypeString() const override { return "outgoing group"; };
 #endif
 
+    // Peer node ID is unused: users care about the group, not the node
     ScopedNodeId GetPeer() const override { return ScopedNodeId(); }
+    // Local node ID is unused: users care about the group, not the node
+    ScopedNodeId GetLocalScopedNodeId() const override { return ScopedNodeId(); }
 
     Access::SubjectDescriptor GetSubjectDescriptor() const override
     {

diff --git a/src/transport/PairingSession.cpp b/src/transport/PairingSession.cpp
@@ -48,7 +48,8 @@ CHIP_ERROR PairingSession::ActivateSecureSession(const Transport::PeerAddress &
 
     // Call Activate last, otherwise errors on anything after would lead to
     // a partially valid session.
-    secureSession->Activate(GetSecureSessionType(), GetPeer(), GetPeerCATs(), peerSessionId, mRemoteMRPConfig);
+    secureSession->Activate(GetSecureSessionType(), GetLocalScopedNodeId(), GetPeer(), GetPeerCATs(), peerSessionId,
+                            mRemoteMRPConfig);
 
     ChipLogDetail(Inet, "New secure session created for device " ChipLogFormatScopedNodeId ", LSID:%d PSID:%d!",
                   ChipLogValueScopedNodeId(GetPeer()), secureSession->GetLocalSessionId(), peerSessionId);

diff --git a/src/transport/PairingSession.h b/src/transport/PairingSession.h
@@ -44,6 +44,7 @@ class DLL_EXPORT PairingSession
 
     virtual Transport::SecureSession::Type GetSecureSessionType() const = 0;
     virtual ScopedNodeId GetPeer() const                                = 0;
+    virtual ScopedNodeId GetLocalScopedNodeId() const                   = 0;
     virtual CATValues GetPeerCATs() const                               = 0;
 
     Optional<uint16_t> GetLocalSessionId() const

diff --git a/src/transport/SecureSession.cpp b/src/transport/SecureSession.cpp
@@ -20,11 +20,6 @@
 namespace chip {
 namespace Transport {
 
-ScopedNodeId SecureSession::GetPeer() const
-{
-    return ScopedNodeId(mPeerNodeId, GetFabricIndex());
-}
-
 Access::SubjectDescriptor SecureSession::GetSubjectDescriptor() const
 {
     Access::SubjectDescriptor subjectDescriptor;

diff --git a/src/transport/SecureSession.h b/src/transport/SecureSession.h
@@ -72,11 +72,11 @@ class SecureSession : public Session
 
     // TODO: This constructor should be private.  Tests should allocate a
     // kPending session and then call Activate(), just like non-test code does.
-    SecureSession(Type secureSessionType, uint16_t localSessionId, NodeId peerNodeId, CATValues peerCATs, uint16_t peerSessionId,
-                  FabricIndex fabric, const ReliableMessageProtocolConfig & config) :
+    SecureSession(Type secureSessionType, uint16_t localSessionId, NodeId localNodeId, NodeId peerNodeId, CATValues peerCATs,
+                  uint16_t peerSessionId, FabricIndex fabric, const ReliableMessageProtocolConfig & config) :
         mSecureSessionType(secureSessionType),
-        mPeerNodeId(peerNodeId), mPeerCATs(peerCATs), mLocalSessionId(localSessionId), mPeerSessionId(peerSessionId),
-        mLastActivityTime(System::SystemClock().GetMonotonicTimestamp()),
+        mLocalSessionId(localSessionId), mLocalNodeId(localNodeId), mPeerNodeId(peerNodeId), mPeerCATs(peerCATs),
+        mPeerSessionId(peerSessionId), mLastActivityTime(System::SystemClock().GetMonotonicTimestamp()),
         mLastPeerActivityTime(System::SystemClock().GetMonotonicTimestamp()), mMRPConfig(config)
     {
         SetFabricIndex(fabric);
@@ -89,7 +89,8 @@ class SecureSession : public Session
      *   receives a local session ID, but no other state.
      */
     SecureSession(uint16_t localSessionId) :
-        SecureSession(Type::kPending, localSessionId, kUndefinedNodeId, CATValues{}, 0, kUndefinedFabricIndex, GetLocalMRPConfig())
+        SecureSession(Type::kPending, localSessionId, kUndefinedNodeId, kUndefinedNodeId, CATValues{}, 0, kUndefinedFabricIndex,
+                      GetLocalMRPConfig())
     {}
 
     /**
@@ -98,15 +99,26 @@ class SecureSession : public Session
      *   PASE, setting internal state according to the parameters used and
      *   discovered during session establishment.
      */
-    void Activate(Type secureSessionType, const ScopedNodeId & peer, CATValues peerCATs, uint16_t peerSessionId,
-                  const ReliableMessageProtocolConfig & config)
+    void Activate(Type secureSessionType, const ScopedNodeId & localNode, const ScopedNodeId & peerNode, CATValues peerCATs,
+                  uint16_t peerSessionId, const ReliableMessageProtocolConfig & config)
     {
+        VerifyOrDie(peerNode.GetFabricIndex() == localNode.GetFabricIndex());
+
+        // PASE sessions must always start unassociated with a Fabric!
+        VerifyOrDie(!((secureSessionType == Type::kPASE) && (peerNode.GetFabricIndex() != kUndefinedFabricIndex)));
+        // CASE sessions must always start "associated" a given Fabric!
+        VerifyOrDie(!((secureSessionType == Type::kCASE) && (peerNode.GetFabricIndex() == kUndefinedFabricIndex)));
+        // CASE sessions can only be activated against operational node IDs!
+        VerifyOrDie(!((secureSessionType == Type::kCASE) &&
+                      (!IsOperationalNodeId(peerNode.GetNodeId()) || !IsOperationalNodeId(localNode.GetNodeId()))));
+
         mSecureSessionType = secureSessionType;
-        mPeerNodeId        = peer.GetNodeId();
+        mPeerNodeId        = peerNode.GetNodeId();
+        mLocalNodeId       = localNode.GetNodeId();
         mPeerCATs          = peerCATs;
         mPeerSessionId     = peerSessionId;
         mMRPConfig         = config;
-        SetFabricIndex(peer.GetFabricIndex());
+        SetFabricIndex(peerNode.GetFabricIndex());
     }
     ~SecureSession() override { NotifySessionReleased(); }
 
@@ -120,7 +132,10 @@ class SecureSession : public Session
     const char * GetSessionTypeString() const override { return "secure"; };
 #endif
 
-    ScopedNodeId GetPeer() const override;
+    ScopedNodeId GetPeer() const override { return ScopedNodeId(mPeerNodeId, GetFabricIndex()); }
+
+    ScopedNodeId GetLocalScopedNodeId() const override { return ScopedNodeId(mLocalNodeId, GetFabricIndex()); }
+
     Access::SubjectDescriptor GetSubjectDescriptor() const override;
 
     bool RequireMRP() const override { return GetPeerAddress().GetTransportType() == Transport::Type::kUdp; }
@@ -147,6 +162,8 @@ class SecureSession : public Session
     bool IsPASESession() const { return GetSecureSessionType() == Type::kPASE; }
     bool IsActiveSession() const { return GetSecureSessionType() != Type::kPending; }
     NodeId GetPeerNodeId() const { return mPeerNodeId; }
+    NodeId GetLocalNodeId() const { return mLocalNodeId; }
+
     CATValues GetPeerCATs() const { return mPeerCATs; }
 
     void SetMRPConfig(const ReliableMessageProtocolConfig & config) { mMRPConfig = config; }
@@ -191,9 +208,10 @@ class SecureSession : public Session
 
 private:
     Type mSecureSessionType;
+    const uint16_t mLocalSessionId;
+    NodeId mLocalNodeId;
     NodeId mPeerNodeId;
     CATValues mPeerCATs;
-    const uint16_t mLocalSessionId;
     uint16_t mPeerSessionId;
 
     PeerAddress mPeerAddress;