From 4b3fff690cb7a531fe2b893d5a8a3d4e1ef683f0 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Tue, 19 Nov 2024 12:27:00 -0500 Subject: [PATCH] Handle ACL and readability in `reporting/Engine.cpp` (#36488) * Move ACL validation in reporting engine for reads * Fix up logic for ACL & return codes * Take into consideration global attributes: their ACL is ok * Make testread pass (with hacks because the test is not sane) * Update comment and model setting. * Another comment update * Fix includes * Fix typo * Restyled by clang-format * Update comment * Update ACL test logic and revert testread to original setup * Restyle * Address review comments * Restyle * Update src/app/reporting/Engine.cpp Co-authored-by: Boris Zbarsky * Update src/app/reporting/Engine.cpp Co-authored-by: Boris Zbarsky * Update src/app/reporting/Engine.cpp Co-authored-by: Boris Zbarsky * Update src/app/reporting/Engine.cpp Co-authored-by: Boris Zbarsky * Update src/app/reporting/Engine.cpp Co-authored-by: Boris Zbarsky * Restyle * Fix extra bracket --------- Co-authored-by: Restyled.io Co-authored-by: Boris Zbarsky Co-authored-by: Andrei Litvin --- src/app/BUILD.gn | 5 +- .../CodegenDataModelProvider_Read.cpp | 27 ---- .../tests/TestCodegenModelViaMocks.cpp | 32 ----- src/app/data-model-provider/Provider.h | 10 +- src/app/reporting/Engine.cpp | 126 +++++++++++++++--- 5 files changed, 110 insertions(+), 90 deletions(-) diff --git a/src/app/BUILD.gn b/src/app/BUILD.gn index 8b008c4e9b238b..26f9515bc188cc 100644 --- a/src/app/BUILD.gn +++ b/src/app/BUILD.gn @@ -201,7 +201,10 @@ static_library("interaction-model") { "reporting/reporting.h", ] - deps = [ "${chip_root}/src/app:events" ] + deps = [ + "${chip_root}/src/app:events", + "${chip_root}/src/app:global-attributes", + ] # Temporary dependency: codegen data provider instance should be provided # by the application diff --git a/src/app/codegen-data-model-provider/CodegenDataModelProvider_Read.cpp b/src/app/codegen-data-model-provider/CodegenDataModelProvider_Read.cpp index a558b8e986298a..88989b52071810 100644 --- a/src/app/codegen-data-model-provider/CodegenDataModelProvider_Read.cpp +++ b/src/app/codegen-data-model-provider/CodegenDataModelProvider_Read.cpp @@ -104,33 +104,6 @@ DataModel::ActionReturnStatus CodegenDataModelProvider::ReadAttribute(const Data ChipLogValueMEI(request.path.mClusterId), request.path.mEndpointId, ChipLogValueMEI(request.path.mAttributeId), request.path.mExpanded); - // ACL check for non-internal requests - if (!request.operationFlags.Has(DataModel::OperationFlags::kInternal)) - { - VerifyOrReturnError(request.subjectDescriptor != nullptr, CHIP_ERROR_INVALID_ARGUMENT); - - Access::RequestPath requestPath{ .cluster = request.path.mClusterId, - .endpoint = request.path.mEndpointId, - .requestType = Access::RequestType::kAttributeReadRequest, - .entityId = request.path.mAttributeId }; - CHIP_ERROR err = Access::GetAccessControl().Check(*request.subjectDescriptor, requestPath, - RequiredPrivilege::ForReadAttribute(request.path)); - if (err != CHIP_NO_ERROR) - { - VerifyOrReturnError((err == CHIP_ERROR_ACCESS_DENIED) || (err == CHIP_ERROR_ACCESS_RESTRICTED_BY_ARL), err); - - // Implementation of 8.4.3.2 of the spec for path expansion - if (request.path.mExpanded) - { - return CHIP_NO_ERROR; - } - - // access denied and access restricted have specific codes for IM - return err == CHIP_ERROR_ACCESS_DENIED ? CHIP_IM_GLOBAL_STATUS(UnsupportedAccess) - : CHIP_IM_GLOBAL_STATUS(AccessRestricted); - } - } - auto metadata = Ember::FindAttributeMetadata(request.path); // Explicit failure in finding a suitable metadata diff --git a/src/app/codegen-data-model-provider/tests/TestCodegenModelViaMocks.cpp b/src/app/codegen-data-model-provider/tests/TestCodegenModelViaMocks.cpp index 8ce369b7bd8220..c9b2a1b99a375c 100644 --- a/src/app/codegen-data-model-provider/tests/TestCodegenModelViaMocks.cpp +++ b/src/app/codegen-data-model-provider/tests/TestCodegenModelViaMocks.cpp @@ -1321,20 +1321,6 @@ TEST(TestCodegenModelViaMocks, CommandHandlerInterfaceAcceptedCommands) EXPECT_FALSE(model.GetAcceptedCommandInfo(ConcreteCommandPath(kMockEndpoint1, MockClusterId(1), 33)).has_value()); } -TEST(TestCodegenModelViaMocks, EmberAttributeReadAclDeny) -{ - UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModelProviderWithContext model; - ScopedMockAccessControl accessControl; - - ReadOperation testRequest(kMockEndpoint1, MockClusterId(1), MockAttributeId(10)); - testRequest.SetSubjectDescriptor(kDenySubjectDescriptor); - - std::unique_ptr encoder = testRequest.StartEncoding(); - - ASSERT_EQ(model.ReadAttribute(testRequest.GetRequest(), *encoder), Status::UnsupportedAccess); -} - TEST(TestCodegenModelViaMocks, ReadForInvalidGlobalAttributePath) { UseMockNodeConfig config(gTestNodeConfig); @@ -1392,24 +1378,6 @@ TEST(TestCodegenModelViaMocks, EmberAttributeInvalidRead) } } -TEST(TestCodegenModelViaMocks, EmberAttributePathExpansionAccessDeniedRead) -{ - UseMockNodeConfig config(gTestNodeConfig); - CodegenDataModelProviderWithContext model; - ScopedMockAccessControl accessControl; - - ReadOperation testRequest(kMockEndpoint1, MockClusterId(1), MockAttributeId(10)); - testRequest.SetSubjectDescriptor(kDenySubjectDescriptor); - testRequest.SetPathExpanded(true); - - std::unique_ptr encoder = testRequest.StartEncoding(); - - // For expanded paths, access control failures succeed without encoding anything - // This is temporary until ACL checks are moved inside the IM/ReportEngine - ASSERT_EQ(model.ReadAttribute(testRequest.GetRequest(), *encoder), CHIP_NO_ERROR); - ASSERT_FALSE(encoder->TriedEncode()); -} - TEST(TestCodegenModelViaMocks, AccessInterfaceUnsupportedRead) { UseMockNodeConfig config(gTestNodeConfig); diff --git a/src/app/data-model-provider/Provider.h b/src/app/data-model-provider/Provider.h index 16dd7327a35a37..c547b75b81984f 100644 --- a/src/app/data-model-provider/Provider.h +++ b/src/app/data-model-provider/Provider.h @@ -60,15 +60,7 @@ class Provider : public ProviderMetadataTree virtual InteractionModelContext CurrentContext() const { return mContext; } /// TEMPORARY/TRANSITIONAL requirement for transitioning from ember-specific code - /// ReadAttribute is REQUIRED to perform: - /// - ACL validation (see notes on OperationFlags::kInternal) - /// - Validation of readability/writability (also controlled by OperationFlags::kInternal) - /// - use request.path.mExpanded to skip encoding replies for data according - /// to 8.4.3.2 of the spec: - /// > If the path indicates attribute data that is not readable, then the path SHALL - /// be discarded. - /// > Else if reading from the attribute in the path requires a privilege that is not - /// granted to access the cluster in the path, then the path SHALL be discarded. + /// ReadAttribute is REQUIRED to respond to GlobalAttribute read requests /// /// Return value notes: /// ActionReturnStatus::IsOutOfSpaceEncodingResponse diff --git a/src/app/reporting/Engine.cpp b/src/app/reporting/Engine.cpp index ea4c0014e71397..63d71bfc92db7a 100644 --- a/src/app/reporting/Engine.cpp +++ b/src/app/reporting/Engine.cpp @@ -16,17 +16,24 @@ * limitations under the License. */ +#include +#include #include #include +#include #include #include #include +#include #include #include #include #include #include +#include #include +#include +#include #include #if CHIP_CONFIG_ENABLE_ICD_SERVER @@ -52,9 +59,81 @@ Status EventPathValid(DataModel::Provider * model, const ConcreteEventPath & eve return Status::Success; } -DataModel::ActionReturnStatus RetrieveClusterData(DataModel::Provider * dataModel, - const Access::SubjectDescriptor & subjectDescriptor, bool isFabricFiltered, - AttributeReportIBs::Builder & reportBuilder, +/// Returns the status of ACL validation. +/// If the return value has a status set, that means the ACL check failed, +/// the read must not be performed, and the returned status (which may +/// be success, when dealing with non-concrete paths) should be used +/// as the status for the read. +/// +/// If the returned value is std::nullopt, that means the ACL check passed and the +/// read should proceed. +std::optional ValidateReadAttributeACL(DataModel::Provider * dataModel, const SubjectDescriptor & subjectDescriptor, + const ConcreteReadAttributePath & path) +{ + + RequestPath requestPath{ .cluster = path.mClusterId, + .endpoint = path.mEndpointId, + .requestType = RequestType::kAttributeReadRequest, + .entityId = path.mAttributeId }; + + std::optional info = dataModel->GetAttributeInfo(path); + + // If the attribute exists, we know whether it is readable (readPrivilege has value) + // and what the required access privilege is. However for attributes missing from the metatada + // (e.g. global attributes) or completely missing attributes we do not actually know of a required + // privilege and default to kView (this is correct for global attributes and a reasonable check + // for others) + Privilege requiredPrivilege = Privilege::kView; + if (info.has_value() && info->readPrivilege.has_value()) + { + // attribute exists and is readable, set the correct read privilege + requiredPrivilege = *info->readPrivilege; + } + + CHIP_ERROR err = GetAccessControl().Check(subjectDescriptor, requestPath, requiredPrivilege); + if (err == CHIP_NO_ERROR) + { + if (IsSupportedGlobalAttributeNotInMetadata(path.mAttributeId)) + { + // Global attributes passing a kView check is ok + return std::nullopt; + } + + // We want to return "success" (i.e. nulopt) IF AND ONLY IF the attribute exists and is readable (has read privilege). + // Since the Access control check above may have passed with kView, we do another check here: + // - Attribute exists (info has value) + // - Attribute is readable (readProvilege has value) and not "write only" + // If the attribute exists and is not readable, we will return UnsupportedRead (spec 8.4.3.2: "Else if the path indicates + // attribute data that is not readable, an AttributeStatusIB SHALL be generated with the UNSUPPORTED_READ Status Code.") + // + // TODO:: https://github.com/CHIP-Specifications/connectedhomeip-spec/pull/9024 requires interleaved ordering that + // is NOT implemented here. Spec requires: + // - check cluster access check (done here as kView at least) + // - unsupported endpoint/cluster/attribute check (NOT done here) when the attribute is missing. + // this SHOULD be done here when info does not have a value. This was not done as a first pass to + // minimize amount of delta in the initial PR. + // - "write-only" attributes should return UNSUPPORTED_READ (this is done here) + if (info.has_value() && !info->readPrivilege.has_value()) + { + return CHIP_IM_GLOBAL_STATUS(UnsupportedRead); + } + + return std::nullopt; + } + VerifyOrReturnError((err == CHIP_ERROR_ACCESS_DENIED) || (err == CHIP_ERROR_ACCESS_RESTRICTED_BY_ARL), err); + + // Implementation of 8.4.3.2 of the spec for path expansion + if (path.mExpanded) + { + return CHIP_NO_ERROR; + } + + // access denied and access restricted have specific codes for IM + return err == CHIP_ERROR_ACCESS_DENIED ? CHIP_IM_GLOBAL_STATUS(UnsupportedAccess) : CHIP_IM_GLOBAL_STATUS(AccessRestricted); +} + +DataModel::ActionReturnStatus RetrieveClusterData(DataModel::Provider * dataModel, const SubjectDescriptor & subjectDescriptor, + bool isFabricFiltered, AttributeReportIBs::Builder & reportBuilder, const ConcreteReadAttributePath & path, AttributeEncodeState * encoderState) { ChipLogDetail(DataManagement, " Cluster %" PRIx32 ", Attribute %" PRIx32 " is dirty", path.mClusterId, @@ -64,10 +143,7 @@ DataModel::ActionReturnStatus RetrieveClusterData(DataModel::Provider * dataMode DataModel::ReadAttributeRequest readRequest; - if (isFabricFiltered) - { - readRequest.readFlags.Set(DataModel::ReadFlags::kFabricFiltered); - } + readRequest.readFlags.Set(DataModel::ReadFlags::kFabricFiltered, isFabricFiltered); readRequest.subjectDescriptor = &subjectDescriptor; readRequest.path = path; @@ -84,9 +160,17 @@ DataModel::ActionReturnStatus RetrieveClusterData(DataModel::Provider * dataMode TLV::TLVWriter checkpoint; reportBuilder.Checkpoint(checkpoint); + DataModel::ActionReturnStatus status(CHIP_NO_ERROR); AttributeValueEncoder attributeValueEncoder(reportBuilder, subjectDescriptor, path, version, isFabricFiltered, encoderState); - DataModel::ActionReturnStatus status = dataModel->ReadAttribute(readRequest, attributeValueEncoder); + if (auto access_status = ValidateReadAttributeACL(dataModel, subjectDescriptor, path); access_status.has_value()) + { + status = *access_status; + } + else + { + status = dataModel->ReadAttribute(readRequest, attributeValueEncoder); + } if (status.IsSuccess()) { @@ -435,13 +519,13 @@ CHIP_ERROR Engine::CheckAccessDeniedEventPaths(TLV::TLVWriter & aWriter, bool & aHasEncodedData = true; } - Access::RequestPath requestPath{ .cluster = current->mValue.mClusterId, - .endpoint = current->mValue.mEndpointId, - .requestType = RequestType::kEventReadRequest, - .entityId = current->mValue.mEventId }; - Access::Privilege requestPrivilege = RequiredPrivilege::ForReadEvent(path); + RequestPath requestPath{ .cluster = current->mValue.mClusterId, + .endpoint = current->mValue.mEndpointId, + .requestType = RequestType::kEventReadRequest, + .entityId = current->mValue.mEventId }; + Privilege requestPrivilege = RequiredPrivilege::ForReadEvent(path); - err = Access::GetAccessControl().Check(apReadHandler->GetSubjectDescriptor(), requestPath, requestPrivilege); + err = GetAccessControl().Check(apReadHandler->GetSubjectDescriptor(), requestPath, requestPrivilege); if ((err != CHIP_ERROR_ACCESS_DENIED) && (err != CHIP_ERROR_ACCESS_RESTRICTED_BY_ARL)) { ReturnErrorOnFailure(err); @@ -580,13 +664,13 @@ CHIP_ERROR Engine::BuildSingleReportDataEventReports(ReportDataMessage::Builder CHIP_ERROR Engine::BuildAndSendSingleReportData(ReadHandler * apReadHandler) { CHIP_ERROR err = CHIP_NO_ERROR; - chip::System::PacketBufferTLVWriter reportDataWriter; + System::PacketBufferTLVWriter reportDataWriter; ReportDataMessage::Builder reportDataBuilder; - chip::System::PacketBufferHandle bufHandle = nullptr; - uint16_t reservedSize = 0; - bool hasMoreChunks = false; - bool needCloseReadHandler = false; - size_t reportBufferMaxSize = 0; + System::PacketBufferHandle bufHandle = nullptr; + uint16_t reservedSize = 0; + bool hasMoreChunks = false; + bool needCloseReadHandler = false; + size_t reportBufferMaxSize = 0; // Reserved size for the MoreChunks boolean flag, which takes up 1 byte for the control tag and 1 byte for the context tag. const uint32_t kReservedSizeForMoreChunksFlag = 1 + 1; @@ -623,7 +707,7 @@ CHIP_ERROR Engine::BuildAndSendSingleReportData(ReadHandler * apReadHandler) // Always limit the size of the generated packet to fit within the max size returned by the ReadHandler regardless // of the available buffer capacity. // Also, we need to reserve some extra space for the MIC field. - reportDataWriter.ReserveBuffer(static_cast(reservedSize + chip::Crypto::CHIP_CRYPTO_AEAD_MIC_LENGTH_BYTES)); + reportDataWriter.ReserveBuffer(static_cast(reservedSize + Crypto::CHIP_CRYPTO_AEAD_MIC_LENGTH_BYTES)); // Create a report data. err = reportDataBuilder.Init(&reportDataWriter);