Ensure that we send MRP acks to incoming messages as needed. (#19398)

There were two ways we could fail to send an ack to an incoming reliable message:

1) If we found no matching handler, and hence created an ephemeral exchange to
handle the message, but the message was unencrypted.  In this case our ephemeral
exchange would return true for IsEncryptionRequired(), because it would default
to an ApplicationExchangeDispatch, and we would never call into
ExchangeContext::HandleMessage.

2) If ExchangeMessageDispatch::MessagePermitted returned false for the message.
In particular, for an ApplicationExchangeDispatch, this would happen for all the
handshake messages except StatusReport.

The fix for issue 1 is to ensure we always call into HandleMEssage if we manage
to allocate an exchange.  If there is an encryption mismatch, which only matters
when the exchange is non-ephemeral, we close the exchange first to prevent event
delivery to the delegate.

The fix for issue 2 is to move the MRP processing out of ExchangeMessageDispatch
and into ExchangeContext, and to move the MessagePermitted check so the only
thing it prevents is delivery of the message to the delegate, not any other
processing by the exchange.

Fixes #10515

src/messaging/ApplicationExchangeDispatch.cpp

@@ -26,12 +26,11 @@
 namespace chip {
 namespace Messaging {
 
-bool ApplicationExchangeDispatch::MessagePermitted(uint16_t protocol, uint8_t type)
+bool ApplicationExchangeDispatch::MessagePermitted(Protocols::Id protocol, uint8_t type)
 {
     // TODO: Change this check to only include the protocol and message types that are allowed
-    switch (protocol)
+    if (protocol == Protocols::SecureChannel::Id)
     {
-    case Protocols::SecureChannel::Id.GetProtocolId():
         switch (type)
         {
         case static_cast<uint8_t>(Protocols::SecureChannel::MsgType::PBKDFParamRequest):
@@ -49,11 +48,8 @@ bool ApplicationExchangeDispatch::MessagePermitted(uint16_t protocol, uint8_t ty
         default:
             break;
         }
-        break;
-
-    default:
-        break;
     }
+
     return true;
 }
 

src/messaging/ApplicationExchangeDispatch.h

@@ -43,7 +43,7 @@ class ApplicationExchangeDispatch : public ExchangeMessageDispatch
     ~ApplicationExchangeDispatch() override {}
 
 protected:
-    bool MessagePermitted(uint16_t protocol, uint8_t type) override;
+    bool MessagePermitted(Protocols::Id protocol, uint8_t type) override;
 };
 
 } // namespace Messaging

src/messaging/ExchangeContext.cpp

@@ -458,7 +458,21 @@ CHIP_ERROR ExchangeContext::HandleMessage(uint32_t messageCounter, const Payload
         MessageHandled();
     });
 
