feat: apply security enhancements from security evaluation output res…

…ults ** More specific: 1. The cipher has changed from "AES/CBC/PKCS7Padding" to "AES/GCM/NoPadding" and an automatic migration flow is provided. 2. SecretKeyFactory.getInstance("PBKDF2WithHmacSHA1") -> SecretKeyFactory.getInstance("PBKDF2WithHmacSHA512") and an automatic migration flow is provided. 3. SecretKeySpec(keyBytes, "AES") -> SecretKeySpec(keyBytes, "AES/CFB/PKCS5Padding") with no need for migration.
ptsiogas4 · Feb 17, 2022 · 67d3cff · 67d3cff
1 parent 53256a6
commit 67d3cff
Show file tree

Hide file tree

Showing 5 changed files with 116 additions and 17 deletions.
diff --git a/.idea/sonarlint/issuestore/index.pb b/.idea/sonarlint/issuestore/index.pb
diff --git a/.idea/vcs.xml b/.idea/vcs.xml
diff --git a/securebox/src/main/java/ptsiogas/gr/securebox/DecryptedResult.kt b/securebox/src/main/java/ptsiogas/gr/securebox/DecryptedResult.kt
@@ -0,0 +1,18 @@
+package ptsiogas.gr.securebox
+
+class DecryptedResult {
+    var result: ByteArray? = null
+    var needsMigration: Boolean = false
+
+    fun isValid(): Boolean {
+        return result != null
+    }
+
+    fun getStringResult(): String? {
+        return if (result != null) {
+            String(result!!)
+        } else {
+            null
+        }
+    }
+}
diff --git a/securebox/src/main/java/ptsiogas/gr/securebox/SBHEncryptionUtils.kt b/securebox/src/main/java/ptsiogas/gr/securebox/SBHEncryptionUtils.kt
@@ -12,11 +12,21 @@ import javax.crypto.spec.SecretKeySpec
 
 class SBHEncryptionUtils {
     companion object {
+        @Deprecated(
+            "This method is only used for migration reasons. It will be totally removed in the coming versions",
+            ReplaceWith(
+                "getCipherV2()"
+            )
+        )
         private fun getCipher(): Cipher {
             return Cipher.getInstance("AES/CBC/PKCS7Padding")
         }
 
-        private fun getCipherArray(
+        private fun getCipherV2(): Cipher {
+            return Cipher.getInstance("AES/GCM/NoPadding")
+        }
+
+        private fun getCipherArray_migration(
             inputArray: ByteArray?, optMode: Int, keySpec: SecretKeySpec,
             ivSpec: IvParameterSpec
         ): ByteArray {
@@ -25,7 +35,26 @@ class SBHEncryptionUtils {
             return cipher.doFinal(inputArray)
         }
 
+        private fun getCipherArray(
+            inputArray: ByteArray?, optMode: Int, keySpec: SecretKeySpec,
+            ivSpec: IvParameterSpec
+        ): ByteArray {
+            val cipher = getCipherV2()
+            cipher.init(optMode, keySpec, ivSpec)
+            return cipher.doFinal(inputArray)
+        }
+
         private fun getSecretKeyFactory(): SecretKeyFactory {
+            return SecretKeyFactory.getInstance("PBKDF2WithHmacSHA512")
+        }
+
+        @Deprecated(
+            "This method is only used for migration reasons. It will be totally removed in the coming versions",
+            ReplaceWith(
+                "getSecretKeyFactory()"
+            )
+        )
+        private fun getSecretKeyFactory_migration(): SecretKeyFactory {
             return SecretKeyFactory.getInstance("PBKDF2WithHmacSHA1")
         }
 
@@ -60,14 +89,35 @@ class SBHEncryptionUtils {
                 keySpec = keySpec,
                 ivSpec = ivSpec
             )
+        }
 
+        fun decryptByteArray_migration(
+            inputArray: ByteArray?,
+            keySpec: SecretKeySpec,
+            ivSpec: IvParameterSpec
+        ): ByteArray {
+            return getCipherArray_migration(
+                inputArray = inputArray,
+                optMode = Cipher.DECRYPT_MODE,
+                keySpec = keySpec,
+                ivSpec = ivSpec
+            )
         }
 
-        fun getKeySpec(passwordString: String, salt: ByteArray?): SecretKeySpec {
+        fun getKeySpec(
+            passwordString: String,
+            salt: ByteArray?,
+            useMigrationFactory: Boolean = false
+        ): SecretKeySpec {
             val passwordChar = passwordString.toCharArray() //Turn password into char[] array
             val pbKeySpec = PBEKeySpec(passwordChar, salt, 1324, 256) //1324 iterations
-            val keyBytes = getSecretKeyFactory().generateSecret(pbKeySpec).encoded
-            return SecretKeySpec(keyBytes, "AES")
+            if (useMigrationFactory) {
+                val keyBytes = getSecretKeyFactory_migration().generateSecret(pbKeySpec).encoded
+                return SecretKeySpec(keyBytes, "AES/CFB/PKCS5Padding")
+            } else {
+                val keyBytes = getSecretKeyFactory().generateSecret(pbKeySpec).encoded
+                return SecretKeySpec(keyBytes, "AES/CFB/PKCS5Padding")
+            }
         }
 
         @SuppressLint("HardwareIds")

diff --git a/securebox/src/main/java/ptsiogas/gr/securebox/SecureBoxHelper.kt b/securebox/src/main/java/ptsiogas/gr/securebox/SecureBoxHelper.kt
@@ -6,6 +6,7 @@ import java.io.File
 import java.io.IOException
 import java.io.ObjectInputStream
 import java.io.ObjectOutputStream
+import javax.crypto.AEADBadTagException
 import javax.crypto.spec.IvParameterSpec
 
 class SecureBoxHelper {
@@ -82,17 +83,23 @@ class SecureBoxHelper {
             return null
         }
         try {
+            var decryptedResult: DecryptedResult = DecryptedResult()
             val fileInputStream = context?.openFileInput(variableName + ".dat")
             fileInputStream?.use { fileInputStream ->
                 val objectInputStream = ObjectInputStream(fileInputStream)
                 val map = objectInputStream.readObject() as HashMap<String, ByteArray>
-                val decryptedByteArray = decryptString(map, passwordString)
-                if (decryptedByteArray != null) {
-                    return String(decryptedByteArray)
-                } else {
+                decryptedResult = decryptString(map, passwordString)
+                if (!decryptedResult.isValid()) {
                     ErrorMessage.logGeneralError()
                 }
             }
+
+            if (decryptedResult.isValid()) {
+                if (decryptedResult.needsMigration) {
+                    encryptString(variableName, decryptedResult.getStringResult()!!, passwordString)
+                }
+                return decryptedResult.getStringResult()
+            }
         } catch (e: ClassNotFoundException) {
             e.printStackTrace()
         } catch (e: IOException) {
@@ -175,8 +182,11 @@ class SecureBoxHelper {
         return map
     }
 
-    private fun decryptString(map: HashMap<String, ByteArray>, passwordString: String): ByteArray? {
-        var decrypted: ByteArray? = null
+    private fun decryptString(
+        map: HashMap<String, ByteArray>,
+        passwordString: String
+    ): DecryptedResult {
+        val decryptedResult: DecryptedResult = DecryptedResult()
         try {
             val salt = map["salt"]
             val iv = map["iv"]
@@ -187,11 +197,24 @@ class SecureBoxHelper {
 
             //Decrypt
             val ivSpec = IvParameterSpec(iv)
-            decrypted = SBHEncryptionUtils.decryptByteArray(encrypted, keySpec, ivSpec)
+
+            try {
+                decryptedResult.result =
+                    SBHEncryptionUtils.decryptByteArray(encrypted, keySpec, ivSpec)
+            } catch (e: Exception) {
+                if (e is AEADBadTagException) {
+                    //Migration flow
+                        val keySpecMig = SBHEncryptionUtils.getKeySpec(passwordString, salt, useMigrationFactory = true)
+                    decryptedResult.result = SBHEncryptionUtils.decryptByteArray_migration(encrypted, keySpecMig, ivSpec)
+                    decryptedResult.needsMigration = true
+                } else {
+                    e.printStackTrace()
+                }
+            }
         } catch (e: Exception) {
             e.printStackTrace()
         }
 
-        return decrypted
+        return decryptedResult
     }
 }