-
Notifications
You must be signed in to change notification settings - Fork 2k
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
[EFR32] Lock-app updates, adding multiple users and credentials #19229
Conversation
PR #19229: Size comparison from 9c1660c to 97d2504 Increases above 0.2%:
Increases (1 build for efr32)
Full report (41 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
PR #19229: Size comparison from c868f81 to 3fa0780 Increases (6 builds for cc13x2_26x2, nrfconnect)
Decreases (20 builds for cc13x2_26x2, cyw30739, esp32, k32w, p6, telink)
Full report (36 builds for cc13x2_26x2, cyw30739, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fast tracking platform changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mykrupp I think the credentials bits here are still broken, in a way that is an exploitable security hole.
EFR32Config::ReadConfigValueBin(EFR32Config::kConfigKey_LockUser, reinterpret_cast<uint8_t *>(&mLockUsers), | ||
sizeof(EmberAfPluginDoorLockUserInfo) * ArraySize(mLockUsers), outLen); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can still leave part of mLockUsers
un-initialized if the size increased, or fail to read if the size decreased.....
EFR32Config::ReadConfigValueBin(EFR32Config::kConfigKey_UserCredentials, reinterpret_cast<uint8_t *>(mCredentials[0].Get()), | ||
sizeof(DlCredential) * mMaxUsers * mMaxCredentialsPerUser, outLen); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes no sense. There are mMaxUsers buffers, each of size mMaxCredentialsPerUser, right? But this is going to read mMaxUsers * mMaxCredentialsPerUser
credentials into the first buffer, writing outside its bounds, trashing the heap, and not necessarily filling the other buffers at all. Unless I am missing something? What am I missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I didn't test thoroughly enough here. The members in mCredentials(credentialType and credentialIndex) are only accessed by the getUser command. I was testing this by sending a lock/unlock command after reset.
Since there's no way to pass a memory offset to the ReadConfigValueBin function, I guess each user will need it's own config key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in this PR - b7677b1
} | ||
|
||
// Check the PIN code | ||
for (uint8_t i; i < 10; i++) | ||
for (uint8_t i = 0; i < mMaxCredentialsPerUser; i++) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This loop needs to go to MAX_CREDENTIALS, no? If MAX_CREDENTIALS is 20 and mMaxCredentialsPerUser is 5, say, this loop does not do the right thing...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in this PR - b7677b1
…ect-chip#19229) * code review comments. Fix mCredentials, fix NVM flash saving * restyle * cleanup * remove unused array * remove TotalCredentials key * Check mMaxUsers against DOOR_LOCK_MAX_USERS
…ect-chip#19229) * code review comments. Fix mCredentials, fix NVM flash saving * restyle * cleanup * remove unused array * remove TotalCredentials key * Check mMaxUsers against DOOR_LOCK_MAX_USERS
Problem
Door-lock-server was updated to accommodate multiple users and credentials
Change overview
EFR32 platform now accommodates multiple users and credentials.
Added logic for RequirePINForRemoteOperation
Read maxNumberOfCredentials per user
Store new users and credentials in nvm flash
Fix non-happy path cases(i.e. lock command sent door is already locked)
Testing
This was tested by sending relevant commands affected by the recent door-lock-server changes
./out/chip-tool doorlock set-user 0 1 mike 5 1 0 0 1 1 --timedInteractionTimeoutMs 1000
./out/chip-tool doorlock set-user 0 2 matt 6 1 0 0 1 1 --timedInteractionTimeoutMs 1000
./out/chip-tool doorlock set-user 0 3 nick 7 1 0 0 1 1 --timedInteractionTimeoutMs 1000
// set credential
./out/chip-tool doorlock set-credential 0 '{ "credentialType": 1, "credentialIndex": 1 }' "123456" 1 null null 1 1 --timedInteractionTimeoutMs 1000
./out/chip-tool doorlock set-credential 0 '{ "credentialType": 1, "credentialIndex": 2 }' "123457” 1 null null 1 1 --timedInteractionTimeoutMs 1000
// get user
./out/chip-tool doorlock get-user 1 1 1 --timedInteractionTimeoutMs 1000
Reset EFR32
./out/chip-tool doorlock get-user 1 1 1 --timedInteractionTimeoutMs 1000
See that users and credentials are still available after reset