Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] [DRLK] Modifying a credential overwrites credentials of other types in a user's credential list #34816

Closed
nlagueAlle opened this issue Aug 6, 2024 · 1 comment · Fixed by #34841
Labels
bug Something isn't working needs triage

Comments

@nlagueAlle
Copy link

Reproduction steps

Description
The function DoorLockServer::modifyCredentialForUser() does not check the credential type when searching for the existing credential in the list of user credentials. This can lead to credentials being easily overwritten in the user's list depending on the order they were added to the user.

Reproduction steps

  1. ./chip-tool doorlock set-user 0 1 "test" 123 1 0 0 <destination-id> <endpoint-id> --timedInteractionTimeoutMs <ms_value>
  2. ./chip-tool doorlock set-credential 0 '{ "credentialType": 2, "credentialIndex": 1 }' <CredentialData> 1 null null <destination-id> <endpoint-id> --timedInteractionTimeoutMs <ms_value>
  3. ./chip-tool doorlock set-credential 0 '{ "credentialType": 1, "credentialIndex": 1 }' "1234" 1 null null <destination-id> <endpoint-id> --timedInteractionTimeoutMs <ms_value>
  4. ./chip-tool doorlock get-user 1 <destination-id> <endpoint-id>
  5. ./chip-tool doorlock set-credential 2 '{ "credentialType": 1, "credentialIndex": 1 }' "5678" 1 null null <destination-id> <endpoint-id> --timedInteractionTimeoutMs <ms_value>
  6. ./chip-tool doorlock get-user 1 <destination-id> <endpoint-id>

Output of Get User at Step 4
GetUserResponse: {
userIndex: 1
userName: test
userUniqueID: 123
userStatus: 1
userType: 0
credentialRule: 0
credentials: 1 entries
[1]: {
CredentialType: 2
CredentialIndex: 1
}
[2]: {
CredentialType: 1
CredentialIndex: 1
}
}
creatorFabricIndex: 1
lastModifiedFabricIndex: 1
nextUserIndex: null
}

Output of Get User at Step 6
GetUserResponse: {
userIndex: 1
userName: test
userUniqueID: 123
userStatus: 1
userType: 0
credentialRule: 0
credentials: 1 entries
[1]: {
CredentialType: 1
CredentialIndex: 1
}
[2]: {
CredentialType: 1
CredentialIndex: 1
}
}
creatorFabricIndex: 1
lastModifiedFabricIndex: 1
nextUserIndex: null
}

Potential Fix
Add a check for the credential type to DoorLockServer::modifyCredentialForUser() alongside the credential index check.

Bug prevalence

100% reproducible

GitHub hash of the SDK that was being used

main

Platform

core

Platform Version(s)

No response

Anything else?

No response

@nlagueAlle nlagueAlle added bug Something isn't working needs triage labels Aug 6, 2024
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Aug 7, 2024
Credential indices are per-type, so we should be checking both when locating the
credential to be modified.

Fixes project-chip#34816
@bzbarsky-apple
Copy link
Contributor

@nlagueAlle Thank you for catching this!

austina-csa pushed a commit to austina-csa/connectedhomeip that referenced this issue Aug 12, 2024
…hip#34841)

Credential indices are per-type, so we should be checking both when locating the
credential to be modified.

Fixes project-chip#34816
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Aug 19, 2024
Credential indices are per-type, so we should be checking both when locating the
credential to be modified.

Fixes project-chip#34816
ArekBalysNordic pushed a commit to ArekBalysNordic/sdk-connectedhomeip that referenced this issue Nov 28, 2024
Credential indices are per-type, so we should be checking both when locating the
credential to be modified.

Fixes project-chip/connectedhomeip#34816
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working needs triage
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants