Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,20 @@

// 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";
/**
* !!! WARNING !!!
* "RSA/ECB/PKCS1Padding" is cryptographically deprecated due to vulnerabilities
* (e.g. Bleichenbacher padding oracle attacks) and MUST NOT be used for encrypting
* new data or for any general-purpose RSA operations.
*
* This transformation exists solely to DECRYPT pre-existing legacy data that was
* originally encrypted with PKCS#1 v1.5 padding, so that it can be re-encrypted
* using the secure OAEP-based {@link #RSA_TRANSFORMATION}. Once all legacy data has
* been migrated, support for this constant and any code paths that use it should be
* removed.
*/
private static final String LEGACY_PKCS1_RSA_TRANSFORMATION = "RSA/ECB/PKCS1Padding";
Comment thread Dismissed
// https://developer.android.com/reference/javax/crypto/Cipher.html
@SuppressWarnings("SpellCheckingInspection")
private static final String AES_TRANSFORMATION = "AES/GCM/NOPADDING";
Expand All @@ -62,7 +75,7 @@
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;
Comment thread
pmathew92 marked this conversation as resolved.
Outdated

private static final byte FORMAT_MARKER = 0x01;

Expand Down Expand Up @@ -91,6 +104,31 @@
this.storage = storage;
}

/**
* Decrypts data that was encrypted using legacy RSA/PKCS1 padding.
* <p>
* WARNING: This must only be used for decrypting legacy data during migration.
* New code must always use OAEP padding for RSA encryption/decryption.
*
* @param encryptedData The data encrypted with PKCS1 padding
* @param privateKey The private key for decryption
* @return The decrypted data
* @throws NoSuchPaddingException If PKCS1 padding is not available
* @throws NoSuchAlgorithmException If RSA algorithm is not available
* @throws InvalidKeyException If the private key is invalid
* @throws BadPaddingException If the encrypted data has invalid padding
* @throws IllegalBlockSizeException If the encrypted data size is invalid
*/
@NonNull
private static byte[] RSADecryptLegacyPKCS1(@NonNull byte[] encryptedData,
@NonNull PrivateKey privateKey)
throws NoSuchPaddingException, NoSuchAlgorithmException, InvalidKeyException,
BadPaddingException, IllegalBlockSizeException {
Cipher rsaPkcs1Cipher = Cipher.getInstance(LEGACY_PKCS1_RSA_TRANSFORMATION);
Comment thread Dismissed
rsaPkcs1Cipher.init(Cipher.DECRYPT_MODE, privateKey);
return rsaPkcs1Cipher.doFinal(encryptedData);
}

