Fix incoming command validity checks in Set Credential. (#25264)

* When Set Credential happens with OperationType set to Modify and UserIndex set to null, that means we are modifying the credential for the programming user. In this case, UserStatus must be null and UserType must be ProgrammingUser, but we were not checking that. * When Set Credential happens with OperationType set to Modify and UserIndex not null, UserStatus and UserType must both be null. We were incorrectly allowing UserType to be ProgrammingUser in this case. * When Set Credential happens with OperationType set to Add and UserIndex not null, UserStatus and UserType must both be null. We were not checking for this at all. Fixes #25259
project-chip · Sep 29, 2023 · 3113640 · 3113640
1 parent d28ace7
commit 3113640
Show file tree

Hide file tree

Showing 4 changed files with 1,118 additions and 504 deletions.
diff --git a/src/app/clusters/door-lock-server/door-lock-server.cpp b/src/app/clusters/door-lock-server/door-lock-server.cpp
@@ -700,6 +700,12 @@ void DoorLockServer::setCredentialCommandHandler(
         // if userIndex is NULL then we're changing the programming user PIN
         if (userIndex.IsNull())
         {
+            if (!userStatus.IsNull() || userType != UserTypeEnum::kProgrammingUser)
+            {
+                emberAfDoorLockClusterPrintln("[SetCredential] Unable to modify programming PIN: invalid argument "
+                                              "[endpointId=%d,credentialIndex=%d]",
+                                              commandPath.mEndpointId, credentialIndex);
+            }
             status = modifyProgrammingPIN(commandPath.mEndpointId, fabricIdx, sourceNodeId, credentialIndex, credentialType,
                                           existingCredential, credentialData);
             sendSetCredentialResponse(commandObj, commandPath, status, 0, nextAvailableCredentialSlot);
@@ -2245,7 +2251,16 @@ DlStatus DoorLockServer::createCredential(chip::EndpointId endpointId, chip::Fab
     }
     else
     {
-        // appclusters, 5.2.4.40: if user index is NULL, we should try to modify the existing user
+        // appclusters, 5.2.4.40: if user index is NULL, we should try to modify
+        // the existing user.  In this case userStatus and userType shall both
+        // be null.
+        if (!userStatus.IsNull() || !userType.IsNull())
+        {
+            emberAfDoorLockClusterPrintln("[SetCredential] Unable to add credential: invalid arguments "
+                                          "[endpointId=%d,credentialIndex=%d,credentialType=%u]",
+                                          endpointId, credentialIndex, to_underlying(credentialType));
+            return DlStatus::kInvalidField;
+        }
         status = createNewCredentialAndAddItToUser(endpointId, creatorFabricIdx, userIndex.Value(), credential, credentialData);
     }
 
@@ -2314,7 +2329,7 @@ DlStatus DoorLockServer::modifyCredential(chip::EndpointId endpointId, chip::Fab
 {
 
     // appclusters, 5.2.4.40: when modifying a credential, userStatus and userType shall both be NULL.
-    if (!userStatus.IsNull() || (!userType.IsNull() && UserTypeEnum::kProgrammingUser != userType.Value()))
+    if (!userStatus.IsNull() || !userType.IsNull())
     {
         emberAfDoorLockClusterPrintln("[SetCredential] Unable to modify the credential: invalid arguments "
                                       "[endpointId=%d,credentialIndex=%d,credentialType=%u]",

diff --git a/src/app/tests/suites/DL_UsersAndCredentials.yaml b/src/app/tests/suites/DL_UsersAndCredentials.yaml
@@ -1199,15 +1199,18 @@ tests:
               - name: "NextCredentialIndex"
                 value: null
 
-    # Duplicate of the previous test that does not check the value of nextCredentialIndex
     - label:
-          "Reading RFID credential with index 0 returns no credential duplicate
-          with bug workaround"
+          "Reading RFID credential with out-of-bounds index returns no
+          credential"
       command: "GetCredentialStatus"
       arguments:
           values:
               - name: "Credential"
-                value: { CredentialType: 2, CredentialIndex: 0 }
+                value:
+                    {
+                        CredentialType: 2,
+                        CredentialIndex: NumberOfRFIDUsersSupported + 1,
+                    }
       response:
           values:
               - name: "CredentialExists"
@@ -1218,19 +1221,15 @@ tests:
                 value: null
               - name: "LastModifiedFabricIndex"
                 value: null
+              - name: "NextCredentialIndex"
+                value: null
 
-    - label:
-          "Reading RFID credential with out-of-bounds index returns no
-          credential"
+    - label: "Check that RFID credential does not exist"
       command: "GetCredentialStatus"
       arguments:
           values:
               - name: "Credential"
-                value:
-                    {
-                        CredentialType: 2,
-                        CredentialIndex: NumberOfRFIDUsersSupported + 1,
-                    }
+                value: { CredentialType: 2, CredentialIndex: 2 }
       response:
           values:
               - name: "CredentialExists"
@@ -1244,24 +1243,89 @@ tests:
               - name: "NextCredentialIndex"
                 value: null
 
-    - label: "Check that RFID credential does not exist"
-      command: "GetCredentialStatus"
+    - label:
+          "Create new RFID credential and add it to existing user with non-null
+          UserStatus should fail"
+      command: "SetCredential"
+      timedInteractionTimeoutMs: 10000
       arguments:
           values:
+              - name: "OperationType"
+                value: 0
               - name: "Credential"
-                value: { CredentialType: 2, CredentialIndex: 2 }
+                value: { CredentialType: 2, CredentialIndex: 1 }
+              - name: "CredentialData"
+                value: "rfid_data_123456"
+              - name: "UserIndex"
+                value: 1
+              - name: "UserStatus"
+                value: 1
+              - name: "UserType"
+                value: null
       response:
           values:
-              - name: "CredentialExists"
-                value: false
+              - name: "Status"
+                value: 0x85 # INVALID_COMMAND
               - name: "UserIndex"
                 value: null
-              - name: "CreatorFabricIndex"
+              - name: "NextCredentialIndex"
+                value: 2
+
+    - label:
+          "Create new RFID credential and add it to existing user with non-null
+          UserType should fail"
+      command: "SetCredential"
+      timedInteractionTimeoutMs: 10000
+      arguments:
+          values:
+              - name: "OperationType"
+                value: 0
+              - name: "Credential"
+                value: { CredentialType: 2, CredentialIndex: 1 }
+              - name: "CredentialData"
+                value: "rfid_data_123456"
+              - name: "UserIndex"
+                value: 1
+              - name: "UserStatus"
                 value: null
-              - name: "LastModifiedFabricIndex"
+              - name: "UserType"
+                value: 0
+      response:
+          values:
+              - name: "Status"
+                value: 0x85 # INVALID_COMMAND
+              - name: "UserIndex"
                 value: null
               - name: "NextCredentialIndex"
+                value: 2
+
+    - label:
+          "Create new RFID credential and add it to existing user with non-null
+          UserType and UserStatus should fail"
+      command: "SetCredential"
+      timedInteractionTimeoutMs: 10000
+      arguments:
+          values:
+              - name: "OperationType"
+                value: 0
+              - name: "Credential"
+                value: { CredentialType: 2, CredentialIndex: 1 }
+              - name: "CredentialData"
+                value: "rfid_data_123456"
+              - name: "UserIndex"
+                value: 1
+              - name: "UserStatus"
+                value: 1
+              - name: "UserType"
+                value: 0
+      response:
+          values:
+              - name: "Status"
+                value: 0x85 # INVALID_COMMAND
+              - name: "UserIndex"
                 value: null
+              - name: "NextCredentialIndex"
+                value: 2
 
     - label: "Create new RFID credential and add it to existing user"
       command: "SetCredential"
@@ -1632,6 +1696,90 @@ tests:
               - name: "NextCredentialIndex"
                 value: 3
 
+    - label:
+          "Modify credentialData of existing PIN credential with non-null
+          UserStatus should fail"
+      command: "SetCredential"
+      timedInteractionTimeoutMs: 10000
+      arguments:
+          values:
+              - name: "OperationType"
+                value: 2
+              - name: "Credential"
+                value: { CredentialType: 1, CredentialIndex: 1 }
+              - name: "CredentialData"
+                value: "123456"
+              - name: "UserIndex"
+                value: 1
+              - name: "UserStatus"
+                value: 1
+              - name: "UserType"
+                value: null
+      response:
+          values:
+              - name: "Status"
+                value: 0x85 # INVALID_COMMAND
+              - name: "UserIndex"
+                value: null
+              - name: "NextCredentialIndex"
+                value: 2
+
+    - label:
+          "Modify credentialData of existing PIN credential with non-null
+          UserType should fail"
+      command: "SetCredential"
+      timedInteractionTimeoutMs: 10000
+      arguments:
+          values:
+              - name: "OperationType"
+                value: 2
+              - name: "Credential"
+                value: { CredentialType: 1, CredentialIndex: 1 }
+              - name: "CredentialData"
+                value: "123456"
+              - name: "UserIndex"
+                value: 1
+              - name: "UserStatus"
+                value: null
+              - name: "UserType"
+                value: 0
+      response:
+          values:
+              - name: "Status"
+                value: 0x85 # INVALID_COMMAND
+              - name: "UserIndex"
+                value: null
+              - name: "NextCredentialIndex"
+                value: 2
+
+    - label:
+          "Modify credentialData of existing PIN credential with non-null
+          UserStatus and UserType should fail"
+      command: "SetCredential"
+      timedInteractionTimeoutMs: 10000
+      arguments:
+          values:
+              - name: "OperationType"
+                value: 2
+              - name: "Credential"
+                value: { CredentialType: 1, CredentialIndex: 1 }
+              - name: "CredentialData"
+                value: "123456"
+              - name: "UserIndex"
+                value: 1
+              - name: "UserStatus"
+                value: 1
+              - name: "UserType"
+                value: 0
+      response:
+          values:
+              - name: "Status"
+                value: 0x85 # INVALID_COMMAND
+              - name: "UserIndex"
+                value: null
+              - name: "NextCredentialIndex"
+                value: 2
+
     - label: "Modify credentialData of existing PIN credential"
       command: "SetCredential"
       timedInteractionTimeoutMs: 10000