From 129347292d0665e377e1ba07f0cadb2a705ef6c5 Mon Sep 17 00:00:00 2001 From: Pankaj Garg Date: Fri, 20 Aug 2021 17:06:50 -0700 Subject: [PATCH] Use local key ID to identify stale connections (#9168) * Use local key ID to identify stale connections * address review comments * reset mOldConnectionDropped before each test * drop PASE based secure channel after commissioning is complete --- src/protocols/secure_channel/CASEServer.cpp | 7 +++ src/transport/SecureSessionMgr.cpp | 11 ++-- src/transport/tests/TestSecureSessionMgr.cpp | 66 +++++++++++++++++++- 3 files changed, 78 insertions(+), 6 deletions(-) diff --git a/src/protocols/secure_channel/CASEServer.cpp b/src/protocols/secure_channel/CASEServer.cpp index e2897072bbfa50..740194038229ec 100644 --- a/src/protocols/secure_channel/CASEServer.cpp +++ b/src/protocols/secure_channel/CASEServer.cpp @@ -54,6 +54,13 @@ CHIP_ERROR CASEServer::InitCASEHandshake(Messaging::ExchangeContext * ec) { ReturnErrorCodeIf(ec == nullptr, CHIP_ERROR_INVALID_ARGUMENT); + // Mark any PASE sessions used for commissioning as stale. + // This is a workaround, as we currently don't have a way to identify + // secure sessions established via PASE protocol. + // TODO - Identify which PASE base secure channel was used + // for commissioning and drop it once commissioning is complete. + mSessionMgr->ExpireAllPairings(kUndefinedNodeId, kUndefinedFabricIndex); + ReturnErrorOnFailure(mIDAllocator->Allocate(mSessionKeyId)); // Setup CASE state machine using the credentials for the current fabric. diff --git a/src/transport/SecureSessionMgr.cpp b/src/transport/SecureSessionMgr.cpp index 5ed219d7402c4c..a9911a47dc3f41 100644 --- a/src/transport/SecureSessionMgr.cpp +++ b/src/transport/SecureSessionMgr.cpp @@ -213,12 +213,13 @@ void SecureSessionMgr::ExpireAllPairings(NodeId peerNodeId, FabricIndex fabric) CHIP_ERROR SecureSessionMgr::NewPairing(const Optional & peerAddr, NodeId peerNodeId, PairingSession * pairing, SecureSession::SessionRole direction, FabricIndex fabric) { - uint16_t peerKeyId = pairing->GetPeerKeyId(); - uint16_t localKeyId = pairing->GetLocalKeyId(); - PeerConnectionState * state = mPeerConnections.FindPeerConnectionState(Optional::Value(peerNodeId), peerKeyId, nullptr); + uint16_t peerKeyId = pairing->GetPeerKeyId(); + uint16_t localKeyId = pairing->GetLocalKeyId(); + PeerConnectionState * state = + mPeerConnections.FindPeerConnectionStateByLocalKey(Optional::Value(peerNodeId), localKeyId, nullptr); - // Find any existing connection with the same node and key ID - if (state && (state->GetFabricIndex() == Transport::kUndefinedFabricIndex || state->GetFabricIndex() == fabric)) + // Find any existing connection with the same local key ID + if (state) { mPeerConnections.MarkConnectionExpired( state, [this](const Transport::PeerConnectionState & state1) { HandleConnectionExpired(state1); }); diff --git a/src/transport/tests/TestSecureSessionMgr.cpp b/src/transport/tests/TestSecureSessionMgr.cpp index 53ad6d66605b08..2092c29e712468 100644 --- a/src/transport/tests/TestSecureSessionMgr.cpp +++ b/src/transport/tests/TestSecureSessionMgr.cpp @@ -93,7 +93,9 @@ class TestSessMgrCallback : public SecureSessionMgrDelegate mLocalToRemoteSession = session; NewConnectionHandlerCallCount++; } - void OnConnectionExpired(SessionHandle session) override {} + void OnConnectionExpired(SessionHandle session) override { mOldConnectionDropped = true; } + + bool mOldConnectionDropped = false; nlTestSuite * mSuite = nullptr; SessionHandle mRemoteToLocalSession; @@ -400,6 +402,67 @@ void SendBadEncryptedPacketTest(nlTestSuite * inSuite, void * inContext) NL_TEST_ASSERT(inSuite, callback.ReceiveHandlerCallCount == 2); } +void StaleConnectionDropTest(nlTestSuite * inSuite, void * inContext) +{ + TestContext & ctx = *reinterpret_cast(inContext); + + IPAddress addr; + IPAddress::FromString("::1", addr); + CHIP_ERROR err = CHIP_NO_ERROR; + + TransportMgr transportMgr; + SecureSessionMgr secureSessionMgr; + secure_channel::MessageCounterManager gMessageCounterManager; + + err = transportMgr.Init("LOOPBACK"); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + Transport::FabricTable fabrics; + err = secureSessionMgr.Init(ctx.GetInetLayer().SystemLayer(), &transportMgr, &fabrics, &gMessageCounterManager); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + callback.mSuite = inSuite; + + secureSessionMgr.SetDelegate(&callback); + + Optional peer(Transport::PeerAddress::UDP(addr, CHIP_PORT)); + + // First pairing + SecurePairingUsingTestSecret pairing1(1, 1); + callback.mOldConnectionDropped = false; + err = secureSessionMgr.NewPairing(peer, kSourceNodeId, &pairing1, SecureSession::SessionRole::kInitiator, 1); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + NL_TEST_ASSERT(inSuite, !callback.mOldConnectionDropped); + + // New pairing with different peer node ID and different local key ID (same peer key ID) + SecurePairingUsingTestSecret pairing2(1, 2); + callback.mOldConnectionDropped = false; + err = secureSessionMgr.NewPairing(peer, kSourceNodeId, &pairing2, SecureSession::SessionRole::kResponder, 0); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + NL_TEST_ASSERT(inSuite, !callback.mOldConnectionDropped); + + // New pairing with undefined node ID and different local key ID (same peer key ID) + SecurePairingUsingTestSecret pairing3(1, 3); + callback.mOldConnectionDropped = false; + err = secureSessionMgr.NewPairing(peer, kUndefinedNodeId, &pairing3, SecureSession::SessionRole::kResponder, 0); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + NL_TEST_ASSERT(inSuite, !callback.mOldConnectionDropped); + + // New pairing with same local key ID, and a given node ID + SecurePairingUsingTestSecret pairing4(1, 2); + callback.mOldConnectionDropped = false; + err = secureSessionMgr.NewPairing(peer, kSourceNodeId, &pairing4, SecureSession::SessionRole::kResponder, 0); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + NL_TEST_ASSERT(inSuite, callback.mOldConnectionDropped); + + // New pairing with same local key ID, and undefined node ID + SecurePairingUsingTestSecret pairing5(1, 1); + callback.mOldConnectionDropped = false; + err = secureSessionMgr.NewPairing(peer, kUndefinedNodeId, &pairing5, SecureSession::SessionRole::kResponder, 0); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + NL_TEST_ASSERT(inSuite, callback.mOldConnectionDropped); +} + // Test Suite /** @@ -412,6 +475,7 @@ const nlTest sTests[] = NL_TEST_DEF("Message Self Test", CheckMessageTest), NL_TEST_DEF("Send Encrypted Packet Test", SendEncryptedPacketTest), NL_TEST_DEF("Send Bad Encrypted Packet Test", SendBadEncryptedPacketTest), + NL_TEST_DEF("Drop stale connection Test", StaleConnectionDropTest), NL_TEST_SENTINEL() };