From 2326832f09a3c2b9cb2e336a25c2a9623e335b42 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Fri, 8 Jul 2022 12:19:34 -0400 Subject: [PATCH] Enforce constraints for sizes of labelstruct label and value lengths (#20431) * Enforce constraints for sizes of labelstruct label and value lengths * Add a test user label cluster constraint integration test * regen for tests * Restyle --- .../user-label-server/user-label-server.cpp | 27 ++++ .../TestUserLabelClusterConstraints.yaml | 57 +++++++ src/app/tests/suites/tests.js | 1 + .../chip-tool/zap-generated/test/Commands.h | 114 +++++++++++++ .../zap-generated/test/Commands.h | 152 ++++++++++++++++++ 5 files changed, 351 insertions(+) create mode 100644 src/app/tests/suites/TestUserLabelClusterConstraints.yaml diff --git a/src/app/clusters/user-label-server/user-label-server.cpp b/src/app/clusters/user-label-server/user-label-server.cpp index bb5b2a54228344..f1e6f5d66ae9fc 100644 --- a/src/app/clusters/user-label-server/user-label-server.cpp +++ b/src/app/clusters/user-label-server/user-label-server.cpp @@ -53,6 +53,29 @@ class UserLabelAttrAccess : public AttributeAccessInterface CHIP_ERROR WriteLabelList(const ConcreteDataAttributePath & aPath, AttributeValueDecoder & aDecoder); }; +/// Matches constraints on a LabelStruct. +bool IsValidLabelEntry(const Structs::LabelStruct::Type & entry) +{ + constexpr size_t kMaxLabelSize = 16; + constexpr size_t kMaxValueSize = 16; + + // NOTE: spec default for label and value is empty, so empty is accepted here + return (entry.label.size() <= kMaxLabelSize) && (entry.value.size() <= kMaxValueSize); +} + +bool IsValidLabelEntryList(const LabelList::TypeInfo::DecodableType & list) +{ + auto iter = list.begin(); + while (iter.Next()) + { + if (!IsValidLabelEntry(iter.GetValue())) + { + return false; + } + } + return true; +} + UserLabelAttrAccess gAttrAccess; CHIP_ERROR UserLabelAttrAccess::ReadLabelList(EndpointId endpoint, AttributeValueEncoder & aEncoder) @@ -106,6 +129,7 @@ CHIP_ERROR UserLabelAttrAccess::WriteLabelList(const ConcreteDataAttributePath & LabelList::TypeInfo::DecodableType decodablelist; ReturnErrorOnFailure(aDecoder.Decode(decodablelist)); + ReturnErrorCodeIf(!IsValidLabelEntryList(decodablelist), CHIP_ERROR_INVALID_ARGUMENT); auto iter = decodablelist.begin(); while (iter.Next()) @@ -120,7 +144,10 @@ CHIP_ERROR UserLabelAttrAccess::WriteLabelList(const ConcreteDataAttributePath & if (aPath.mListOp == ConcreteDataAttributePath::ListOperation::AppendItem) { Structs::LabelStruct::DecodableType entry; + ReturnErrorOnFailure(aDecoder.Decode(entry)); + ReturnErrorCodeIf(!IsValidLabelEntry(entry), CHIP_ERROR_INVALID_ARGUMENT); + return provider->AppendUserLabel(endpoint, entry); } diff --git a/src/app/tests/suites/TestUserLabelClusterConstraints.yaml b/src/app/tests/suites/TestUserLabelClusterConstraints.yaml new file mode 100644 index 00000000000000..2072b91f8df64b --- /dev/null +++ b/src/app/tests/suites/TestUserLabelClusterConstraints.yaml @@ -0,0 +1,57 @@ +# Copyright (c) 2022 Project CHIP Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +name: User Label Cluster Tests + +config: + nodeId: 0x12344321 + cluster: "User Label" + endpoint: 0 + +tests: + - label: "Wait for the commissioned device to be retrieved" + cluster: "DelayCommands" + command: "WaitForCommissionee" + arguments: + values: + - name: "nodeId" + value: nodeId + + - label: "Attempt to write overly long item for label" + command: "writeAttribute" + attribute: "label list" + arguments: + value: + [ + { + label: "this is longer than sixteen characters", + value: "bedroom 2", + }, + ] + response: + error: FAILURE + + - label: "Attempt to write overly long item for value" + command: "writeAttribute" + attribute: "label list" + arguments: + value: + [ + { + label: "test", + value: "this is longer than sixteen characters", + }, + ] + response: + error: FAILURE diff --git a/src/app/tests/suites/tests.js b/src/app/tests/suites/tests.js index 97fa30a28d5f71..dee8f0728dbe0a 100644 --- a/src/app/tests/suites/tests.js +++ b/src/app/tests/suites/tests.js @@ -717,6 +717,7 @@ function getTests() { "TestSystemCommands", "TestBinding", "TestUserLabelCluster", + "TestUserLabelClusterConstraints", "TestArmFailSafe", "TestFanControl", ]; diff --git a/zzz_generated/chip-tool/zap-generated/test/Commands.h b/zzz_generated/chip-tool/zap-generated/test/Commands.h index 45f66866c49886..73abd09ecf42b6 100644 --- a/zzz_generated/chip-tool/zap-generated/test/Commands.h +++ b/zzz_generated/chip-tool/zap-generated/test/Commands.h @@ -207,6 +207,7 @@ class TestList : public Command printf("TestSystemCommands\n"); printf("TestBinding\n"); printf("TestUserLabelCluster\n"); + printf("TestUserLabelClusterConstraints\n"); printf("TestArmFailSafe\n"); printf("TestFanControl\n"); printf("TestMultiAdmin\n"); @@ -51861,6 +51862,118 @@ class TestUserLabelClusterSuite : public TestCommand } }; +class TestUserLabelClusterConstraintsSuite : public TestCommand +{ +public: + TestUserLabelClusterConstraintsSuite(CredentialIssuerCommands * credsIssuerConfig) : + TestCommand("TestUserLabelClusterConstraints", 3, credsIssuerConfig) + { + AddArgument("nodeId", 0, UINT64_MAX, &mNodeId); + AddArgument("cluster", &mCluster); + AddArgument("endpoint", 0, UINT16_MAX, &mEndpoint); + AddArgument("timeout", 0, UINT16_MAX, &mTimeout); + } + + ~TestUserLabelClusterConstraintsSuite() {} + + chip::System::Clock::Timeout GetWaitDuration() const override + { + return chip::System::Clock::Seconds16(mTimeout.ValueOr(kTimeoutInSeconds)); + } + +private: + chip::Optional mNodeId; + chip::Optional mCluster; + chip::Optional mEndpoint; + chip::Optional mTimeout; + + chip::EndpointId GetEndpoint(chip::EndpointId endpoint) { return mEndpoint.HasValue() ? mEndpoint.Value() : endpoint; } + + // + // Tests methods + // + + void OnResponse(const chip::app::StatusIB & status, chip::TLV::TLVReader * data) override + { + bool shouldContinue = false; + + switch (mTestIndex - 1) + { + case 0: + VerifyOrReturn(CheckValue("status", chip::to_underlying(status.mStatus), 0)); + shouldContinue = true; + break; + case 1: + VerifyOrReturn(CheckValue("status", chip::to_underlying(status.mStatus), EMBER_ZCL_STATUS_FAILURE)); + break; + case 2: + VerifyOrReturn(CheckValue("status", chip::to_underlying(status.mStatus), EMBER_ZCL_STATUS_FAILURE)); + break; + default: + LogErrorOnFailure(ContinueOnChipMainThread(CHIP_ERROR_INVALID_ARGUMENT)); + } + + if (shouldContinue) + { + ContinueOnChipMainThread(CHIP_NO_ERROR); + } + } + + CHIP_ERROR DoTestStep(uint16_t testIndex) override + { + using namespace chip::app::Clusters; + switch (testIndex) + { + case 0: { + LogStep(0, "Wait for the commissioned device to be retrieved"); + ListFreer listFreer; + chip::app::Clusters::DelayCommands::Commands::WaitForCommissionee::Type value; + value.nodeId = mNodeId.HasValue() ? mNodeId.Value() : 305414945ULL; + return WaitForCommissionee(kIdentityAlpha, value); + } + case 1: { + LogStep(1, "Attempt to write overly long item for label"); + ListFreer listFreer; + chip::app::DataModel::List value; + + { + auto * listHolder_0 = new ListHolder(1); + listFreer.add(listHolder_0); + + listHolder_0->mList[0].label = + chip::Span("this is longer than sixteen charactersgarbage: not in length on purpose", 38); + listHolder_0->mList[0].value = chip::Span("bedroom 2garbage: not in length on purpose", 9); + + value = + chip::app::DataModel::List(listHolder_0->mList, 1); + } + return WriteAttribute(kIdentityAlpha, GetEndpoint(0), UserLabel::Id, UserLabel::Attributes::LabelList::Id, value, + chip::NullOptional, chip::NullOptional); + } + case 2: { + LogStep(2, "Attempt to write overly long item for value"); + ListFreer listFreer; + chip::app::DataModel::List value; + + { + auto * listHolder_0 = new ListHolder(1); + listFreer.add(listHolder_0); + + listHolder_0->mList[0].label = chip::Span("testgarbage: not in length on purpose", 4); + listHolder_0->mList[0].value = + chip::Span("this is longer than sixteen charactersgarbage: not in length on purpose", 38); + + value = + chip::app::DataModel::List(listHolder_0->mList, 1); + } + return WriteAttribute(kIdentityAlpha, GetEndpoint(0), UserLabel::Id, UserLabel::Attributes::LabelList::Id, value, + chip::NullOptional, chip::NullOptional); + } + } + return CHIP_NO_ERROR; + } +}; + class TestArmFailSafeSuite : public TestCommand { public: @@ -86564,6 +86677,7 @@ void registerCommandsTests(Commands & commands, CredentialIssuerCommands * creds make_unique(credsIssuerConfig), make_unique(credsIssuerConfig), make_unique(credsIssuerConfig), + make_unique(credsIssuerConfig), make_unique(credsIssuerConfig), make_unique(credsIssuerConfig), make_unique(credsIssuerConfig), diff --git a/zzz_generated/darwin-framework-tool/zap-generated/test/Commands.h b/zzz_generated/darwin-framework-tool/zap-generated/test/Commands.h index 19d16d3c2942fe..34ab4da3903ef8 100644 --- a/zzz_generated/darwin-framework-tool/zap-generated/test/Commands.h +++ b/zzz_generated/darwin-framework-tool/zap-generated/test/Commands.h @@ -195,6 +195,7 @@ class TestList : public Command { printf("TestSystemCommands\n"); printf("TestBinding\n"); printf("TestUserLabelCluster\n"); + printf("TestUserLabelClusterConstraints\n"); printf("TestArmFailSafe\n"); printf("TestFanControl\n"); printf("TestMultiAdmin\n"); @@ -89207,6 +89208,156 @@ class TestUserLabelCluster : public TestCommandBridge { } }; +class TestUserLabelClusterConstraints : public TestCommandBridge { +public: + // NOLINTBEGIN(clang-analyzer-nullability.NullPassedToNonnull): Test constructor nullability not enforced + TestUserLabelClusterConstraints() + : TestCommandBridge("TestUserLabelClusterConstraints") + , mTestIndex(0) + { + AddArgument("nodeId", 0, UINT64_MAX, &mNodeId); + AddArgument("cluster", &mCluster); + AddArgument("endpoint", 0, UINT16_MAX, &mEndpoint); + AddArgument("timeout", 0, UINT16_MAX, &mTimeout); + } + // NOLINTEND(clang-analyzer-nullability.NullPassedToNonnull) + + ~TestUserLabelClusterConstraints() {} + + /////////// TestCommand Interface ///////// + void NextTest() override + { + CHIP_ERROR err = CHIP_NO_ERROR; + + if (0 == mTestIndex) { + ChipLogProgress(chipTool, " **** Test Start: TestUserLabelClusterConstraints\n"); + } + + if (mTestCount == mTestIndex) { + ChipLogProgress(chipTool, " **** Test Complete: TestUserLabelClusterConstraints\n"); + SetCommandExitStatus(CHIP_NO_ERROR); + return; + } + + Wait(); + + // Ensure we increment mTestIndex before we start running the relevant + // command. That way if we lose the timeslice after we send the message + // but before our function call returns, we won't end up with an + // incorrect mTestIndex value observed when we get the response. + switch (mTestIndex++) { + case 0: + ChipLogProgress(chipTool, " ***** Test Step 0 : Wait for the commissioned device to be retrieved\n"); + err = TestWaitForTheCommissionedDeviceToBeRetrieved_0(); + break; + case 1: + ChipLogProgress(chipTool, " ***** Test Step 1 : Attempt to write overly long item for label\n"); + err = TestAttemptToWriteOverlyLongItemForLabel_1(); + break; + case 2: + ChipLogProgress(chipTool, " ***** Test Step 2 : Attempt to write overly long item for value\n"); + err = TestAttemptToWriteOverlyLongItemForValue_2(); + break; + } + + if (CHIP_NO_ERROR != err) { + ChipLogError(chipTool, " ***** Test Failure: %s\n", chip::ErrorStr(err)); + SetCommandExitStatus(err); + } + } + + void OnStatusUpdate(const chip::app::StatusIB & status) override + { + switch (mTestIndex - 1) { + case 0: + VerifyOrReturn(CheckValue("status", chip::to_underlying(status.mStatus), 0)); + break; + case 1: + VerifyOrReturn(CheckValue("status", chip::to_underlying(status.mStatus), EMBER_ZCL_STATUS_FAILURE)); + break; + case 2: + VerifyOrReturn(CheckValue("status", chip::to_underlying(status.mStatus), EMBER_ZCL_STATUS_FAILURE)); + break; + } + + // Go on to the next test. + ContinueOnChipMainThread(CHIP_NO_ERROR); + } + + chip::System::Clock::Timeout GetWaitDuration() const override + { + return chip::System::Clock::Seconds16(mTimeout.ValueOr(kTimeoutInSeconds)); + } + +private: + std::atomic_uint16_t mTestIndex; + const uint16_t mTestCount = 3; + + chip::Optional mNodeId; + chip::Optional mCluster; + chip::Optional mEndpoint; + chip::Optional mTimeout; + + CHIP_ERROR TestWaitForTheCommissionedDeviceToBeRetrieved_0() + { + chip::app::Clusters::DelayCommands::Commands::WaitForCommissionee::Type value; + value.nodeId = mNodeId.HasValue() ? mNodeId.Value() : 305414945ULL; + return WaitForCommissionee("alpha", value); + } + + CHIP_ERROR TestAttemptToWriteOverlyLongItemForLabel_1() + { + MTRDevice * device = GetDevice("alpha"); + MTRTestUserLabel * cluster = [[MTRTestUserLabel alloc] initWithDevice:device endpoint:0 queue:mCallbackQueue]; + VerifyOrReturnError(cluster != nil, CHIP_ERROR_INCORRECT_STATE); + + id labelListArgument; + { + NSMutableArray * temp_0 = [[NSMutableArray alloc] init]; + temp_0[0] = [[MTRUserLabelClusterLabelStruct alloc] init]; + ((MTRUserLabelClusterLabelStruct *) temp_0[0]).label = @"this is longer than sixteen characters"; + ((MTRUserLabelClusterLabelStruct *) temp_0[0]).value = @"bedroom 2"; + + labelListArgument = temp_0; + } + [cluster writeAttributeLabelListWithValue:labelListArgument + completionHandler:^(NSError * _Nullable err) { + NSLog(@"Attempt to write overly long item for label Error: %@", err); + + VerifyOrReturn(CheckValue("status", err ? err.code : 0, EMBER_ZCL_STATUS_FAILURE)); + NextTest(); + }]; + + return CHIP_NO_ERROR; + } + + CHIP_ERROR TestAttemptToWriteOverlyLongItemForValue_2() + { + MTRDevice * device = GetDevice("alpha"); + MTRTestUserLabel * cluster = [[MTRTestUserLabel alloc] initWithDevice:device endpoint:0 queue:mCallbackQueue]; + VerifyOrReturnError(cluster != nil, CHIP_ERROR_INCORRECT_STATE); + + id labelListArgument; + { + NSMutableArray * temp_0 = [[NSMutableArray alloc] init]; + temp_0[0] = [[MTRUserLabelClusterLabelStruct alloc] init]; + ((MTRUserLabelClusterLabelStruct *) temp_0[0]).label = @"test"; + ((MTRUserLabelClusterLabelStruct *) temp_0[0]).value = @"this is longer than sixteen characters"; + + labelListArgument = temp_0; + } + [cluster writeAttributeLabelListWithValue:labelListArgument + completionHandler:^(NSError * _Nullable err) { + NSLog(@"Attempt to write overly long item for value Error: %@", err); + + VerifyOrReturn(CheckValue("status", err ? err.code : 0, EMBER_ZCL_STATUS_FAILURE)); + NextTest(); + }]; + + return CHIP_NO_ERROR; + } +}; + class TestArmFailSafe : public TestCommandBridge { public: // NOLINTBEGIN(clang-analyzer-nullability.NullPassedToNonnull): Test constructor nullability not enforced @@ -108425,6 +108576,7 @@ void registerCommandsTests(Commands & commands) make_unique(), make_unique(), make_unique(), + make_unique(), make_unique(), make_unique(), make_unique(),