From 2863d8ddff29024eb49d6be9d358fb05346e4e21 Mon Sep 17 00:00:00 2001 From: Martin Asprusten Date: Thu, 29 Feb 2024 19:37:16 +0100 Subject: [PATCH] Require biometrics to delete key. Add logging to cleanup functions --- .../microg/gms/fido/core/RequestHandling.kt | 2 +- .../screenlock/ScreenLockCredentialStore.kt | 6 ++++- .../screenlock/ScreenLockTransportHandler.kt | 14 +++++++---- .../core/ui/CredentialSelectorFragment.kt | 23 +++++++++++-------- 4 files changed, 29 insertions(+), 16 deletions(-) diff --git a/play-services-fido/core/src/main/kotlin/org/microg/gms/fido/core/RequestHandling.kt b/play-services-fido/core/src/main/kotlin/org/microg/gms/fido/core/RequestHandling.kt index 5f167ec042..226bc4fb97 100644 --- a/play-services-fido/core/src/main/kotlin/org/microg/gms/fido/core/RequestHandling.kt +++ b/play-services-fido/core/src/main/kotlin/org/microg/gms/fido/core/RequestHandling.kt @@ -39,7 +39,7 @@ class UserInfo( @Parcelize class AuthenticatorResponseWrapper ( val responseChoices: List AuthenticatorResponse>>, - val deleteFunctions: List<() -> Unit> = listOf() + val deleteFunctions: List Boolean> = ArrayList() ) : Parcelable val RequestOptions.registerOptions: PublicKeyCredentialCreationOptions diff --git a/play-services-fido/core/src/main/kotlin/org/microg/gms/fido/core/transport/screenlock/ScreenLockCredentialStore.kt b/play-services-fido/core/src/main/kotlin/org/microg/gms/fido/core/transport/screenlock/ScreenLockCredentialStore.kt index d07f43ccd6..357ff2cf01 100644 --- a/play-services-fido/core/src/main/kotlin/org/microg/gms/fido/core/transport/screenlock/ScreenLockCredentialStore.kt +++ b/play-services-fido/core/src/main/kotlin/org/microg/gms/fido/core/transport/screenlock/ScreenLockCredentialStore.kt @@ -112,7 +112,9 @@ class ScreenLockCredentialStore(val context: Context) : SQLiteOpenHelper(context } fun deleteKey(rpId:String, keyId: ByteArray) { - keyStore.deleteEntry(getAlias(rpId, keyId)) + val keyAlias = getAlias(rpId, keyId) + Log.w(TAG, "Deleting key with alias $keyAlias") + keyStore.deleteEntry(keyAlias) } fun clearInvalidatedKeys() { @@ -129,6 +131,7 @@ class ScreenLockCredentialStore(val context: Context) : SQLiteOpenHelper(context val signature = Signature.getInstance("SHA256withECDSA") signature.initSign(key) } catch (e: KeyPermanentlyInvalidatedException) { + Log.w(TAG, "Removing permanently invalidated key with alias $alias") keysToDelete.add(alias) } catch (e: Exception) { // Any other exception, we just continue @@ -248,6 +251,7 @@ class ScreenLockCredentialStore(val context: Context) : SQLiteOpenHelper(context // Use prepared statements to avoid SQL injections val preparedDeleteStatement = db.compileStatement("DELETE FROM $TABLE_DISPLAY_NAMES WHERE $COLUMN_KEY_ALIAS = ?") for (aliasToDelete in aliasesToDelete) { + Log.w(TAG, "Removing userinfo for key with alias $aliasToDelete, since key no longer exists") preparedDeleteStatement.bindString(1, aliasToDelete) preparedDeleteStatement.executeUpdateDelete() } diff --git a/play-services-fido/core/src/main/kotlin/org/microg/gms/fido/core/transport/screenlock/ScreenLockTransportHandler.kt b/play-services-fido/core/src/main/kotlin/org/microg/gms/fido/core/transport/screenlock/ScreenLockTransportHandler.kt index 237f04a7b0..64f1c91da3 100644 --- a/play-services-fido/core/src/main/kotlin/org/microg/gms/fido/core/transport/screenlock/ScreenLockTransportHandler.kt +++ b/play-services-fido/core/src/main/kotlin/org/microg/gms/fido/core/transport/screenlock/ScreenLockTransportHandler.kt @@ -220,7 +220,7 @@ class ScreenLockTransportHandler(private val activity: FragmentActivity, callbac suspend fun sign( options: RequestOptions, callerPackage: String - ): Pair AuthenticatorAssertionResponse>>, List<() -> Unit>> { + ): Pair AuthenticatorAssertionResponse>>, List Boolean>> { if (options.type != RequestOptionsType.SIGN) throw RequestHandlingException(ErrorCode.INVALID_STATE_ERR) val candidates = mutableListOf() for (descriptor in options.signOptions.allowList.orEmpty()) { @@ -266,7 +266,7 @@ class ScreenLockTransportHandler(private val activity: FragmentActivity, callbac val (clientData, clientDataHash) = getClientDataAndHash(activity, options, callerPackage) val credentialList = ArrayList AuthenticatorAssertionResponse>>() - val deleteFunctions = ArrayList<() -> Unit>() + val deleteFunctions = ArrayList Boolean>() for (credentialId in candidates) { val keyId = credentialId.data @@ -287,8 +287,14 @@ class ScreenLockTransportHandler(private val activity: FragmentActivity, callbac ) } - val deleteFunction = { - store.deleteKey(options.rpId, keyId) + val deleteFunction = suspend { + try { + showBiometricPrompt(getApplicationName(activity, options, callerPackage), null) + store.deleteKey(options.rpId, keyId) + true + } catch (e: RequestHandlingException) { + false + } } credentialList.add(userInfo to responseCallable) diff --git a/play-services-fido/core/src/main/kotlin/org/microg/gms/fido/core/ui/CredentialSelectorFragment.kt b/play-services-fido/core/src/main/kotlin/org/microg/gms/fido/core/ui/CredentialSelectorFragment.kt index e14c2d6b59..8cddcbcc61 100644 --- a/play-services-fido/core/src/main/kotlin/org/microg/gms/fido/core/ui/CredentialSelectorFragment.kt +++ b/play-services-fido/core/src/main/kotlin/org/microg/gms/fido/core/ui/CredentialSelectorFragment.kt @@ -25,8 +25,8 @@ import org.microg.gms.fido.core.transport.Transport class CredentialListAdapter( private val responseWrapper: AuthenticatorResponseWrapper, private val listSelectionFunction: (suspend () -> AuthenticatorResponse) -> Unit, - private val deleteCredentialFunction: (() -> Unit) -> Unit - ) : BaseAdapter() { + private val deleteCredentialFunction: (suspend () -> Boolean) -> Unit +) : BaseAdapter() { override fun getCount(): Int { return responseWrapper.responseChoices.size } @@ -70,7 +70,6 @@ class CredentialListAdapter( return view } - } class CredentialSelectorFragment : AuthenticatorActivityFragment() { @@ -110,13 +109,17 @@ class CredentialSelectorFragment : AuthenticatorActivityFragment() { } } - fun credentialDeletion(deleteFunction: () -> Unit) { - deleteFunction.invoke() - if (!findNavController().navigateUp()) { - findNavController().navigate( - R.id.transportSelectionFragment, - arguments, - navOptions { popUpTo(R.id.usbFragment) { inclusive = true } }) + fun credentialDeletion(deleteFunction: suspend () -> Boolean) { + binding.fidoCredentialListView.isEnabled = false + authenticatorActivity?.lifecycleScope?.launch { + val deletionSucceeded = deleteFunction.invoke() + + // If credential is deleted, make leave the fragment, since the list is no longer valid + // There is probably some way to update the list, but it doesn't seem to work from inside + // the authenticatorActivity's lifecycleScope + if (deletionSucceeded) { + findNavController().navigateUp() + } } } } \ No newline at end of file