Skip to content
This repository has been archived by the owner on Jun 24, 2022. It is now read-only.

MOB-M-7: iOS reject not called in failure case CovidShield::getRandomBytes #444

Closed
obrien-j opened this issue Jul 5, 2020 · 2 comments · Fixed by #524
Closed

MOB-M-7: iOS reject not called in failure case CovidShield::getRandomBytes #444

obrien-j opened this issue Jul 5, 2020 · 2 comments · Fixed by #524

Comments

@obrien-j
Copy link

obrien-j commented Jul 5, 2020

Defect Summary
When reviewing code in ../covid-shield- mobile/blob/add_device_capabilities/ios/CovidShield/CovidShield.m#L18, the SecRandomCopyBytes has returned an error.

Analysis of the Issue
Following the code path, SecRandomCopyBytes is supposed to be resolved later on withing the React framework. There was no evidence that was handled and the error was not handled properly.

image

Recommendation
Suggest to add error handling against SecRandomCopyBytes within React framework

@obrien-j
Copy link
Author

obrien-j commented Jul 6, 2020

https://developer.apple.com/documentation/security/1399291-secrandomcopybytes

Always test the returned status to make sure that the array has been updated with new, random data before trying to use the values. For example, to create 10 random bytes:

var bytes = [Int8](repeating: 0, count: 10)
let status = SecRandomCopyBytes(kSecRandomDefault, bytes.count, &bytes)

if status == errSecSuccess { // Always test the status.
    print(bytes)
    // Prints something different every time you run.
}

@obrien-j
Copy link
Author

obrien-j commented Jul 6, 2020

While our code mimics the example as provided by Apple, we don't handle the failure case at all, so would recommend either throwing an exception up out of the IOS layer (reject?)

I'm not sure how we could validate at the react layer that the buffer returned isn't a successful random value, as even an empty buffer would be feasible.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants