Do proper ACL checks on event reads/subscriptions. (#26761)

* Do proper ACL checks on event reads/subscriptions.

This adds the following functionality:

1. We now correctly detect subsciptions that don't have any access to anything,
   even if they have an event path in the subscribe request.  For paths with a
   wildcard event id, this check assumes read privileges are needed when event
   lists are disabled, and uses the actual event-specific privileges when event
   lists are enabled.
2. When doing reads of an unsupported event, correctly return an
   errors instead of an empty event list.
3. Fix various unit test mocks to provide the information needed for the new
   checks.
4. Update expectation in existing YAML test that was checking an "unimplemented
   event" case.

* Address review comments.

* Fix darwin build.

* Fix Darwin tests, now that we get errors for unsupported events.

* Move function declarations to a non-codegen-dependent header.

* Handle ACL checks for event wildcards even if we have no EventList.

* Update to spec change for unsupported event errors.

* Address review comments.

Author: bzbarsky-apple

src/app/InteractionModelEngine.cpp

@@ -30,11 +30,11 @@
 #include "access/RequestPath.h"
 #include "access/SubjectDescriptor.h"
 #include <app/RequiredPrivilege.h>
+#include <app/util/af-types.h>
+#include <app/util/endpoint-config-api.h>
 #include <lib/core/TLVUtilities.h>
 #include <lib/support/CodeUtils.h>
 
-extern bool emberAfContainsAttribute(chip::EndpointId endpoint, chip::ClusterId clusterId, chip::AttributeId attributeId);
-
 namespace chip {
 namespace app {
 
@@ -351,10 +351,8 @@ CHIP_ERROR InteractionModelEngine::ParseAttributePaths(const Access::SubjectDesc
     aHasValidAttributePath       = false;
     aRequestedAttributePathCount = 0;
 
-    while (CHIP_NO_ERROR == (err = pathReader.Next()))
+    while (CHIP_NO_ERROR == (err = pathReader.Next(TLV::AnonymousTag())))
     {
-        VerifyOrReturnError(TLV::AnonymousTag() == pathReader.GetTag(), CHIP_ERROR_INVALID_TLV_TAG);
-
         AttributePathIB::Parser path;
         //
         // We create an iterator to point to a single item object list that tracks the path we just parsed.
@@ -413,6 +411,152 @@ CHIP_ERROR InteractionModelEngine::ParseAttributePaths(const Access::SubjectDesc
     return err;
 }
 
+static bool CanAccess(const Access::SubjectDescriptor & aSubjectDescriptor, const ConcreteClusterPath & aPath,
+                      Access::Privilege aNeededPrivilege)
+{
+    Access::RequestPath requestPath{ .cluster = aPath.mClusterId, .endpoint = aPath.mEndpointId };
+    CHIP_ERROR err = Access::GetAccessControl().Check(aSubjectDescriptor, requestPath, aNeededPrivilege);
+    return (err == CHIP_NO_ERROR);
+}
+
+static bool CanAccess(const Access::SubjectDescriptor & aSubjectDescriptor, const ConcreteEventPath & aPath)
+{
+    return CanAccess(aSubjectDescriptor, aPath, RequiredPrivilege::ForReadEvent(aPath));
+}
+
+/**
+ * Helper to handle wildcard events in the event path.
+ */
+static bool HasValidEventPathForEndpointAndCluster(EndpointId aEndpoint, const EmberAfCluster * aCluster,
+                                                   const EventPathParams & aEventPath,
+                                                   const Access::SubjectDescriptor & aSubjectDescriptor)
+{
+    if (aEventPath.HasWildcardEventId())
+    {
+#if CHIP_CONFIG_ENABLE_EVENTLIST_ATTRIBUTE
+        for (decltype(aCluster->eventCount) idx = 0; idx > aCluster->eventCount; ++idx)
+        {
+            ConcreteEventPath path(aEndpoint, aCluster->clusterId, aCluster->eventList[idx]);
+            // If we get here, the path exists.  We just have to do an ACL check for it.
+            bool isValid = CanAccess(aSubjectDescriptor, path);
+            if (isValid)
+            {
+                return true;
+            }
+        }
+
+        return false;
+#else
+        // We have no way to expand wildcards.  Just assume that we would need
+        // View permissions for whatever events are involved.
+        ConcreteClusterPath clusterPath(aEndpoint, aCluster->clusterId);
+        return CanAccess(aSubjectDescriptor, clusterPath, Access::Privilege::kView);
+#endif
+    }
+
+    ConcreteEventPath path(aEndpoint, aCluster->clusterId, aEventPath.mEventId);
+    if (CheckEventSupportStatus(path) != Status::Success)
+    {
+        // Not an existing event path.
+        return false;
+    }
+    return CanAccess(aSubjectDescriptor, path);
+}
+
+/**
+ * Helper to handle wildcard clusters in the event path.
+ */
+static bool HasValidEventPathForEndpoint(EndpointId aEndpoint, const EventPathParams & aEventPath,
+                                         const Access::SubjectDescriptor & aSubjectDescriptor)
+{
+    if (aEventPath.HasWildcardClusterId())
+    {
+        auto * endpointType = emberAfFindEndpointType(aEndpoint);
+        if (endpointType == nullptr)
+        {
+            // Not going to have any valid paths in here.
+            return false;
+        }
+
+        for (decltype(endpointType->clusterCount) idx = 0; idx < endpointType->clusterCount; ++idx)
+        {
+            bool hasValidPath =
+                HasValidEventPathForEndpointAndCluster(aEndpoint, &endpointType->cluster[idx], aEventPath, aSubjectDescriptor);
+            if (hasValidPath)
+            {
+                return true;
+            }
+        }
+
+        return false;
+    }
+
+    auto * cluster = emberAfFindServerCluster(aEndpoint, aEventPath.mClusterId);
+    if (cluster == nullptr)
+    {
+        // Nothing valid here.
+        return false;
+    }
+    return HasValidEventPathForEndpointAndCluster(aEndpoint, cluster, aEventPath, aSubjectDescriptor);
+}
+
+CHIP_ERROR InteractionModelEngine::ParseEventPaths(const Access::SubjectDescriptor & aSubjectDescriptor,
+                                                   EventPathIBs::Parser & aEventPathListParser, bool & aHasValidEventPath,
+                                                   size_t & aRequestedEventPathCount)
+{
+    TLV::TLVReader pathReader;
+    aEventPathListParser.GetReader(&pathReader);
+    CHIP_ERROR err = CHIP_NO_ERROR;
+
+    aHasValidEventPath       = false;
+    aRequestedEventPathCount = 0;
+
+    while (CHIP_NO_ERROR == (err = pathReader.Next(TLV::AnonymousTag())))
+    {
+        EventPathIB::Parser path;
+        ReturnErrorOnFailure(path.Init(pathReader));
+
+        EventPathParams eventPath;
+        ReturnErrorOnFailure(path.ParsePath(eventPath));
+
+        ++aRequestedEventPathCount;
+
+        if (aHasValidEventPath)
+        {
+            // Can skip all the rest of the checking.
+            continue;
+        }
+
+        // The definition of "valid path" is "path exists and ACL allows
+        // access".  We need to do some expansion of wildcards to handle that.
+        if (eventPath.HasWildcardEndpointId())
+        {
+            for (uint16_t endpointIndex = 0; !aHasValidEventPath && endpointIndex < emberAfEndpointCount(); ++endpointIndex)
+            {
+                if (!emberAfEndpointIndexIsEnabled(endpointIndex))
+                {
+                    continue;
+                }
+                aHasValidEventPath =
+                    HasValidEventPathForEndpoint(emberAfEndpointFromIndex(endpointIndex), eventPath, aSubjectDescriptor);
+            }
+        }
+        else
+        {
+            // No need to check whether the endpoint is enabled, because
+            // emberAfFindEndpointType returns null for disabled endpoints.
+            aHasValidEventPath = HasValidEventPathForEndpoint(eventPath.mEndpointId, eventPath, aSubjectDescriptor);
+        }
+    }
+
+    if (err == CHIP_ERROR_END_OF_TLV)
+    {
+        err = CHIP_NO_ERROR;
+    }
+
+    return err;
+}
+
 Protocols::InteractionModel::Status InteractionModelEngine::OnReadInitialRequest(Messaging::ExchangeContext * apExchangeContext,
                                                                                  const PayloadHeader & aPayloadHeader,
                                                                                  System::PacketBufferHandle && aPayload,
@@ -472,6 +616,7 @@ Protocols::InteractionModel::Status InteractionModelEngine::OnReadInitialRequest
             size_t requestedEventPathCount     = 0;
             AttributePathIBs::Parser attributePathListParser;
             bool hasValidAttributePath = false;
+            bool hasValidEventPath     = false;
 
             CHIP_ERROR err = subscribeRequestParser.GetAttributeRequests(&attributePathListParser);
             if (err == CHIP_NO_ERROR)
@@ -489,14 +634,16 @@ Protocols::InteractionModel::Status InteractionModelEngine::OnReadInitialRequest
                 return Status::InvalidAction;
             }
 
-            EventPathIBs::Parser eventpathListParser;
-            err = subscribeRequestParser.GetEventRequests(&eventpathListParser);
+            EventPathIBs::Parser eventPathListParser;
+            err = subscribeRequestParser.GetEventRequests(&eventPathListParser);
             if (err == CHIP_NO_ERROR)
             {
-                TLV::TLVReader pathReader;
-                eventpathListParser.GetReader(&pathReader);
-                ReturnErrorCodeIf(TLV::Utilities::Count(pathReader, requestedEventPathCount, false) != CHIP_NO_ERROR,
-                                  Status::InvalidAction);
+                auto subjectDescriptor = apExchangeContext->GetSessionHandle()->AsSecureSession()->GetSubjectDescriptor();
+                err = ParseEventPaths(subjectDescriptor, eventPathListParser, hasValidEventPath, requestedEventPathCount);
+                if (err != CHIP_NO_ERROR)
+                {
+                    return Status::InvalidAction;
+                }
             }
             else if (err != CHIP_ERROR_END_OF_TLV)
             {
@@ -512,12 +659,7 @@ Protocols::InteractionModel::Status InteractionModelEngine::OnReadInitialRequest
                 return Status::InvalidAction;
             }
 
-            //
-            // TODO: We don't have an easy way to do a similar 'path expansion' for events to deduce
-            // access so for now, assume that the presence of any path in the event list means they
-            // might have some access to those events.
-            //
-            if (!hasValidAttributePath && requestedEventPathCount == 0)
+            if (!hasValidAttributePath && !hasValidEventPath)
             {
                 ChipLogError(InteractionModel,
                              "Subscription from [%u:" ChipLogFormatX64 "] has no access at all. Rejecting request.",

src/app/InteractionModelEngine.h

@@ -46,6 +46,7 @@
 #include <app/CommandSender.h>
 #include <app/ConcreteAttributePath.h>
 #include <app/ConcreteCommandPath.h>
+#include <app/ConcreteEventPath.h>
 #include <app/DataVersionFilter.h>
 #include <app/EventPathParams.h>
 #include <app/ObjectList.h>
@@ -398,6 +399,19 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler,
                                           AttributePathIBs::Parser & aAttributePathListParser, bool & aHasValidAttributePath,
                                           size_t & aRequestedAttributePathCount);
 
+    /**
+     * This parses the event path list to ensure it is well formed. If so, for each path in the list, it will expand to a list
+     * of concrete paths and walk each path to check if it has privileges to read that event.
+     *
+     * If there is AT LEAST one "existent path" (as the spec calls it) that has sufficient privilege, aHasValidEventPath
+     * will be set to true. Otherwise, it will be set to false.
+     *
+     * aRequestedEventPathCount will be updated to reflect the number of event paths in the request.
+     */
+    static CHIP_ERROR ParseEventPaths(const Access::SubjectDescriptor & aSubjectDescriptor,
+                                      EventPathIBs::Parser & aEventPathListParser, bool & aHasValidEventPath,
+                                      size_t & aRequestedEventPathCount);
+
     /**
      * Called when Interaction Model receives a Read Request message.  Errors processing
      * the Read Request are handled entirely within this function.  If the
@@ -677,7 +691,12 @@ bool IsDeviceTypeOnEndpoint(DeviceTypeId deviceType, EndpointId endpoint);
  *
  * @retval The metadata of the attribute, will return null if the given attribute does not exists.
  */
-const EmberAfAttributeMetadata * GetAttributeMetadata(const ConcreteAttributePath & aConcreteClusterPath);
+const EmberAfAttributeMetadata * GetAttributeMetadata(const ConcreteAttributePath & aPath);
+
+/**
+ * Returns the event support status for the given event, as an interaction model status.
+ */
+Protocols::InteractionModel::Status CheckEventSupportStatus(const ConcreteEventPath & aPath);
 
 } // namespace app
 } // namespace chip

src/app/reporting/Engine.cpp

@@ -295,6 +295,8 @@ CHIP_ERROR Engine::BuildSingleReportDataAttributeReportIBs(ReportDataMessage::Bu
 
 CHIP_ERROR Engine::CheckAccessDeniedEventPaths(TLV::TLVWriter & aWriter, bool & aHasEncodedData, ReadHandler * apReadHandler)
 {
+    using Protocols::InteractionModel::Status;
+
     CHIP_ERROR err = CHIP_NO_ERROR;
     for (auto current = apReadHandler->mpEventPathList; current != nullptr;)
     {
@@ -304,8 +306,21 @@ CHIP_ERROR Engine::CheckAccessDeniedEventPaths(TLV::TLVWriter & aWriter, bool &
             continue;
         }
 
-        Access::RequestPath requestPath{ .cluster = current->mValue.mClusterId, .endpoint = current->mValue.mEndpointId };
         ConcreteEventPath path(current->mValue.mEndpointId, current->mValue.mClusterId, current->mValue.mEventId);
+        Status status = CheckEventSupportStatus(path);
+        if (status != Status::Success)
+        {
+            TLV::TLVWriter checkpoint = aWriter;
+            err                       = EventReportIB::ConstructEventStatusIB(aWriter, path, StatusIB(status));
+            if (err != CHIP_NO_ERROR)
+            {
+                aWriter = checkpoint;
+                break;
+            }
+            aHasEncodedData = true;
+        }
+
+        Access::RequestPath requestPath{ .cluster = current->mValue.mClusterId, .endpoint = current->mValue.mEndpointId };
         Access::Privilege requestPrivilege = RequiredPrivilege::ForReadEvent(path);
 
         err = Access::GetAccessControl().Check(apReadHandler->GetSubjectDescriptor(), requestPath, requestPrivilege);
@@ -316,8 +331,7 @@ CHIP_ERROR Engine::CheckAccessDeniedEventPaths(TLV::TLVWriter & aWriter, bool &
         else
         {
             TLV::TLVWriter checkpoint = aWriter;
-            err                       = EventReportIB::ConstructEventStatusIB(aWriter, path,
-                                                        StatusIB(Protocols::InteractionModel::Status::UnsupportedAccess));
+            err                       = EventReportIB::ConstructEventStatusIB(aWriter, path, StatusIB(Status::UnsupportedAccess));
             if (err != CHIP_NO_ERROR)
             {
                 aWriter = checkpoint;

src/app/tests/TestAclAttribute.cpp

@@ -19,6 +19,8 @@
 #include "lib/support/CHIPMem.h"
 #include <access/examples/PermissiveAccessControlDelegate.h>
 #include <app/AttributeAccessInterface.h>
+#include <app/ConcreteAttributePath.h>
+#include <app/ConcreteEventPath.h>
 #include <app/InteractionModelEngine.h>
 #include <app/MessageDef/AttributeReportIBs.h>
 #include <app/MessageDef/EventDataIB.h>
@@ -140,6 +142,16 @@ bool ConcreteAttributePathExists(const ConcreteAttributePath & aPath)
     return aPath.mClusterId != kTestDeniedClusterId1;
 }
 
+Protocols::InteractionModel::Status CheckEventSupportStatus(const ConcreteEventPath & aPath)
+{
+    if (aPath.mClusterId == kTestDeniedClusterId1)
+    {
+        return Protocols::InteractionModel::Status::UnsupportedCluster;
+    }
+
+    return Protocols::InteractionModel::Status::Success;
+}
+
 class TestAclAttribute
 {
 public: