Enforce length constraints in access control (#17817)

mlepage-google · pull[bot] · commit 1133511e93aa · 2023-10-23T23:31:32.000Z
Add some APIs for length constraints. Use them in system module and in
cluster to enforce length constraints.
diff --git a/src/access/AccessControl.cpp b/src/access/AccessControl.cpp
@@ -193,6 +193,64 @@ CHIP_ERROR AccessControl::Finish()
     return retval;
 }
 
+CHIP_ERROR AccessControl::CreateEntry(const SubjectDescriptor * subjectDescriptor, FabricIndex fabric, size_t * index,
+                                      const Entry & entry)
+{
+    VerifyOrReturnError(IsInitialized(), CHIP_ERROR_INCORRECT_STATE);
+
+    size_t count    = 0;
+    size_t maxCount = 0;
+    ReturnErrorOnFailure(mDelegate->GetEntryCount(fabric, count));
+    ReturnErrorOnFailure(mDelegate->GetMaxEntriesPerFabric(maxCount));
+
+    VerifyOrReturnError((count + 1) <= maxCount, CHIP_ERROR_BUFFER_TOO_SMALL);
+
+    ReturnErrorCodeIf(!IsValid(entry), CHIP_ERROR_INVALID_ARGUMENT);
+
+    size_t i = 0;
+    ReturnErrorOnFailure(mDelegate->CreateEntry(&i, entry, &fabric));
+
+    if (index)
+    {
+        *index = i;
+    }
+
+    NotifyEntryChanged(subjectDescriptor, fabric, i, &entry, EntryListener::ChangeType::kAdded);
+    return CHIP_NO_ERROR;
+}
+
+CHIP_ERROR AccessControl::UpdateEntry(const SubjectDescriptor * subjectDescriptor, FabricIndex fabric, size_t index,
+                                      const Entry & entry)
+{
+    VerifyOrReturnError(IsInitialized(), CHIP_ERROR_INCORRECT_STATE);
+    ReturnErrorCodeIf(!IsValid(entry), CHIP_ERROR_INVALID_ARGUMENT);
+    ReturnErrorOnFailure(mDelegate->UpdateEntry(index, entry, &fabric));
+    NotifyEntryChanged(subjectDescriptor, fabric, index, &entry, EntryListener::ChangeType::kUpdated);
+    return CHIP_NO_ERROR;
+}
+
+CHIP_ERROR AccessControl::DeleteEntry(const SubjectDescriptor * subjectDescriptor, FabricIndex fabric, size_t index)
+{
+    VerifyOrReturnError(IsInitialized(), CHIP_ERROR_INCORRECT_STATE);
+    Entry entry;
+    Entry * p = nullptr;
+    if (mEntryListener != nullptr && ReadEntry(fabric, index, entry) == CHIP_NO_ERROR)
+    {
+        p = &entry;
+    }
+    ReturnErrorOnFailure(mDelegate->DeleteEntry(index, &fabric));
+    if (p && p->HasDefaultDelegate())
+    {
+        // The entry was read prior to deletion so its latest value could be provided
+        // to the listener after deletion. If it's been reset to its default delegate,
+        // that best effort attempt to retain the latest value failed. This is
+        // regrettable but OK.
+        p = nullptr;
+    }
+    NotifyEntryChanged(subjectDescriptor, fabric, index, p, EntryListener::ChangeType::kRemoved);
+    return CHIP_NO_ERROR;
+}
+
 void AccessControl::AddEntryListener(EntryListener & listener)
 {
     if (mEntryListener == nullptr)
diff --git a/src/access/AccessControl.h b/src/access/AccessControl.h
@@ -345,13 +345,29 @@ class AccessControl
         virtual CHIP_ERROR Finish() { return CHIP_NO_ERROR; }
 
         // Capabilities
-        virtual CHIP_ERROR GetMaxEntryCount(size_t & value) const
+        virtual CHIP_ERROR GetMaxEntriesPerFabric(size_t & value) const
+        {
+            value = 0;
+            return CHIP_NO_ERROR;
+        }
+
+        virtual CHIP_ERROR GetMaxSubjectsPerEntry(size_t & value) const
         {
             value = 0;
             return CHIP_NO_ERROR;
         }
 
-        // TODO: add more capabilities
+        virtual CHIP_ERROR GetMaxTargetsPerEntry(size_t & value) const
+        {
+            value = 0;
+            return CHIP_NO_ERROR;
+        }
+
+        virtual CHIP_ERROR GetMaxEntryCount(size_t & value) const
+        {
+            value = 0;
+            return CHIP_NO_ERROR;
+        }
 
         // Actualities
         virtual CHIP_ERROR GetEntryCount(FabricIndex fabric, size_t & value) const
@@ -417,6 +433,24 @@ class AccessControl
     CHIP_ERROR Finish();
 
     // Capabilities
+    CHIP_ERROR GetMaxEntriesPerFabric(size_t & value) const
+    {
+        VerifyOrReturnError(IsInitialized(), CHIP_ERROR_INCORRECT_STATE);
+        return mDelegate->GetMaxEntriesPerFabric(value);
+    }
+
+    CHIP_ERROR GetMaxSubjectsPerEntry(size_t & value) const
+    {
+        VerifyOrReturnError(IsInitialized(), CHIP_ERROR_INCORRECT_STATE);
+        return mDelegate->GetMaxSubjectsPerEntry(value);
+    }
+
+    CHIP_ERROR GetMaxTargetsPerEntry(size_t & value) const
+    {
+        VerifyOrReturnError(IsInitialized(), CHIP_ERROR_INCORRECT_STATE);
+        return mDelegate->GetMaxTargetsPerEntry(value);
+    }
+
     CHIP_ERROR GetMaxEntryCount(size_t & value) const
     {
         VerifyOrReturnError(IsInitialized(), CHIP_ERROR_INCORRECT_STATE);
@@ -457,19 +491,7 @@ class AccessControl
      * @param [out] index             (If not nullptr) index of created entry (relative to fabric).
      * @param [in]  entry             Entry from which created entry is copied.
      */
-    CHIP_ERROR CreateEntry(const SubjectDescriptor * subjectDescriptor, FabricIndex fabric, size_t * index, const Entry & entry)
-    {
-        VerifyOrReturnError(IsInitialized(), CHIP_ERROR_INCORRECT_STATE);
-        ReturnErrorCodeIf(!IsValid(entry), CHIP_ERROR_INVALID_ARGUMENT);
-        size_t i;
-        ReturnErrorOnFailure(mDelegate->CreateEntry(&i, entry, &fabric));
-        if (index)
-        {
-            *index = i;
-        }
-        NotifyEntryChanged(subjectDescriptor, fabric, i, &entry, EntryListener::ChangeType::kAdded);
-        return CHIP_NO_ERROR;
-    }
+    CHIP_ERROR CreateEntry(const SubjectDescriptor * subjectDescriptor, FabricIndex fabric, size_t * index, const Entry & entry);
 
     /**
      * Creates an entry in the access control list.
@@ -519,14 +541,7 @@ class AccessControl
      * @param [in] index             Index of entry to update (relative to fabric).
      * @param [in] entry             Entry from which updated entry is copied.
      */
-    CHIP_ERROR UpdateEntry(const SubjectDescriptor * subjectDescriptor, FabricIndex fabric, size_t index, const Entry & entry)
-    {
-        VerifyOrReturnError(IsInitialized(), CHIP_ERROR_INCORRECT_STATE);
-        ReturnErrorCodeIf(!IsValid(entry), CHIP_ERROR_INVALID_ARGUMENT);
-        ReturnErrorOnFailure(mDelegate->UpdateEntry(index, entry, &fabric));
-        NotifyEntryChanged(subjectDescriptor, fabric, index, &entry, EntryListener::ChangeType::kUpdated);
-        return CHIP_NO_ERROR;
-    }
+    CHIP_ERROR UpdateEntry(const SubjectDescriptor * subjectDescriptor, FabricIndex fabric, size_t index, const Entry & entry);
 
     /**
      * Updates an entry in the access control list.
@@ -549,27 +564,7 @@ class AccessControl
      * @param [in] fabric            Index of fabric in which to delete entry.
      * @param [in] index             Index of entry to delete (relative to fabric).
      */
-    CHIP_ERROR DeleteEntry(const SubjectDescriptor * subjectDescriptor, FabricIndex fabric, size_t index)
-    {
-        VerifyOrReturnError(IsInitialized(), CHIP_ERROR_INCORRECT_STATE);
-        Entry entry;
-        Entry * p = nullptr;
-        if (mEntryListener != nullptr && ReadEntry(fabric, index, entry) == CHIP_NO_ERROR)
-        {
-            p = &entry;
-        }
-        ReturnErrorOnFailure(mDelegate->DeleteEntry(index, &fabric));
-        if (p && p->HasDefaultDelegate())
-        {
-            // The entry was read prior to deletion so its latest value could be provided
-            // to the listener after deletion. If it's been reset to its default delegate,
-            // that best effort attempt to retain the latest value failed. This is
-            // regretable but OK.
-            p = nullptr;
-        }
-        NotifyEntryChanged(subjectDescriptor, fabric, index, p, EntryListener::ChangeType::kRemoved);
-        return CHIP_NO_ERROR;
-    }
+    CHIP_ERROR DeleteEntry(const SubjectDescriptor * subjectDescriptor, FabricIndex fabric, size_t index);
 
     /**
      * Deletes an entry from the access control list.
diff --git a/src/access/examples/ExampleAccessControlDelegate.cpp b/src/access/examples/ExampleAccessControlDelegate.cpp
@@ -971,6 +971,24 @@ class AccessControlDelegate : public AccessControl::Delegate
         return CHIP_NO_ERROR;
     }
 
+    CHIP_ERROR GetMaxEntriesPerFabric(size_t & value) const override
+    {
+        value = EntryStorage::kEntriesPerFabric;
+        return CHIP_NO_ERROR;
+    }
+
+    CHIP_ERROR GetMaxSubjectsPerEntry(size_t & value) const override
+    {
+        value = EntryStorage::kMaxSubjects;
+        return CHIP_NO_ERROR;
+    }
+
+    CHIP_ERROR GetMaxTargetsPerEntry(size_t & value) const override
+    {
+        value = EntryStorage::kMaxTargets;
+        return CHIP_NO_ERROR;
+    }
+
     CHIP_ERROR GetMaxEntryCount(size_t & value) const override
     {
         value = ArraySize(EntryStorage::acl);
diff --git a/src/app/clusters/access-control-server/access-control-server.cpp b/src/app/clusters/access-control-server/access-control-server.cpp
@@ -104,20 +104,20 @@ CHIP_ERROR AccessControlAttribute::Read(const ConcreteReadAttributePath & aPath,
         return ReadAcl(aEncoder);
     case AccessControlCluster::Attributes::Extension::Id:
         return ReadExtension(aEncoder);
-    // TODO(#14455): use API to get actual capabilities
     case AccessControlCluster::Attributes::SubjectsPerAccessControlEntry::Id: {
-        uint16_t value = CHIP_CONFIG_EXAMPLE_ACCESS_CONTROL_MAX_SUBJECTS_PER_ENTRY;
-        return aEncoder.Encode(value);
+        size_t value = 0;
+        ReturnErrorOnFailure(GetAccessControl().GetMaxSubjectsPerEntry(value));
+        return aEncoder.Encode(static_cast<uint16_t>(value));
     }
-    // TODO(#14455): use API to get actual capabilities
     case AccessControlCluster::Attributes::TargetsPerAccessControlEntry::Id: {
-        uint16_t value = CHIP_CONFIG_EXAMPLE_ACCESS_CONTROL_MAX_TARGETS_PER_ENTRY;
-        return aEncoder.Encode(value);
+        size_t value = 0;
+        ReturnErrorOnFailure(GetAccessControl().GetMaxTargetsPerEntry(value));
+        return aEncoder.Encode(static_cast<uint16_t>(value));
     }
-    // TODO(#14455): use API to get actual capabilities
     case AccessControlCluster::Attributes::AccessControlEntriesPerFabric::Id: {
-        uint16_t value = CHIP_CONFIG_EXAMPLE_ACCESS_CONTROL_MAX_ENTRIES_PER_FABRIC;
-        return aEncoder.Encode(value);
+        size_t value = 0;
+        ReturnErrorOnFailure(GetAccessControl().GetMaxEntriesPerFabric(value));
+        return aEncoder.Encode(static_cast<uint16_t>(value));
     }
     case AccessControlCluster::Attributes::ClusterRevision::Id:
         return aEncoder.Encode(kClusterRevision);
@@ -193,23 +193,20 @@ CHIP_ERROR AccessControlAttribute::WriteAcl(const ConcreteDataAttributePath & aP
 {
     FabricIndex accessingFabricIndex = aDecoder.AccessingFabricIndex();
 
+    size_t oldCount;
+    ReturnErrorOnFailure(GetAccessControl().GetEntryCount(accessingFabricIndex, oldCount));
+    size_t maxCount;
+    ReturnErrorOnFailure(GetAccessControl().GetMaxEntriesPerFabric(maxCount));
+
     if (!aPath.IsListItemOperation())
     {
         DataModel::DecodableList<AclStorage::DecodableEntry> list;
         ReturnErrorOnFailure(aDecoder.Decode(list));
 
-        size_t allCount;
-        size_t oldCount;
         size_t newCount;
-        size_t maxCount;
-
-        ReturnErrorOnFailure(GetAccessControl().GetEntryCount(allCount));
-        ReturnErrorOnFailure(GetAccessControl().GetEntryCount(accessingFabricIndex, oldCount));
         ReturnErrorOnFailure(list.ComputeSize(&newCount));
-        ReturnErrorOnFailure(GetAccessControl().GetMaxEntryCount(maxCount));
-        VerifyOrReturnError(allCount >= oldCount, CHIP_ERROR_INTERNAL);
-        VerifyOrReturnError(static_cast<size_t>(allCount - oldCount + newCount) <= maxCount,
-                            CHIP_IM_GLOBAL_STATUS(ConstraintError));
+
+        VerifyOrReturnError(newCount <= maxCount, CHIP_IM_GLOBAL_STATUS(ResourceExhausted));
 
         auto iterator = list.begin();
         size_t i      = 0;
@@ -237,6 +234,8 @@ CHIP_ERROR AccessControlAttribute::WriteAcl(const ConcreteDataAttributePath & aP
     }
     else if (aPath.mListOp == ConcreteDataAttributePath::ListOperation::AppendItem)
     {
+        VerifyOrReturnError((oldCount + 1) <= maxCount, CHIP_IM_GLOBAL_STATUS(ResourceExhausted));
+
         AclStorage::DecodableEntry decodableEntry;
         ReturnErrorOnFailure(aDecoder.Decode(decodableEntry));
 
diff --git a/src/app/tests/suites/TestAccessControlCluster.yaml b/src/app/tests/suites/TestAccessControlCluster.yaml
@@ -423,6 +423,107 @@ tests:
                   },
               ]
 
+    - label: "Write too many entries"
+      command: "writeAttribute"
+      attribute: "ACL"
+      arguments:
+          value: [
+                  {
+                      FabricIndex: 0,
+                      Privilege: 5, # administer
+                      AuthMode: 2, # case
+                      Subjects: null,
+                      Targets:
+                          [
+                              { Cluster: null, Endpoint: 0, DeviceType: null },
+                              { Cluster: 1, Endpoint: null, DeviceType: null },
+                              { Cluster: 2, Endpoint: 3, DeviceType: null },
+                          ],
+                  },
+                  {
+                      FabricIndex: 0,
+                      Privilege: 1, # view
+                      AuthMode: 2, # case
+                      Subjects: [4, 5, 6, 7],
+                      Targets:
+                          [
+                              { Cluster: null, Endpoint: 8, DeviceType: null },
+                              { Cluster: 9, Endpoint: null, DeviceType: null },
+                              { Cluster: 10, Endpoint: 11, DeviceType: null },
+                          ],
+                  },
+                  {
+                      FabricIndex: 0,
+                      Privilege: 3, # operate
+                      AuthMode: 3, # group
+                      Subjects: [12, 13, 14, 15],
+                      Targets:
+                          [
+                              { Cluster: null, Endpoint: 16, DeviceType: null },
+                              { Cluster: 17, Endpoint: null, DeviceType: null },
+                              { Cluster: 18, Endpoint: 19, DeviceType: null },
+                          ],
+                  },
+                  {
+                      FabricIndex: 0,
+                      Privilege: 1, # view
+                      AuthMode: 2, # case
+                      Subjects: [20, 21, 22, 23],
+                      Targets:
+                          [
+                              { Cluster: null, Endpoint: 24, DeviceType: null },
+                              { Cluster: 25, Endpoint: null, DeviceType: null },
+                              { Cluster: 26, Endpoint: 27, DeviceType: null },
+                          ],
+                  },
+              ]
+      response:
+          error: RESOURCE_EXHAUSTED
+
+    - label: "Verify"
+      command: "readAttribute"
+      attribute: "ACL"
+      response:
+          value: [
+                  {
+                      FabricIndex: 1,
+                      Privilege: 5, # administer
+                      AuthMode: 2, # case
+                      Subjects: null,
+                      Targets:
+                          [
+                              { Cluster: null, Endpoint: 0, DeviceType: null },
+                              { Cluster: 1, Endpoint: null, DeviceType: null },
+                              { Cluster: 2, Endpoint: 3, DeviceType: null },
+                          ],
+                  },
+                  {
+                      FabricIndex: 1,
+                      Privilege: 1, # view
+                      AuthMode: 2, # case
+                      Subjects: [4, 5, 6, 7],
+                      Targets:
+                          [
+                              { Cluster: null, Endpoint: 8, DeviceType: null },
+                              { Cluster: 9, Endpoint: null, DeviceType: null },
+                              { Cluster: 10, Endpoint: 11, DeviceType: null },
+                          ],
+                  },
+                  {
+                      FabricIndex: 1,
+                      Privilege: 3, # operate
+                      AuthMode: 3, # group
+                      Subjects: [12, 13, 14, 15],
+                      Targets:
+                          [
+                              { Cluster: null, Endpoint: 16, DeviceType: null },
+                              { Cluster: 17, Endpoint: null, DeviceType: null },
+                              { Cluster: 18, Endpoint: 19, DeviceType: null },
+                          ],
+                  },
+              ]
+
+    # note missing last entry
     - label: "Restore ACL"
       command: "writeAttribute"
       attribute: "ACL"
diff --git a/zzz_generated/chip-tool-darwin/zap-generated/test/Commands.h b/zzz_generated/chip-tool-darwin/zap-generated/test/Commands.h
diff --git a/zzz_generated/chip-tool/zap-generated/test/Commands.h b/zzz_generated/chip-tool/zap-generated/test/Commands.h
