A command with no data struct will cause use of an un-initialized TLV reader

[bzbarsky-apple]

Problem

CommandHandler::ProcessCommandDataElement does this:

    chip::TLV::TLVReader commandDataReader;
...

    err = aCommandElement.GetData(&commandDataReader);
    if (CHIP_END_OF_TLV == err)
    {
        err = CHIP_NO_ERROR;
        ChipLogDetail(DataManagement, "Received command without data for cluster " ChipLogFormatMEI, ChipLogValueMEI(clusterId));
    }

But now commandDataReader is not initialized, and we proceed to pass it to DispatchSingleClusterCommand which will try to read from it.

Proposed Solution

1. Write a test that exercises this case. In particular, this should send no data for a command that normally does have fields that get sent.
2. Fix the code to handle this correctly.

@yunhanw-google @mrjerryjohns

[bzbarsky-apple]

In particular, we need to decide how to represent "there is no command data here" when calling DispatchSingleClusterCommand. Perhaps it should take the reader as a pointer, not a reference?

[mrjerryjohns]

Our current CommandSender implementation always inserts the data payload struct (even if it is empty) right? So, that condition should never get triggered on the handler?

What should be the spec defined behavior here? Is not sending the data element permitted? Looking at the spec, it's quite mum on this...

[mrjerryjohns]

I'd be fine with just initializing the TLVReader with a null buffer. That ensures that subsequent decode will fail. This also reduces the API churn.

[bzbarsky-apple]

So, that condition should never get triggered on the handler?

You're assuming that the sender is using our code. And not being malicious. We should not be assuming that.

[bzbarsky-apple]

We can't defer this: this is a known bug where we read un-initialized memory and then act on it, which is really bad from a security perspective.

[erjiaqing]

I think this issue is already fixed in previous refactors

1. In CommandHandler::ProcessInvokeRequest

connectedhomeip/src/app/CommandHandler.cpp

Lines 97 to 103 in 0861153

	ReturnErrorOnFailure(invokeRequestMessage.Init(reader));
	#if CHIP_CONFIG_IM_ENABLE_SCHEMA_CHECK
	ReturnErrorOnFailure(invokeRequestMessage.CheckSchemaValidity());
	#endif
	ReturnErrorOnFailure(invokeRequestMessage.GetSuppressResponse(&mSuppressResponse));
	ReturnErrorOnFailure(invokeRequestMessage.GetTimedRequest(&mTimedRequest));
	ReturnErrorOnFailure(invokeRequestMessage.GetInvokeRequests(&invokeRequests));

(Which ensures invokeRequests is a valid parser with a reader holding lists.

2. Then we have

connectedhomeip/src/app/CommandHandler.cpp

Lines 128 to 136 in 0861153

	invokeRequests.GetReader(&invokeRequestsReader);
	{
	// We don't support handling multiple commands but the protocol is ready to support it in the future, reject all of them and
	// IM Engine will send a status response.
	size_t commandCount = 0;
	TLV::Utilities::Count(invokeRequestsReader, commandCount, false /* recurse */);
	VerifyOrReturnError(commandCount == 1, CHIP_ERROR_UNSUPPORTED_CHIP_FEATURE);
	}

(Which ensures it is holding exactly one list (as we only supports one request today))

3. And we have

connectedhomeip/src/app/CommandHandler.cpp

Lines 305 to 326 in 0861153

	err = aCommandElement.GetFields(&commandDataReader);
	if (CHIP_END_OF_TLV == err)
	{
	ChipLogDetail(DataManagement,
	"Received command without data for Endpoint=%u Cluster=" ChipLogFormatMEI " Command=" ChipLogFormatMEI,
	concretePath.mEndpointId, ChipLogValueMEI(concretePath.mClusterId), ChipLogValueMEI(concretePath.mCommandId));
	err = CHIP_NO_ERROR;
	}
	if (CHIP_NO_ERROR == err)
	{
	ChipLogDetail(DataManagement, "Received command for Endpoint=%u Cluster=" ChipLogFormatMEI " Command=" ChipLogFormatMEI,
	concretePath.mEndpointId, ChipLogValueMEI(concretePath.mClusterId), ChipLogValueMEI(concretePath.mCommandId));
	SuccessOrExit(MatterPreCommandReceivedCallback(concretePath));
	mpCallback->DispatchCommand(*this, concretePath, commandDataReader);
	MatterPostCommandReceivedCallback(concretePath);
	}
	exit:
	if (err != CHIP_NO_ERROR)
	{
	return AddStatus(concretePath, Status::InvalidCommand);
	}

This should ensure the payload passed to DispatchCommand is a valid reader.

For group writes, we also have

connectedhomeip/src/app/CommandHandler.cpp

Lines 362 to 370 in 0861153

	err = aCommandElement.GetFields(&commandDataReader);
	if (CHIP_END_OF_TLV == err)
	{
	ChipLogDetail(DataManagement,
	"Received command without data for Group=%u Cluster=" ChipLogFormatMEI " Command=" ChipLogFormatMEI, groupId,
	ChipLogValueMEI(clusterId), ChipLogValueMEI(commandId));
	err = CHIP_NO_ERROR;
	}
	SuccessOrExit(err);

[bzbarsky-apple]

This should ensure the payload passed to DispatchCommand is a valid reader.

It does not. What happens if GetFields returns CHIP_END_OF_TLV? The answer is that commandDataReader is left un-unitialized, no?

Group commands have the same problem, as you note.

[bzbarsky-apple]

Attaching a change to the commands test that shows the problem. The decoding of the command either fails if there is no fields struct (which the spec allows), if the random "control byte" value is not of type struct, or we hit ASAN errors trying to decode if the control byte happens to be "struct".

Which actually points out a second problem: validly not including a field struct at all for a command that has no fields will fail decode! This is an absolute must-fix for spec compliance.
demonstrate-uninitialized-reader.txt