From ea0a7d298c7c7af6ad1254304ce03aec8c1e3f5f Mon Sep 17 00:00:00 2001 From: Adam Velebil Date: Thu, 12 Sep 2024 13:09:33 +0200 Subject: [PATCH 1/3] catch exceptions during initial NFC connections --- .../yubico/authenticator/AppContextManager.kt | 2 +- .../com/yubico/authenticator/MainActivity.kt | 86 +++++++++++-------- .../fido/FidoConnectionHelper.kt | 10 +-- .../yubico/authenticator/fido/FidoManager.kt | 15 ++-- .../yubico/authenticator/fido/data/Session.kt | 28 ++++++ .../yubico/authenticator/oath/OathManager.kt | 16 ++-- .../authenticator/yubikit/DeviceInfoHelper.kt | 10 +-- lib/android/fido/state.dart | 3 +- lib/app/views/main_page.dart | 3 +- lib/fido/views/delete_credential_dialog.dart | 23 +++-- lib/oath/views/manage_password_dialog.dart | 36 ++++---- lib/oath/views/rename_account_dialog.dart | 6 +- 12 files changed, 143 insertions(+), 95 deletions(-) diff --git a/android/app/src/main/kotlin/com/yubico/authenticator/AppContextManager.kt b/android/app/src/main/kotlin/com/yubico/authenticator/AppContextManager.kt index 4467b6ad..7d0af7c0 100755 --- a/android/app/src/main/kotlin/com/yubico/authenticator/AppContextManager.kt +++ b/android/app/src/main/kotlin/com/yubico/authenticator/AppContextManager.kt @@ -28,7 +28,7 @@ abstract class AppContextManager { open fun onPause() {} - open fun onError() {} + open fun onError(e: Exception) {} } class ContextDisposedException : Exception() \ No newline at end of file diff --git a/android/app/src/main/kotlin/com/yubico/authenticator/MainActivity.kt b/android/app/src/main/kotlin/com/yubico/authenticator/MainActivity.kt index 57301c5f..ae0b4a60 100644 --- a/android/app/src/main/kotlin/com/yubico/authenticator/MainActivity.kt +++ b/android/app/src/main/kotlin/com/yubico/authenticator/MainActivity.kt @@ -79,6 +79,7 @@ import kotlinx.coroutines.launch import org.json.JSONObject import org.slf4j.LoggerFactory import java.io.Closeable +import java.io.IOException import java.security.NoSuchAlgorithmException import java.util.concurrent.Executors import javax.crypto.Mac @@ -310,43 +311,58 @@ class MainActivity : FlutterFragmentActivity() { } private suspend fun processYubiKey(device: YubiKeyDevice) { - val deviceInfo = getDeviceInfo(device) + val deviceInfo = try { - if (deviceInfo == null) { - deviceManager.setDeviceInfo(null) - return - } - - if (device is NfcYubiKeyDevice) { - appMethodChannel.nfcStateChanged(NfcState.ONGOING) - } - - deviceManager.scpKeyParams = null - // If NFC and FIPS check for SCP11b key - if (device.transport == Transport.NFC && deviceInfo.fipsCapable != 0) { - logger.debug("Checking for usable SCP11b key...") - deviceManager.scpKeyParams = try { - device.withConnection { connection -> - val scp = SecurityDomainSession(connection) - val keyRef = scp.keyInformation.keys.firstOrNull { it.kid == ScpKid.SCP11b } - keyRef?.let { - val certs = scp.getCertificateBundle(it) - if (certs.isNotEmpty()) Scp11KeyParams( - keyRef, - certs[certs.size - 1].publicKey - ) else null - }?.also { - logger.debug("Found SCP11b key: {}", keyRef) - } - } - } catch (e: Exception) { - logger.debug("Exception while getting scp keys: ", e) - contextManager?.onError() - if (device is NfcYubiKeyDevice) { - appMethodChannel.nfcStateChanged(NfcState.FAILURE) - } - null + if (device is NfcYubiKeyDevice) { + appMethodChannel.nfcStateChanged(NfcState.ONGOING) } + + val deviceInfo = getDeviceInfo(device) + + deviceManager.scpKeyParams = null + // If NFC and FIPS check for SCP11b key + if (device.transport == Transport.NFC && deviceInfo.fipsCapable != 0) { + logger.debug("Checking for usable SCP11b key...") + deviceManager.scpKeyParams = try { + device.withConnection { connection -> + val scp = SecurityDomainSession(connection) + val keyRef = scp.keyInformation.keys.firstOrNull { it.kid == ScpKid.SCP11b } + keyRef?.let { + val certs = scp.getCertificateBundle(it) + if (certs.isNotEmpty()) Scp11KeyParams( + keyRef, + certs[certs.size - 1].publicKey + ) else null + }?.also { + logger.debug("Found SCP11b key: {}", keyRef) + } + } + } catch (e: Exception) { + logger.error("Exception when reading SCP key information: ", e) + // we throw IO exception to unify handling failures as we don't want + // th clear device info + throw IOException("Failure getting SCP keys") + } + } + deviceInfo + } catch (e: Exception) { + logger.debug("Exception while getting device info and scp keys: ", e) + contextManager?.onError(e) + if (device is NfcYubiKeyDevice) { + logger.debug("Setting NFC state to failure") + appMethodChannel.nfcStateChanged(NfcState.FAILURE) + } + + // do not clear the device info when IOException's occur, + // this allows for retries of failed actions + if (e !is IOException) { + logger.debug("Resetting device info") + deviceManager.setDeviceInfo(null) + } else { + logger.debug("Keeping device info") + } + + return } // this YubiKey provides SCP11b key but the phone cannot perform AESCMAC diff --git a/android/app/src/main/kotlin/com/yubico/authenticator/fido/FidoConnectionHelper.kt b/android/app/src/main/kotlin/com/yubico/authenticator/fido/FidoConnectionHelper.kt index bc81e92e..8ead1d5b 100644 --- a/android/app/src/main/kotlin/com/yubico/authenticator/fido/FidoConnectionHelper.kt +++ b/android/app/src/main/kotlin/com/yubico/authenticator/fido/FidoConnectionHelper.kt @@ -42,14 +42,6 @@ class FidoConnectionHelper(private val deviceManager: DeviceManager) { return requestHandled } - fun failPending(e: Exception) { - pendingAction?.let { action -> - logger.error("Failing pending action with {}", e.message) - action.invoke(Result.failure(e)) - pendingAction = null - } - } - fun cancelPending() { pendingAction?.let { action -> action.invoke(Result.failure(CancellationException())) @@ -80,7 +72,7 @@ class FidoConnectionHelper(private val deviceManager: DeviceManager) { block(YubiKitFidoSession(it)) }.also { if (updateDeviceInfo) { - deviceManager.setDeviceInfo(getDeviceInfo(device)) + deviceManager.setDeviceInfo(runCatching { getDeviceInfo(device) }.getOrNull()) } } diff --git a/android/app/src/main/kotlin/com/yubico/authenticator/fido/FidoManager.kt b/android/app/src/main/kotlin/com/yubico/authenticator/fido/FidoManager.kt index 8f6908ae..581a821c 100644 --- a/android/app/src/main/kotlin/com/yubico/authenticator/fido/FidoManager.kt +++ b/android/app/src/main/kotlin/com/yubico/authenticator/fido/FidoManager.kt @@ -174,9 +174,9 @@ class FidoManager( } } - override fun onError() { - super.onError() - logger.debug("Cancel any pending action because of upstream error") + override fun onError(e: Exception) { + super.onError(e) + logger.error("Cancelling pending action. Cause: ", e) connectionHelper.cancelPending() } @@ -204,13 +204,12 @@ class FidoManager( } if (updateDeviceInfo.getAndSet(false)) { - deviceManager.setDeviceInfo(getDeviceInfo(device)) + deviceManager.setDeviceInfo(runCatching { getDeviceInfo(device) }.getOrNull()) } } catch (e: Exception) { - // something went wrong, try to get DeviceInfo from any available connection type - logger.error("Failure when processing YubiKey: ", e) - connectionHelper.failPending(e) + logger.error("Cancelling pending action. Cause: ", e) + connectionHelper.cancelPending() if (e !is IOException) { // we don't clear the session on IOExceptions so that the session is ready for @@ -240,7 +239,7 @@ class FidoManager( currentSession ) - val sameDevice = currentSession == previousSession + val sameDevice = currentSession.sameDevice(previousSession) if (device is NfcYubiKeyDevice && (sameDevice || resetHelper.inProgress)) { requestHandled = connectionHelper.invokePending(fidoSession) diff --git a/android/app/src/main/kotlin/com/yubico/authenticator/fido/data/Session.kt b/android/app/src/main/kotlin/com/yubico/authenticator/fido/data/Session.kt index 93cd770b..835a3eca 100644 --- a/android/app/src/main/kotlin/com/yubico/authenticator/fido/data/Session.kt +++ b/android/app/src/main/kotlin/com/yubico/authenticator/fido/data/Session.kt @@ -41,6 +41,19 @@ data class Options( infoData.getOptionsBoolean("ep"), ) + fun sameDevice(other: Options) : Boolean { + if (this === other) return true + + if (clientPin != other.clientPin) return false + if (credMgmt != other.credMgmt) return false + if (credentialMgmtPreview != other.credentialMgmtPreview) return false + if (bioEnroll != other.bioEnroll) return false + // alwaysUv may differ + // ep may differ + + return true + } + companion object { private fun InfoData.getOptionsBoolean( key: String @@ -67,6 +80,21 @@ data class SessionInfo( infoData.remainingDiscoverableCredentials ) + // this is a more permissive comparison, which does not take in an account properties, + // which might change by using the FIDO authenticator + fun sameDevice(other: SessionInfo?): Boolean { + if (other == null) return false + if (this === other) return true + + if (!options.sameDevice(other.options)) return false + if (!aaguid.contentEquals(other.aaguid)) return false + // minPinLength may differ + // forcePinChange may differ + // remainingDiscoverableCredentials may differ + + return true + } + override fun equals(other: Any?): Boolean { if (this === other) return true if (javaClass != other?.javaClass) return false diff --git a/android/app/src/main/kotlin/com/yubico/authenticator/oath/OathManager.kt b/android/app/src/main/kotlin/com/yubico/authenticator/oath/OathManager.kt index c8073b38..9f78a6a5 100644 --- a/android/app/src/main/kotlin/com/yubico/authenticator/oath/OathManager.kt +++ b/android/app/src/main/kotlin/com/yubico/authenticator/oath/OathManager.kt @@ -110,9 +110,9 @@ class OathManager( private val updateDeviceInfo = AtomicBoolean(false) private var deviceInfoTimer: TimerTask? = null - override fun onError() { - super.onError() - logger.debug("Cancel any pending action because of upstream error") + override fun onError(e: Exception) { + super.onError(e) + logger.error("Cancelling pending action in onError. Cause: ", e) pendingAction?.let { action -> action.invoke(Result.failure(CancellationException())) pendingAction = null @@ -343,16 +343,16 @@ class OathManager( ) if (updateDeviceInfo.getAndSet(false)) { - deviceManager.setDeviceInfo(getDeviceInfo(device)) + deviceManager.setDeviceInfo(runCatching { getDeviceInfo(device) }.getOrNull()) } } catch (e: Exception) { // OATH not enabled/supported, try to get DeviceInfo over other USB interfaces logger.error("Exception during SmartCard connection/OATH session creation: ", e) - // Remove any pending action + // Cancel any pending action pendingAction?.let { action -> - logger.error("Failing pending action with {}", e.message) - action.invoke(Result.failure(e)) + logger.error("Cancelling pending action. Cause: ", e) + action.invoke(Result.failure(CancellationException())) pendingAction = null } @@ -782,7 +782,7 @@ class OathManager( block(getOathSession(it)) }.also { if (updateDeviceInfo) { - deviceManager.setDeviceInfo(getDeviceInfo(device)) + deviceManager.setDeviceInfo(runCatching { getDeviceInfo(device) }.getOrNull()) } } diff --git a/android/app/src/main/kotlin/com/yubico/authenticator/yubikit/DeviceInfoHelper.kt b/android/app/src/main/kotlin/com/yubico/authenticator/yubikit/DeviceInfoHelper.kt index a0f04d77..938014a3 100644 --- a/android/app/src/main/kotlin/com/yubico/authenticator/yubikit/DeviceInfoHelper.kt +++ b/android/app/src/main/kotlin/com/yubico/authenticator/yubikit/DeviceInfoHelper.kt @@ -47,17 +47,17 @@ class DeviceInfoHelper { private val restrictedNfcBytes = byteArrayOf(0x00, 0x1F, 0xD1.toByte(), 0x01, 0x1b, 0x55, 0x04) + uri - suspend fun getDeviceInfo(device: YubiKeyDevice): Info? { + suspend fun getDeviceInfo(device: YubiKeyDevice): Info { SessionVersionOverride.set(null) var deviceInfo = readDeviceInfo(device) - if (deviceInfo?.version?.major == 0.toByte()) { + if (deviceInfo.version.major == 0.toByte()) { SessionVersionOverride.set(Version(5, 7, 2)) deviceInfo = readDeviceInfo(device) } return deviceInfo } - private suspend fun readDeviceInfo(device: YubiKeyDevice): Info? { + private suspend fun readDeviceInfo(device: YubiKeyDevice): Info { val pid = (device as? UsbYubiKeyDevice)?.pid val deviceInfo = runCatching { @@ -106,8 +106,8 @@ class DeviceInfoHelper { } } catch (e: Exception) { // no smart card connectivity - logger.error("Failure getting device info", e) - return null + logger.error("Failure getting device info: ", e) + throw e } } diff --git a/lib/android/fido/state.dart b/lib/android/fido/state.dart index 3afcfc5f..5daf430e 100644 --- a/lib/android/fido/state.dart +++ b/lib/android/fido/state.dart @@ -365,9 +365,8 @@ class _FidoCredentialsNotifier extends FidoCredentialsNotifier { var decodedException = pe.decode(); if (decodedException is CancellationException) { _log.debug('User cancelled delete credential FIDO operation'); - } else { - throw decodedException; } + throw decodedException; } } } diff --git a/lib/app/views/main_page.dart b/lib/app/views/main_page.dart index c94e435a..5df54e3e 100755 --- a/lib/app/views/main_page.dart +++ b/lib/app/views/main_page.dart @@ -63,7 +63,8 @@ class MainPage extends ConsumerWidget { final prevSerial = prev?.hasValue == true ? prev?.value?.info.serial : null; if ((serial != null && serial == prevSerial) || - (next.hasValue && (prev != null && prev.isLoading))) { + (next.hasValue && (prev != null && prev.isLoading)) || + next.isLoading) { return; } diff --git a/lib/fido/views/delete_credential_dialog.dart b/lib/fido/views/delete_credential_dialog.dart index 0a07dc7b..d2ea2fbc 100755 --- a/lib/fido/views/delete_credential_dialog.dart +++ b/lib/fido/views/delete_credential_dialog.dart @@ -23,6 +23,7 @@ import 'package:flutter_riverpod/flutter_riverpod.dart'; import '../../app/message.dart'; import '../../app/models.dart'; import '../../app/state.dart'; +import '../../exception/cancellation_exception.dart'; import '../../widgets/responsive_dialog.dart'; import '../models.dart'; import '../state.dart'; @@ -57,15 +58,19 @@ class DeleteCredentialDialog extends ConsumerWidget { actions: [ TextButton( onPressed: () async { - await ref - .read(credentialProvider(devicePath).notifier) - .deleteCredential(credential); - await ref.read(withContextProvider)( - (context) async { - Navigator.of(context).pop(true); - showMessage(context, l10n.s_passkey_deleted); - }, - ); + try { + await ref + .read(credentialProvider(devicePath).notifier) + .deleteCredential(credential); + await ref.read(withContextProvider)( + (context) async { + Navigator.of(context).pop(true); + showMessage(context, l10n.s_passkey_deleted); + }, + ); + } on CancellationException catch (_) { + // ignored + } }, child: Text(l10n.s_delete), ), diff --git a/lib/oath/views/manage_password_dialog.dart b/lib/oath/views/manage_password_dialog.dart index 14c34773..782db8fb 100755 --- a/lib/oath/views/manage_password_dialog.dart +++ b/lib/oath/views/manage_password_dialog.dart @@ -22,6 +22,7 @@ import 'package:material_symbols_icons/symbols.dart'; import '../../app/message.dart'; import '../../app/models.dart'; import '../../app/state.dart'; +import '../../exception/cancellation_exception.dart'; import '../../management/models.dart'; import '../../widgets/app_input_decoration.dart'; import '../../widgets/app_text_field.dart'; @@ -71,23 +72,28 @@ class _ManagePasswordDialogState extends ConsumerState { _submit() async { _removeFocus(); - final result = await ref - .read(oathStateProvider(widget.path).notifier) - .setPassword(_currentPasswordController.text, _newPassword); - if (result) { - if (mounted) { - await ref.read(withContextProvider)((context) async { - Navigator.of(context).pop(); - showMessage(context, AppLocalizations.of(context)!.s_password_set); + try { + final result = await ref + .read(oathStateProvider(widget.path).notifier) + .setPassword(_currentPasswordController.text, _newPassword); + if (result) { + if (mounted) { + await ref.read(withContextProvider)((context) async { + Navigator.of(context).pop(); + showMessage(context, AppLocalizations.of(context)!.s_password_set); + }); + } + } else { + _currentPasswordController.selection = TextSelection( + baseOffset: 0, + extentOffset: _currentPasswordController.text.length); + _currentPasswordFocus.requestFocus(); + setState(() { + _currentIsWrong = true; }); } - } else { - _currentPasswordController.selection = TextSelection( - baseOffset: 0, extentOffset: _currentPasswordController.text.length); - _currentPasswordFocus.requestFocus(); - setState(() { - _currentIsWrong = true; - }); + } on CancellationException catch (_) { + // ignored } } diff --git a/lib/oath/views/rename_account_dialog.dart b/lib/oath/views/rename_account_dialog.dart index cd6995d5..b2916be0 100755 --- a/lib/oath/views/rename_account_dialog.dart +++ b/lib/oath/views/rename_account_dialog.dart @@ -90,7 +90,7 @@ class RenameAccountDialog extends ConsumerStatefulWidget { context, AppLocalizations.of(context)!.s_account_renamed)); return renamed; } on CancellationException catch (_) { - // ignored + return CancellationException(); } catch (e) { _log.error('Failed to rename account', e); final String errorMessage; @@ -140,7 +140,9 @@ class _RenameAccountDialogState extends ConsumerState { final nav = Navigator.of(context); final renamed = await widget.rename(_issuer.isNotEmpty ? _issuer : null, _name); - nav.pop(renamed); + if (renamed is! CancellationException) { + nav.pop(renamed); + } } @override From ce95119a73fdf65b963d30a05f5def4913251a04 Mon Sep 17 00:00:00 2001 From: Adam Velebil Date: Thu, 12 Sep 2024 14:49:12 +0200 Subject: [PATCH 2/3] review fixes --- .../src/main/kotlin/com/yubico/authenticator/MainActivity.kt | 5 +---- lib/fido/views/delete_credential_dialog.dart | 2 +- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/android/app/src/main/kotlin/com/yubico/authenticator/MainActivity.kt b/android/app/src/main/kotlin/com/yubico/authenticator/MainActivity.kt index ae0b4a60..a543dfc2 100644 --- a/android/app/src/main/kotlin/com/yubico/authenticator/MainActivity.kt +++ b/android/app/src/main/kotlin/com/yubico/authenticator/MainActivity.kt @@ -349,17 +349,14 @@ class MainActivity : FlutterFragmentActivity() { logger.debug("Exception while getting device info and scp keys: ", e) contextManager?.onError(e) if (device is NfcYubiKeyDevice) { - logger.debug("Setting NFC state to failure") appMethodChannel.nfcStateChanged(NfcState.FAILURE) } - // do not clear the device info when IOException's occur, + // do not clear deviceInfo on IOExceptions, // this allows for retries of failed actions if (e !is IOException) { logger.debug("Resetting device info") deviceManager.setDeviceInfo(null) - } else { - logger.debug("Keeping device info") } return diff --git a/lib/fido/views/delete_credential_dialog.dart b/lib/fido/views/delete_credential_dialog.dart index d2ea2fbc..1a7f49df 100755 --- a/lib/fido/views/delete_credential_dialog.dart +++ b/lib/fido/views/delete_credential_dialog.dart @@ -1,5 +1,5 @@ /* - * Copyright (C) 2022 Yubico. + * Copyright (C) 2022-2024 Yubico. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. From 3f8e69a51056abe927da4244631261fdc31a7567 Mon Sep 17 00:00:00 2001 From: Adam Velebil Date: Fri, 13 Sep 2024 08:37:57 +0200 Subject: [PATCH 3/3] refactor exception handling in rename dialog --- lib/oath/views/rename_account_dialog.dart | 86 +++++++++++------------ 1 file changed, 40 insertions(+), 46 deletions(-) diff --git a/lib/oath/views/rename_account_dialog.dart b/lib/oath/views/rename_account_dialog.dart index b2916be0..6bbfbdb3 100755 --- a/lib/oath/views/rename_account_dialog.dart +++ b/lib/oath/views/rename_account_dialog.dart @@ -67,49 +67,15 @@ class RenameAccountDialog extends ConsumerStatefulWidget { OathCredential credential, List<(String? issuer, String name)> existing) { return RenameAccountDialog( - devicePath: devicePath, - issuer: credential.issuer, - name: credential.name, - oathType: credential.oathType, - period: credential.period, - existing: existing, - rename: (issuer, name) async { - final withContext = ref.read(withContextProvider); - try { - // Rename credentials - final renamed = await ref - .read(credentialListProvider(devicePath).notifier) - .renameAccount(credential, issuer, name); - - // Update favorite - ref - .read(favoritesProvider.notifier) - .renameCredential(credential.id, renamed.id); - - await withContext((context) async => showMessage( - context, AppLocalizations.of(context)!.s_account_renamed)); - return renamed; - } on CancellationException catch (_) { - return CancellationException(); - } catch (e) { - _log.error('Failed to rename account', e); - final String errorMessage; - // TODO: Make this cleaner than importing desktop specific RpcError. - if (e is RpcError) { - errorMessage = e.message; - } else { - errorMessage = e.toString(); - } - await withContext((context) async => showMessage( - context, - AppLocalizations.of(context)! - .l_rename_account_failed(errorMessage), - duration: const Duration(seconds: 4), - )); - return null; - } - }, - ); + devicePath: devicePath, + issuer: credential.issuer, + name: credential.name, + oathType: credential.oathType, + period: credential.period, + existing: existing, + rename: (issuer, name) async => await ref + .read(credentialListProvider(devicePath).notifier) + .renameAccount(credential, issuer, name)); } } @@ -138,10 +104,38 @@ class _RenameAccountDialogState extends ConsumerState { _issuerFocus.unfocus(); _nameFocus.unfocus(); final nav = Navigator.of(context); - final renamed = - await widget.rename(_issuer.isNotEmpty ? _issuer : null, _name); - if (renamed is! CancellationException) { + final withContext = ref.read(withContextProvider); + + try { + // Rename credentials + final renamed = + await widget.rename(_issuer.isNotEmpty ? _issuer : null, _name); + + // Update favorite + ref + .read(favoritesProvider.notifier) + .renameCredential(renamed.id, renamed.id); + + await withContext((context) async => showMessage( + context, AppLocalizations.of(context)!.s_account_renamed)); + nav.pop(renamed); + } on CancellationException catch (_) { + // ignored + } catch (e) { + _log.error('Failed to rename account', e); + final String errorMessage; + // TODO: Make this cleaner than importing desktop specific RpcError. + if (e is RpcError) { + errorMessage = e.message; + } else { + errorMessage = e.toString(); + } + await withContext((context) async => showMessage( + context, + AppLocalizations.of(context)!.l_rename_account_failed(errorMessage), + duration: const Duration(seconds: 4), + )); } }