Skip to content

Commit

Permalink
Add strict IM encoding ordering check (#12549)
Browse files Browse the repository at this point in the history
  • Loading branch information
@yunhanw-google @pull
yunhanw-google authored and pull[bot] committed Feb 3, 2024
1 parent c8ed37f commit 1711905
Show file tree
Hide file tree
Showing 13 changed files with 127 additions and 82 deletions.
12 changes: 5 additions & 7 deletions src/app/EventManagement.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -449,12 +449,6 @@ CHIP_ERROR EventManagement::CopyAndAdjustDeltaTime(const TLVReader & aReader, si
CopyAndAdjustDeltaTimeContext * ctx = static_cast<CopyAndAdjustDeltaTimeContext *>(apContext);
TLVReader reader(aReader);

if (aReader.GetTag() == TLV::ContextTag(to_underlying(EventDataIB::Tag::kPath)))
{
err =
ctx->mpWriter->Put(TLV::ContextTag(to_underlying(EventDataIB::Tag::kEventNumber)), ctx->mpContext->mCurrentEventNumber);
}

if (aReader.GetTag() == TLV::ContextTag(to_underlying(EventDataIB::Tag::kDeltaSystemTimestamp)) ||
aReader.GetTag() == TLV::ContextTag(to_underlying(EventDataIB::Tag::kDeltaEpochTimestamp)))
{
Expand Down Expand Up @@ -490,6 +484,11 @@ CHIP_ERROR EventManagement::CopyAndAdjustDeltaTime(const TLVReader & aReader, si
err = ctx->mpWriter->CopyElement(reader);
}

if (aReader.GetTag() == TLV::ContextTag(to_underlying(EventDataIB::Tag::kPath)))
{
err =
ctx->mpWriter->Put(TLV::ContextTag(to_underlying(EventDataIB::Tag::kEventNumber)), ctx->mpContext->mCurrentEventNumber);
}
return err;
}

Expand Down Expand Up @@ -762,7 +761,6 @@ CHIP_ERROR EventManagement::FetchEventsSince(TLVWriter & aWriter, ClusterInfo *
}

exit:
ChipLogProgress(EventLogging, "Debug log, err: %s\n", chip::ErrorStr(err));
aEventNumber = context.mCurrentEventNumber;
aEventCount += context.mEventCount;
return err;
Expand Down
10 changes: 5 additions & 5 deletions src/app/MessageDef/ReportDataMessage.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,11 @@ namespace app {
namespace ReportDataMessage {
enum
{
kCsTag_SuppressResponse = 0,
kCsTag_SubscriptionId = 1,
kCsTag_AttributeReportIBs = 2,
kCsTag_EventReports = 3,
kCsTag_MoreChunkedMessages = 4,
kCsTag_SubscriptionId = 0,
kCsTag_AttributeReportIBs = 1,
kCsTag_EventReports = 2,
kCsTag_MoreChunkedMessages = 3,
kCsTag_SuppressResponse = 4,
};

class Parser : public StructParser
Expand Down
30 changes: 30 additions & 0 deletions src/app/MessageDef/StructParser.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,37 @@ CHIP_ERROR StructParser::Init(const TLV::TLVReader & aReader)
mReader.Init(aReader);
VerifyOrReturnError(TLV::kTLVType_Structure == mReader.GetType(), CHIP_ERROR_WRONG_TLV_TYPE);
ReturnErrorOnFailure(mReader.EnterContainer(mOuterContainerType));
ReturnErrorOnFailure(CheckSchemaOrdering());
return CHIP_NO_ERROR;
}

CHIP_ERROR StructParser::CheckSchemaOrdering() const
{
CHIP_ERROR err = CHIP_NO_ERROR;
TLV::TLVReader reader;
reader.Init(mReader);
uint32_t preTagNum = 0;
bool first = true;
while (CHIP_NO_ERROR == (err = reader.Next()))
{
VerifyOrReturnError(TLV::IsContextTag(reader.GetTag()), CHIP_ERROR_INVALID_TLV_TAG);
uint32_t tagNum = TLV::TagNumFromTag(reader.GetTag());
if (first || (preTagNum < tagNum))
{
preTagNum = tagNum;
}
else
{
return CHIP_ERROR_INVALID_TLV_TAG;
}
first = false;
}
if (CHIP_END_OF_TLV == err)
{
err = CHIP_NO_ERROR;
}
ReturnErrorOnFailure(err);
return reader.ExitContainer(mOuterContainerType);
}
} // namespace app
} // namespace chip
2 changes: 2 additions & 0 deletions src/app/MessageDef/StructParser.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ class StructParser : public Parser
* @return #CHIP_NO_ERROR on success
*/
CHIP_ERROR Init(const TLV::TLVReader & aReader);

CHIP_ERROR CheckSchemaOrdering() const;
};
} // namespace app
} // namespace chip
50 changes: 25 additions & 25 deletions src/app/ReadClient.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,15 @@ CHIP_ERROR ReadClient::SendReadRequest(ReadPrepareParams & aReadPrepareParams)
err = request.Init(&writer);
SuccessOrExit(err);

