Validate InvokeRequestMessage TLV properly terminated before running …

…commands (#31218) * Validate InvokeRequestMessage TLV properly terminated before running commands * Address PR comment
project-chip · Feb 10, 2024 · 1621556 · 1621556
1 parent 36e1029
commit 1621556
Show file tree

Hide file tree

Showing 3 changed files with 15 additions and 45 deletions.
diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp
@@ -105,11 +105,16 @@ void CommandHandler::OnInvokeCommandRequest(Messaging::ExchangeContext * ec, con
     mGoneAsync = true;
 }
 
-CHIP_ERROR CommandHandler::ValidateInvokeRequestsAndBuildRegistry(TLV::TLVReader & invokeRequestsReader)
+CHIP_ERROR CommandHandler::ValidateInvokeRequestMessageAndBuildRegistry(InvokeRequestMessage::Parser & invokeRequestMessage)
 {
     CHIP_ERROR err          = CHIP_NO_ERROR;
     size_t commandCount     = 0;
     bool commandRefExpected = false;
+    InvokeRequests::Parser invokeRequests;
+
+    ReturnErrorOnFailure(invokeRequestMessage.GetInvokeRequests(&invokeRequests));
+    TLV::TLVReader invokeRequestsReader;
+    invokeRequests.GetReader(&invokeRequestsReader);
 
     ReturnErrorOnFailure(TLV::Utilities::Count(invokeRequestsReader, commandCount, false /* recurse */));
 
@@ -166,7 +171,8 @@ CHIP_ERROR CommandHandler::ValidateInvokeRequestsAndBuildRegistry(TLV::TLVReader
     {
         err = CHIP_NO_ERROR;
     }
-    return err;
+    ReturnErrorOnFailure(err);
+    return invokeRequestMessage.ExitContainer();
 }
 
 Status CommandHandler::ProcessInvokeRequest(System::PacketBufferHandle && payload, bool isTimedInvoke)
@@ -191,9 +197,8 @@ Status CommandHandler::ProcessInvokeRequest(System::PacketBufferHandle && payloa
     VerifyOrReturnError(mTimedRequest == isTimedInvoke, Status::UnsupportedAccess);
 
     {
-        TLV::TLVReader validationInvokeRequestsReader;
-        invokeRequests.GetReader(&validationInvokeRequestsReader);
-        VerifyOrReturnError(ValidateInvokeRequestsAndBuildRegistry(validationInvokeRequestsReader) == CHIP_NO_ERROR,
+        InvokeRequestMessage::Parser validationInvokeRequestMessage = invokeRequestMessage;
+        VerifyOrReturnError(ValidateInvokeRequestMessageAndBuildRegistry(validationInvokeRequestMessage) == CHIP_NO_ERROR,
                             Status::InvalidAction);
     }
 

diff --git a/src/app/CommandHandler.h b/src/app/CommandHandler.h
@@ -209,12 +209,13 @@ class CommandHandler : public Messaging::ExchangeDelegate
 
     /**
      * Checks that all CommandDataIB within InvokeRequests satisfy the spec's general
-     * constraints for CommandDataIB.
+     * constraints for CommandDataIB. Additionally checks that InvokeRequestMessage is
+     * properly formatted.
      *
      * This also builds a registry that to ensure that all commands can be responded
      * to with the data required as per spec.
      */
-    CHIP_ERROR ValidateInvokeRequestsAndBuildRegistry(TLV::TLVReader & invokeRequestsReader);
+    CHIP_ERROR ValidateInvokeRequestMessageAndBuildRegistry(InvokeRequestMessage::Parser & invokeRequestMessage);
 
     /**
      * Adds the given command status and returns any failures in adding statuses (e.g. out

diff --git a/src/app/tests/TestCommandInteraction.cpp b/src/app/tests/TestCommandInteraction.cpp
@@ -247,7 +247,6 @@ class TestCommandInteraction
     static void TestCommandInvalidMessage3(nlTestSuite * apSuite, void * apContext);
     static void TestCommandInvalidMessage4(nlTestSuite * apSuite, void * apContext);
     static void TestCommandHandlerInvalidMessageSync(nlTestSuite * apSuite, void * apContext);
-    static void TestCommandHandlerInvalidMessageAsync(nlTestSuite * apSuite, void * apContext);
     static void TestCommandHandlerCommandEncodeExternalFailure(nlTestSuite * apSuite, void * apContext);
     static void TestCommandHandlerWithSendSimpleStatusCode(nlTestSuite * apSuite, void * apContext);
     static void TestCommandHandlerWithSendEmptyResponse(nlTestSuite * apSuite, void * apContext);
@@ -1079,48 +1078,14 @@ void TestCommandInteraction::TestCommandHandlerInvalidMessageSync(nlTestSuite *
     mockCommandSenderDelegate.ResetCounter();
     app::CommandSender commandSender(&mockCommandSenderDelegate, &ctx.GetExchangeManager());
 
+    chip::isCommandDispatched = false;
     AddInvalidInvokeRequestData(apSuite, apContext, &commandSender);
     err = commandSender.SendCommandRequest(ctx.GetSessionBobToAlice());
     NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
 
     ctx.DrainAndServiceIO();
 
-    NL_TEST_ASSERT(apSuite,
-                   mockCommandSenderDelegate.onResponseCalledTimes == 0 && mockCommandSenderDelegate.onFinalCalledTimes == 1 &&
-                       mockCommandSenderDelegate.onErrorCalledTimes == 1);
-    NL_TEST_ASSERT(apSuite, mockCommandSenderDelegate.mError == CHIP_IM_GLOBAL_STATUS(InvalidAction));
-    NL_TEST_ASSERT(apSuite, GetNumActiveHandlerObjects() == 0);
-    NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0);
-}
-
-// Command Sender sends malformed invoke request, this command is aysnc command, handler fails to process it and sends status
-// report with invalid action
-void TestCommandInteraction::TestCommandHandlerInvalidMessageAsync(nlTestSuite * apSuite, void * apContext)
-{
-    TestContext & ctx = *static_cast<TestContext *>(apContext);
-    CHIP_ERROR err    = CHIP_NO_ERROR;
-
-    mockCommandSenderDelegate.ResetCounter();
-    app::CommandSender commandSender(&mockCommandSenderDelegate, &ctx.GetExchangeManager());
-    asyncCommand = true;
-    AddInvalidInvokeRequestData(apSuite, apContext, &commandSender);
-    err = commandSender.SendCommandRequest(ctx.GetSessionBobToAlice());
-    NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
-
-    ctx.DrainAndServiceIO();
-
-    // Error status response has been sent already; it's not waiting for the handle.
-    NL_TEST_ASSERT(apSuite,
-                   mockCommandSenderDelegate.onResponseCalledTimes == 0 && mockCommandSenderDelegate.onFinalCalledTimes == 1 &&
-                       mockCommandSenderDelegate.onErrorCalledTimes == 1);
-    NL_TEST_ASSERT(apSuite, GetNumActiveHandlerObjects() == 1);
-    NL_TEST_ASSERT(apSuite, asyncCommand == false);
-
-    // Decrease CommandHandler refcount. No additional message should be sent since error response already sent.
-    asyncCommandHandle = nullptr;
-
-    ctx.DrainAndServiceIO();
-
+    NL_TEST_ASSERT(apSuite, !chip::isCommandDispatched);
     NL_TEST_ASSERT(apSuite,
                    mockCommandSenderDelegate.onResponseCalledTimes == 0 && mockCommandSenderDelegate.onFinalCalledTimes == 1 &&
                        mockCommandSenderDelegate.onErrorCalledTimes == 1);
@@ -1725,7 +1690,6 @@ const nlTest sTests[] =
     NL_TEST_DEF("TestCommandSenderCommandFailureResponseFlow", chip::app::TestCommandInteraction::TestCommandSenderCommandFailureResponseFlow),
     NL_TEST_DEF("TestCommandSenderAbruptDestruction", chip::app::TestCommandInteraction::TestCommandSenderAbruptDestruction),
     NL_TEST_DEF("TestCommandHandlerInvalidMessageSync", chip::app::TestCommandInteraction::TestCommandHandlerInvalidMessageSync),
-    NL_TEST_DEF("TestCommandHandlerInvalidMessageAsync", chip::app::TestCommandInteraction::TestCommandHandlerInvalidMessageAsync),
     NL_TEST_SENTINEL()
 };
 // clang-format on