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
(https://github.com/project-chip/connectedhomeip/issues/16748#issuecomment-1082228657)
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 <commits@restyled.io>diff --git a/src/transport/SecureSessionTable.h b/src/transport/SecureSessionTable.h
index 1804ea4..06bd80e 100644
--- a/src/transport/SecureSessionTable.h
+++ b/src/transport/SecureSessionTable.h
@@ -48,10 +48,11 @@
*
* @param secureSessionType secure session type
* @param localSessionId unique identifier for the local node's secure unicast session context
+ * @param localNodeId represents the local Node ID for this node
* @param peerNodeId represents peer Node's ID
* @param peerCATs represents peer CASE Authenticated Tags
* @param peerSessionId represents the encryption key ID assigned by peer node
- * @param fabric represents fabric ID for the session
+ * @param fabricIndex represents fabric index for the session
* @param config represents the reliable message protocol configuration
*
* @note the newly created state will have an 'active' time set based on the current time source.
@@ -61,11 +62,35 @@
*/
CHECK_RETURN_VALUE
Optional<SessionHandle> CreateNewSecureSessionForTest(SecureSession::Type secureSessionType, uint16_t localSessionId,
- NodeId peerNodeId, CATValues peerCATs, uint16_t peerSessionId,
- FabricIndex fabric, const ReliableMessageProtocolConfig & config)
+ NodeId localNodeId, NodeId peerNodeId, CATValues peerCATs,
+ uint16_t peerSessionId, FabricIndex fabricIndex,
+ const ReliableMessageProtocolConfig & config)
{
- SecureSession * result =
- mEntries.CreateObject(secureSessionType, localSessionId, peerNodeId, peerCATs, peerSessionId, fabric, config);
+ if (secureSessionType == SecureSession::Type::kCASE)
+ {
+ if ((fabricIndex == kUndefinedFabricIndex) || (localNodeId == kUndefinedNodeId) || (peerNodeId == kUndefinedNodeId))
+ {
+ return Optional<SessionHandle>::Missing();
+ }
+ }
+ else if (secureSessionType == SecureSession::Type::kPASE)
+ {
+ if ((fabricIndex != kUndefinedFabricIndex) || (localNodeId != kUndefinedNodeId) || (peerNodeId != kUndefinedNodeId))
+ {
+ // TODO: This secure session type is infeasible! We must fix the tests
+ if (false)
+ {
+ return Optional<SessionHandle>::Missing();
+ }
+ else
+ {
+ (void) fabricIndex;
+ }
+ }
+ }
+
+ SecureSession * result = mEntries.CreateObject(secureSessionType, localSessionId, localNodeId, peerNodeId, peerCATs,
+ peerSessionId, fabricIndex, config);
return result != nullptr ? MakeOptional<SessionHandle>(*result) : Optional<SessionHandle>::Missing();
}