/**
* Attempts to recover the existing RSA Private Key entry or generates a new one as secure as
* this device and Android version allows it if none is found.
Expand Down Expand Up @@ -372,30 +410,106 @@
@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);
try {
return RSADecrypt(encryptedAESBytes);
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing validation of the decrypted AES key length after PKCS1 decryption. The original getAESKey implementation validated that the AES key has the expected length (32 bytes for 256-bit AES) before returning it. Without this validation, a corrupted or malformed legacy key could be migrated and cause issues later. Consider validating that decryptedAESKey.length equals AES_KEY_SIZE / 8 before proceeding with re-encryption.

Suggested change
return RSADecrypt(encryptedAESBytes);
byte[] decryptedAESKey = RSADecrypt(encryptedAESBytes);
// Validate that the decrypted AES key has the expected length (e.g. 32 bytes for 256-bit AES)
if (decryptedAESKey == null || decryptedAESKey.length != AES_KEY_SIZE / 8) {
// Treat this as corrupted key material: clean up and signal an error
deleteRSAKeys();
deleteAESKeys();
throw new CryptoException("The RSA decrypted AES key has an unexpected length.");
}
return decryptedAESKey;

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current code already throws exceptions on decryption failures. If additional validation is desired, it should be addressed in a separate PR to avoid scope creep and maintain backward compatibility

} catch (IncompatibleDeviceException e) {
String fullMessage = e.toString();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This entire code block can be refactored to make it more readable

Throwable cause = e.getCause();
while (cause != null) {
fullMessage += "\n" + cause.toString();
cause = cause.getCause();
}

if (fullMessage.contains("Incompatible padding mode") ||
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

String comparison for checking the old padding doesn't looks like a good method as it might return inconsistent behaviour. Would suggest to do some other approach to differentiate

fullMessage.contains("INCOMPATIBLE_PADDING_MODE")) {
try {
KeyStore keyStore = KeyStore.getInstance(ANDROID_KEY_STORE);
keyStore.load(null);

// Get the RSA key from KeyStore (could be at KEY_ALIAS or OLD_KEY_ALIAS)
KeyStore.PrivateKeyEntry rsaKey = null;
String keyAliasUsed = null;

if (keyStore.containsAlias(KEY_ALIAS)) {
rsaKey = getKeyEntryCompat(keyStore, KEY_ALIAS);
keyAliasUsed = KEY_ALIAS;
} else if (keyStore.containsAlias(OLD_KEY_ALIAS)) {
rsaKey = getKeyEntryCompat(keyStore, OLD_KEY_ALIAS);
keyAliasUsed = OLD_KEY_ALIAS;
}

if (rsaKey != null && keyAliasUsed != null) {
// WARNING: Using PKCS1 padding here is intentional and ONLY for decrypting legacy data.
// This cipher must NEVER be used for encryption or for any new data; always use OAEP instead.
byte[] decryptedAESKey = RSADecryptLegacyPKCS1(
encryptedAESBytes,
rsaKey.getPrivateKey()
);
deleteRSAKeys();

// Re-encrypt AES key with NEW OAEP RSA key (4096-bit)
byte[] encryptedAESWithOAEP = RSAEncrypt(decryptedAESKey);
String newEncodedEncryptedAES = new String(
Base64.encode(encryptedAESWithOAEP, Base64.DEFAULT),
StandardCharsets.UTF_8
);
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing validation of the decrypted AES key length after PKCS1 decryption from OLD_KEY_ALIAS. The original getAESKey implementation validated that the AES key has the expected length (32 bytes for 256-bit AES) before returning it. Without this validation, a corrupted or malformed legacy key could be migrated. Consider validating that decryptedAESKey.length equals AES_KEY_SIZE / 8 before proceeding with re-encryption.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While adding AES key length validation would be good defensive programming, it's beyond the scope of this PR, which focuses specifically on migrating from PKCS1 to OAEP padding for the security fix.

The existing error handling already addresses corrupted keys: if the decrypted AES key is malformed, subsequent encryption operations will fail and trigger the cleanup path that deletes corrupted keys and regenerates them. This behavior is consistent with the pre-migration code.

storage.store(KEY_ALIAS, newEncodedEncryptedAES);

return decryptedAESKey;
}
} catch (CryptoException | KeyStoreException | CertificateException |
IOException | NoSuchAlgorithmException | UnrecoverableEntryException |
NoSuchPaddingException | InvalidKeyException |
IllegalBlockSizeException | BadPaddingException ex) {
Log.e(TAG, "Could not migrate. A new key will be generated.", ex);
deleteRSAKeys();
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Catching a generic Exception here is overly broad and could hide unexpected issues. The catch block swallows all exceptions (including potential runtime exceptions) and simply generates a new key, which could mask serious bugs. Consider catching only the specific checked exceptions that can be thrown by the operations inside the try block (NoSuchAlgorithmException, NoSuchPaddingException, InvalidKeyException, IllegalBlockSizeException, BadPaddingException, CryptoException, KeyStoreException, CertificateException, IOException, UnrecoverableEntryException) to maintain better error visibility.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This exception handling is in the OLD_KEY_ALIAS migration path (lines 464-467), which is unrelated to the PKCS1→OAEP security migration that this PR addresses. The generic Exception catch block existed in the original code and was not modified by this PR.

deleteAESKeys();
}
} else {
throw e;
}
} catch (CryptoException e) {
// RSA decryption failed - the encrypted AES key is corrupted or the RSA key is invalid
// Delete keys and regenerate them
Log.w(TAG, "RSA decryption failed with CryptoException. Keys may be corrupted. Will regenerate.", e);
deleteRSAKeys();
deleteAESKeys();
}
}
Comment on lines +550 to 563
Copy link

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The migration logic attempts PKCS1 decryption whenever OAEP decryption fails with any CryptoException. This is overly broad and could mask genuine errors unrelated to padding. For example, if the RSA key is corrupted or if there's a legitimate cryptographic failure, the code will still attempt PKCS1 migration, potentially causing confusion.

Consider adding specific detection for padding-related errors by examining the exception message or type. For instance, check if the exception or its causes contain messages like "Incompatible padding mode", "padding", or specific error codes. This would make the migration logic more precise and reduce unnecessary PKCS1 attempts for unrelated errors.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Android Keystore ecosystem is very fragmented, and error messages/codes for padding mismatches are inconsistent across manufacturers (e.g. Samsung vs Pixel) and Android versions. "Incompatible padding" often shows up as generic "internal error -1000" or other localized strings.

If we filter by specific error messages, we risk wiping valid user data on devices that throw non-standard errors.

To be safe, we try migration on any failure. If migration fails too, we re-throw the original exception, so no genuine errors are actually swallowed. This prioritizes data recovery over skipping a quick check.

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);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to handle both the OLD_KEY_ALIAS and KEY_ALIAS in separate block here ? Can't a single block like in the original work ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two blocks handle different migration scenarios and must remain separate:

KEY_ALIAS block: Handles the current storage location. When OAEP decryption fails, it attempts PKCS1 decryption as a migration path (same storage key, different RSA padding).

OLD_KEY_ALIAS block: Handles a legacy storage alias from much older SDK versions. This is an alias migration (moving data from old location to new location), not just a padding migration.

The original code also had this separation - it just wasn't as clearly structured. The two scenarios are:

Padding migration: KEY_ALIAS with PKCS1 → KEY_ALIAS with OAEP
Alias migration: OLD_KEY_ALIAS → KEY_ALIAS

if (!TextUtils.isEmpty(encodedOldAES)) {
try {
byte[] encryptedOldAESBytes = Base64.decode(encodedOldAES, Base64.DEFAULT);
KeyStore.PrivateKeyEntry rsaKeyEntry = getRSAKeyEntry();
// WARNING: Using PKCS1 padding here is intentional and ONLY for decrypting legacy data.
// This cipher must NEVER be used for encryption or for any new data; always use OAEP padding instead.
byte[] decryptedAESKey = RSADecryptLegacyPKCS1(
encryptedOldAESBytes,
rsaKeyEntry.getPrivateKey()
);

byte[] encryptedAESWithOAEP = RSAEncrypt(decryptedAESKey);
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Catching a generic Exception here is overly broad and could mask programming errors or unexpected runtime exceptions. The outer catch for the key generation logic should only catch the specific exceptions that can be thrown. Since this is a new key generation path, catching Exception and wrapping it in CryptoException reduces visibility into actual failures. Consider catching only IncompatibleDeviceException and CryptoException that might propagate from RSAEncrypt.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The generic Exception catch block existed in the original code and was not modified by this PR. While catching specific exceptions would improve code quality, changing this would expand the scope beyond the security fix.

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) {
Copy link

Copilot AI Aug 19, 2025

Choose a reason for hiding this comment

The 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
} catch (Exception e) {
} catch (NoSuchAlgorithmException | NoSuchPaddingException | InvalidKeyException | BadPaddingException | IllegalBlockSizeException | KeyStoreException | UnrecoverableEntryException e) {

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

made new changes on 17 dec will check on copilot review on it.

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:
Expand All @@ -407,6 +521,9 @@
*/
Log.e(TAG, "Error while creating the AES key.", e);
throw new IncompatibleDeviceException(e);
} catch (Exception e) {
Copy link

Copilot AI Aug 19, 2025

Choose a reason for hiding this comment

The 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
} catch (Exception e) {
} catch (InvalidKeyException
| NoSuchPaddingException
| IllegalBlockSizeException
| BadPaddingException
| KeyStoreException
| UnrecoverableEntryException
| CertificateException
| IOException
| ProviderException e) {

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

made new changes on 17 dec will check on copilot review on it.

Log.e(TAG, "Unexpected error while creating the new AES key.", e);
throw new CryptoException("Unexpected error while creating the new AES key.", e);
}
}

Expand Down
Loading
Loading