From 6e1e31bac1e61aaec10e957d3c9340bb757c18e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jorge=20Mart=C3=ADn?= Date: Tue, 9 Aug 2022 09:53:08 +0200 Subject: [PATCH] Avoid crashes from unknown exceptions on lockscreen key migration. --- changelog.d/6769.bugfix | 1 + .../lockscreen/crypto/LockScreenKeysMigrator.kt | 2 +- .../migrations/MissingSystemKeyMigrator.kt | 17 ++++++++--------- .../crypto/migrations/SystemKeyV1Migrator.kt | 11 +++++++---- ...rTests.kt => LockScreenKeysMigratorTests.kt} | 6 +++--- .../migrations/MissingSystemKeyMigratorTests.kt | 10 +++++----- .../migrations/SystemKeyV1MigratorTests.kt | 4 +++- 7 files changed, 28 insertions(+), 23 deletions(-) create mode 100644 changelog.d/6769.bugfix rename vector/src/test/java/im/vector/app/features/pin/lockscreen/crypto/migrations/{LockScreenTestMigratorTests.kt => LockScreenKeysMigratorTests.kt} (94%) diff --git a/changelog.d/6769.bugfix b/changelog.d/6769.bugfix new file mode 100644 index 0000000000..5d65bff449 --- /dev/null +++ b/changelog.d/6769.bugfix @@ -0,0 +1 @@ +Catch all exceptions on lockscreen system key migrations. diff --git a/vector/src/main/java/im/vector/app/features/pin/lockscreen/crypto/LockScreenKeysMigrator.kt b/vector/src/main/java/im/vector/app/features/pin/lockscreen/crypto/LockScreenKeysMigrator.kt index 68acfcebf3..bb55ceb1b7 100644 --- a/vector/src/main/java/im/vector/app/features/pin/lockscreen/crypto/LockScreenKeysMigrator.kt +++ b/vector/src/main/java/im/vector/app/features/pin/lockscreen/crypto/LockScreenKeysMigrator.kt @@ -40,7 +40,7 @@ class LockScreenKeysMigrator @Inject constructor( suspend fun migrateIfNeeded() { if (legacyPinCodeMigrator.isMigrationNeeded()) { legacyPinCodeMigrator.migrate() - missingSystemKeyMigrator.migrate() + missingSystemKeyMigrator.migrateIfNeeded() } if (systemKeyV1Migrator.isMigrationNeeded() && versionProvider.get() >= Build.VERSION_CODES.M) { diff --git a/vector/src/main/java/im/vector/app/features/pin/lockscreen/crypto/migrations/MissingSystemKeyMigrator.kt b/vector/src/main/java/im/vector/app/features/pin/lockscreen/crypto/migrations/MissingSystemKeyMigrator.kt index 75a68c66b7..4c33c14954 100644 --- a/vector/src/main/java/im/vector/app/features/pin/lockscreen/crypto/migrations/MissingSystemKeyMigrator.kt +++ b/vector/src/main/java/im/vector/app/features/pin/lockscreen/crypto/migrations/MissingSystemKeyMigrator.kt @@ -18,8 +18,6 @@ package im.vector.app.features.pin.lockscreen.crypto.migrations import android.annotation.SuppressLint import android.os.Build -import android.security.keystore.KeyPermanentlyInvalidatedException -import android.security.keystore.UserNotAuthenticatedException import im.vector.app.features.pin.lockscreen.crypto.KeyStoreCrypto import im.vector.app.features.pin.lockscreen.di.BiometricKeyAlias import im.vector.app.features.settings.VectorPreferences @@ -41,14 +39,15 @@ class MissingSystemKeyMigrator @Inject constructor( * If user had biometric auth enabled, ensure system key exists, creating one if needed. */ @SuppressLint("NewApi") - fun migrate() { + fun migrateIfNeeded() { if (buildVersionSdkIntProvider.get() >= Build.VERSION_CODES.M && vectorPreferences.useBiometricsToUnlock()) { - try { - keystoreCryptoFactory.provide(systemKeyAlias, true).ensureKey() - } catch (e: KeyPermanentlyInvalidatedException) { - Timber.e("Could not automatically create biometric key because it was invalidated.", e) - } catch (e: UserNotAuthenticatedException) { - Timber.e("Could not automatically create biometric key because there are no enrolled biometric authenticators.", e) + val systemKeyStoreCrypto = keystoreCryptoFactory.provide(systemKeyAlias, true) + runCatching { + systemKeyStoreCrypto.ensureKey() + }.onFailure { e -> + Timber.e(e, "Could not automatically create biometric key. Biometric authentication will be disabled.") + systemKeyStoreCrypto.deleteKey() + vectorPreferences.setUseBiometricToUnlock(false) } } } diff --git a/vector/src/main/java/im/vector/app/features/pin/lockscreen/crypto/migrations/SystemKeyV1Migrator.kt b/vector/src/main/java/im/vector/app/features/pin/lockscreen/crypto/migrations/SystemKeyV1Migrator.kt index 10c7505f27..748001af8b 100644 --- a/vector/src/main/java/im/vector/app/features/pin/lockscreen/crypto/migrations/SystemKeyV1Migrator.kt +++ b/vector/src/main/java/im/vector/app/features/pin/lockscreen/crypto/migrations/SystemKeyV1Migrator.kt @@ -17,10 +17,10 @@ package im.vector.app.features.pin.lockscreen.crypto.migrations import android.os.Build -import android.security.keystore.UserNotAuthenticatedException import androidx.annotation.RequiresApi import im.vector.app.features.pin.lockscreen.crypto.KeyStoreCrypto import im.vector.app.features.pin.lockscreen.di.BiometricKeyAlias +import im.vector.app.features.settings.VectorPreferences import timber.log.Timber import java.security.KeyStore import javax.inject.Inject @@ -32,6 +32,7 @@ class SystemKeyV1Migrator @Inject constructor( @BiometricKeyAlias private val systemKeyAlias: String, private val keyStore: KeyStore, private val keystoreCryptoFactory: KeyStoreCrypto.Factory, + private val vectorPreferences: VectorPreferences, ) { /** @@ -41,10 +42,12 @@ class SystemKeyV1Migrator @Inject constructor( fun migrate() { keyStore.deleteEntry(SYSTEM_KEY_ALIAS_V1) val systemKeyStoreCrypto = keystoreCryptoFactory.provide(systemKeyAlias, keyNeedsUserAuthentication = true) - try { + runCatching { systemKeyStoreCrypto.ensureKey() - } catch (e: UserNotAuthenticatedException) { - Timber.e("Could not migrate v1 biometric key because there are no enrolled biometric authenticators.", e) + }.onFailure { e -> + Timber.e(e, "Could not migrate v1 biometric key. Biometric authentication will be disabled.") + systemKeyStoreCrypto.deleteKey() + vectorPreferences.setUseBiometricToUnlock(false) } } diff --git a/vector/src/test/java/im/vector/app/features/pin/lockscreen/crypto/migrations/LockScreenTestMigratorTests.kt b/vector/src/test/java/im/vector/app/features/pin/lockscreen/crypto/migrations/LockScreenKeysMigratorTests.kt similarity index 94% rename from vector/src/test/java/im/vector/app/features/pin/lockscreen/crypto/migrations/LockScreenTestMigratorTests.kt rename to vector/src/test/java/im/vector/app/features/pin/lockscreen/crypto/migrations/LockScreenKeysMigratorTests.kt index 73f71dbf2b..588fb12382 100644 --- a/vector/src/test/java/im/vector/app/features/pin/lockscreen/crypto/migrations/LockScreenTestMigratorTests.kt +++ b/vector/src/test/java/im/vector/app/features/pin/lockscreen/crypto/migrations/LockScreenKeysMigratorTests.kt @@ -26,7 +26,7 @@ import io.mockk.verify import kotlinx.coroutines.runBlocking import org.junit.Test -class LockScreenTestMigratorTests { +class LockScreenKeysMigratorTests { private val legacyPinCodeMigrator = mockk(relaxed = true) private val missingSystemKeyMigrator = mockk(relaxed = true) @@ -42,7 +42,7 @@ class LockScreenTestMigratorTests { runBlocking { migrator.migrateIfNeeded() } coVerify(exactly = 0) { legacyPinCodeMigrator.migrate() } - verify(exactly = 0) { missingSystemKeyMigrator.migrate() } + verify(exactly = 0) { missingSystemKeyMigrator.migrateIfNeeded() } // When migration is needed every { legacyPinCodeMigrator.isMigrationNeeded() } returns true @@ -50,7 +50,7 @@ class LockScreenTestMigratorTests { runBlocking { migrator.migrateIfNeeded() } coVerify { legacyPinCodeMigrator.migrate() } - verify { missingSystemKeyMigrator.migrate() } + verify { missingSystemKeyMigrator.migrateIfNeeded() } } @Test diff --git a/vector/src/test/java/im/vector/app/features/pin/lockscreen/crypto/migrations/MissingSystemKeyMigratorTests.kt b/vector/src/test/java/im/vector/app/features/pin/lockscreen/crypto/migrations/MissingSystemKeyMigratorTests.kt index 3098187962..e211fc8a0e 100644 --- a/vector/src/test/java/im/vector/app/features/pin/lockscreen/crypto/migrations/MissingSystemKeyMigratorTests.kt +++ b/vector/src/test/java/im/vector/app/features/pin/lockscreen/crypto/migrations/MissingSystemKeyMigratorTests.kt @@ -44,7 +44,7 @@ class MissingSystemKeyMigratorTests { every { keyStoreCryptoFactory.provide(any(), any()) } returns keyStoreCryptoMock every { vectorPreferences.useBiometricsToUnlock() } returns true - missingSystemKeyMigrator.migrate() + missingSystemKeyMigrator.migrateIfNeeded() verify { keyStoreCryptoMock.ensureKey() } } @@ -57,7 +57,7 @@ class MissingSystemKeyMigratorTests { every { keyStoreCryptoFactory.provide(any(), any()) } returns keyStoreCryptoMock every { vectorPreferences.useBiometricsToUnlock() } returns true - invoking { missingSystemKeyMigrator.migrate() } shouldNotThrow KeyPermanentlyInvalidatedException::class + invoking { missingSystemKeyMigrator.migrateIfNeeded() } shouldNotThrow KeyPermanentlyInvalidatedException::class } @Test @@ -68,7 +68,7 @@ class MissingSystemKeyMigratorTests { every { keyStoreCryptoFactory.provide(any(), any()) } returns keyStoreCryptoMock every { vectorPreferences.useBiometricsToUnlock() } returns true - invoking { missingSystemKeyMigrator.migrate() } shouldNotThrow UserNotAuthenticatedException::class + invoking { missingSystemKeyMigrator.migrateIfNeeded() } shouldNotThrow UserNotAuthenticatedException::class } @Test @@ -79,7 +79,7 @@ class MissingSystemKeyMigratorTests { every { keyStoreCryptoFactory.provide(any(), any()) } returns keyStoreCryptoMock every { vectorPreferences.useBiometricsToUnlock() } returns false - missingSystemKeyMigrator.migrate() + missingSystemKeyMigrator.migrateIfNeeded() verify(exactly = 0) { keyStoreCryptoMock.ensureKey() } } @@ -93,7 +93,7 @@ class MissingSystemKeyMigratorTests { every { keyStoreCryptoFactory.provide(any(), any()) } returns keyStoreCryptoMock every { vectorPreferences.useBiometricsToUnlock() } returns false - missingSystemKeyMigrator.migrate() + missingSystemKeyMigrator.migrateIfNeeded() verify(exactly = 0) { keyStoreCryptoMock.ensureKey() } } diff --git a/vector/src/test/java/im/vector/app/features/pin/lockscreen/crypto/migrations/SystemKeyV1MigratorTests.kt b/vector/src/test/java/im/vector/app/features/pin/lockscreen/crypto/migrations/SystemKeyV1MigratorTests.kt index 5cbb828f71..825b251f3e 100644 --- a/vector/src/test/java/im/vector/app/features/pin/lockscreen/crypto/migrations/SystemKeyV1MigratorTests.kt +++ b/vector/src/test/java/im/vector/app/features/pin/lockscreen/crypto/migrations/SystemKeyV1MigratorTests.kt @@ -18,6 +18,7 @@ package im.vector.app.features.pin.lockscreen.crypto.migrations import android.security.keystore.UserNotAuthenticatedException import im.vector.app.features.pin.lockscreen.crypto.KeyStoreCrypto +import im.vector.app.features.settings.VectorPreferences import io.mockk.every import io.mockk.mockk import io.mockk.verify @@ -31,7 +32,8 @@ class SystemKeyV1MigratorTests { private val keyStoreCryptoFactory = mockk() private val keyStore = mockk(relaxed = true) - private val systemKeyV1Migrator = SystemKeyV1Migrator("vector.system_new", keyStore, keyStoreCryptoFactory) + private val vectorPreferences = mockk(relaxed = true) + private val systemKeyV1Migrator = SystemKeyV1Migrator("vector.system_new", keyStore, keyStoreCryptoFactory, vectorPreferences) @Test fun isMigrationNeededReturnsTrueIfV1KeyExists() {