fix: prevent AES-CBC encryption, enforce GCM for all new data (#984)

[franco-zalamena-iterable]

Problem

IterableDataEncryptor contained an encryptLegacy() path that used AES/CBC/PKCS5Padding (TRANSFORMATION_LEGACY) for encryption on older Android versions. CBC mode (with PKCS5 padding) is vulnerable to Padding Oracle Attacks and must never be used for encryption.

Changes

• encrypt() always uses GCM — removed the SDK-version branch that fell back to CBC; since minSdkVersion is 21 (> KITKAT 19) this branch was already dead code, but its existence was a latent risk.
• encryptLegacy() deleted — AES/CBC can no longer be used for any write path.
• decryptLegacy() retained (read-only) — decorated with a prominent security comment explaining it exists solely for backward-compatible decryption of data written by older SDK versions.
• decryptAndMigrate() added — decrypts a value and, if it was CBC-encrypted, immediately re-encrypts it with GCM and returns the new ciphertext. Callers can persist the updated ciphertext to eliminate future CBC reads entirely.
• Dead code removed — generateIV(), GCM_IV_LENGTH, CBC_IV_LENGTH, unused @TargetApi annotations, and SecureRandom import.
• TRANSFORMATION_LEGACY comment — clearly documents that this constant is for decryption-only migration use.

Security Impact

• Eliminates AES/CBC mode from all encryption operations going forward.
• Maintains full backward compatibility: data written by older SDK versions (CBC-encrypted) can still be decrypted.
• Provides a decryptAndMigrate() helper so apps can automatically upgrade persisted CBC ciphertexts to GCM on first read.

Test plan

• Verify data encrypted with old SDK (CBC) can still be decrypted via decrypt()
• Verify encrypt() always produces GCM ciphertext (isModern flag = 1)
• Verify decryptAndMigrate() returns a GCM ciphertext when given a CBC input
• Verify decryptAndMigrate() returns the original ciphertext unchanged when input is already GCM
• Run existing encryption unit tests

Made with Cursor

[franco-zalamena-iterable]

PR Analysis

Problem: IterableDataEncryptor retains an AES/CBC/PKCS5Padding encryption path (encryptLegacy()) that is vulnerable to Padding Oracle Attacks. Since minSdkVersion is 21 (above KitKat/19), this CBC encryption path is dead code but remains a latent security risk. The legacy constant itself must stay for backward-compatible decryption of data written by older SDK versions.

Ideal fix plan:

• Remove encryptLegacy() entirely so no code path can write CBC ciphertext
• Keep TRANSFORMATION_LEGACY and decryptLegacy() for read-only backward compatibility
• Remove the SDK version branch in encrypt() that dispatches to encryptLegacy() (dead code given minSdk 21)
• Clean up dead code: generateIV(), unused constants (GCM_IV_LENGTH, CBC_IV_LENGTH), stale @TargetApi annotations
• Provide a migration path so existing CBC-encrypted data gets re-encrypted with GCM on first read
• Wire the migration into IterableKeychain (the actual consumer) so it happens automatically
• Update existing tests: testEncryptionMethodFlag, testLegacyEncryptionAndDecryption, testEncryptionAcrossApiLevels all assert legacy encryption behavior (flag == 0) that will now break since encrypt() always produces GCM (flag == 1)
• Add new tests for decryptAndMigrate()

What the PR did:

• Removed encryptLegacy() and the version-branch in encrypt() -- good
• Retained TRANSFORMATION_LEGACY and decryptLegacy() with clear security comments -- good
• Removed dead code (generateIV, unused constants, stale annotations) -- good
• Added decryptAndMigrate() helper that decrypts CBC data and re-encrypts with GCM -- good idea
• Only modified IterableDataEncryptor.kt (single file)

Assessment:

• Root cause identified: yes -- correctly identified AES/CBC encryption as the vulnerability
• Fix correctness: partially correct -- the encryptor changes are sound, but the fix is incomplete
• Missed:
  • decryptAndMigrate() is added but never wired into any caller. IterableKeychain (the main consumer) still calls decrypt(), not decryptAndMigrate(), so CBC-encrypted data will remain as CBC ciphertext indefinitely on existing installs -- the migration never actually runs.
  • No test changes or additions. At least three existing tests (testEncryptionMethodFlag, testLegacyEncryptionAndDecryption, testEncryptionAcrossApiLevels) assert that encrypt() produces CBC output (flag == 0) on low API levels. These tests will now fail since encrypt() always produces GCM. The PR should either update those tests to reflect the new always-GCM behavior or remove the legacy-encryption test paths.
  • No tests for the new decryptAndMigrate() method at all.
  • The decrypt() method still has a dead-code guard for Build.VERSION.SDK_INT < KITKAT that can never be reached given minSdk 21. Minor, but could be cleaned up for consistency since other dead code was removed.
• Wrong assessment: none -- the PR description is accurate about what it does
• Tests: needed but missing -- existing tests will break, and the new public method decryptAndMigrate() has no coverage