Do not allow creating secure session handle on the fly (#13313)

project-chip · Jan 5, 2022 · 6ddb3e4 · 6ddb3e4
1 parent 65a2e8d
commit 6ddb3e4
Show file tree

Hide file tree

Showing 7 changed files with 13 additions and 101 deletions.
diff --git a/src/app/tests/TestReportingEngine.cpp b/src/app/tests/TestReportingEngine.cpp
@@ -76,9 +76,8 @@ void TestReportingEngine::TestBuildAndSendSingleReportData(nlTestSuite * apSuite
 
  err = InteractionModelEngine::GetInstance()->Init(&ctx.GetExchangeManager(), nullptr);
  NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
- Messaging::ExchangeContext * exchangeCtx = ctx.GetExchangeManager().NewContext(SessionHandle(0, 0, 0, 0), nullptr);
  TestExchangeDelegate delegate;
- exchangeCtx->SetDelegate(&delegate);
+ Messaging::ExchangeContext * exchangeCtx = ctx.NewExchangeToAlice(&delegate);
 
  writer.Init(std::move(readRequestbuf));
  err = readRequestBuilder.Init(&writer);
@@ -104,7 +103,8 @@ void TestReportingEngine::TestBuildAndSendSingleReportData(nlTestSuite * apSuite
  readHandler.Init(&ctx.GetExchangeManager(), nullptr, exchangeCtx, chip::app::ReadHandler::InteractionType::Read);
  readHandler.OnReadInitialRequest(std::move(readRequestbuf));
  err = InteractionModelEngine::GetInstance()->GetReportingEngine().BuildAndSendSingleReportData(&readHandler);
- NL_TEST_ASSERT(apSuite, err == CHIP_ERROR_NOT_CONNECTED);
+ NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
+ readHandler.Shutdown(app::ReadHandler::ShutdownOptions::AbortCurrentExchange);
 }
 
 } // namespace reporting

diff --git a/src/app/util/chip-message-send.cpp b/src/app/util/chip-message-send.cpp
@@ -110,7 +110,7 @@ EmberStatus chipSendUnicast(NodeId destination, EmberApsFrame * apsFrame, uint16
  }
 
  Messaging::ExchangeContext * exchange =
- exchangeMgr->NewContext(SessionHandle(destination, 0, Transport::kAnyKeyId, 0), nullptr);
+ exchangeMgr->NewContext(exchangeMgr->GetSessionManager()->FindSecureSessionForNode(destination), nullptr);
  if (exchange == nullptr)
  {
  return EMBER_DELIVERY_FAILED;

diff --git a/src/transport/SessionHandle.h b/src/transport/SessionHandle.h
@@ -39,11 +39,10 @@ class SessionHandle
  mPeerNodeId(kPlaceholderNodeId), mFabric(kUndefinedFabricIndex), mUnauthenticatedSessionHandle(session)
  {}
 
- SessionHandle(NodeId peerNodeId, uint16_t localSessionId, uint16_t peerSessionId, FabricIndex fabric) :
- mPeerNodeId(peerNodeId), mFabric(fabric)
+ SessionHandle(Transport::SecureSession & session) : mPeerNodeId(session.GetPeerNodeId()), mFabric(session.GetFabricIndex())
  {
- mLocalSessionId.SetValue(localSessionId);
- mPeerSessionId.SetValue(peerSessionId);
+ mLocalSessionId.SetValue(session.GetLocalSessionId());
+ mPeerSessionId.SetValue(session.GetPeerSessionId());
  }
 
  SessionHandle(NodeId peerNodeId, GroupId groupId, FabricIndex fabric) : mPeerNodeId(peerNodeId), mFabric(fabric)

diff --git a/src/transport/SessionManager.cpp b/src/transport/SessionManager.cpp
@@ -340,7 +340,7 @@ CHIP_ERROR SessionManager::NewPairing(SessionHolder & sessionHolder, const Optio
  ReturnErrorOnFailure(pairing->DeriveSecureSession(session->GetCryptoContext(), direction));
 
  session->GetSessionMessageCounter().GetPeerMessageCounter().SetCounter(pairing->GetPeerCounter());
- sessionHolder.Grab(SessionHandle(session->GetPeerNodeId(), session->GetLocalSessionId(), session->GetPeerSessionId(), fabric));
+ sessionHolder.Grab(SessionHandle(*session));
 
  return CHIP_NO_ERROR;
 }
@@ -530,8 +530,7 @@ void SessionManager::SecureUnicastMessageDispatch(const PacketHeader & packetHea
 
  if (mCB != nullptr)
  {
- SessionHandle sessionHandle(session->GetPeerNodeId(), session->GetLocalSessionId(), session->GetPeerSessionId(),
- session->GetFabricIndex());
+ SessionHandle sessionHandle(*session);
  mCB->OnMessageReceived(packetHeader, payloadHeader, sessionHandle, peerAddress, isDuplicate, std::move(msg));
  }
 }
@@ -606,13 +605,12 @@ void SessionManager::SecureGroupMessageDispatch(const PacketHeader & packetHeade
  }
 }
 
-void SessionManager::HandleConnectionExpired(const Transport::SecureSession & session)
+void SessionManager::HandleConnectionExpired(Transport::SecureSession & session)
 {
  ChipLogDetail(Inet, "Marking old secure session for device 0x" ChipLogFormatX64 " as expired",
  ChipLogValueX64(session.GetPeerNodeId()));
 
- SessionHandle sessionHandle(session.GetPeerNodeId(), session.GetLocalSessionId(), session.GetPeerSessionId(),
- session.GetFabricIndex());
+ SessionHandle sessionHandle(session);
  mSessionReleaseDelegates.ForEachActiveObject([&](std::reference_wrapper<SessionReleaseDelegate> * cb) {
  cb->get().OnSessionReleased(sessionHandle);
  return Loop::Continue;
@@ -659,7 +657,7 @@ SessionHandle SessionManager::FindSecureSessionForNode(NodeId peerNodeId)
  });
 
  VerifyOrDie(found != nullptr);
- return SessionHandle(found->GetPeerNodeId(), found->GetLocalSessionId(), found->GetPeerSessionId(), found->GetFabricIndex());
+ return SessionHandle(*found);
 }
 
 } // namespace chip
diff --git a/src/transport/SessionManager.h b/src/transport/SessionManager.h
@@ -294,7 +294,7 @@ class DLL_EXPORT SessionManager : public TransportMgrDelegate
  /**
  * Called when a specific connection expires.
  */
- void HandleConnectionExpired(const Transport::SecureSession & state);
+ void HandleConnectionExpired(Transport::SecureSession & state);
 
  /**
  * Callback for timer expiry check

diff --git a/src/transport/tests/BUILD.gn b/src/transport/tests/BUILD.gn
@@ -26,7 +26,6 @@ chip_test_suite("tests") {
  "TestPairingSession.cpp",
  "TestPeerConnections.cpp",
  "TestSecureSession.cpp",
- "TestSessionHandle.cpp",
  "TestSessionManager.cpp",
  ]
 

diff --git a/src/transport/tests/TestSessionHandle.cpp b/src/transport/tests/TestSessionHandle.cpp