Encrypt credentials in memory #2723
Conversation
|
@damagatchi retest this please |
|
@damagatchi retest this please |
|
@damagatchi retest this please |
|
@avazirna There is no description or context in these PRs to associate PR reviews. Can we add more context to these PRs in terms of what necessitate these changes, safety story and testing around different Android OS if there is dependency on different Android versions with Android Key store involved here etc. |
|
@damagatchi retest this please |
Yes, I was looking into an issue that was causing most of the instrumentation tests to fail. It should be sorted now. |
|
@damagatchi retest this please |
avazirna
force-pushed
the
encrypt-credentials-in-memory
branch
2 times, most recently
from
782573d
to
0ac80b0
Compare
avazirna
force-pushed
the
encrypt-credentials-in-memory
branch
from
0ac80b0
to
efe1908
Compare
app/build.gradle
Outdated
| * map. The task registerServiceProviders is responsible for iterating over it and create the | ||
| * configuration files under META-INF/services | ||
| */ | ||
| SERVICE_PROVIDERS = ['org.commcare.util.IEncryptionKeyProvider' : 'org.commcare.utils.EncryptionKeyProvider'] |
There was a problem hiding this comment.
What's the reasoning behind specifying it here instead of Java - registerServiceKeyProvider(new EncryptionKeyProvider() ?
There was a problem hiding this comment.
@shubham1g5 this is just to create the static configuration files, the instantiation is done by the JRE using the ServiceLoader. The reasoning here is to decouple the service interface from its implementations, more concretely, it removes any dependency between commcare-android and commcare-core when it comes to IEncryptionKeyProvider implementations.
| @@ -228,6 +231,8 @@ public void onCreate() { | |||
| setRoots(); | |||
| prepareTemporaryStorage(); | |||
|
|
|||
| getEncryptionKeyProvider().generateCryptographicKeyInKeyStore(USER_CREDENTIALS_KEY_ALIAS); | |||
There was a problem hiding this comment.
Since this key is not user specific, can we call it something general like cc_in_memory_encryption_key ?
app/build.gradle
Outdated
| @@ -632,3 +640,17 @@ downloadLicenses { | |||
| includeProjectDependencies = true | |||
| dependencyConfiguration = 'compile' | |||
| } | |||
|
|
|||
| task registerServiceProviders { | |||
There was a problem hiding this comment.
Why add it at build time instead of just having a static file ?
There was a problem hiding this comment.
@shubham1g5 that was the initial approach but I thought about making it easy for future additions and also to put this on others radar, a static file can very easily be overlooked. So, not necessarily strong reasons.
There was a problem hiding this comment.
Got it, I would still prefer a static file if we go with service provider approach as this adds an unnecessary build step without any strong advantages.
avazirna
force-pushed
the
encrypt-credentials-in-memory
branch
from
0138c07
to
7d66b9f
Compare
avazirna
force-pushed
the
encrypt-credentials-in-memory
branch
from
cae26be
to
db7ef66
Compare
|
@damagatchi retest this please |
Summary
This PR addresses an issue flagged by the auditors in which CommCare stores sensitive data (user credentials) in heap memory in plaintext, meaning that a memory dump would be sufficient for an attacker to gain unauthorised access to the app. The proposal was to encrypt the user's credentials while in memory and store any cryptographic keys used in the process in the Android keyStore. The tricky part is that
KeyGenParameterSpec, which is the recommended API to generate and store keys in the KeyStore, is only available from API 23, so for devices running Android 5 - 5.1.1, a different API had to be used,KeyPairGeneratorSpec.KeyPairGeneratorSpecgenerates key pairs which are used in asymmetric algorithms, such asRSA, this forced the need to support both algorithmsAESandRSA.Ticket: https://dimagi-dev.atlassian.net/browse/INDIV-98
Safety Assurance
Safety story
This feature shouldn't affect the user in any way, the principle is just to encrypt the
usernameandpasswordwhen these are set and decrypt when returned. However, we still need to consider any potential slowness caused by running cryptographic operations. This will be properly assessed during QA.A note for reviewers is around the generation of the cryptographic key, this is currently done during app initialisation, and this is a process that needs to happen in order to install the key in the KeyStore, the question is whether there are other scenarios to consider?
Cross-request: dimagi/commcare-core#1349