From 16215566dbc6539e2cc7722af6d55238749d0ec5 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Wed, 3 Jan 2024 12:44:49 -0500 Subject: [PATCH] Validate InvokeRequestMessage TLV properly terminated before running commands (#31218) * Validate InvokeRequestMessage TLV properly terminated before running commands * Address PR comment --- src/app/CommandHandler.cpp | 15 ++++++--- src/app/CommandHandler.h | 5 +-- src/app/tests/TestCommandInteraction.cpp | 40 ++---------------------- 3 files changed, 15 insertions(+), 45 deletions(-) diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index 1b4f8ea92998ba..a48db7c697e375 100644 --- 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 index 037e2c5619f50e..5f5bf85d05377b 100644 --- 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 index 5ac6e53c9ae8b7..2ece2268f6c522 100644 --- 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(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