if (aReadPrepareParams.mAttributePathParamsListSize != 0 && aReadPrepareParams.mpAttributePathParamsList != nullptr)
{
AttributePathIBs::Builder attributePathListBuilder = request.CreateAttributeRequests();
SuccessOrExit(err = attributePathListBuilder.GetError());
err = GenerateAttributePathList(attributePathListBuilder, aReadPrepareParams.mpAttributePathParamsList,
aReadPrepareParams.mAttributePathParamsListSize);
SuccessOrExit(err);
}

if (aReadPrepareParams.mEventPathParamsListSize != 0 && aReadPrepareParams.mpEventPathParamsList != nullptr)
{
EventPathIBs::Builder & eventPathListBuilder = request.CreateEventRequests();
Expand All @@ -160,15 +169,6 @@ CHIP_ERROR ReadClient::SendReadRequest(ReadPrepareParams & aReadPrepareParams)
}
}

if (aReadPrepareParams.mAttributePathParamsListSize != 0 && aReadPrepareParams.mpAttributePathParamsList != nullptr)
{
AttributePathIBs::Builder attributePathListBuilder = request.CreateAttributeRequests();
SuccessOrExit(err = attributePathListBuilder.GetError());
err = GenerateAttributePathList(attributePathListBuilder, aReadPrepareParams.mpAttributePathParamsList,
aReadPrepareParams.mAttributePathParamsListSize);
SuccessOrExit(err);
}

request.IsFabricFiltered(false).EndOfReadRequestMessage();
SuccessOrExit(err = request.GetError());

