element-hq · jmartinesp · Jul 27, 2022 · Jul 12, 2022 · Jul 27, 2022
diff --git a/changelog.d/6522.feature b/changelog.d/6522.feature
@@ -0,0 +1 @@
+Improve lock screen implementation with extra security measures
diff --git a/...-sdk-android/src/main/java/org/matrix/android/sdk/api/securestorage/SecretStoringUtils.kt b/...-sdk-android/src/main/java/org/matrix/android/sdk/api/securestorage/SecretStoringUtils.kt
@@ -180,11 +180,11 @@ class SecretStoringUtils @Inject constructor(
             is KeyStore.PrivateKeyEntry -> keyEntry.certificate.publicKey
             else -> throw IllegalStateException("Unknown KeyEntry type.")
         }
-        val cipherMode = when {
+        val cipherAlgorithm = when {
             buildVersionSdkIntProvider.get() >= Build.VERSION_CODES.M -> AES_MODE
             else -> RSA_MODE
         }
-        val cipher = Cipher.getInstance(cipherMode)
+        val cipher = Cipher.getInstance(cipherAlgorithm)
         cipher.init(Cipher.ENCRYPT_MODE, key)
         return cipher
     }
@@ -204,13 +204,17 @@ class SecretStoringUtils @Inject constructor(
                     .setBlockModes(KeyProperties.BLOCK_MODE_GCM)
                     .setEncryptionPaddings(KeyProperties.ENCRYPTION_PADDING_NONE)
                     .setKeySize(128)
+                    .setUserAuthenticationRequired(keyNeedsUserAuthentication)
                     .apply {
-                        setUserAuthenticationRequired(keyNeedsUserAuthentication)
-                        if (buildVersionSdkIntProvider.get() >= Build.VERSION_CODES.N) {
-                            setInvalidatedByBiometricEnrollment(true)
+                        if (keyNeedsUserAuthentication) {
+                            buildVersionSdkIntProvider.whenAtLeast(Build.VERSION_CODES.N) {
+                                setInvalidatedByBiometricEnrollment(true)
+                            }
+                            buildVersionSdkIntProvider.whenAtLeast(Build.VERSION_CODES.P) {
+                                setUnlockedDeviceRequired(true)
+                            }
                         }
                     }
-                    .setUserAuthenticationRequired(keyNeedsUserAuthentication)
                     .build()
             generator.init(keyGenSpec)
             return generator.generateKey()

diff --git a/...x-sdk-android/src/main/java/org/matrix/android/sdk/api/util/BuildVersionSdkIntProvider.kt b/...x-sdk-android/src/main/java/org/matrix/android/sdk/api/util/BuildVersionSdkIntProvider.kt
@@ -21,4 +21,14 @@ interface BuildVersionSdkIntProvider {
      * Return the current version of the Android SDK.
      */
     fun get(): Int
+
+    /**
+     * Checks the if the current OS version is equal or greater than [version].
+     * @return A `non-null` result if true, `null` otherwise.
+     */
+    fun <T> whenAtLeast(version: Int, result: () -> T): T? {
+        return if (get() >= version) {
+            result()
+        } else null
+    }
 }
diff --git a/...androidTest/java/im/vector/app/features/pin/lockscreen/biometrics/BiometricHelperTests.kt b/...androidTest/java/im/vector/app/features/pin/lockscreen/biometrics/BiometricHelperTests.kt
@@ -24,13 +24,16 @@ import androidx.biometric.BiometricManager.Authenticators.BIOMETRIC_WEAK
 import androidx.biometric.BiometricManager.Authenticators.DEVICE_CREDENTIAL
 import androidx.biometric.BiometricManager.BIOMETRIC_ERROR_NONE_ENROLLED
 import androidx.biometric.BiometricManager.BIOMETRIC_SUCCESS
+import androidx.biometric.BiometricPrompt
 import androidx.lifecycle.lifecycleScope
 import androidx.test.core.app.ActivityScenario
+import androidx.test.filters.SdkSuppress
 import androidx.test.platform.app.InstrumentationRegistry
 import im.vector.app.TestBuildVersionSdkIntProvider
 import im.vector.app.features.pin.lockscreen.configuration.LockScreenConfiguration
 import im.vector.app.features.pin.lockscreen.configuration.LockScreenConfiguratorProvider
 import im.vector.app.features.pin.lockscreen.configuration.LockScreenMode
+import im.vector.app.features.pin.lockscreen.crypto.LockScreenCryptoConstants
 import im.vector.app.features.pin.lockscreen.crypto.LockScreenKeyRepository
 import im.vector.app.features.pin.lockscreen.tests.LockScreenTestActivity
 import im.vector.app.features.pin.lockscreen.ui.fallbackprompt.FallbackBiometricDialogFragment
@@ -56,6 +59,9 @@ import org.amshove.kluent.shouldBeTrue
 import org.junit.Before
 import org.junit.Ignore
 import org.junit.Test
+import org.matrix.android.sdk.api.securestorage.SecretStoringUtils
+import java.security.KeyStore
+import java.util.UUID
 import java.util.concurrent.CountDownLatch
 import java.util.concurrent.TimeUnit
 
@@ -64,6 +70,13 @@ class BiometricHelperTests {
     private val biometricManager = mockk<BiometricManager>(relaxed = true)
     private val lockScreenKeyRepository = mockk<LockScreenKeyRepository>(relaxed = true)
     private val buildVersionSdkIntProvider = TestBuildVersionSdkIntProvider()
+    private val keyStore = KeyStore.getInstance(LockScreenCryptoConstants.ANDROID_KEY_STORE).also { it.load(null) }
+    private val secretStoringUtils = SecretStoringUtils(
+            InstrumentationRegistry.getInstrumentation().targetContext,
+            keyStore,
+            buildVersionSdkIntProvider,
+            false,
+    )
 
     @Before
     fun setup() {
@@ -188,8 +201,10 @@ class BiometricHelperTests {
     }
 
     @OptIn(ExperimentalCoroutinesApi::class)
+    @SdkSuppress(minSdkVersion = Build.VERSION_CODES.R) // Due to some issues with mockk and CryptoObject initialization
     @Test
     fun authenticateInDeviceWithIssuesShowsFallbackPromptDialog() = runTest {
+        buildVersionSdkIntProvider.value = Build.VERSION_CODES.M
         mockkStatic("kotlinx.coroutines.flow.FlowKt")
         val mockAuthChannel: Channel<Boolean> = mockk(relaxed = true) {
             // Empty flow to keep the dialog open
@@ -201,6 +216,9 @@ class BiometricHelperTests {
         mockkObject(DevicePromptCheck)
         every { DevicePromptCheck.isDeviceWithNoBiometricUI } returns true
         every { lockScreenKeyRepository.isSystemKeyValid() } returns true
+
+        val keyAlias = UUID.randomUUID().toString()
+        every { biometricUtils.getAuthCryptoObject() } returns BiometricPrompt.CryptoObject(secretStoringUtils.getEncryptCipher(keyAlias))
         val latch = CountDownLatch(1)
         val intent = Intent(InstrumentationRegistry.getInstrumentation().targetContext, LockScreenTestActivity::class.java)
         with(ActivityScenario.launch<LockScreenTestActivity>(intent)) {
@@ -214,11 +232,13 @@ class BiometricHelperTests {
             }
         }
         latch.await(1, TimeUnit.SECONDS)
+        keyStore.deleteEntry(keyAlias)
         unmockkObject(DevicePromptCheck)
         unmockkStatic("kotlinx.coroutines.flow.FlowKt")
     }
 
     @Test
+    @SdkSuppress(minSdkVersion = Build.VERSION_CODES.R) // Due to some issues with mockk and CryptoObject initialization
     fun authenticateCreatesSystemKeyIfNeededOnSuccessOnAndroidM() = runTest {
         buildVersionSdkIntProvider.value = Build.VERSION_CODES.M
         every { lockScreenKeyRepository.isSystemKeyValid() } returns true

diff --git a/.../src/androidTest/java/im/vector/app/features/pin/lockscreen/crypto/KeyStoreCryptoTests.kt b/.../src/androidTest/java/im/vector/app/features/pin/lockscreen/crypto/KeyStoreCryptoTests.kt
@@ -43,7 +43,9 @@ class KeyStoreCryptoTests {
     private val versionProvider = TestBuildVersionSdkIntProvider().also { it.value = Build.VERSION_CODES.M }
     private val secretStoringUtils = spyk(SecretStoringUtils(context, keyStore, versionProvider))
     private val keyStoreCrypto = spyk(
-            KeyStoreCrypto(alias, false, context, versionProvider, keyStore, secretStoringUtils)
+            KeyStoreCrypto(alias, false, context, versionProvider, keyStore).also {
+                it.secretStoringUtils = secretStoringUtils
+            }
     )
 
     @After
@@ -146,10 +148,10 @@ class KeyStoreCryptoTests {
 
     @Test
     fun getCryptoObjectUsesCipherFromSecretStoringUtils() {
-        keyStoreCrypto.getCryptoObject()
+        keyStoreCrypto.getAuthCryptoObject()
         verify { secretStoringUtils.getEncryptCipher(any()) }
 
         every { secretStoringUtils.getEncryptCipher(any()) } throws KeyPermanentlyInvalidatedException()
-        invoking { keyStoreCrypto.getCryptoObject() } shouldThrow KeyPermanentlyInvalidatedException::class
+        invoking { keyStoreCrypto.getAuthCryptoObject() } shouldThrow KeyPermanentlyInvalidatedException::class
     }
 }
diff --git a/...oidTest/java/im/vector/app/features/pin/lockscreen/crypto/LockScreenKeyRepositoryTests.kt b/...oidTest/java/im/vector/app/features/pin/lockscreen/crypto/LockScreenKeyRepositoryTests.kt
@@ -16,21 +16,16 @@
 
 package im.vector.app.features.pin.lockscreen.crypto
 
-import android.security.keystore.KeyPermanentlyInvalidatedException
 import androidx.test.platform.app.InstrumentationRegistry
+import im.vector.app.features.pin.lockscreen.crypto.migrations.LegacyPinCodeMigrator
 import im.vector.app.features.settings.VectorPreferences
 import io.mockk.clearAllMocks
-import io.mockk.coVerify
 import io.mockk.every
 import io.mockk.mockk
 import io.mockk.spyk
-import io.mockk.verify
-import kotlinx.coroutines.test.runTest
-import org.amshove.kluent.coInvoking
 import org.amshove.kluent.shouldBeEqualTo
 import org.amshove.kluent.shouldBeFalse
 import org.amshove.kluent.shouldBeTrue
-import org.amshove.kluent.shouldNotThrow
 import org.junit.After
 import org.junit.Before
 import org.junit.Test
@@ -49,7 +44,7 @@ class LockScreenKeyRepositoryTests {
     }
 
     private lateinit var lockScreenKeyRepository: LockScreenKeyRepository
-    private val pinCodeMigrator: PinCodeMigrator = mockk(relaxed = true)
+    private val legacyPinCodeMigrator: LegacyPinCodeMigrator = mockk(relaxed = true)
     private val vectorPreferences: VectorPreferences = mockk(relaxed = true)
 
     private val keyStore: KeyStore by lazy {
@@ -58,7 +53,7 @@ class LockScreenKeyRepositoryTests {
 
     @Before
     fun setup() {
-        lockScreenKeyRepository = spyk(LockScreenKeyRepository("base", pinCodeMigrator, vectorPreferences, keyStoreCryptoFactory))
+        lockScreenKeyRepository = spyk(LockScreenKeyRepository("base.pin_code", "base.system", keyStoreCryptoFactory))
     }
 
     @After
@@ -141,44 +136,4 @@ class LockScreenKeyRepositoryTests {
 
         lockScreenKeyRepository.hasPinCodeKey().shouldBeFalse()
     }
-
-    @Test
-    fun migrateKeysIfNeededReturnsEarlyIfNotNeeded() = runTest {
-        every { pinCodeMigrator.isMigrationNeeded() } returns false
-
-        lockScreenKeyRepository.migrateKeysIfNeeded()
-
-        coVerify(exactly = 0) { pinCodeMigrator.migrate(any()) }
-    }
-
-    @Test
-    fun migrateKeysIfNeededWillMigratePinCodeAndKeys() = runTest {
-        every { pinCodeMigrator.isMigrationNeeded() } returns true
-
-        lockScreenKeyRepository.migrateKeysIfNeeded()
-
-        coVerify { pinCodeMigrator.migrate(any()) }
-    }
-
-    @Test
-    fun migrateKeysIfNeededWillCreateSystemKeyIfNeeded() = runTest {
-        every { pinCodeMigrator.isMigrationNeeded() } returns true
-        every { vectorPreferences.useBiometricsToUnlock() } returns true
-        every { lockScreenKeyRepository.ensureSystemKey() } returns mockk()
-
-        lockScreenKeyRepository.migrateKeysIfNeeded()
-
-        verify { lockScreenKeyRepository.ensureSystemKey() }
-    }
-
-    @Test
-    fun migrateKeysIfNeededWillHandleKeyPermanentlyInvalidatedException() = runTest {
-        every { pinCodeMigrator.isMigrationNeeded() } returns true
-        every { vectorPreferences.useBiometricsToUnlock() } returns true
-        every { lockScreenKeyRepository.ensureSystemKey() } throws KeyPermanentlyInvalidatedException()
-
-        coInvoking { lockScreenKeyRepository.migrateKeysIfNeeded() } shouldNotThrow KeyPermanentlyInvalidatedException::class
-
-        verify { lockScreenKeyRepository.ensureSystemKey() }
-    }
 }
diff --git a/...lockscreen/crypto/PinCodeMigratorTests.kt → .../migrations/LegacyPinCodeMigratorTests.kt b/...lockscreen/crypto/PinCodeMigratorTests.kt → .../migrations/LegacyPinCodeMigratorTests.kt
@@ -16,7 +16,7 @@
 
 @file:Suppress("DEPRECATION")
 
-package im.vector.app.features.pin.lockscreen.crypto
+package im.vector.app.features.pin.lockscreen.crypto.migrations
 
 import android.os.Build
 import android.security.KeyPairGeneratorSpec
@@ -57,7 +57,7 @@ import javax.crypto.spec.PSource
 import javax.security.auth.x500.X500Principal
 import kotlin.math.abs
 
-class PinCodeMigratorTests {
+class LegacyPinCodeMigratorTests {
 
     private val alias = UUID.randomUUID().toString()
 
@@ -72,7 +72,9 @@ class PinCodeMigratorTests {
     private val secretStoringUtils: SecretStoringUtils = spyk(
             SecretStoringUtils(context, keyStore, buildVersionSdkIntProvider)
     )
-    private val pinCodeMigrator = spyk(PinCodeMigrator(pinCodeStore, keyStore, secretStoringUtils, buildVersionSdkIntProvider))
+    private val legacyPinCodeMigrator = spyk(
+            LegacyPinCodeMigrator(alias, pinCodeStore, keyStore, secretStoringUtils, buildVersionSdkIntProvider)
+    )
 
     @After
     fun tearDown() {
@@ -87,35 +89,35 @@ class PinCodeMigratorTests {
 
     @Test
     fun isMigrationNeededReturnsTrueIfLegacyKeyExists() {
-        pinCodeMigrator.isMigrationNeeded() shouldBe false
+        legacyPinCodeMigrator.isMigrationNeeded() shouldBe false
 
         generateLegacyKey()
 
-        pinCodeMigrator.isMigrationNeeded() shouldBe true
+        legacyPinCodeMigrator.isMigrationNeeded() shouldBe true
     }
 
     @Test
     fun migrateWillReturnEarlyIfPinCodeDoesNotExist() = runTest {
-        every { pinCodeMigrator.isMigrationNeeded() } returns false
+        every { legacyPinCodeMigrator.isMigrationNeeded() } returns false
         coEvery { pinCodeStore.getPinCode() } returns null
 
-        pinCodeMigrator.migrate(alias)
+        legacyPinCodeMigrator.migrate()
 
-        coVerify(exactly = 0) { pinCodeMigrator.getDecryptedPinCode() }
+        coVerify(exactly = 0) { legacyPinCodeMigrator.getDecryptedPinCode() }
         verify(exactly = 0) { secretStoringUtils.securelyStoreBytes(any(), any()) }
         coVerify(exactly = 0) { pinCodeStore.savePinCode(any()) }
         verify(exactly = 0) { keyStore.deleteEntry(LEGACY_PIN_CODE_KEY_ALIAS) }
     }
 
     @Test
     fun migrateWillReturnEarlyIfIsNotNeeded() = runTest {
-        every { pinCodeMigrator.isMigrationNeeded() } returns false
-        coEvery { pinCodeMigrator.getDecryptedPinCode() } returns "1234"
+        every { legacyPinCodeMigrator.isMigrationNeeded() } returns false
+        coEvery { legacyPinCodeMigrator.getDecryptedPinCode() } returns "1234"
         every { secretStoringUtils.securelyStoreBytes(any(), any()) } returns ByteArray(0)
 
-        pinCodeMigrator.migrate(alias)
+        legacyPinCodeMigrator.migrate()
 
-        coVerify(exactly = 0) { pinCodeMigrator.getDecryptedPinCode() }
+        coVerify(exactly = 0) { legacyPinCodeMigrator.getDecryptedPinCode() }
         verify(exactly = 0) { secretStoringUtils.securelyStoreBytes(any(), any()) }
         coVerify(exactly = 0) { pinCodeStore.savePinCode(any()) }
         verify(exactly = 0) { keyStore.deleteEntry(LEGACY_PIN_CODE_KEY_ALIAS) }
@@ -126,9 +128,9 @@ class PinCodeMigratorTests {
         val pinCode = "1234"
         saveLegacyPinCode(pinCode)
 
-        pinCodeMigrator.migrate(alias)
+        legacyPinCodeMigrator.migrate()
 
-        coVerify { pinCodeMigrator.getDecryptedPinCode() }
+        coVerify { legacyPinCodeMigrator.getDecryptedPinCode() }
         verify { secretStoringUtils.securelyStoreBytes(any(), any()) }
         coVerify { pinCodeStore.savePinCode(any()) }
         verify { keyStore.deleteEntry(LEGACY_PIN_CODE_KEY_ALIAS) }
@@ -145,9 +147,9 @@ class PinCodeMigratorTests {
         every { buildVersionSdkIntProvider.get() } returns Build.VERSION_CODES.LOLLIPOP
         saveLegacyPinCode(pinCode)
 
-        pinCodeMigrator.migrate(alias)
+        legacyPinCodeMigrator.migrate()
 
-        coVerify { pinCodeMigrator.getDecryptedPinCode() }
+        coVerify { legacyPinCodeMigrator.getDecryptedPinCode() }
         verify { secretStoringUtils.securelyStoreBytes(any(), any()) }
         coVerify { pinCodeStore.savePinCode(any()) }
         verify { keyStore.deleteEntry(LEGACY_PIN_CODE_KEY_ALIAS) }
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		Improve lock screen implementation with extra security measures