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

[Android] Implement security level guarantees. #176

Merged
merged 7 commits into from Feb 13, 2019

Conversation

Projects
None yet
4 participants
@mandrigin
Copy link
Collaborator

mandrigin commented Dec 18, 2018

Background.
Sometimes, we want to make sure that the encryption key that is generated for keychain on Android has a minimum security guarantee. It shouldn't be possible to add items to keychain that might compromise the security.

Also, sometimes we just want to disable certain features if security guarantees are too weak.

We needed that for our application (Status.im), and we thought that that is a good idea to upstream.

I'll be happy for any feedback!


Implementation

Supported security levels:

  • ANY
  • SECURE_SOFTWARE
  • SECURE_HARDWARE (TEE or SE guarantees).

(1) Add getSecurityLevel() API that returns which security level is
supported on this Android version and the specific device.

(2) For APIs that store credentials, an additional optional parameter was added
that fails storing the credentials if the security level is not what is
expected.

// Store the credentials.
// Will fail if Keychain can't guarantee at least SECURE_HARDWARE level of encryption key.
await Keychain.setGenericPassword(username, password, Keychain.SECURITY_LEVEL.SECURE_HARDWARE);

(3) StrongBox support on Android 9+ (and supported devices [Pixel 3]). Try to generate the key in StrongBox first, and use regular Keychain if that fails.

@mandrigin mandrigin force-pushed the status-im:upstream-security-guarantees branch 3 times, most recently from 6fd2beb to 7c26803 Dec 18, 2018

README.md Outdated
@@ -45,7 +45,7 @@ See `KeychainExample` for fully working project example.

Both `setGenericPassword` and `setInternetCredentials` are limited to strings only, so if you need to store objects etc, please use `JSON.stringify`/`JSON.parse` when you store/access it.

### `setGenericPassword(username, password, [{ accessControl, accessible, accessGroup, service }])`
### `setGenericPassword(username, password, securityLevel, [{ accessControl, accessible, accessGroup, service }])`

This comment has been minimized.

@vonovak

vonovak Dec 19, 2018

Collaborator

the way securityLevel was added as a param seems like a breaking change.(?)
would it be possible to add this into the params object? ({ accessControl, accessible, accessGroup, service }).

this is just a quick observation, I'll try to do proper review some time next week. Thanks for the PR!

This comment has been minimized.

@mandrigin

mandrigin Dec 19, 2018

Author Collaborator

Thanks! It's optional, I think I did the docs wrong... I will wait for more feedback and will fix everything at once then.

This comment has been minimized.

@Bautista-Baiocchi-lora

Bautista-Baiocchi-lora Jan 16, 2019

Is there any progress on this merge? Very nice feature.

This comment has been minimized.

@mandrigin

mandrigin Jan 16, 2019

Author Collaborator

oh, right. is there anything else to tweak except the param object?

@vonovak
Copy link
Collaborator

vonovak left a comment

hi! thanks a lot for your time, I left some comments, but overall it looks good. I did not test though, just read the code. I'll test once the comments are resolved, thanks.