Expand Down Expand Up @@ -632,12 +632,27 @@ CHIP_ERROR ReadClient::SendSubscribeRequest(ReadPrepareParams & aReadPreparePara
VerifyOrExit(mpCallback != nullptr, err = CHIP_ERROR_INCORRECT_STATE);
msgBuf = System::PacketBufferHandle::New(kMaxSecureSduLengthBytes);
VerifyOrExit(!msgBuf.IsNull(), err = CHIP_ERROR_NO_MEMORY);
VerifyOrExit(aReadPrepareParams.mMinIntervalFloorSeconds < aReadPrepareParams.mMaxIntervalCeilingSeconds,
err = CHIP_ERROR_INVALID_ARGUMENT);

writer.Init(std::move(msgBuf));

err = request.Init(&writer);
SuccessOrExit(err);

request.KeepSubscriptions(aReadPrepareParams.mKeepSubscriptions)
.MinIntervalFloorSeconds(aReadPrepareParams.mMinIntervalFloorSeconds)
.MaxIntervalCeilingSeconds(aReadPrepareParams.mMaxIntervalCeilingSeconds);

if (aReadPrepareParams.mAttributePathParamsListSize != 0 && aReadPrepareParams.mpAttributePathParamsList != nullptr)
{
AttributePathIBs::Builder & attributePathListBuilder = request.CreateAttributeRequests();
SuccessOrExit(err = attributePathListBuilder.GetError());
err = GenerateAttributePathList(attributePathListBuilder, aReadPrepareParams.mpAttributePathParamsList,
aReadPrepareParams.mAttributePathParamsListSize);
SuccessOrExit(err);
}

if (aReadPrepareParams.mEventPathParamsListSize != 0 && aReadPrepareParams.mpEventPathParamsList != nullptr)
{
EventPathIBs::Builder & eventPathListBuilder = request.CreateEventRequests();
Expand All @@ -659,22 +674,7 @@ CHIP_ERROR ReadClient::SendSubscribeRequest(ReadPrepareParams & aReadPreparePara
}
}

if (aReadPrepareParams.mAttributePathParamsListSize != 0 && aReadPrepareParams.mpAttributePathParamsList != nullptr)
{
AttributePathIBs::Builder & attributePathListBuilder = request.CreateAttributeRequests();
SuccessOrExit(err = attributePathListBuilder.GetError());
err = GenerateAttributePathList(attributePathListBuilder, aReadPrepareParams.mpAttributePathParamsList,
aReadPrepareParams.mAttributePathParamsListSize);
SuccessOrExit(err);
}

VerifyOrExit(aReadPrepareParams.mMinIntervalFloorSeconds < aReadPrepareParams.mMaxIntervalCeilingSeconds,
err = CHIP_ERROR_INVALID_ARGUMENT);
request.MinIntervalFloorSeconds(aReadPrepareParams.mMinIntervalFloorSeconds)
.MaxIntervalCeilingSeconds(aReadPrepareParams.mMaxIntervalCeilingSeconds)
.KeepSubscriptions(aReadPrepareParams.mKeepSubscriptions)
.IsFabricFiltered(false)
.EndOfSubscribeRequestMessage();
request.IsFabricFiltered(false).EndOfSubscribeRequestMessage();
SuccessOrExit(err = request.GetError());

err = writer.Finalize(&msgBuf);
Expand Down
11 changes: 5 additions & 6 deletions src/app/WriteClient.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,10 @@ CHIP_ERROR WriteClient::Init(Messaging::ExchangeManager * apExchangeMgr, Callbac
mMessageWriter.Init(std::move(packet));

ReturnErrorOnFailure(mWriteRequestBuilder.Init(&mMessageWriter));
mWriteRequestBuilder.TimedRequest(false).IsFabricFiltered(false);
mWriteRequestBuilder.TimedRequest(false);
ReturnErrorOnFailure(mWriteRequestBuilder.GetError());
attributeDataIBsBuilder = mWriteRequestBuilder.CreateWriteRequests();
ReturnErrorOnFailure(attributeDataIBsBuilder.GetError());

ClearExistingExchangeContext();
mpExchangeMgr = apExchangeMgr;
mpCallback = apCallback;
Expand Down Expand Up @@ -139,6 +138,9 @@ CHIP_ERROR WriteClient::PrepareAttribute(const AttributePathParams & attributePa
VerifyOrReturnError(attributePathParams.IsValidAttributePath(), CHIP_ERROR_INVALID_PATH_LIST);
AttributeDataIB::Builder attributeDataIB = mWriteRequestBuilder.GetWriteRequests().CreateAttributeDataIBBuilder();
ReturnErrorOnFailure(attributeDataIB.GetError());
// TODO: Add attribute version support
attributeDataIB.DataVersion(0);
ReturnErrorOnFailure(attributeDataIB.GetError());
ReturnErrorOnFailure(attributeDataIB.CreatePath().Encode(attributePathParams));
return CHIP_NO_ERROR;
}
Expand All @@ -148,9 +150,6 @@ CHIP_ERROR WriteClient::FinishAttribute()
CHIP_ERROR err = CHIP_NO_ERROR;

AttributeDataIB::Builder AttributeDataIB = mWriteRequestBuilder.GetWriteRequests().GetAttributeDataIBBuilder();

// TODO: Add attribute version support
AttributeDataIB.DataVersion(0);
AttributeDataIB.EndOfAttributeDataIB();
SuccessOrExit(err = AttributeDataIB.GetError());
MoveToState(State::AddAttribute);
Expand All @@ -173,7 +172,7 @@ CHIP_ERROR WriteClient::FinalizeMessage(System::PacketBufferHandle & aPacket)
err = AttributeDataIBsBuilder.GetError();
SuccessOrExit(err);

mWriteRequestBuilder.EndOfWriteRequestMessage();
mWriteRequestBuilder.IsFabricFiltered(false).EndOfWriteRequestMessage();
err = mWriteRequestBuilder.GetError();
SuccessOrExit(err);

Expand Down
33 changes: 17 additions & 16 deletions src/app/tests/TestMessageDef.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,7 @@ void BuildEventDataIB(nlTestSuite * apSuite, EventDataIB::Builder & aEventDataIB
NL_TEST_ASSERT(apSuite, eventPathBuilder.GetError() == CHIP_NO_ERROR);
BuildEventPath(apSuite, eventPathBuilder);

aEventDataIBBuilder.Priority(2).EventNumber(3).EpochTimestamp(4).SystemTimestamp(5).DeltaEpochTimestamp(6).DeltaSystemTimestamp(
aEventDataIBBuilder.EventNumber(2).Priority(3).EpochTimestamp(4).SystemTimestamp(5).DeltaEpochTimestamp(6).DeltaSystemTimestamp(
7);
err = aEventDataIBBuilder.GetError();
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
Expand Down Expand Up @@ -365,10 +365,10 @@ void ParseEventDataIB(nlTestSuite * apSuite, EventDataIB::Parser & aEventDataIBP
err = aEventDataIBParser.GetPath(&eventPath);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
}
err = aEventDataIBParser.GetPriority(&priorityLevel);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR && priorityLevel == 2);
err = aEventDataIBParser.GetEventNumber(&number);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR && number == 3);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR && number == 2);
err = aEventDataIBParser.GetPriority(&priorityLevel);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR && priorityLevel == 3);
err = aEventDataIBParser.GetEpochTimestamp(&EpochTimestamp);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR && EpochTimestamp == 4);
err = aEventDataIBParser.GetSystemTimestamp(&systemTimestamp);
Expand Down Expand Up @@ -535,6 +535,7 @@ void BuildAttributeDataIB(nlTestSuite * apSuite, AttributeDataIB::Builder & aAtt
{
CHIP_ERROR err = CHIP_NO_ERROR;

aAttributeDataIBBuilder.DataVersion(2);
AttributePathIB::Builder attributePathBuilder = aAttributeDataIBBuilder.CreatePath();
NL_TEST_ASSERT(apSuite, aAttributeDataIBBuilder.GetError() == CHIP_NO_ERROR);
BuildAttributePathIB(apSuite, attributePathBuilder);
Expand All @@ -553,7 +554,7 @@ void BuildAttributeDataIB(nlTestSuite * apSuite, AttributeDataIB::Builder & aAtt
err = pWriter->EndContainer(dummyType);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
}
aAttributeDataIBBuilder.DataVersion(2);

err = aAttributeDataIBBuilder.GetError();
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
aAttributeDataIBBuilder.EndOfAttributeDataIB();
Expand Down Expand Up @@ -963,7 +964,7 @@ void BuildReportDataMessage(nlTestSuite * apSuite, chip::TLV::TLVWriter & aWrite
err = reportDataMessageBuilder.Init(&aWriter);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);

reportDataMessageBuilder.SuppressResponse(true).SubscriptionId(2);
reportDataMessageBuilder.SubscriptionId(2);
NL_TEST_ASSERT(apSuite, reportDataMessageBuilder.GetError() == CHIP_NO_ERROR);

AttributeReportIBs::Builder AttributeReportIBs = reportDataMessageBuilder.CreateAttributeReportIBs();
Expand All @@ -974,7 +975,7 @@ void BuildReportDataMessage(nlTestSuite * apSuite, chip::TLV::TLVWriter & aWrite
NL_TEST_ASSERT(apSuite, reportDataMessageBuilder.GetError() == CHIP_NO_ERROR);
BuildEventReports(apSuite, EventReportIBs);

reportDataMessageBuilder.MoreChunkedMessages(true);
reportDataMessageBuilder.MoreChunkedMessages(true).SuppressResponse(true);
NL_TEST_ASSERT(apSuite, reportDataMessageBuilder.GetError() == CHIP_NO_ERROR);

reportDataMessageBuilder.EndOfReportDataMessage();
Expand Down Expand Up @@ -1170,26 +1171,26 @@ void BuildSubscribeRequestMessage(nlTestSuite * apSuite, chip::TLV::TLVWriter &
err = subscribeRequestBuilder.Init(&aWriter);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);

AttributePathIBs::Builder attributePathIBs = subscribeRequestBuilder.CreateAttributeRequests();
subscribeRequestBuilder.KeepSubscriptions(true);
NL_TEST_ASSERT(apSuite, subscribeRequestBuilder.GetError() == CHIP_NO_ERROR);
BuildAttributePathList(apSuite, attributePathIBs);

EventPathIBs::Builder eventPathList = subscribeRequestBuilder.CreateEventRequests();
subscribeRequestBuilder.MinIntervalFloorSeconds(2);
NL_TEST_ASSERT(apSuite, subscribeRequestBuilder.GetError() == CHIP_NO_ERROR);
BuildEventPaths(apSuite, eventPathList);

EventFilterIBs::Builder eventFilters = subscribeRequestBuilder.CreateEventFilters();
subscribeRequestBuilder.MaxIntervalCeilingSeconds(3);
NL_TEST_ASSERT(apSuite, subscribeRequestBuilder.GetError() == CHIP_NO_ERROR);
BuildEventFilters(apSuite, eventFilters);

subscribeRequestBuilder.MinIntervalFloorSeconds(2);
AttributePathIBs::Builder attributePathIBs = subscribeRequestBuilder.CreateAttributeRequests();
NL_TEST_ASSERT(apSuite, subscribeRequestBuilder.GetError() == CHIP_NO_ERROR);
BuildAttributePathList(apSuite, attributePathIBs);

subscribeRequestBuilder.MaxIntervalCeilingSeconds(3);
EventPathIBs::Builder eventPathList = subscribeRequestBuilder.CreateEventRequests();
NL_TEST_ASSERT(apSuite, subscribeRequestBuilder.GetError() == CHIP_NO_ERROR);
BuildEventPaths(apSuite, eventPathList);

subscribeRequestBuilder.KeepSubscriptions(true);
EventFilterIBs::Builder eventFilters = subscribeRequestBuilder.CreateEventFilters();
NL_TEST_ASSERT(apSuite, subscribeRequestBuilder.GetError() == CHIP_NO_ERROR);
BuildEventFilters(apSuite, eventFilters);

subscribeRequestBuilder.IsProxy(true);
NL_TEST_ASSERT(apSuite, subscribeRequestBuilder.GetError() == CHIP_NO_ERROR);
Expand Down
Loading

0 comments on commit 1711905

Please sign in to comment.