macOS keychain sanity test #5885
Conversation
When requesting kSecClassIdentity items from the keychain sometimes invalid items are returned. These cause errSecInvalidItemRef to be returned from SecKeychainItemCopyAttributesAndData and result in an empty row in the table. Catch the error and avoid returning empty rows.
Add a simple sanity test for macOS keychain items. Closes: osquery#5021
willnewton
force-pushed
the
macos-keychain-tests
branch
from
e105018 to
1a1f75a
Compare
| // This attr does not exist, skip it. | ||
| continue; | ||
| } else if (status != errSecSuccess) { | ||
| // If this keychain item is not valid then don't add it to results. |
There was a problem hiding this comment.
I think we will have to free attr_list here with SecKeychainItemFreeAttributesAndData(...) before returning.
Something along the lines of:
if (attr_list != null) {
SecKeychainItemFreeAttributesAndData(attr_list, nullptr);
}There was a problem hiding this comment.
As far as I can tell we only get a non-NULL attr_list when the call succeeded (status == errSecSuccess) so I don't think we have anything to deallocate in the error case.
Rename variable from status to os_status following review.
Add an assert for number of rows to the macOS keychain items test following review.
There was a problem hiding this comment.
This LGTM. @sharvilshah are you ok with the response? @willnewton any chance you're participating in Hacktoberfest? Happy to tag if that's the case, thanks!
Yes, this looks good. I had a cursory look and these APIs are a bit different than iOKit one. Thanks @willnewton |
|
@directionless looks like this is just on your review, mind taking another quick glance and then stamping if you're all set? |
Yes, if you could tag for Hacktoberfest that would be great. |
The assert should be checking for greater than zero rows, not greater than or equal to zero.
|
LGTM |
This adds a sanity test for the macOS keychain table. Running this test exposed an issue which was causing a number of blank rows to be returned at the end of the table. An additional patch avoids this issue by adding some more error checking to the keychain APIs used and allows the test to pass.
This PR is intended to close issue #5021