From 5a7dc198dd92437ae5ca24923c1fb4a8d2d4e70b Mon Sep 17 00:00:00 2001 From: Patrick Honkonen <1883101+SaintPatrck@users.noreply.github.com> Date: Fri, 16 Aug 2024 15:13:41 -0400 Subject: [PATCH] [PM-10884] Catch ProviderException when generating a secure key (#3733) --- .../manager/BiometricsEncryptionManager.kt | 4 +- .../BiometricsEncryptionManagerImpl.kt | 117 +++++++++++++----- .../feature/accountsetup/SetupUnlockScreen.kt | 18 ++- .../accountsetup/SetupUnlockViewModel.kt | 47 ++++++- .../accountsecurity/AccountSecurityScreen.kt | 8 ++ .../AccountSecurityViewModel.kt | 37 ++++-- .../accountsetup/SetupUnlockScreenTest.kt | 34 +++++ .../accountsetup/SetupUnlockViewModelTest.kt | 51 ++++++++ .../AccountSecurityScreenTest.kt | 49 ++++++++ .../AccountSecurityViewModelTest.kt | 22 +++- 10 files changed, 340 insertions(+), 47 deletions(-) diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/manager/BiometricsEncryptionManager.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/manager/BiometricsEncryptionManager.kt index 308fafd82f..8eb391fc2a 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/platform/manager/BiometricsEncryptionManager.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/manager/BiometricsEncryptionManager.kt @@ -9,9 +9,9 @@ interface BiometricsEncryptionManager { /** * Creates a [Cipher] built from a keystore. */ - fun createCipher( + fun createCipherOrNull( userId: String, - ): Cipher + ): Cipher? /** * Gets the [Cipher] built from a keystore, or creates one if it doesn't already exist. diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/manager/BiometricsEncryptionManagerImpl.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/manager/BiometricsEncryptionManagerImpl.kt index ebed034849..534d3e0ab9 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/platform/manager/BiometricsEncryptionManagerImpl.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/manager/BiometricsEncryptionManagerImpl.kt @@ -6,13 +6,20 @@ import android.security.keystore.KeyProperties import com.x8bit.bitwarden.BuildConfig import com.x8bit.bitwarden.data.platform.annotation.OmitFromCoverage import com.x8bit.bitwarden.data.platform.datasource.disk.SettingsDiskSource +import java.io.IOException import java.security.InvalidAlgorithmParameterException import java.security.InvalidKeyException import java.security.KeyStore +import java.security.KeyStoreException +import java.security.NoSuchAlgorithmException +import java.security.NoSuchProviderException +import java.security.ProviderException import java.security.UnrecoverableKeyException +import java.security.cert.CertificateException import java.util.UUID import javax.crypto.Cipher import javax.crypto.KeyGenerator +import javax.crypto.NoSuchPaddingException import javax.crypto.SecretKey /** @@ -39,9 +46,20 @@ class BiometricsEncryptionManagerImpl( .setInvalidatedByBiometricEnrollment(true) .build() - override fun createCipher(userId: String): Cipher { - val secretKey: SecretKey = generateKey() - val cipher = Cipher.getInstance(CIPHER_TRANSFORMATION) + override fun createCipherOrNull(userId: String): Cipher? { + val secretKey: SecretKey = generateKeyOrNull() + ?: run { + // user removed all biometrics from the device + settingsDiskSource.systemBiometricIntegritySource = null + return null + } + val cipher = try { + Cipher.getInstance(CIPHER_TRANSFORMATION) + } catch (e: NoSuchAlgorithmException) { + return null + } catch (e: NoSuchPaddingException) { + return null + } // This should never fail to initialize / return false because the cipher is newly generated initializeCipher( userId = userId, @@ -52,13 +70,14 @@ class BiometricsEncryptionManagerImpl( } override fun getOrCreateCipher(userId: String): Cipher? { - val secretKey = try { - getSecretKey() ?: generateKey() - } catch (e: InvalidAlgorithmParameterException) { - // user removed all biometrics from the device - settingsDiskSource.systemBiometricIntegritySource = null - return null - } + val secretKey = getSecretKeyOrNull() + ?: generateKeyOrNull() + ?: run { + // user removed all biometrics from the device + settingsDiskSource.systemBiometricIntegritySource = null + return null + } + val cipher = Cipher.getInstance(CIPHER_TRANSFORMATION) val isCipherInitialized = initializeCipher( userId = userId, @@ -88,24 +107,67 @@ class BiometricsEncryptionManagerImpl( } /** - * Generates a [SecretKey] from which the [Cipher] will be generated. + * Generates a [SecretKey] from which the [Cipher] will be generated, or `null` if a key cannot + * be generated. */ - private fun generateKey(): SecretKey { - val keyGen = KeyGenerator.getInstance( - KeyProperties.KEY_ALGORITHM_AES, - ENCRYPTION_KEYSTORE_NAME, - ) - keyGen.init(keyGenParameterSpec) - keyGen.generateKey() - return requireNotNull(getSecretKey()) + private fun generateKeyOrNull(): SecretKey? { + val keyGen = try { + KeyGenerator.getInstance( + KeyProperties.KEY_ALGORITHM_AES, + ENCRYPTION_KEYSTORE_NAME, + ) + } catch (e: NoSuchAlgorithmException) { + return null + } catch (e: NoSuchProviderException) { + return null + } catch (e: IllegalArgumentException) { + return null + } + + try { + keyGen.init(keyGenParameterSpec) + keyGen.generateKey() + } catch (e: InvalidAlgorithmParameterException) { + return null + } catch (e: ProviderException) { + return null + } + + return getSecretKeyOrNull() } /** * Returns the [SecretKey] stored in the keystore, or null if there isn't one. */ - private fun getSecretKey(): SecretKey? { - keystore.load(null) - return keystore.getKey(ENCRYPTION_KEY_NAME, null) as? SecretKey + private fun getSecretKeyOrNull(): SecretKey? { + try { + keystore.load(null) + } catch (e: IllegalArgumentException) { + // keystore could not be loaded because [param] is unrecognized. + return null + } catch (e: IOException) { + // keystore data format is invalid or the password is incorrect. + return null + } catch (e: NoSuchAlgorithmException) { + // keystore integrity could not be checked due to missing algorithm. + return null + } catch (e: CertificateException) { + // keystore certificates could not be loaded + return null + } + + return try { + keystore.getKey(ENCRYPTION_KEY_NAME, null) as? SecretKey + } catch (e: KeyStoreException) { + // keystore was not loaded + null + } catch (e: NoSuchAlgorithmException) { + // keystore algorithm cannot be found + null + } catch (e: UnrecoverableKeyException) { + // key could not be recovered + null + } } /** @@ -137,7 +199,7 @@ class BiometricsEncryptionManagerImpl( * Validates the keystore key and decrypts it using the user-provided [cipher]. */ private fun isSystemBiometricIntegrityValid(userId: String, cipher: Cipher?): Boolean { - val secretKey = getSecretKey() + val secretKey = getSecretKeyOrNull() return if (cipher != null && secretKey != null) { initializeCipher( userId = userId, @@ -165,12 +227,9 @@ class BiometricsEncryptionManagerImpl( value = true, ) - try { - createCipher(userId) - } catch (e: Exception) { - // Catch silently to allow biometrics to function on devices that are in - // a state where key generation is not functioning - } + // Ignore result so biometrics function on devices that are in a state where key generation + // is not functioning + createCipherOrNull(userId) } } diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/accountsetup/SetupUnlockScreen.kt b/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/accountsetup/SetupUnlockScreen.kt index 20438248f2..460fd06d27 100644 --- a/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/accountsetup/SetupUnlockScreen.kt +++ b/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/accountsetup/SetupUnlockScreen.kt @@ -43,6 +43,8 @@ import com.x8bit.bitwarden.ui.platform.base.util.standardHorizontalMargin import com.x8bit.bitwarden.ui.platform.components.appbar.BitwardenTopAppBar import com.x8bit.bitwarden.ui.platform.components.button.BitwardenFilledButton import com.x8bit.bitwarden.ui.platform.components.button.BitwardenTextButton +import com.x8bit.bitwarden.ui.platform.components.dialog.BasicDialogState +import com.x8bit.bitwarden.ui.platform.components.dialog.BitwardenBasicDialog import com.x8bit.bitwarden.ui.platform.components.dialog.BitwardenLoadingDialog import com.x8bit.bitwarden.ui.platform.components.dialog.BitwardenTwoButtonDialog import com.x8bit.bitwarden.ui.platform.components.dialog.LoadingDialogState @@ -86,7 +88,12 @@ fun SetupUnlockScreen( } } - SetupUnlockScreenDialogs(dialogState = state.dialogState) + SetupUnlockScreenDialogs( + dialogState = state.dialogState, + onDismissRequest = remember(viewModel) { + { viewModel.trySendAction(SetupUnlockAction.DismissDialog) } + }, + ) val scrollBehavior = TopAppBarDefaults.pinnedScrollBehavior(rememberTopAppBarState()) BitwardenScaffold( @@ -292,12 +299,21 @@ private fun SetupUnlockHeaderLandscape( @Composable private fun SetupUnlockScreenDialogs( dialogState: SetupUnlockState.DialogState?, + onDismissRequest: () -> Unit, ) { when (dialogState) { is SetupUnlockState.DialogState.Loading -> BitwardenLoadingDialog( visibilityState = LoadingDialogState.Shown(text = dialogState.title), ) + is SetupUnlockState.DialogState.Error -> BitwardenBasicDialog( + visibilityState = BasicDialogState.Shown( + title = dialogState.title, + message = dialogState.message, + ), + onDismissRequest = onDismissRequest, + ) + null -> Unit } } diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/accountsetup/SetupUnlockViewModel.kt b/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/accountsetup/SetupUnlockViewModel.kt index 201b1e06d1..f77051653e 100644 --- a/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/accountsetup/SetupUnlockViewModel.kt +++ b/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/accountsetup/SetupUnlockViewModel.kt @@ -56,6 +56,7 @@ class SetupUnlockViewModel @Inject constructor( SetupUnlockAction.ContinueClick -> handleContinueClick() SetupUnlockAction.EnableBiometricsClick -> handleEnableBiometricsClick() SetupUnlockAction.SetUpLaterClick -> handleSetUpLaterClick() + SetupUnlockAction.DismissDialog -> handleDismissDialog() is SetupUnlockAction.UnlockWithBiometricToggle -> { handleUnlockWithBiometricToggle(action) } @@ -70,18 +71,38 @@ class SetupUnlockViewModel @Inject constructor( } private fun handleEnableBiometricsClick() { - sendEvent( - SetupUnlockEvent.ShowBiometricsPrompt( - // Generate a new key in case the previous one was invalidated - cipher = biometricsEncryptionManager.createCipher(userId = state.userId), - ), - ) + biometricsEncryptionManager + .createCipherOrNull(userId = state.userId) + ?.let { + sendEvent( + SetupUnlockEvent.ShowBiometricsPrompt( + // Generate a new key in case the previous one was invalidated + cipher = it, + ), + ) + } + ?: run { + mutableStateFlow.update { + it.copy( + dialogState = SetupUnlockState.DialogState.Error( + title = R.string.an_error_has_occurred.asText(), + message = R.string.generic_error_message.asText(), + ), + ) + } + } } private fun handleSetUpLaterClick() { sendEvent(SetupUnlockEvent.NavigateToSetupAutofill) } + private fun handleDismissDialog() { + mutableStateFlow.update { + it.copy(dialogState = null) + } + } + private fun handleUnlockWithBiometricToggle( action: SetupUnlockAction.UnlockWithBiometricToggle, ) { @@ -182,6 +203,15 @@ data class SetupUnlockState( data class Loading( val title: Text, ) : DialogState() + + /** + * Displays an error dialog with a title, message, and an acknowledgement button. + */ + @Parcelize + data class Error( + val title: Text, + val message: Text, + ) : DialogState() } } @@ -235,6 +265,11 @@ sealed class SetupUnlockAction { */ data object SetUpLaterClick : SetupUnlockAction() + /** + * The user has dismissed the dialog. + */ + data object DismissDialog : SetupUnlockAction() + /** * Models actions that can be sent by the view model itself. */ diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/AccountSecurityScreen.kt b/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/AccountSecurityScreen.kt index e4be0292d9..1ee643c58c 100644 --- a/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/AccountSecurityScreen.kt +++ b/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/AccountSecurityScreen.kt @@ -376,6 +376,14 @@ private fun AccountSecurityDialogs( visibilityState = LoadingDialogState.Shown(text = dialogState.message), ) + is AccountSecurityDialog.Error -> BitwardenBasicDialog( + visibilityState = BasicDialogState.Shown( + title = dialogState.title, + message = dialogState.message, + ), + onDismissRequest = onDismissRequest, + ) + null -> Unit } } diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/AccountSecurityViewModel.kt b/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/AccountSecurityViewModel.kt index 5743ebd680..e06dbf0d1a 100644 --- a/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/AccountSecurityViewModel.kt +++ b/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/AccountSecurityViewModel.kt @@ -158,14 +158,26 @@ class AccountSecurityViewModel @Inject constructor( } private fun handleEnableBiometricsClick() { - sendEvent( - AccountSecurityEvent.ShowBiometricsPrompt( - // Generate a new key in case the previous one was invalidated - cipher = biometricsEncryptionManager.createCipher( - userId = state.userId, - ), - ), - ) + biometricsEncryptionManager + .createCipherOrNull(userId = state.userId) + ?.let { + sendEvent( + AccountSecurityEvent.ShowBiometricsPrompt( + // Generate a new key in case the previous one was invalidated + cipher = it, + ), + ) + } + ?: run { + mutableStateFlow.update { + it.copy( + dialog = AccountSecurityDialog.Error( + title = R.string.an_error_has_occurred.asText(), + message = R.string.generic_error_message.asText(), + ), + ) + } + } } private fun handleFingerPrintLearnMoreClick() { @@ -407,6 +419,15 @@ sealed class AccountSecurityDialog : Parcelable { data class Loading( val message: Text, ) : AccountSecurityDialog() + + /** + * Displays an error dialog with a title and message. + */ + @Parcelize + data class Error( + val title: Text, + val message: Text, + ) : AccountSecurityDialog() } /** diff --git a/app/src/test/java/com/x8bit/bitwarden/ui/auth/feature/accountsetup/SetupUnlockScreenTest.kt b/app/src/test/java/com/x8bit/bitwarden/ui/auth/feature/accountsetup/SetupUnlockScreenTest.kt index 4476930b46..938c031309 100644 --- a/app/src/test/java/com/x8bit/bitwarden/ui/auth/feature/accountsetup/SetupUnlockScreenTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/ui/auth/feature/accountsetup/SetupUnlockScreenTest.kt @@ -586,6 +586,40 @@ class SetupUnlockScreenTest : BaseComposeTest() { mutableStateFlow.update { it.copy(dialogState = null) } composeTestRule.assertNoDialogExists() } + + @Suppress("MaxLineLength") + @Test + fun `Error Dialog should be displayed according to state and send DismissDialog action on click`() { + val title = "title" + val message = "message" + composeTestRule.assertNoDialogExists() + + mutableStateFlow.update { + it.copy( + dialogState = SetupUnlockState.DialogState.Error( + title = title.asText(), + message = message.asText(), + ), + ) + } + + composeTestRule + .onAllNodesWithText(text = title) + .filterToOne(hasAnyAncestor(isDialog())) + .assertIsDisplayed() + + composeTestRule + .onAllNodesWithText("Ok") + .filterToOne(hasAnyAncestor(isDialog())) + .performClick() + + verify { + viewModel.trySendAction(SetupUnlockAction.DismissDialog) + } + + mutableStateFlow.update { it.copy(dialogState = null) } + composeTestRule.assertNoDialogExists() + } } private const val DEFAULT_USER_ID: String = "user_id" diff --git a/app/src/test/java/com/x8bit/bitwarden/ui/auth/feature/accountsetup/SetupUnlockViewModelTest.kt b/app/src/test/java/com/x8bit/bitwarden/ui/auth/feature/accountsetup/SetupUnlockViewModelTest.kt index 1b0318235e..a05bda7cd2 100644 --- a/app/src/test/java/com/x8bit/bitwarden/ui/auth/feature/accountsetup/SetupUnlockViewModelTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/ui/auth/feature/accountsetup/SetupUnlockViewModelTest.kt @@ -40,6 +40,7 @@ class SetupUnlockViewModelTest : BaseViewModelTest() { every { isBiometricIntegrityValid(userId = DEFAULT_USER_ID, cipher = CIPHER) } returns false + every { createCipherOrNull(DEFAULT_USER_ID) } returns CIPHER } @Test @@ -210,6 +211,56 @@ class SetupUnlockViewModelTest : BaseViewModelTest() { } } + @Test + fun `on DismissDialog should hide dialog`() { + val viewModel = createViewModel() + + viewModel.trySendAction(SetupUnlockAction.DismissDialog) + + assertEquals( + DEFAULT_STATE.copy(dialogState = null), + viewModel.stateFlow.value, + ) + } + + @Test + fun `EnableBiometricsClick action should create a new biometrics cipher and emit result`() = + runTest { + val viewModel = createViewModel() + + viewModel.trySendAction(SetupUnlockAction.EnableBiometricsClick) + + verify { + biometricsEncryptionManager.getOrCreateCipher(DEFAULT_USER_ID) + } + + viewModel.eventFlow.test { + assertEquals( + SetupUnlockEvent.ShowBiometricsPrompt(CIPHER), + awaitItem(), + ) + } + } + @Test + fun `EnableBiometricsClick actin should show error dialog when cipher is null`() { + every { + biometricsEncryptionManager.createCipherOrNull(DEFAULT_USER_ID) + } returns null + val viewModel = createViewModel() + + viewModel.trySendAction(SetupUnlockAction.EnableBiometricsClick) + + assertEquals( + DEFAULT_STATE.copy( + dialogState = SetupUnlockState.DialogState.Error( + title = R.string.an_error_has_occurred.asText(), + message = R.string.generic_error_message.asText(), + ), + ), + viewModel.stateFlow.value, + ) + } + private fun createViewModel( state: SetupUnlockState? = null, ): SetupUnlockViewModel = diff --git a/app/src/test/java/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/AccountSecurityScreenTest.kt b/app/src/test/java/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/AccountSecurityScreenTest.kt index acf046832f..dde89bc814 100644 --- a/app/src/test/java/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/AccountSecurityScreenTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/AccountSecurityScreenTest.kt @@ -1287,6 +1287,55 @@ class AccountSecurityScreenTest : BaseComposeTest() { verify { viewModel.trySendAction(AccountSecurityAction.DismissDialog) } } + @Test + fun `Error dialog should be shown or hidden according to state`() { + val title = "title" + val message = "message" + composeTestRule.assertNoDialogExists() + + mutableStateFlow.update { + it.copy( + dialog = AccountSecurityDialog.Error( + title = title.asText(), + message = message.asText(), + ), + ) + } + composeTestRule + .onNodeWithText(title) + .assert(hasAnyAncestor(isDialog())) + .assertIsDisplayed() + composeTestRule + .onNodeWithText(message) + .assert(hasAnyAncestor(isDialog())) + .assertIsDisplayed() + + mutableStateFlow.update { it.copy(dialog = null) } + + composeTestRule.assertNoDialogExists() + } + + @Test + fun `Error dialog dismiss should send DismissDialog`() { + val title = "title" + val message = "message" + mutableStateFlow.update { + it.copy( + dialog = AccountSecurityDialog.Error( + title = title.asText(), + message = message.asText(), + ), + ) + } + + composeTestRule + .onAllNodesWithText("Ok") + .filterToOne(hasAnyAncestor(isDialog())) + .performClick() + + verify { viewModel.trySendAction(AccountSecurityAction.DismissDialog) } + } + @Test fun `fingerprint phrase dialog should be shown or hidden according to the state`() { composeTestRule.onNode(isDialog()).assertDoesNotExist() diff --git a/app/src/test/java/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/AccountSecurityViewModelTest.kt b/app/src/test/java/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/AccountSecurityViewModelTest.kt index 815c84a1e4..133cbb9e77 100644 --- a/app/src/test/java/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/AccountSecurityViewModelTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/AccountSecurityViewModelTest.kt @@ -57,7 +57,7 @@ class AccountSecurityViewModelTest : BaseViewModelTest() { } private val mutableActivePolicyFlow = bufferedMutableSharedFlow>() private val biometricsEncryptionManager: BiometricsEncryptionManager = mockk { - every { createCipher(DEFAULT_USER_STATE.activeUserId) } returns CIPHER + every { createCipherOrNull(DEFAULT_USER_STATE.activeUserId) } returns CIPHER } private val policyManager: PolicyManager = mockk { every { @@ -341,6 +341,26 @@ class AccountSecurityViewModelTest : BaseViewModelTest() { } } + @Test + fun `on EnableBiometricsClick should show Error dialog when cipher is null`() { + every { + biometricsEncryptionManager.createCipherOrNull(DEFAULT_USER_STATE.activeUserId) + } returns null + val viewModel = createViewModel() + + viewModel.trySendAction(AccountSecurityAction.EnableBiometricsClick) + + assertEquals( + DEFAULT_STATE.copy( + dialog = AccountSecurityDialog.Error( + title = R.string.an_error_has_occurred.asText(), + message = R.string.generic_error_message.asText(), + ), + ), + viewModel.stateFlow.value, + ) + } + @Test fun `on UnlockWithBiometricToggle false should call clearBiometricsKey and update the state`() = runTest {