-
Notifications
You must be signed in to change notification settings - Fork 168
RSA encryption padding change from PKCS1Padding to OAEPWithSHA1And… #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
Changes from 1 commit
d9bce21
e641aa1
967376e
715bcc8
6a44236
0cc81ea
db797c3
399ddb0
59357a1
f992aea
859113f
a401e51
f79f8f1
bc6b467
f65ad52
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,7 +53,8 @@ | |||||||||||||||||||||
|
|
||||||||||||||||||||||
| // 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"; | ||||||||||||||||||||||
| private static final String OLD_PKCS1_RSA_TRANSFORMATION = "RSA/ECB/PKCS1Padding"; | ||||||||||||||||||||||
| // https://developer.android.com/reference/javax/crypto/Cipher.html | ||||||||||||||||||||||
| @SuppressWarnings("SpellCheckingInspection") | ||||||||||||||||||||||
| private static final String AES_TRANSFORMATION = "AES/GCM/NOPADDING"; | ||||||||||||||||||||||
|
|
@@ -62,7 +63,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; | ||||||||||||||||||||||
|
pmathew92 marked this conversation as resolved.
Outdated
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| private static final byte FORMAT_MARKER = 0x01; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
@@ -372,30 +373,100 @@ | |||||||||||||||||||||
| @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); | ||||||||||||||||||||||
|
||||||||||||||||||||||
| 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; |
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.
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
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 entire code block can be refactored to make it more readable
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.
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
Copilot
AI
Dec 19, 2025
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.
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.
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.
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.
Copilot
AI
Dec 19, 2025
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.
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.
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 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.
Copilot
AI
Jan 23, 2026
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.
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.
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.
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.
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.
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 ?
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.
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
Copilot
AI
Aug 19, 2025
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.
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.
| } catch (Exception e) { | |
| } catch (NoSuchAlgorithmException | NoSuchPaddingException | InvalidKeyException | BadPaddingException | IllegalBlockSizeException | KeyStoreException | UnrecoverableEntryException e) { |
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.
made new changes on 17 dec will check on copilot review on it.
Copilot
AI
Aug 19, 2025
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.
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.
| } catch (Exception e) { | |
| } catch (InvalidKeyException | |
| | NoSuchPaddingException | |
| | IllegalBlockSizeException | |
| | BadPaddingException | |
| | KeyStoreException | |
| | UnrecoverableEntryException | |
| | CertificateException | |
| | IOException | |
| | ProviderException e) { |
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.
made new changes on 17 dec will check on copilot review on it.
Uh oh!
There was an error while loading. Please reload this page.