-    ReturnErrorOnFailure(mDispatch.OnMessageReceived(messageCounter, payloadHeader, msgFlags, GetReliableMessageContext()));
+    if (mDispatch.IsReliableTransmissionAllowed() && !IsGroupExchangeContext())
+    {
+        if (!msgFlags.Has(MessageFlagValues::kDuplicateMessage) && payloadHeader.IsAckMsg() &&
+            payloadHeader.GetAckMessageCounter().HasValue())
+        {
+            HandleRcvdAck(payloadHeader.GetAckMessageCounter().Value());
+        }
+
+        if (payloadHeader.NeedsAck())
+        {
+            // An acknowledgment needs to be sent back to the peer for this message on this exchange,
+
+            HandleNeedsAck(messageCounter, msgFlags);
+        }
+    }
 
     if (IsAckPending() && !mDelegate)
     {
@@ -487,7 +501,9 @@ CHIP_ERROR ExchangeContext::HandleMessage(uint32_t messageCounter, const Payload
     // is implicitly that response.
     SetResponseExpected(false);
 
-    if (mDelegate != nullptr)
+    // Don't send messages on to our delegate if our dispatch does not allow
+    // those messages.
+    if (mDelegate != nullptr && mDispatch.MessagePermitted(payloadHeader.GetProtocolID(), payloadHeader.GetMessageType()))
     {
         return mDelegate->OnMessageReceived(this, payloadHeader, std::move(msgBuf));
     }

src/messaging/ExchangeMessageDispatch.cpp

@@ -46,7 +46,7 @@ CHIP_ERROR ExchangeMessageDispatch::SendMessage(SessionManager * sessionManager,
                                                 bool isReliableTransmission, Protocols::Id protocol, uint8_t type,
                                                 System::PacketBufferHandle && message)
 {
-    ReturnErrorCodeIf(!MessagePermitted(protocol.GetProtocolId(), type), CHIP_ERROR_INVALID_ARGUMENT);
+    ReturnErrorCodeIf(!MessagePermitted(protocol, type), CHIP_ERROR_INVALID_ARGUMENT);
 
     PayloadHeader payloadHeader;
     payloadHeader.SetExchangeID(exchangeId).SetMessageType(protocol, type).SetInitiator(isInitiator);
@@ -113,30 +113,5 @@ CHIP_ERROR ExchangeMessageDispatch::SendMessage(SessionManager * sessionManager,
     return CHIP_NO_ERROR;
 }
 
-CHIP_ERROR ExchangeMessageDispatch::OnMessageReceived(uint32_t messageCounter, const PayloadHeader & payloadHeader,
-                                                      MessageFlags msgFlags, ReliableMessageContext * reliableMessageContext)
-{
-    ReturnErrorCodeIf(!MessagePermitted(payloadHeader.GetProtocolID().GetProtocolId(), payloadHeader.GetMessageType()),
-                      CHIP_ERROR_INVALID_ARGUMENT);
-
-    if (IsReliableTransmissionAllowed() && !reliableMessageContext->GetExchangeContext()->IsGroupExchangeContext())
-    {
-        if (!msgFlags.Has(MessageFlagValues::kDuplicateMessage) && payloadHeader.IsAckMsg() &&
-            payloadHeader.GetAckMessageCounter().HasValue())
-        {
-            reliableMessageContext->HandleRcvdAck(payloadHeader.GetAckMessageCounter().Value());
-        }
-
-        if (payloadHeader.NeedsAck())
-        {
-            // An acknowledgment needs to be sent back to the peer for this message on this exchange,
-
-            ReturnErrorOnFailure(reliableMessageContext->HandleNeedsAck(messageCounter, msgFlags));
-        }
-    }
-
-    return CHIP_NO_ERROR;
-}
-
 } // namespace Messaging
 } // namespace chip

src/messaging/ExchangeMessageDispatch.h

@@ -24,6 +24,7 @@
 #pragma once
 
 #include <messaging/Flags.h>
+#include <protocols/Protocols.h>
 #include <transport/SessionManager.h>
 
 namespace chip {
@@ -42,11 +43,8 @@ class ExchangeMessageDispatch
     CHIP_ERROR SendMessage(SessionManager * sessionManager, const SessionHandle & session, uint16_t exchangeId, bool isInitiator,
                            ReliableMessageContext * reliableMessageContext, bool isReliableTransmission, Protocols::Id protocol,
                            uint8_t type, System::PacketBufferHandle && message);
-    CHIP_ERROR OnMessageReceived(uint32_t messageCounter, const PayloadHeader & payloadHeader, MessageFlags msgFlags,
-                                 ReliableMessageContext * reliableMessageContext);
 
-protected:
-    virtual bool MessagePermitted(uint16_t protocol, uint8_t type) = 0;
+    virtual bool MessagePermitted(Protocols::Id protocol, uint8_t type) = 0;
 
     // TODO: remove IsReliableTransmissionAllowed, this function should be provided over session.
     virtual bool IsReliableTransmissionAllowed() const { return true; }

src/messaging/ExchangeMgr.cpp

@@ -310,11 +310,23 @@ void ExchangeManager::OnMessageReceived(const PacketHeader & packetHeader, const
         ChipLogDetail(ExchangeManager, "Handling via exchange: " ChipLogFormatExchange ", Delegate: %p", ChipLogValueExchange(ec),
                       ec->GetDelegate());
 
-        if (ec->IsEncryptionRequired() != packetHeader.IsEncrypted())
+        // Make sure the exchange stays alive through the code below even if we
+        // close it before calling HandleMessage.
+        ExchangeHandle ref(*ec);
+
+        // Ignore encryption-required mismatches for emphemeral exchanges,
+        // because those never have delegates anyway.
+        if (matchingUMH != nullptr && ec->IsEncryptionRequired() != packetHeader.IsEncrypted())
         {
-            ChipLogError(ExchangeManager, "OnMessageReceived failed, err = %s", ErrorStr(CHIP_ERROR_INVALID_MESSAGE_TYPE));
+            // We want to still to do MRP processing for this message, but we do
+            // not want to deliver it to the application.  Just close the
+            // exchange (which will notify the delegate, null it out, etc), then
+            // go ahead and call HandleMessage() on it to do the MRP
+            // processing.null out the delegate on the exchange, pretend to
+            // matchingUMH that exchange creation failed, so it cleans up the
+            // delegate, then tell the exchagne to handle the message.
+            ChipLogProgress(ExchangeManager, "OnMessageReceived encryption mismatch");
             ec->Close();
-            return;
         }
 
         CHIP_ERROR err = ec->HandleMessage(packetHeader.GetMessageCounter(), payloadHeader, msgFlags, std::move(msgBuf));

src/messaging/tests/TestReliableMessageProtocol.cpp

@@ -132,7 +132,7 @@ class MockSessionEstablishmentExchangeDispatch : public Messaging::ApplicationEx
 public:
     bool IsReliableTransmissionAllowed() const override { return mRetainMessageOnSend; }
 
-    bool MessagePermitted(uint16_t protocol, uint8_t type) override { return true; }
+    bool MessagePermitted(Protocols::Id protocol, uint8_t type) override { return true; }
 
     bool IsEncryptionRequired() const override { return mRequireEncryption; }
 

src/protocols/secure_channel/SessionEstablishmentExchangeDispatch.cpp

@@ -28,11 +28,10 @@ namespace chip {
 
 using namespace Messaging;
 
-bool SessionEstablishmentExchangeDispatch::MessagePermitted(uint16_t protocol, uint8_t type)
+bool SessionEstablishmentExchangeDispatch::MessagePermitted(Protocols::Id protocol, uint8_t type)
 {
-    switch (protocol)
+    if (protocol == Protocols::SecureChannel::Id)
     {
-    case Protocols::SecureChannel::Id.GetProtocolId():
         switch (type)
         {
         case static_cast<uint8_t>(Protocols::SecureChannel::MsgType::StandaloneAck):
@@ -52,11 +51,8 @@ bool SessionEstablishmentExchangeDispatch::MessagePermitted(uint16_t protocol, u
         default:
             break;
         }
-        break;
-
-    default:
-        break;
     }
+
     return false;
 }
 

src/protocols/secure_channel/SessionEstablishmentExchangeDispatch.h

@@ -41,7 +41,7 @@ class SessionEstablishmentExchangeDispatch : public Messaging::ExchangeMessageDi
     ~SessionEstablishmentExchangeDispatch() override {}
 
 protected:
-    bool MessagePermitted(uint16_t protocol, uint8_t type) override;
+    bool MessagePermitted(Protocols::Id, uint8_t type) override;
     bool IsEncryptionRequired() const override { return false; }
 };
 

src/protocols/secure_channel/tests/TestCASESession.cpp

@@ -236,12 +236,11 @@ void CASE_SecurePairingStartTest(nlTestSuite * inSuite, void * inContext)
     ctx.DrainAndServiceIO();
 
     auto & loopback = ctx.GetLoopback();
-    NL_TEST_ASSERT(inSuite, loopback.mSentMessageCount == 1);
+    // There should have been two message sent: Sigma1 and an ack.
+    NL_TEST_ASSERT(inSuite, loopback.mSentMessageCount == 2);
 
-    // Clear pending packet in CRMP
-    ReliableMessageMgr * rm     = ctx.GetExchangeManager().GetReliableMessageMgr();
-    ReliableMessageContext * rc = context->GetReliableMessageContext();
-    rm->ClearRetransTable(rc);
+    ReliableMessageMgr * rm = ctx.GetExchangeManager().GetReliableMessageMgr();
+    NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 0);
 
     loopback.mMessageSendError = CHIP_ERROR_BAD_REQUEST;
 

src/protocols/secure_channel/tests/TestPASESession.cpp

@@ -177,12 +177,11 @@ void SecurePairingStartTest(nlTestSuite * inSuite, void * inContext)
                                 &delegate) == CHIP_NO_ERROR);
     ctx.DrainAndServiceIO();
 
-    NL_TEST_ASSERT(inSuite, loopback.mSentMessageCount == 1);
+    // There should have been two messages sent: PBKDFParamRequest and an ack.
+    NL_TEST_ASSERT(inSuite, loopback.mSentMessageCount == 2);
 
-    // Clear pending packet in CRMP
-    ReliableMessageMgr * rm     = ctx.GetExchangeManager().GetReliableMessageMgr();
-    ReliableMessageContext * rc = context->GetReliableMessageContext();
-    rm->ClearRetransTable(rc);
+    ReliableMessageMgr * rm = ctx.GetExchangeManager().GetReliableMessageMgr();
+    NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 0);
 
     loopback.Reset();
     loopback.mSentMessageCount = 0;