[ACE] perform initial ACL checks before existence checks to avoid leaking non-existence (project-chip#39415)

* CommandHandler: returning AccessFailures before Existence Failures

* WriteHandler: returning ACL check failures before Attribute existence failures

* Adapting TC_ACE_2.2 for WriteHandler Changes

* Adapting changes to new spec

* removing TODO comment

* Adding Unit Tests

* minor fixes and gemini comments

* Unit Tests fix

* Integrating Comments

Author: Alami-Amine

src/app/CommandHandlerImpl.cpp

@@ -536,6 +536,11 @@ Status CommandHandlerImpl::ProcessGroupCommandDataIB(CommandDataIB::Parser & aCo
             request.subjectDescriptor = &subjectDescriptor;
             request.invokeFlags.Set(DataModel::InvokeFlags::kTimed, IsTimedInvoke());
 
+            // SPEC-DIVERGENCE: The spec mandates only one ACL check after the existence check for non-concrete paths (Group
+            // Commands). However, calling ValidateCommandCanBeDispatched here introduces an additional ACL check before the
+            // existence check, because that function also performs an early access check (it is shared with the concrete path
+            // case). This results in two ACL checks for group commands. In practice, this divergence is not observable if all
+            // commands require at least Operate privilege.
             Status preCheckStatus = mpCallback->ValidateCommandCanBeDispatched(request);
             if (preCheckStatus != Status::Success)
             {

src/app/InteractionModelEngine.cpp

@@ -1796,7 +1796,16 @@ Protocols::InteractionModel::Status InteractionModelEngine::ValidateCommandCanBe
 
     DataModel::AcceptedCommandEntry acceptedCommandEntry;
 
-    Status status = CheckCommandExistence(request.path, acceptedCommandEntry);
+    // Execute the ACL Access Granting Algorithm before existence checks, assuming the required_privilege for the element is
+    // Operate, to determine if the subject would have had at least some access against the concrete path. This is done so we don't
+    // leak information if we do fail existence checks.
+    // SPEC-DIVERGENCE: For non-concrete paths (Group Commands), the spec mandates only one ACL check AFTER the existence check.
+    // However, because this code is also used in the group path case, we end up performing an ADDITIONAL ACL check before the
+    // existence check. In practice, this divergence is not observable if all commands require at least Operate privilege.
+    Status status = CheckCommandAccess(request, Access::Privilege::kOperate);
+    VerifyOrReturnValue(status == Status::Success, status);
+
+    status = CheckCommandExistence(request.path, acceptedCommandEntry);
 
     if (status != Status::Success)
     {
@@ -1805,14 +1814,14 @@ Protocols::InteractionModel::Status InteractionModelEngine::ValidateCommandCanBe
         return status;
     }
 
-    status = CheckCommandAccess(request, acceptedCommandEntry);
+    status = CheckCommandAccess(request, acceptedCommandEntry.GetInvokePrivilege());
     VerifyOrReturnValue(status == Status::Success, status);
 
     return CheckCommandFlags(request, acceptedCommandEntry);
 }
 
 Protocols::InteractionModel::Status InteractionModelEngine::CheckCommandAccess(const DataModel::InvokeRequest & aRequest,
-                                                                               const DataModel::AcceptedCommandEntry & entry)
+                                                                               const Access::Privilege aRequiredPrivilege)
 {
     if (aRequest.subjectDescriptor == nullptr)
     {
@@ -1824,7 +1833,7 @@ Protocols::InteractionModel::Status InteractionModelEngine::CheckCommandAccess(c
                                      .requestType = Access::RequestType::kCommandInvokeRequest,
                                      .entityId    = aRequest.path.mCommandId };
 
-    CHIP_ERROR err = Access::GetAccessControl().Check(*aRequest.subjectDescriptor, requestPath, entry.GetInvokePrivilege());
+    CHIP_ERROR err = Access::GetAccessControl().Check(*aRequest.subjectDescriptor, requestPath, aRequiredPrivilege);
     if (err != CHIP_NO_ERROR)
     {
         if ((err != CHIP_ERROR_ACCESS_DENIED) && (err != CHIP_ERROR_ACCESS_RESTRICTED_BY_ARL))

src/app/InteractionModelEngine.h

@@ -630,7 +630,7 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler,
      * Validates that the command exists and on success returns the data for the command in `entry`.
      */
     Status CheckCommandExistence(const ConcreteCommandPath & aCommandPath, DataModel::AcceptedCommandEntry & entry);
-    Status CheckCommandAccess(const DataModel::InvokeRequest & aRequest, const DataModel::AcceptedCommandEntry & entry);
+    Status CheckCommandAccess(const DataModel::InvokeRequest & aRequest, const Access::Privilege aRequiredPrivilege);
     Status CheckCommandFlags(const DataModel::InvokeRequest & aRequest, const DataModel::AcceptedCommandEntry & entry);
 
     /**

src/app/WriteHandler.cpp

@@ -764,14 +764,15 @@ void WriteHandler::MoveToState(const State aTargetState)
 DataModel::ActionReturnStatus WriteHandler::CheckWriteAllowed(const Access::SubjectDescriptor & aSubject,
                                                               const ConcreteAttributePath & aPath)
 {
-    // TODO: ordering is to check writability/existence BEFORE ACL and this seems wrong, however
-    //       existing unit tests (TC_AcessChecker.py) validate that we get UnsupportedWrite instead of UnsupportedAccess
-    //
-    //       This should likely be fixed in spec (probably already fixed by
-    //       https://github.com/CHIP-Specifications/connectedhomeip-spec/pull/9024)
-    //       and tests and implementation
-    //
-    //       Open issue that needs fixing: https://github.com/project-chip/connectedhomeip/issues/33735
+
+    // Execute the ACL Access Granting Algorithm before existence checks, assuming the required_privilege for the element is
+    // View, to determine if the subject would have had at least some access against the concrete path. This is done so we don't
+    // leak information if we do fail existence checks.
+    // SPEC-DIVERGENCE: For non-concrete paths, the spec mandates only one ACL check AFTER the existence check.
+    // However, because this code is also used in the group path case, we end up performing an ADDITIONAL ACL check before the
+    // existence check. In practice, this divergence is not observable.
+    Status writeAccessStatus = CheckWriteAccess(aSubject, aPath, Access::Privilege::kView);
+    VerifyOrReturnValue(writeAccessStatus == Status::Success, writeAccessStatus);
 
     DataModel::AttributeFinder finder(mDataModelProvider);
 
@@ -790,6 +791,21 @@ DataModel::ActionReturnStatus WriteHandler::CheckWriteAllowed(const Access::Subj
     // Allow writes on writable attributes only
     VerifyOrReturnValue(attributeEntry->GetWritePrivilege().has_value(), Status::UnsupportedWrite);
 
+    // Execute the ACL Access Granting Algorithm against the concrete path a second time, using the actual required_privilege
+    writeAccessStatus = CheckWriteAccess(aSubject, aPath, *attributeEntry->GetWritePrivilege());
+    VerifyOrReturnValue(writeAccessStatus == Status::Success, writeAccessStatus);
+
+    // validate that timed write is enforced
+    VerifyOrReturnValue(IsTimedWrite() || !attributeEntry->HasFlags(DataModel::AttributeQualityFlags::kTimed),
+                        Status::NeedsTimedInteraction);
+
+    return Status::Success;
+}
+
+Status WriteHandler::CheckWriteAccess(const Access::SubjectDescriptor & aSubject, const ConcreteAttributePath & aPath,
+                                      const Access::Privilege aRequiredPrivilege)
+{
+
     bool checkAcl = true;
     if (mLastSuccessfullyWrittenPath.has_value())
     {
@@ -812,22 +828,25 @@ DataModel::ActionReturnStatus WriteHandler::CheckWriteAllowed(const Access::Subj
                                          .requestType = Access::RequestType::kAttributeWriteRequest,
                                          .entityId    = aPath.mAttributeId };
 
-        // NOTE: we know that attributeEntry has a GetWriteProvilege based on the check above.
-        //       so we just directly reference it.
-        CHIP_ERROR err = Access::GetAccessControl().Check(aSubject, requestPath, *attributeEntry->GetWritePrivilege());
+        CHIP_ERROR err = Access::GetAccessControl().Check(aSubject, requestPath, aRequiredPrivilege);
 
-        if (err != CHIP_NO_ERROR)
+        if (err == CHIP_NO_ERROR)
         {
-            VerifyOrReturnValue(err != CHIP_ERROR_ACCESS_DENIED, Status::UnsupportedAccess);
-            VerifyOrReturnValue(err != CHIP_ERROR_ACCESS_RESTRICTED_BY_ARL, Status::AccessRestricted);
+            return Status::Success;
+        }
 
-            return err;
+        if (err == CHIP_ERROR_ACCESS_DENIED)
+        {
+            return Status::UnsupportedAccess;
         }
-    }
 
-    // validate that timed write is enforced
-    VerifyOrReturnValue(IsTimedWrite() || !attributeEntry->HasFlags(DataModel::AttributeQualityFlags::kTimed),
-                        Status::NeedsTimedInteraction);
+        if (err == CHIP_ERROR_ACCESS_RESTRICTED_BY_ARL)
+        {
+            return Status::AccessRestricted;
+        }
+
+        return Status::Failure;
+    }
 
     return Status::Success;
 }

src/app/WriteHandler.h

@@ -194,6 +194,12 @@ class WriteHandler : public Messaging::ExchangeDelegate
     DataModel::ActionReturnStatus CheckWriteAllowed(const Access::SubjectDescriptor & aSubject,
                                                     const ConcreteAttributePath & aPath);
 
+    /// Validate that a write on the given path has aRequiredPrivilege.
+    ///
+    /// Returns a success status if the ACL check is successful, otherwise the relevant status will be returned.
+    Status CheckWriteAccess(const Access::SubjectDescriptor & aSubject, const ConcreteAttributePath & aPath,
+                            const Access::Privilege aRequiredPrivilege);
+
     // Write the given data to the given path
     CHIP_ERROR WriteClusterData(const Access::SubjectDescriptor & aSubject, const ConcreteDataAttributePath & aPath,
                                 TLV::TLVReader & aData);

src/app/tests/BUILD.gn

@@ -249,6 +249,7 @@ chip_test_suite("tests") {
 
   test_sources = [
     "TestAclAttribute.cpp",
+    "TestAclCommand.cpp",
     "TestAclEvent.cpp",
     "TestActionsCluster.cpp",
     "TestAttributeAccessInterfaceCache.cpp",