From 73c14d87f8981cd9ece21e9fb310eec8c1254815 Mon Sep 17 00:00:00 2001 From: Andrew Kozlik Date: Mon, 20 Feb 2023 11:52:24 +0100 Subject: [PATCH] feat(core): Ignore channel ID in U2F. Safari browser changes the CID for every single operation. We need to keep up the same U2F dialog for different CIDs as long as the dialog corresponds to the same request data. --- core/.changelog.d/2205.changed | 1 + core/src/apps/webauthn/fido2.py | 59 +++++++++++++++------------------ 2 files changed, 28 insertions(+), 32 deletions(-) create mode 100644 core/.changelog.d/2205.changed diff --git a/core/.changelog.d/2205.changed b/core/.changelog.d/2205.changed new file mode 100644 index 000000000..799b01ec2 --- /dev/null +++ b/core/.changelog.d/2205.changed @@ -0,0 +1 @@ +Ignore channel ID in U2F. diff --git a/core/src/apps/webauthn/fido2.py b/core/src/apps/webauthn/fido2.py index 3790102de..0b99867c0 100644 --- a/core/src/apps/webauthn/fido2.py +++ b/core/src/apps/webauthn/fido2.py @@ -216,8 +216,8 @@ _MAX_CRED_COUNT_IN_LIST = const(10) # The CID of the last WINK command. Used to ensure that we do only one WINK at a time on any given CID. _last_wink_cid = 0 -# The CID of the last successful U2F_AUTHENTICATE check-only request. -_last_good_auth_check_cid = 0 +# Indicates whether the last U2F_AUTHENTICATE had a valid key handle. +_last_auth_valid = False class CborError(Exception): @@ -544,10 +544,7 @@ async def handle_reports(iface: io.HID) -> None: req = await read_cmd(iface) if req is None: continue - if dialog_mgr.is_busy() and req.cid not in ( - dialog_mgr.get_cid(), - _CID_BROADCAST, - ): + if not dialog_mgr.allow_cid(req.cid): resp: Cmd | None = cmd_error(req.cid, _ERR_CHANNEL_BUSY) else: resp = dispatch_cmd(req, dialog_mgr) @@ -589,6 +586,10 @@ class State: self.iface = iface self.finished = False + def allow_cid(self, cid: int) -> bool: + # Generally allow any CID, because Safari browser changes CID for every U2F message. + return True + def keepalive_status(self) -> int: # Run the keepalive loop to check for timeout, but do not send any keepalive messages. return _KEEPALIVE_STATUS_NONE @@ -633,14 +634,9 @@ class U2fState(State, ConfirmInfo): class U2fConfirmRegister(U2fState): - def __init__( - self, cid: int, iface: io.HID, req_data: bytes, cred: U2fCredential - ) -> None: - super().__init__(cid, iface, req_data, cred) - async def confirm_dialog(self) -> bool: if self._cred.rp_id_hash in _BOGUS_APPIDS: - if self.cid == _last_good_auth_check_cid: + if _last_auth_valid: await show_popup( title="U2F", subtitle="Already registered.", @@ -663,18 +659,11 @@ class U2fConfirmRegister(U2fState): def __eq__(self, other: object) -> bool: return ( - isinstance(other, U2fConfirmRegister) - and self.cid == other.cid - and self._req_data == other._req_data + isinstance(other, U2fConfirmRegister) and self._req_data == other._req_data ) class U2fConfirmAuthenticate(U2fState): - def __init__( - self, cid: int, iface: io.HID, req_data: bytes, cred: Credential - ) -> None: - super().__init__(cid, iface, req_data, cred) - def get_header(self) -> str: return "U2F Authenticate" @@ -684,7 +673,6 @@ class U2fConfirmAuthenticate(U2fState): def __eq__(self, other: object) -> bool: return ( isinstance(other, U2fConfirmAuthenticate) - and self.cid == other.cid and self._req_data == other._req_data ) @@ -709,8 +697,9 @@ class U2fUnlock(State): class Fido2State(State): - def __init__(self, cid: int, iface: io.HID) -> None: - super().__init__(cid, iface) + def allow_cid(self, cid: int) -> bool: + # In FIDO2 lock out other channels until user interaction or timeout. + return cid == self.cid def keepalive_status(self) -> int: return _KEEPALIVE_STATUS_UP_NEEDED @@ -908,6 +897,10 @@ class Fido2ConfirmGetAssertion(Fido2State, ConfirmInfo, Pageable): class Fido2ConfirmNoPin(State): + def allow_cid(self, cid: int) -> bool: + # In FIDO2 lock out other channels until user interaction or timeout. + return cid == self.cid + def timeout_ms(self) -> int: return _FIDO2_CONFIRM_TIMEOUT_MS @@ -948,9 +941,6 @@ class Fido2ConfirmNoCredentials(Fido2ConfirmGetAssertion): class Fido2ConfirmReset(Fido2State): - def __init__(self, cid: int, iface: io.HID) -> None: - super().__init__(cid, iface) - async def confirm_dialog(self) -> bool: return await confirm_webauthn_reset() @@ -987,10 +977,12 @@ class DialogManager: loop.close(self.keepalive) self._clear() - def get_cid(self) -> int: - if self.state is None: - return 0 - return self.state.cid + def allow_cid(self, cid: int) -> bool: + return ( + not self.is_busy() + or cid == _CID_BROADCAST + or (self.state is not None and self.state.allow_cid(cid)) + ) def is_busy(self) -> bool: if utime.ticks_ms() >= self.deadline: @@ -1266,6 +1258,9 @@ def msg_register_sign(challenge: bytes, cred: U2fCredential) -> bytes: def msg_authenticate(req: Msg, dialog_mgr: DialogManager) -> Cmd: + global _last_auth_valid + _last_auth_valid = False + if not config.is_unlocked(): new_state: State = U2fUnlock(req.cid, dialog_mgr.iface) dialog_mgr.set_state(new_state) @@ -1296,12 +1291,12 @@ def msg_authenticate(req: Msg, dialog_mgr: DialogManager) -> Cmd: # specific error logged in _node_from_key_handle return msg_error(req.cid, _SW_WRONG_DATA) + _last_auth_valid = True + # if _AUTH_CHECK_ONLY is requested, return, because keyhandle has been checked already if req.p1 == _AUTH_CHECK_ONLY: if __debug__: log.info(__name__, "_AUTH_CHECK_ONLY") - global _last_good_auth_check_cid - _last_good_auth_check_cid = req.cid return msg_error(req.cid, _SW_CONDITIONS_NOT_SATISFIED) # from now on, only _AUTH_ENFORCE is supported