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

Support for encryption of shared preferences #593

Closed
QuintinWillison opened this issue Jul 22, 2020 · 3 comments · Fixed by #681
Closed

Support for encryption of shared preferences #593

QuintinWillison opened this issue Jul 22, 2020 · 3 comments · Fixed by #681
Assignees

Comments

@QuintinWillison
Copy link
Contributor

QuintinWillison commented Jul 22, 2020

We persist secrets (e.g. ABLY_DEVICE_SECRET) and tokens (e.g. ABLY_DEVICE_IDENTITY_TOKEN) to SharedPreferences without encryption or any other form of obfuscation, in terms of both values and the keys they are stored under.

This has been raised by a customer who discovered this when doing a security test after they "rooted" their app.

When Ably first implemented LocalDevice storage, there wasn't anything we deemed suitable that would give any material increase in security, hence the implementation was kept simple.

Perhaps the best solution going forward would be to allow app developers to override the storage I/O in order to provide their own encryption and/or platform service designed for secrets. Such a provider might be as simple as:

interface Storage {
    void write(String key, String value);
    String read(String key, String defaultValue);
}

We would provide a default implementation using shared preferences, as we do currently.

I would also suggest we look at fixing #562 when we work on this as it's in the same area of the codebase.

@ben-xD
Copy link
Contributor

ben-xD commented Oct 5, 2021

It wasn't very clear to me what this issue or related PR was doing. After reading the diff of the PR, I noticed in ClientOptions:

/**
* Custom Local Device storage. In the case nothing is provided then a default implementation
* using SharedPreferences is used.
*/
public Storage localStorage = null;

So this issue only allows users to specify a custom implementation of Storage, and doesn't actually do anything with encryption. Users would have to add logic to encrypt the data themselves. This is probably because the clients shouldn't really encrypt the data using a symmetric key which is available locally, since this can be found and used by a threat actor.

@sacOO7
Copy link
Collaborator

sacOO7 commented Oct 5, 2021

@ben-xD Actually, there is a official way to encrypt shared pref. With a key, that's not exposed to a user.
https://developer.android.com/reference/androidx/security/crypto/EncryptedSharedPreferences
Also, there was a discussion related to this, and it was decided that we will not do the encryption, since it adds more efforts and complexity to the code.
That said, I am not sure how much complex it can get, and backward compatibility of encryptedSharedPref API

@ben-xD
Copy link
Contributor

ben-xD commented Oct 5, 2021

Okay thanks for adding that detail, it seems like we missed an opportunity to make things easier for the users who care about security here 😅. IMHO after browsing for some example code, it's not complicated, and I think you agree with me based on your comment above.

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

Successfully merging a pull request may close this issue.

5 participants