index.js Outdated
* @param {object} options Keychain options, iOS only
* @return {Promise} Resolves to `true` when successful
*/
export function setInternetCredentials(
server: string,
username: string,
password: string,
minimumSecurityLevel?: SecMinimumLevel,

This comment has been minimized.

@vonovak

vonovak Jan 16, 2019

Collaborator

this is a breaking change (https://github.com/oblador/react-native-keychain/pull/176/files#r243020977); can you move this to Options?

This comment has been minimized.

@mandrigin

mandrigin Jan 20, 2019

Author Collaborator

done

index.js Outdated
@@ -156,22 +183,34 @@ function getOptionsArgument(serviceOrOptions?: string | Options) {
: serviceOrOptions;
}

function getMinimumSecurityLevel(minimumSecurityLevel?: SecMinimumLevel) {
if (minimumSecurityLevel === undefined) {

This comment has been minimized.

@vonovak

vonovak Jan 16, 2019

Collaborator

this function can be simplified to return minimumSecurityLevel || SECURITY_LEVEL.ANY;

This comment has been minimized.

@mandrigin

mandrigin Jan 20, 2019

Author Collaborator

it is different now, but I tried to keep it simple

README.md Outdated
@@ -89,6 +89,18 @@ Inquire if the type of local authentication policy is supported on this device w

Get what type of hardware biometry support the device has. Resolves to a `Keychain.BIOMETRY_TYPE` value when supported, otherwise `null`.

### `getSecurityLevel()`

This comment has been minimized.

@vonovak

vonovak Jan 16, 2019

Collaborator

please mention also here that this is android-only

This comment has been minimized.

@mandrigin

mandrigin Jan 20, 2019

Author Collaborator

done


If set, `securityLevel` parameter specifies minimum security level that the encryption key storage should guarantee for storing credentials to succeed.

* `ANY` - no security guarantees needed (default value); Credentials can be stored in FB Secure Storage;

This comment has been minimized.

@vonovak

vonovak Jan 16, 2019

Collaborator

can we export these as constants from the native module? (see getConstants in the docs

This comment has been minimized.

@mandrigin

mandrigin Jan 20, 2019

Author Collaborator

yep, I did that. It didn't simplify the code that much though, mostly because it exports them as globals in the module space, and all of the existing code is based on Objects:

what getConstants export is: SECURITY_LEVEL_ANY in the js scope, where we have SECURITY_LEVEL.ANY now.

So what I did is I wrapped these constants into an object. It is a slightly better code, I think, because the values of the constants aren't hardcoded twice, but it is not as huge of a win.

index.js Outdated
* @param {string|object} serviceOrOptions Reverse domain name qualifier for the service, defaults to `bundleId` or an options object.
* @return {Promise} Resolves to `true` when successful
*/
export function setGenericPassword(
username: string,
password: string,
minimumSecurityLevel?: SecMinimumLevel,

This comment has been minimized.

@vonovak

vonovak Jan 16, 2019

Collaborator

this is a breaking change (https://github.com/oblador/react-native-keychain/pull/176/files#r243020977); can you move this to Options?

This comment has been minimized.

@mandrigin

mandrigin Jan 20, 2019

Author Collaborator

done

// clean up the old cipher storage
oldCipherStorage.removeKey(service);

try {

This comment has been minimized.

@vonovak

vonovak Jan 16, 2019

Collaborator

do you think you could refactor the entire if / else part into a separate method (that will return a decryptionResult I suppose)? getGenericPasswordForOptions is becoming a bit too lengthy. Thanks.

This comment has been minimized.

@mandrigin

mandrigin Jan 20, 2019

Author Collaborator

done

@mandrigin

This comment has been minimized.

Copy link
Collaborator Author

mandrigin commented Jan 18, 2019

@vonovak thanks a lot for your feedback! Everything looks sensible! I will try to take a look at the review today. I didn’t disappear, but had a intense week :)

Implement security level guarantees for Android.
Supported security levels:
- ANY
- SECURE_SOFTWARE
- SECURE_HARDWARE (TEE or SE guarantees).

(1) Add `getSecurityLevel()` API that returns which security level is
supported on this Android version and the specific device.

(2) For APIs that store credentials, an additional optional parameter was added
that fails storing the credentials if the security level is not what is
expected.

```
// Store the credentials.
// Will fail if Keychain can't guarantee at least SECURE_HARDWARE level of encryption key.
await Keychain.setGenericPassword(username, password, Keychain.SECURITY_LEVEL.SECURE_HARDWARE);
```

(3) StongBox support on Android 9+ (and supported devices [Pixel 3]).

@mandrigin mandrigin force-pushed the status-im:upstream-security-guarantees branch from 7c26803 to c2701a5 Jan 20, 2019

mandrigin added some commits Jan 20, 2019

@mandrigin

This comment has been minimized.

Copy link
Collaborator Author

mandrigin commented Jan 20, 2019

@vonovak I addressed the feedback, and tested it a bit on an example app on Pixel 2 (Android 9.0). I will keep commits as is for now, but if everything is okay, I will squash everything to a single commit.

@vonovak

This comment has been minimized.

Copy link
Collaborator

vonovak commented Jan 21, 2019

@mandrigin thanks, I'll check it out later. no need to squash, we'll let GH do that. Thanks.

@mandrigin

This comment has been minimized.

Copy link
Collaborator Author

mandrigin commented Feb 1, 2019

@vonovak I'm just wondering if you had some time to take a glance on these changes. Thanks!

@vonovak vonovak referenced this pull request Feb 6, 2019

Merged

add feature to example #9

@vonovak

This comment has been minimized.

Copy link
Collaborator

vonovak commented Feb 6, 2019

@mandrigin I opened a PR to your fork: status-im#9

@mandrigin

This comment has been minimized.

Copy link
Collaborator Author

mandrigin commented Feb 6, 2019

@vonovak thanks! I merged the PR you created.

@vonovak

vonovak approved these changes Feb 6, 2019

@vonovak

This comment has been minimized.

Copy link
Collaborator

vonovak commented Feb 6, 2019

@oblador this looks good to me, care to merge and publish?

@oblador oblador merged commit 01755de into oblador:master Feb 13, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@vonovak

This comment has been minimized.

Copy link
Collaborator

vonovak commented Feb 14, 2019

@mandrigin thanks for your time and contribution, would you be interested in becoming a collaborator on this repo? 😄

@mandrigin

This comment has been minimized.

Copy link
Collaborator Author

mandrigin commented Feb 14, 2019

@vonovak thank you! yes, we rely on it quite extensively at Status, so I'll be glad to!

@oblador

This comment has been minimized.

Copy link
Owner

oblador commented Feb 14, 2019

@mandrigin Sent you an invite. Happy to have you on the team 👍

@mandrigin

This comment has been minimized.

Copy link
Collaborator Author

mandrigin commented Feb 14, 2019

@oblador @vonovak thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment