Skip to content

Remove insecure AES/CBC/PKCS5Padding encryption #1028

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.

Sign up for GitHub

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

Closed
franco-zalamena-iterable wants to merge 1 commit into from
Closed

Remove insecure AES/CBC/PKCS5Padding encryption #1028

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
77 changes: 27 additions & 50 deletions iterableapi/src/main/java/com/iterable/iterableapi/IterableDataEncryptor.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,9 @@
companion object {
private const val TAG = "IterableDataEncryptor"
private const val ANDROID_KEYSTORE = "AndroidKeyStore"
private const val TRANSFORMATION_MODERN = "AES/GCM/NoPadding"
private const val TRANSFORMATION_LEGACY = "AES/CBC/PKCS5Padding"
private const val TRANSFORMATION_GCM = "AES/GCM/NoPadding"
@Deprecated("CBC padding is vulnerable to Padding Oracle Attacks. Used only for one-time migration of legacy data.")
private const val TRANSFORMATION_LEGACY_CBC = "AES/CBC/PKCS5Padding"
private const val ITERABLE_KEY_ALIAS = "iterable_encryption_key"
private const val GCM_TAG_LENGTH = 128
private const val GCM_IV_LENGTH = 12
Expand Down Expand Up @@ -83,8 +84,8 @@
ITERABLE_KEY_ALIAS,
KeyProperties.PURPOSE_ENCRYPT or KeyProperties.PURPOSE_DECRYPT
)
.setBlockModes(KeyProperties.BLOCK_MODE_GCM, KeyProperties.BLOCK_MODE_CBC)
.setEncryptionPaddings(KeyProperties.ENCRYPTION_PADDING_NONE, KeyProperties.ENCRYPTION_PADDING_PKCS7)
.setBlockModes(KeyProperties.BLOCK_MODE_GCM)
.setEncryptionPaddings(KeyProperties.ENCRYPTION_PADDING_NONE)
.build()

keyGenerator.init(keySpec)
Expand Down Expand Up @@ -127,15 +128,11 @@

try {
val data = value.toByteArray(Charsets.UTF_8)
val encryptedData = if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.KITKAT) {
encryptModern(data)
} else {
encryptLegacy(data)
}
val encryptedData = encryptGcm(data)

// Combine isModern flag, IV length, IV, and encrypted data
val combined = ByteArray(1 + 1 + encryptedData.iv.size + encryptedData.data.size)
combined[0] = if (encryptedData.isModernEncryption) 1 else 0
combined[0] = 1 // Always GCM
combined[1] = encryptedData.iv.size.toByte() // Store IV length
System.arraycopy(encryptedData.iv, 0, combined, 2, encryptedData.iv.size)
System.arraycopy(encryptedData.data, 0, combined, 2 + encryptedData.iv.size, encryptedData.data.size)
Expand All @@ -160,17 +157,15 @@
val encrypted = combined.copyOfRange(2 + ivLength, combined.size)

val encryptedData = EncryptedData(encrypted, iv, isModern)

// If it's modern encryption and we're on an old device, fail fast
if (isModern && Build.VERSION.SDK_INT < Build.VERSION_CODES.KITKAT) {
throw DecryptionException("Modern encryption cannot be decrypted on legacy devices")
}

// Use the appropriate decryption method
val decrypted = if (isModern) {
decryptModern(encryptedData)
decryptGcm(encryptedData)
} else {
decryptLegacy(encryptedData)
// Legacy CBC data detected: decrypt with CBC as a one-time migration.
// The caller will re-encrypt with GCM on the next save.
IterableLogger.w(TAG, "Migrating legacy CBC-encrypted data to GCM")
@Suppress("DEPRECATION")
decryptLegacyCbc(encryptedData)
}

return String(decrypted, Charsets.UTF_8)
Expand All @@ -183,54 +178,36 @@
}
}

@TargetApi(Build.VERSION_CODES.KITKAT)
private fun encryptModern(data: ByteArray): EncryptedData {
if (Build.VERSION.SDK_INT < Build.VERSION_CODES.KITKAT) {
return encryptLegacy(data)
}

val cipher = Cipher.getInstance(TRANSFORMATION_MODERN)
private fun encryptGcm(data: ByteArray): EncryptedData {
val cipher = Cipher.getInstance(TRANSFORMATION_GCM)
cipher.init(Cipher.ENCRYPT_MODE, getKey())
val iv = cipher.iv
val encrypted = cipher.doFinal(data)
return EncryptedData(encrypted, iv, true)
}

private fun encryptLegacy(data: ByteArray): EncryptedData {
val cipher = Cipher.getInstance(TRANSFORMATION_LEGACY)
val iv = generateIV(isModern = false)
val spec = IvParameterSpec(iv)
cipher.init(Cipher.ENCRYPT_MODE, getKey(), spec)
val encrypted = cipher.doFinal(data)
return EncryptedData(encrypted, iv, false)
}

@TargetApi(Build.VERSION_CODES.KITKAT)
private fun decryptModern(encryptedData: EncryptedData): ByteArray {
if (Build.VERSION.SDK_INT < Build.VERSION_CODES.KITKAT) {
throw DecryptionException("Cannot decrypt modern encryption on legacy device")
}

val cipher = Cipher.getInstance(TRANSFORMATION_MODERN)
private fun decryptGcm(encryptedData: EncryptedData): ByteArray {
val cipher = Cipher.getInstance(TRANSFORMATION_GCM)
val spec = GCMParameterSpec(GCM_TAG_LENGTH, encryptedData.iv)
cipher.init(Cipher.DECRYPT_MODE, getKey(), spec)
return cipher.doFinal(encryptedData.data)
}

private fun decryptLegacy(encryptedData: EncryptedData): ByteArray {
val cipher = Cipher.getInstance(TRANSFORMATION_LEGACY)
/**
* Decrypts data that was previously encrypted with AES/CBC/PKCS5Padding.
* This method exists solely for backward compatibility to support one-time
* migration of legacy encrypted data to AES/GCM/NoPadding. It is not used
* for new encryption operations.
*/
@Deprecated("Used only for one-time migration of legacy CBC data to GCM.")
private fun decryptLegacyCbc(encryptedData: EncryptedData): ByteArray {
@Suppress("DEPRECATION")
val cipher = Cipher.getInstance(TRANSFORMATION_LEGACY_CBC)

Check failure

Code scanning / CodeQL

Use of a broken or risky cryptographic algorithm High

Cryptographic algorithm
AES/CBC/PKCS5Padding
Loading
is insecure. CBC mode with PKCS#5 or PKCS#7 padding is vulnerable to padding oracle attacks. Consider using GCM instead.
val spec = IvParameterSpec(encryptedData.iv)
cipher.init(Cipher.DECRYPT_MODE, getKey(), spec)
return cipher.doFinal(encryptedData.data)
}

private fun generateIV(isModern: Boolean = false): ByteArray {
val length = if (isModern) GCM_IV_LENGTH else CBC_IV_LENGTH
val iv = ByteArray(length)
SecureRandom().nextBytes(iv)
return iv
}

data class EncryptedData(
val data: ByteArray,
val iv: ByteArray,
Expand Down
143 changes: 10 additions & 133 deletions iterableapi/src/test/java/com/iterable/iterableapi/IterableDataEncryptorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,63 +303,17 @@ public void testDecryptionAfterKeyLoss() {
}

@Test
public void testEncryptionAcrossApiLevels() {
String testData = "test data for cross-version compatibility";

// Test API 16 (Legacy)
setFinalStatic(Build.VERSION.class, "SDK_INT", Build.VERSION_CODES.JELLY_BEAN);
String encryptedOnApi16 = encryptor.encrypt(testData);

// Test API 18 (Legacy)
setFinalStatic(Build.VERSION.class, "SDK_INT", Build.VERSION_CODES.JELLY_BEAN_MR2);
String encryptedOnApi18 = encryptor.encrypt(testData);
assertEquals("Legacy decryption should work on API 18", testData, encryptor.decrypt(encryptedOnApi16));

// Test API 19 (Modern - First version with GCM support)
setFinalStatic(Build.VERSION.class, "SDK_INT", Build.VERSION_CODES.KITKAT);
String encryptedOnApi19 = encryptor.encrypt(testData);
assertEquals("Should decrypt legacy data on API 19", testData, encryptor.decrypt(encryptedOnApi16));
assertEquals("Should decrypt legacy data on API 19", testData, encryptor.decrypt(encryptedOnApi18));

// Test API 23 (Modern with KeyStore)
setFinalStatic(Build.VERSION.class, "SDK_INT", Build.VERSION_CODES.M);
String encryptedOnApi23 = encryptor.encrypt(testData);
assertEquals("Should decrypt legacy data on API 23", testData, encryptor.decrypt(encryptedOnApi16));
assertEquals("Should decrypt API 19 data on API 23", testData, encryptor.decrypt(encryptedOnApi19));

// Test that modern encryption fails on legacy devices
setFinalStatic(Build.VERSION.class, "SDK_INT", Build.VERSION_CODES.JELLY_BEAN);
try {
encryptor.decrypt(encryptedOnApi19);
fail("Should not be able to decrypt modern encryption on legacy device");
} catch (Exception e) {
assertTrue("Should be DecryptionException", e instanceof IterableDataEncryptor.DecryptionException);
assertEquals("Should have correct error message", "Modern encryption cannot be decrypted on legacy devices", e.getMessage());
}
try {
encryptor.decrypt(encryptedOnApi23);
fail("Should not be able to decrypt modern encryption on legacy device");
} catch (Exception e) {
assertTrue("Should be DecryptionException", e instanceof IterableDataEncryptor.DecryptionException);
assertEquals("Should have correct error message", "Modern encryption cannot be decrypted on legacy devices", e.getMessage());
}
}

@Test
public void testEncryptionMethodFlag() {
public void testAlwaysUsesGcmEncryption() {
String testData = "test data for encryption method verification";

// Test legacy encryption flag (API 16)
setFinalStatic(Build.VERSION.class, "SDK_INT", Build.VERSION_CODES.JELLY_BEAN);
String legacyEncrypted = encryptor.encrypt(testData);
byte[] legacyBytes = Base64.decode(legacyEncrypted, Base64.NO_WRAP);
assertEquals("Legacy encryption should have flag 0", 0, legacyBytes[0]);

// Test modern encryption flag (API 19)
setFinalStatic(Build.VERSION.class, "SDK_INT", Build.VERSION_CODES.KITKAT);
String modernEncrypted = encryptor.encrypt(testData);
byte[] modernBytes = Base64.decode(modernEncrypted, Base64.NO_WRAP);
assertEquals("Modern encryption should have flag 1", 1, modernBytes[0]);
// All encryption should use GCM (flag = 1) regardless of API level
String encrypted = encryptor.encrypt(testData);
byte[] encryptedBytes = Base64.decode(encrypted, Base64.NO_WRAP);
assertEquals("Encryption should always use GCM (flag 1)", 1, encryptedBytes[0]);

// Verify decrypt works
String decrypted = encryptor.decrypt(encrypted);
assertEquals("GCM decryption should work", testData, decrypted);
}

@Test
Expand Down Expand Up @@ -400,83 +354,6 @@ public void testDecryptManipulatedIV() {
}
}

@Test
public void testDecryptManipulatedVersionFlag() {
// Test on API 16 device
setFinalStatic(Build.VERSION.class, "SDK_INT", Build.VERSION_CODES.JELLY_BEAN);

String testData = "test data";
String encrypted = encryptor.encrypt(testData);
byte[] bytes = Base64.decode(encrypted, Base64.NO_WRAP);

// Change version flag from legacy (0) to modern (1)
bytes[0] = 1;
String manipulated = Base64.encodeToString(bytes, Base64.NO_WRAP);

try {
encryptor.decrypt(manipulated);
fail("Should throw exception for manipulated version flag");
} catch (Exception e) {
assertTrue("Should be DecryptionException", e instanceof IterableDataEncryptor.DecryptionException);
assertEquals("Modern encryption cannot be decrypted on legacy devices", e.getMessage());
}
}

@Test
public void testLegacyEncryptionAndDecryption() {
// Set to API 16 (Legacy)
setFinalStatic(Build.VERSION.class, "SDK_INT", Build.VERSION_CODES.JELLY_BEAN);

String testData = "test data for legacy encryption";
String encrypted = encryptor.encrypt(testData);
String decrypted = encryptor.decrypt(encrypted);

assertEquals("Legacy encryption/decryption should work on API 16", testData, decrypted);

// Verify it's using legacy encryption
byte[] encryptedBytes = Base64.decode(encrypted, Base64.NO_WRAP);
assertEquals("Should use legacy encryption flag", 0, encryptedBytes[0]);

// Test on API 18
setFinalStatic(Build.VERSION.class, "SDK_INT", Build.VERSION_CODES.JELLY_BEAN_MR2);
String decryptedOnApi18 = encryptor.decrypt(encrypted);
assertEquals("Legacy data should be decryptable on API 18", testData, decryptedOnApi18);

String encryptedOnApi18 = encryptor.encrypt(testData);
String decryptedFromApi18 = encryptor.decrypt(encryptedOnApi18);
assertEquals("API 18 encryption/decryption should work", testData, decryptedFromApi18);

// Verify API 18 also uses legacy encryption
byte[] api18EncryptedBytes = Base64.decode(encryptedOnApi18, Base64.NO_WRAP);
assertEquals("Should use legacy encryption flag on API 18", 0, api18EncryptedBytes[0]);
}

@Test
public void testModernEncryptionAndDecryption() {
String testData = "test data for modern encryption";

// Test on API 19 (First modern version)
setFinalStatic(Build.VERSION.class, "SDK_INT", Build.VERSION_CODES.KITKAT);
String encryptedOnApi19 = encryptor.encrypt(testData);
String decryptedOnApi19 = encryptor.decrypt(encryptedOnApi19);
assertEquals("Modern encryption should work on API 19", testData, decryptedOnApi19);

byte[] api19EncryptedBytes = Base64.decode(encryptedOnApi19, Base64.NO_WRAP);
assertEquals("Should use modern encryption flag on API 19", 1, api19EncryptedBytes[0]);

// Test on API 23
setFinalStatic(Build.VERSION.class, "SDK_INT", Build.VERSION_CODES.M);
String decryptedOnApi23 = encryptor.decrypt(encryptedOnApi19);
assertEquals("API 19 data should be decryptable on API 23", testData, decryptedOnApi23);

String encryptedOnApi23 = encryptor.encrypt(testData);
String decryptedFromApi23 = encryptor.decrypt(encryptedOnApi23);
assertEquals("API 23 encryption/decryption should work", testData, decryptedFromApi23);

byte[] api23EncryptedBytes = Base64.decode(encryptedOnApi23, Base64.NO_WRAP);
assertEquals("Should use modern encryption flag on API 23", 1, api23EncryptedBytes[0]);
}

private static void setFinalStatic(Class<?> clazz, String fieldName, Object newValue) {
try {
Field field = clazz.getDeclaredField(fieldName);
Expand Down Expand Up @@ -515,4 +392,4 @@ private static void setFinalStatic(Class<?> clazz, String fieldName, Object newV
throw new RuntimeException(e);
}
}
}
}
Loading