Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Validate InvokeRequestMessage TLV properly terminated before running commands #31218

Merged
merged 2 commits into from
Jan 3, 2024
Merged

Validate InvokeRequestMessage TLV properly terminated before running commands #31218

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
94ea0c7
Validate InvokeRequestMessage TLV properly terminated before running …
tehampson Jan 2, 2024
4c5c2ae
Address PR comment
tehampson Jan 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions src/app/CommandHandler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 */));

Expand Down Expand Up @@ -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)
Expand All @@ -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);
}

Expand Down
5 changes: 3 additions & 2 deletions src/app/CommandHandler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
40 changes: 2 additions & 38 deletions src/app/tests/TestCommandInteraction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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
Expand Down
Loading