-
Notifications
You must be signed in to change notification settings - Fork 158
RSA encryption padding change from PKCS1Padding to OAEPWithSHA-256And… #834
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
base: main
Are you sure you want to change the base?
Changes from all commits
97d5f2a
fb25660
967e166
212b04c
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 | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -53,16 +53,17 @@ | |||||||||||||||||||||
|
||||||||||||||||||||||
// Transformations available since API 18 | ||||||||||||||||||||||
// https://developer.android.com/training/articles/keystore.html#SupportedCiphers | ||||||||||||||||||||||
private static final String RSA_TRANSFORMATION = "RSA/ECB/PKCS1Padding"; | ||||||||||||||||||||||
private static final String RSA_TRANSFORMATION = "RSA/ECB/OAEPWithSHA-256AndMGF1Padding"; | ||||||||||||||||||||||
// https://developer.android.com/reference/javax/crypto/Cipher.html | ||||||||||||||||||||||
@SuppressWarnings("SpellCheckingInspection") | ||||||||||||||||||||||
private static final String AES_TRANSFORMATION = "AES/GCM/NOPADDING"; | ||||||||||||||||||||||
private static final String OLD_PKCS1_RSA_TRANSFORMATION = "RSA/ECB/PKCS1Padding"; | ||||||||||||||||||||||
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. Add a comment to state why |
||||||||||||||||||||||
|
||||||||||||||||||||||
private static final String ANDROID_KEY_STORE = "AndroidKeyStore"; | ||||||||||||||||||||||
private static final String ALGORITHM_RSA = "RSA"; | ||||||||||||||||||||||
private static final String ALGORITHM_AES = "AES"; | ||||||||||||||||||||||
private static final int AES_KEY_SIZE = 256; | ||||||||||||||||||||||
private static final int RSA_KEY_SIZE = 2048; | ||||||||||||||||||||||
private static final int RSA_KEY_SIZE = 4096; | ||||||||||||||||||||||
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. Increasing RSA key size from 2048 to 4096 bits will significantly impact performance for key generation and encryption/decryption operations. Consider whether this change is necessary for security requirements or if 2048 bits is sufficient for the application's threat model.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback 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. Why do we need to increase the size ? Was this suggested by the security team ? |
||||||||||||||||||||||
|
||||||||||||||||||||||
private final String OLD_KEY_ALIAS; | ||||||||||||||||||||||
private final String OLD_KEY_IV_ALIAS; | ||||||||||||||||||||||
|
@@ -124,7 +125,8 @@ | |||||||||||||||||||||
.setCertificateNotBefore(start.getTime()) | ||||||||||||||||||||||
.setCertificateNotAfter(end.getTime()) | ||||||||||||||||||||||
.setKeySize(RSA_KEY_SIZE) | ||||||||||||||||||||||
.setEncryptionPaddings(KeyProperties.ENCRYPTION_PADDING_RSA_PKCS1) | ||||||||||||||||||||||
.setEncryptionPaddings(KeyProperties.ENCRYPTION_PADDING_RSA_OAEP) | ||||||||||||||||||||||
.setDigests(KeyProperties.DIGEST_SHA256, KeyProperties.DIGEST_SHA1) | ||||||||||||||||||||||
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. Hope this setDigest addition was also reviewed by security ? |
||||||||||||||||||||||
.setBlockModes(KeyProperties.BLOCK_MODE_ECB) | ||||||||||||||||||||||
.build(); | ||||||||||||||||||||||
} else { | ||||||||||||||||||||||
|
@@ -355,6 +357,7 @@ | |||||||||||||||||||||
|
||||||||||||||||||||||
/** | ||||||||||||||||||||||
* Attempts to recover the existing AES Key or generates a new one if none is found. | ||||||||||||||||||||||
* Handles migration from PKCS1Padding-encrypted AES keys to OAEP-encrypted ones. | ||||||||||||||||||||||
* | ||||||||||||||||||||||
* @return a valid AES Key bytes | ||||||||||||||||||||||
* @throws IncompatibleDeviceException in the event the device can't understand the cryptographic settings required | ||||||||||||||||||||||
|
@@ -363,41 +366,45 @@ | |||||||||||||||||||||
@VisibleForTesting | ||||||||||||||||||||||
byte[] getAESKey() throws IncompatibleDeviceException, CryptoException { | ||||||||||||||||||||||
String encodedEncryptedAES = storage.retrieveString(KEY_ALIAS); | ||||||||||||||||||||||
if (TextUtils.isEmpty(encodedEncryptedAES)) { | ||||||||||||||||||||||
encodedEncryptedAES = storage.retrieveString(OLD_KEY_ALIAS); | ||||||||||||||||||||||
if (!TextUtils.isEmpty(encodedEncryptedAES)) { | ||||||||||||||||||||||
byte[] encryptedAESBytes = Base64.decode(encodedEncryptedAES, Base64.DEFAULT); | ||||||||||||||||||||||
return RSADecrypt(encryptedAESBytes); | ||||||||||||||||||||||
} | ||||||||||||||||||||||
if (encodedEncryptedAES != null) { | ||||||||||||||||||||||
//Return existing key | ||||||||||||||||||||||
byte[] encryptedAES = Base64.decode(encodedEncryptedAES, Base64.DEFAULT); | ||||||||||||||||||||||
byte[] existingAES = RSADecrypt(encryptedAES); | ||||||||||||||||||||||
final int aesExpectedLengthInBytes = AES_KEY_SIZE / 8; | ||||||||||||||||||||||
//Prevent returning an 'Empty key' (invalid/corrupted) that was mistakenly saved | ||||||||||||||||||||||
if (existingAES != null && existingAES.length == aesExpectedLengthInBytes) { | ||||||||||||||||||||||
//Key exists and has the right size | ||||||||||||||||||||||
return existingAES; | ||||||||||||||||||||||
String encodedOldAES = storage.retrieveString(OLD_KEY_ALIAS); | ||||||||||||||||||||||
if (!TextUtils.isEmpty(encodedOldAES)) { | ||||||||||||||||||||||
try { | ||||||||||||||||||||||
byte[] encryptedOldAESBytes = Base64.decode(encodedOldAES, Base64.DEFAULT); | ||||||||||||||||||||||
KeyStore.PrivateKeyEntry rsaKeyEntry = getRSAKeyEntry(); | ||||||||||||||||||||||
Cipher rsaPkcs1Cipher = Cipher.getInstance(OLD_PKCS1_RSA_TRANSFORMATION); | ||||||||||||||||||||||
|
||||||||||||||||||||||
rsaPkcs1Cipher.init(Cipher.DECRYPT_MODE, rsaKeyEntry.getPrivateKey()); | ||||||||||||||||||||||
byte[] decryptedAESKey = rsaPkcs1Cipher.doFinal(encryptedOldAESBytes); | ||||||||||||||||||||||
|
||||||||||||||||||||||
byte[] encryptedAESWithOAEP = RSAEncrypt(decryptedAESKey); | ||||||||||||||||||||||
String newEncodedEncryptedAES = new String(Base64.encode(encryptedAESWithOAEP, Base64.DEFAULT), StandardCharsets.UTF_8); | ||||||||||||||||||||||
storage.store(KEY_ALIAS, newEncodedEncryptedAES); | ||||||||||||||||||||||
storage.remove(OLD_KEY_ALIAS); | ||||||||||||||||||||||
return decryptedAESKey; | ||||||||||||||||||||||
} catch (Exception 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. Catching generic Exception is too broad and may hide specific error conditions. Consider catching specific exceptions like BadPaddingException, IllegalBlockSizeException, NoSuchAlgorithmException, etc., to handle different failure scenarios appropriately.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||||||||||||||||||
Log.e(TAG, "Could not migrate the legacy AES key. A new key will be generated.", e); | ||||||||||||||||||||||
deleteAESKeys(); | ||||||||||||||||||||||
} | ||||||||||||||||||||||
} | ||||||||||||||||||||||
//Key doesn't exist. Generate new AES | ||||||||||||||||||||||
|
||||||||||||||||||||||
try { | ||||||||||||||||||||||
KeyGenerator keyGen = KeyGenerator.getInstance(ALGORITHM_AES); | ||||||||||||||||||||||
keyGen.init(AES_KEY_SIZE); | ||||||||||||||||||||||
byte[] aes = keyGen.generateKey().getEncoded(); | ||||||||||||||||||||||
//Save encrypted encoded version | ||||||||||||||||||||||
byte[] encryptedAES = RSAEncrypt(aes); | ||||||||||||||||||||||
String encodedEncryptedAESText = new String(Base64.encode(encryptedAES, Base64.DEFAULT), StandardCharsets.UTF_8); | ||||||||||||||||||||||
storage.store(KEY_ALIAS, encodedEncryptedAESText); | ||||||||||||||||||||||
return aes; | ||||||||||||||||||||||
byte[] decryptedAESKey = keyGen.generateKey().getEncoded(); | ||||||||||||||||||||||
|
||||||||||||||||||||||
byte[] encryptedNewAES = RSAEncrypt(decryptedAESKey); | ||||||||||||||||||||||
String encodedEncryptedNewAESText = new String(Base64.encode(encryptedNewAES, Base64.DEFAULT), StandardCharsets.UTF_8); | ||||||||||||||||||||||
storage.store(KEY_ALIAS, encodedEncryptedNewAESText); | ||||||||||||||||||||||
return decryptedAESKey; | ||||||||||||||||||||||
} catch (NoSuchAlgorithmException e) { | ||||||||||||||||||||||
/* | ||||||||||||||||||||||
* This exceptions are safe to be ignored: | ||||||||||||||||||||||
* | ||||||||||||||||||||||
* - NoSuchAlgorithmException: | ||||||||||||||||||||||
* Thrown if the Algorithm implementation is not available. AES was introduced in API 1 | ||||||||||||||||||||||
* | ||||||||||||||||||||||
* Read more in https://developer.android.com/reference/javax/crypto/KeyGenerator | ||||||||||||||||||||||
*/ | ||||||||||||||||||||||
Log.e(TAG, "Error while creating the AES key.", e); | ||||||||||||||||||||||
Log.e(TAG, "Error while creating the new AES key.", e); | ||||||||||||||||||||||
throw new IncompatibleDeviceException(e); | ||||||||||||||||||||||
} catch (Exception 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. Catching generic Exception is too broad. This catch block should handle specific exceptions that can occur during RSA encryption or storage operations, such as CryptoException or IncompatibleDeviceException.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||||||||||||||||||
Log.e(TAG, "Unexpected error while creating the new AES key.", e); | ||||||||||||||||||||||
throw new CryptoException("Unexpected error while creating the new AES key.", e); | ||||||||||||||||||||||
} | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
|
Check failure
Code scanning / CodeQL
Use of RSA algorithm without OAEP High
Copilot Autofix
AI 9 days ago
To fix the problem, update the migration code to use OAEP padding for RSA decryption, if possible. This means replacing the use of
"RSA/ECB/PKCS1Padding"
with"RSA/ECB/OAEPWithSHA-256AndMGF1Padding"
(or another OAEP variant compatible with the legacy data). However, if the legacy data was encrypted with PKCS#1 padding, it cannot be decrypted with OAEP; in that case, the only safe option is to ensure the migration code is not exposed to untrusted input and is removed after migration. If you must keep the migration code, add comments to clarify its purpose and restrict its use. If you can re-encrypt legacy data with OAEP, do so and remove PKCS#1 padding support.Best fix:
Required changes:
OLD_PKCS1_RSA_TRANSFORMATION
explaining its legacy/migration-only purpose.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.
that was the PR which we are fixing to use OAEP padding.
Uh oh!
There was an error while loading. Please reload this page.
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.
this usage is intentional and strictly limited to a one-time data migration path for existing users.
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.
Lets keep a backlog ticket to remove this once all users are migrated to the 3.10.0 or later