-
Notifications
You must be signed in to change notification settings - Fork 14
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
Encrypt credentials in memory #1349
base: master
Are you sure you want to change the base?
Changes from 15 commits
49c6586
09d9900
c95b7c8
676442b
93f33c8
cd6c546
f20986c
7968312
c5d3f94
7b0a54d
a22ff5a
3647d76
c485e5a
b075ac9
eff308b
99e70fd
efdc30a
448f074
0c39c5a
5b66cc4
8feef57
659f47e
f6ac37d
c67c1df
e151f5f
1069a52
87f6a93
aae5ff2
5f561fb
92cef42
fcf749d
276c050
0a32085
0b39e88
03f0ac0
eb4b3ed
409a6f5
5fe6f00
20b2068
4c429d4
dbb1983
0b771c7
db3d264
7c9f89d
fb70f9b
acd23a9
41cb1f2
dd45f8d
19fc7d8
3767176
5b8f281
2f269e8
df1575a
f7ab55b
10352e1
c3b79b9
94b0062
f176952
7a82c35
96f0b04
ffc0c96
58ffb32
b2a8da9
8c7363a
188e22b
a2928bd
bd512d6
4716bda
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,49 +1,100 @@ | ||
package org.commcare.util; | ||
|
||
import java.io.IOException; | ||
import java.nio.ByteBuffer; | ||
import java.nio.charset.Charset; | ||
import java.security.InvalidAlgorithmParameterException; | ||
import java.security.InvalidKeyException; | ||
import java.security.Key; | ||
import java.security.KeyFactory; | ||
import java.security.KeyStore; | ||
import java.security.KeyStoreException; | ||
import java.security.NoSuchAlgorithmException; | ||
import java.security.Security; | ||
import java.security.UnrecoverableEntryException; | ||
import java.security.cert.CertificateException; | ||
import java.security.spec.InvalidKeySpecException; | ||
import java.security.spec.PKCS8EncodedKeySpec; | ||
import java.security.spec.X509EncodedKeySpec; | ||
|
||
import javax.crypto.BadPaddingException; | ||
import javax.crypto.Cipher; | ||
import javax.crypto.IllegalBlockSizeException; | ||
import javax.crypto.NoSuchPaddingException; | ||
import javax.crypto.SecretKey; | ||
import javax.crypto.spec.GCMParameterSpec; | ||
import javax.crypto.spec.IvParameterSpec; | ||
import javax.crypto.spec.SecretKeySpec; | ||
|
||
public class EncryptionUtils { | ||
private static KeyStore androidKeyStore; | ||
|
||
public static final String USER_CREDENTIALS_KEY_ALIAS = "user-credentials-key-alias"; | ||
|
||
public static final String ANDROID_KEYSTORE_PROVIDER_NAME = "AndroidKeyStore"; | ||
|
||
private enum CryptographicOperation {Encryption, Decryption} | ||
|
||
public static KeyStore getAndroidKeyStore() { | ||
if (androidKeyStore == null) { | ||
try { | ||
androidKeyStore = KeyStore.getInstance(ANDROID_KEYSTORE_PROVIDER_NAME); | ||
androidKeyStore.load(null); | ||
} catch (KeyStoreException | IOException | NoSuchAlgorithmException | | ||
CertificateException e) { | ||
throw new RuntimeException(e); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we throw a specific exception that callers can handle for and take appropriate fallbacks. |
||
} | ||
} | ||
return androidKeyStore; | ||
} | ||
|
||
public static String encryptUsingKeyFromKeyStore(String message, String alias) throws EncryptionException { | ||
Key key; | ||
try { | ||
key = retrieveKeyFromKeyStore(alias, CryptographicOperation.Encryption); | ||
} catch (KeyStoreException | UnrecoverableEntryException | NoSuchAlgorithmException e) { | ||
throw new RuntimeException(e); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should bubble up the specific exceptions to caller. |
||
} | ||
return encrypt(key.getAlgorithm(), message, key); | ||
} | ||
|
||
public static String encryptUsingBase64EncodedKey(String algorithm, String message, String key) throws EncryptionException { | ||
Key secret; | ||
try { | ||
secret = getKey(algorithm, key, CryptographicOperation.Encryption); | ||
} catch (InvalidKeySpecException e) { | ||
throw new EncryptionException("Invalid Key specifications", e); | ||
} | ||
return encrypt(algorithm, message, secret); | ||
} | ||
|
||
/** | ||
* Encrypts a message using the AES encryption and produces a base64 encoded payload containing the ciphertext, and a random IV which was used to encrypt the input. | ||
* Encrypts a message using the AES or RAS algorithms and produces a base64 encoded payload containing the ciphertext, and a random IV which was used to encrypt the input. | ||
* | ||
* @param message a UTF-8 encoded message to be encrypted | ||
* @param key A base64 encoded 256 bit symmetric key | ||
* @param algorithm to be used to encrypt the data | ||
* @param message a UTF-8 encoded message to be encrypted | ||
* @param key a SecretKey or PublicKey, depdending on the algorithm to be used | ||
* @return A base64 encoded payload containing the IV and AES encrypted ciphertext, which can be decoded by this utility's decrypt method and the same symmetric key | ||
*/ | ||
public static String encrypt(String message, String key) throws EncryptionException { | ||
final String ENCRYPT_ALGO = "AES/GCM/NoPadding"; | ||
public static String encrypt(String algorithm, String message, Key key) throws EncryptionException { | ||
final int MIN_IV_LENGTH_BYTE = 1; | ||
final int MAX_IV_LENGTH_BYTE = 255; | ||
SecretKey secret = getSecretKeySpec(key); | ||
|
||
try { | ||
Cipher cipher = Cipher.getInstance(ENCRYPT_ALGO); | ||
cipher.init(Cipher.ENCRYPT_MODE, secret); | ||
Cipher cipher = Cipher.getInstance(getCryptographicTransformation(algorithm)); | ||
cipher.init(Cipher.ENCRYPT_MODE, key); | ||
byte[] encryptedMessage = cipher.doFinal(message.getBytes(Charset.forName("UTF-8"))); | ||
byte[] iv = cipher.getIV(); | ||
if (iv.length < MIN_IV_LENGTH_BYTE || iv.length > MAX_IV_LENGTH_BYTE) { | ||
int ivSize = (iv == null ? 0 : iv.length); | ||
if (ivSize == 0) { | ||
iv = new byte[0]; | ||
} else if (ivSize < MIN_IV_LENGTH_BYTE || ivSize > MAX_IV_LENGTH_BYTE) { | ||
throw new EncryptionException("Initialization vector should be between " + | ||
MIN_IV_LENGTH_BYTE + " and " + MAX_IV_LENGTH_BYTE + | ||
" bytes long, but it is " + iv.length + " bytes"); | ||
" bytes long, but it is " + ivSize + " bytes"); | ||
} | ||
// The conversion of iv.length to byte takes the low 8 bits. To | ||
// convert back, cast to int and mask with 0xFF. | ||
byte[] ivPlusMessage = ByteBuffer.allocate(1 + iv.length + encryptedMessage.length) | ||
.put((byte)iv.length) | ||
byte[] ivPlusMessage = ByteBuffer.allocate(1 + ivSize + encryptedMessage.length) | ||
.put((byte)ivSize) | ||
.put(iv) | ||
.put(encryptedMessage) | ||
.array(); | ||
|
@@ -53,20 +104,99 @@ public static String encrypt(String message, String key) throws EncryptionExcept | |
} | ||
} | ||
|
||
|
||
private static SecretKey getSecretKeySpec(String key) throws EncryptionException { | ||
final int KEY_LENGTH_BIT = 256; | ||
/** | ||
* Converts a base64 encoded key into a SecretKey, PrivateKey or PublicKey, depending on the | ||
* Algorithm and Cryptographic operation | ||
* | ||
* @param algorithm to be used to encrypt/decrypt | ||
* @param base64encodedKey key in String format | ||
* @param cryptographicOperation relevant to the RSA algorithm | ||
* @return Decrypted message for the given AES encrypted message | ||
*/ | ||
private static Key getKey(String algorithm, String base64encodedKey, CryptographicOperation cryptographicOperation) throws EncryptionException, InvalidKeySpecException { | ||
byte[] keyBytes; | ||
try { | ||
keyBytes = Base64.decode(key); | ||
keyBytes = Base64.decode(base64encodedKey); | ||
} catch (Base64DecoderException e) { | ||
throw new EncryptionException("Encryption key base 64 encoding is invalid", e); | ||
} | ||
if (8 * keyBytes.length != KEY_LENGTH_BIT) { | ||
throw new EncryptionException("Key should be " + KEY_LENGTH_BIT + | ||
" bits long, not " + 8 * keyBytes.length); | ||
|
||
if (algorithm.equals("AES")) { | ||
final int KEY_LENGTH_BIT = 256; | ||
|
||
if (8 * keyBytes.length != KEY_LENGTH_BIT) { | ||
throw new EncryptionException("Key should be " + KEY_LENGTH_BIT + | ||
" bits long, not " + 8 * keyBytes.length); | ||
} | ||
return new SecretKeySpec(keyBytes, "AES"); | ||
} else if (algorithm.equals("RSA")) { | ||
// This is not very relevant at the moment as the RSA algorithm is only used to encrypt | ||
// user credentials on devices runnning Android 5.0 - 5.1.1 for the KeyStore | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. unnecessary comment, It's still relevant even if it's getting used on a small set of devices. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not necessarily, we only use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. True, you can just say - |
||
KeyFactory keyFactory = null; | ||
try { | ||
keyFactory = KeyFactory.getInstance("RSA"); | ||
} catch (NoSuchAlgorithmException e) { | ||
throw new EncryptionException("There is no Provider for the selected algorithm", e); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. use |
||
} | ||
|
||
if (cryptographicOperation == CryptographicOperation.Encryption) { | ||
X509EncodedKeySpec keySpec = new X509EncodedKeySpec(keyBytes); | ||
return keyFactory.generatePublic(keySpec); | ||
} else { | ||
PKCS8EncodedKeySpec keySpec = new PKCS8EncodedKeySpec(keyBytes); | ||
return keyFactory.generatePrivate(keySpec); | ||
} | ||
} | ||
// This should cause an error | ||
return null; | ||
} | ||
|
||
private static String getCryptographicTransformation(String algorithm) { | ||
if (algorithm.equals("AES")) { | ||
return "AES/GCM/NoPadding"; | ||
} else if (algorithm.equals("RSA")) { | ||
return "RSA/ECB/PKCS1Padding"; | ||
} else { | ||
// This will cause an error | ||
return null; | ||
} | ||
} | ||
|
||
private static Key retrieveKeyFromKeyStore(String keyAlias, CryptographicOperation operation) throws KeyStoreException, UnrecoverableEntryException, NoSuchAlgorithmException { | ||
if (getAndroidKeyStore().containsAlias(keyAlias)) { | ||
KeyStore.Entry keyEntry = getAndroidKeyStore().getEntry(keyAlias, null); | ||
if (keyEntry instanceof KeyStore.PrivateKeyEntry) { | ||
if (operation == CryptographicOperation.Encryption) { | ||
return ((KeyStore.PrivateKeyEntry)keyEntry).getCertificate().getPublicKey(); | ||
} else { | ||
return ((KeyStore.PrivateKeyEntry)keyEntry).getPrivateKey(); | ||
} | ||
} else { | ||
return ((KeyStore.SecretKeyEntry)keyEntry).getSecretKey(); | ||
} | ||
} else { | ||
throw new KeyStoreException("Key not found in KeyStore"); | ||
} | ||
} | ||
|
||
public static String decryptUsingKeyFromKeyStore(String message, String alias) throws EncryptionException { | ||
Key key; | ||
try { | ||
key = retrieveKeyFromKeyStore(alias, CryptographicOperation.Decryption); | ||
} catch (KeyStoreException | UnrecoverableEntryException | NoSuchAlgorithmException e) { | ||
throw new RuntimeException(e); | ||
} | ||
return new SecretKeySpec(keyBytes, "AES"); | ||
return decrypt(key.getAlgorithm(), message, key); | ||
} | ||
|
||
public static String decryptUsingBase64EncodedKey(String algorithm, String message, String key) throws EncryptionException { | ||
Key secret = null; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: |
||
try { | ||
secret = getKey(algorithm, key, CryptographicOperation.Decryption); | ||
} catch (InvalidKeySpecException e) { | ||
throw new EncryptionException("Invalid Key specifications", e); | ||
} | ||
return decrypt(algorithm, message, secret); | ||
} | ||
|
||
/** | ||
|
@@ -76,10 +206,8 @@ private static SecretKey getSecretKeySpec(String key) throws EncryptionException | |
* @param key key that should be used for decryption | ||
* @return Decrypted message for the given AES encrypted message | ||
*/ | ||
public static String decrypt(String message, String key) throws EncryptionException { | ||
final String ENCRYPT_ALGO = "AES/GCM/NoPadding"; | ||
private static String decrypt(String algorithm, String message, Key key) throws EncryptionException { | ||
final int TAG_LENGTH_BIT = 128; | ||
SecretKey secret = getSecretKeySpec(key); | ||
|
||
try { | ||
byte[] messageBytes = Base64.decode(message); | ||
|
@@ -92,16 +220,25 @@ public static String decrypt(String message, String key) throws EncryptionExcept | |
bb.get(cipherText); | ||
|
||
|
||
Cipher cipher = Cipher.getInstance(ENCRYPT_ALGO); | ||
cipher.init(Cipher.DECRYPT_MODE, secret, new GCMParameterSpec(TAG_LENGTH_BIT, iv)); | ||
Cipher cipher = Cipher.getInstance(getCryptographicTransformation(algorithm)); | ||
if (algorithm.equals("AES")) { | ||
cipher.init(Cipher.DECRYPT_MODE, key, new GCMParameterSpec(TAG_LENGTH_BIT, iv)); | ||
} else { | ||
cipher.init(Cipher.DECRYPT_MODE, key); | ||
} | ||
byte[] plainText = cipher.doFinal(cipherText); | ||
return new String(plainText, Charset.forName("UTF-8")); | ||
} catch (NoSuchAlgorithmException | BadPaddingException | NoSuchPaddingException | | ||
IllegalBlockSizeException | InvalidAlgorithmParameterException | InvalidKeyException | Base64DecoderException e) { | ||
IllegalBlockSizeException | InvalidKeyException | Base64DecoderException | | ||
InvalidAlgorithmParameterException e) { | ||
throw new EncryptionException("Error encountered while decrypting the message", e); | ||
} | ||
} | ||
|
||
public static boolean isAndroidKeyStoreSupported() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should be in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about giving it a generic name (like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 99e70fd. The method was renamed to |
||
return Security.getProvider("AndroidKeyStore") != null; | ||
} | ||
|
||
public static class EncryptionException extends Exception { | ||
|
||
public EncryptionException(String message) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
package org.javarosa.core.model; | ||
|
||
import org.commcare.util.EncryptionUtils; | ||
import org.javarosa.core.model.instance.FormInstance; | ||
import org.javarosa.core.model.instance.TreeReference; | ||
import org.javarosa.core.model.util.restorable.Restorable; | ||
|
@@ -16,6 +17,9 @@ | |
import java.io.IOException; | ||
import java.util.Hashtable; | ||
|
||
import static org.commcare.util.EncryptionUtils.USER_CREDENTIALS_KEY_ALIAS; | ||
import static org.commcare.util.EncryptionUtils.isAndroidKeyStoreSupported; | ||
|
||
/** | ||
* Peristable object representing a CommCare mobile user. | ||
* | ||
|
@@ -55,7 +59,7 @@ public User(String name, String passw, String uniqueID) { | |
} | ||
|
||
public User(String name, String passw, String uniqueID, String userType) { | ||
username = name; | ||
setUsername(name); | ||
passwordHash = passw; | ||
uniqueId = uniqueID; | ||
setUserType(userType); | ||
|
@@ -65,7 +69,7 @@ public User(String name, String passw, String uniqueID, String userType) { | |
// fetch the value for the default user and password from the RMS | ||
@Override | ||
public void readExternal(DataInputStream in, PrototypeFactory pf) throws IOException, DeserializationException { | ||
this.username = ExtUtil.readString(in); | ||
setUsername(ExtUtil.readString(in)); | ||
this.passwordHash = ExtUtil.readString(in); | ||
this.recordId = ExtUtil.readInt(in); | ||
this.uniqueId = ExtUtil.nullIfEmpty(ExtUtil.readString(in)); | ||
|
@@ -77,7 +81,7 @@ public void readExternal(DataInputStream in, PrototypeFactory pf) throws IOExcep | |
|
||
@Override | ||
public void writeExternal(DataOutputStream out) throws IOException { | ||
ExtUtil.writeString(out, username); | ||
ExtUtil.writeString(out, getUsername()); | ||
ExtUtil.writeString(out, passwordHash); | ||
ExtUtil.writeNumeric(out, recordId); | ||
ExtUtil.writeString(out, ExtUtil.emptyIfNull(uniqueId)); | ||
|
@@ -88,7 +92,15 @@ public void writeExternal(DataOutputStream out) throws IOException { | |
} | ||
|
||
public String getUsername() { | ||
return username; | ||
if (!isAndroidKeyStoreSupported()) { | ||
return this.username; | ||
} else { | ||
try { | ||
return EncryptionUtils.decryptUsingKeyFromKeyStore(this.username, USER_CREDENTIALS_KEY_ALIAS); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this will cause issues for transitions from old CC version to new CC version. If There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @shubham1g5 I don't think this is the case, I also had this concern and I ran a few tests around it and worked fine. Something to keep in mind here is that we are only encrypting the credentials when bringing them to memory, this shouldn't affect the way we save/retrieve the credentials to/from the DB. During the login process, we use the credentials provided by the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We do store the fields of this class in SQl DB in user db. I am worried that if we load the user class from the previously stored row in DB and call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think to test it you will need to run the apk without these changes, login with a user, install apk with these changes, login (should be a local login without using internet), Go to a place in code that calls getUserName on this class and test the behaviour. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @shubham1g5 I ran the test as suggested and no issues there too, I also did the opposite and didn't have any problems. The code highlighted above stores the parsed user to the database after a sync, again this shouldn't be affected by this change. The areas we should be concerned are (1) saving the user directly from memory (without using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I could explore this, @shubham1g5. My question is, wouldn't wrapping the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @avazirna - The reason I mentioned There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm open to the annotation idea, but a little tenuous on how much of the job it can actually do independently. If we could compose the value with an annotation (IE: You add the annotation and the getters and setters work automagically) and let us reuse code better I think that would be great. If there's any manual code that could get out of sync, I think it's probably adding more confusion / indirection than value. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ctsims @shubham1g5 I ended up splitting the field in |
||
} catch (EncryptionUtils.EncryptionException e) { | ||
throw new RuntimeException("Error encountered while decrypting the Username ", e); | ||
} | ||
} | ||
} | ||
|
||
public String getPasswordHash() { | ||
|
@@ -118,7 +130,15 @@ public void setUserType(String userType) { | |
} | ||
|
||
public void setUsername(String username) { | ||
this.username = username; | ||
if (!isAndroidKeyStoreSupported()) { | ||
this.username = username; | ||
} else { | ||
try { | ||
this.username = EncryptionUtils.encryptUsingKeyFromKeyStore(username, USER_CREDENTIALS_KEY_ALIAS); | ||
} catch (EncryptionUtils.EncryptionException e) { | ||
throw new RuntimeException("Error encountered while encrypting the Username: ", e); | ||
} | ||
} | ||
} | ||
|
||
public void setPassword(String passwordHash) { | ||
|
@@ -164,7 +184,7 @@ public Object getMetaData(String fieldName) { | |
if (META_UID.equals(fieldName)) { | ||
return uniqueId; | ||
} else if(META_USERNAME.equals(fieldName)) { | ||
return username; | ||
return getUsername(); | ||
} else if(META_ID.equals(fieldName)) { | ||
return recordId; | ||
} else if (META_WRAPPED_KEY.equals(fieldName)) { | ||
|
@@ -184,10 +204,27 @@ public String[] getMetaDataFields() { | |
//Don't ever save! | ||
private String cachedPwd; | ||
public void setCachedPwd(String password) { | ||
this.cachedPwd = password; | ||
if (!isAndroidKeyStoreSupported()) { | ||
this.cachedPwd = password; | ||
} else { | ||
try { | ||
this.cachedPwd = EncryptionUtils.encryptUsingKeyFromKeyStore(password, USER_CREDENTIALS_KEY_ALIAS); | ||
} catch (EncryptionUtils.EncryptionException e) { | ||
throw new RuntimeException("Error encountered while encrypting the Password: ", e); | ||
} | ||
} | ||
} | ||
|
||
public String getCachedPwd() { | ||
return this.cachedPwd; | ||
if (!isAndroidKeyStoreSupported()) { | ||
return this.cachedPwd; | ||
} else { | ||
try { | ||
return EncryptionUtils.decryptUsingKeyFromKeyStore(this.cachedPwd, USER_CREDENTIALS_KEY_ALIAS); | ||
} catch (EncryptionUtils.EncryptionException e) { | ||
throw new RuntimeException("Error encountered while decrypting the Password: ", e); | ||
} | ||
} | ||
} | ||
|
||
public String getLastSyncToken() { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Core classes should not contain any platform specific implementation details. You can probably use the platform classes to abstract the keystore name for respective platforms in their respective platform implementations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 5b66cc4. I opted for a
static non-final field
to store the name of the platform keyStore.