From 66c112acbf4e2f3534e6acbb4719d68885e31103 Mon Sep 17 00:00:00 2001 From: Niklas Dusenlund Date: Fri, 23 May 2025 12:42:34 +0200 Subject: [PATCH 1/2] u2f-tests: compile on darwin --- Makefile | 9 ++++- test/unit-test/CMakeLists.txt | 66 +++++++++++++++++++---------------- 2 files changed, 43 insertions(+), 32 deletions(-) diff --git a/Makefile b/Makefile index 6a844b9f6..6af57d9f1 100644 --- a/Makefile +++ b/Makefile @@ -14,8 +14,15 @@ # This makefile is used as a command runner and not for tracking dependencies between recipies +UNAME_S := $(shell uname -s) + .DEFAULT_GOAL := firmware -SANITIZE ?= ON +# asan/ubsan is not supported on darwin, default to off +ifeq ($(UNAME_S),Darwin) + SANITIZE ?= OFF +else + SANITIZE ?= ON +endif simulator: SANITIZE = OFF bootstrap: diff --git a/test/unit-test/CMakeLists.txt b/test/unit-test/CMakeLists.txt index 9f221ede3..404798db8 100644 --- a/test/unit-test/CMakeLists.txt +++ b/test/unit-test/CMakeLists.txt @@ -14,10 +14,6 @@ # No linker for Mach-O supports the linker argument `--wrap`. Since we use # that, unit tests will never work on macos. Use linux/arm64 in docker instead. -if(${CMAKE_SYSTEM_NAME} MATCHES "Darwin") - message("No linker for Mach-O supports --wrap, will not generate unit-tests") - return() -endif() # We use FindPkgConfig instead of FindPackage because it finds libraries in # both linux and macos @@ -26,7 +22,11 @@ find_package(PkgConfig REQUIRED) # Unit testing uses CMocka pkg_check_modules(CMOCKA REQUIRED cmocka) # u2f tests with hardware uses hidapi-hidraw -pkg_check_modules(HIDAPI REQUIRED hidapi-hidraw) +if(${CMAKE_SYSTEM_NAME} MATCHES "Darwin") + pkg_check_modules(HIDAPI REQUIRED hidapi) +else() + pkg_check_modules(HIDAPI REQUIRED hidapi-hidraw) +endif() #----------------------------------------------------------------------------- # Build bitbox lib to use in tests @@ -176,7 +176,7 @@ target_compile_definitions(bitbox PUBLIC TESTING _UNIT_TEST_) target_link_libraries(bitbox PUBLIC secp256k1 - ${CMOCKA_LIBRARIES} + ${CMOCKA_LDFLAGS} PRIVATE wallycore fatfs @@ -219,7 +219,7 @@ target_include_directories( target_compile_definitions(u2f-util PUBLIC "TESTING" _UNIT_TEST_ PRODUCT_BITBOX_MULTI "APP_U2F=1" "APP_BTC=1" "APP_LTC=1" "APP_ETH=1") target_compile_definitions(u2f-util PUBLIC "USE_KECCAK") -target_link_libraries(u2f-util PUBLIC ${HIDAPI_LIBRARIES}) +target_link_libraries(u2f-util PUBLIC ${HIDAPI_LDFLAGS}) #----------------------------------------------------------------------------- @@ -258,28 +258,32 @@ set(TEST_LIST "" ) -list(LENGTH TEST_LIST TEST_LIST_LEN) -math(EXPR TEST_LIST_LEN ${TEST_LIST_LEN}-1) -foreach(I RANGE 0 ${TEST_LIST_LEN} 2) - math(EXPR I2 ${I}+1) - list(GET TEST_LIST ${I} TEST_NAME) - list(GET TEST_LIST ${I2} TEST_LINK_ARGS) - set(EXE test_${TEST_NAME}) - add_executable(${EXE} test_${TEST_NAME}.c framework/eh_personality.c) - # asan must be first library in linking order - target_link_libraries(${EXE} PRIVATE - $<$:asan> - $<$:-fsanitize=undefined> - -Wl,--start-group - c-unit-tests_rust_c - bitbox - -Wl,--end-group - ${TEST_LINK_ARGS} - ) - if(NOT ${TEST_NAME} STREQUAL "simulator") - add_test(NAME test_${TEST_NAME} COMMAND ${EXE}) - endif() -endforeach() +if(${CMAKE_SYSTEM_NAME} MATCHES "Darwin") + message("No linker for Mach-O supports --wrap, will not generate unit-tests") +else() + list(LENGTH TEST_LIST TEST_LIST_LEN) + math(EXPR TEST_LIST_LEN ${TEST_LIST_LEN}-1) + foreach(I RANGE 0 ${TEST_LIST_LEN} 2) + math(EXPR I2 ${I}+1) + list(GET TEST_LIST ${I} TEST_NAME) + list(GET TEST_LIST ${I2} TEST_LINK_ARGS) + set(EXE test_${TEST_NAME}) + add_executable(${EXE} test_${TEST_NAME}.c framework/eh_personality.c) + # asan must be first library in linking order + target_link_libraries(${EXE} PRIVATE + $<$:asan> + $<$:-fsanitize=undefined> + -Wl,--start-group + c-unit-tests_rust_c + bitbox + -Wl,--end-group + ${TEST_LINK_ARGS} + ) + if(NOT ${TEST_NAME} STREQUAL "simulator") + add_test(NAME test_${TEST_NAME} COMMAND ${EXE}) + endif() + endforeach() +endif() # These unit tests for U2F are special because they don't call any bitbox functions directly, instead they go through hid_read/write. @@ -297,10 +301,10 @@ foreach(TEST_NAME ${U2F_TESTS}) target_link_libraries(${EXE} PRIVATE $<$:asan> $<$:-fsanitize=undefined> - -Wl,--start-group + $<$>:-Wl,--start-group> c-unit-tests_rust_c bitbox - -Wl,--end-group + $<$>:-Wl,--end-group> u2f-util ) target_compile_definitions(${EXE} PRIVATE "TESTING") From 17dd5526696c6aa9f90f73161f7b13b693990562 Mon Sep 17 00:00:00 2001 From: Niklas Dusenlund Date: Fri, 23 May 2025 23:14:13 +0200 Subject: [PATCH 2/2] U2F: Fix on safari/macos U2F is typically a stateless protocol, so between two transactions (request+response) there is not supposed to be any lock held. It was assumed that the "FIDO Client" (typically web browser) would keep sending only U2F `register` or `authenticate` messages when it had started doing so. But it is also legal to send for example a new U2FHID `init` message in between. (the U2F reference test cases do this for example). Before this commit we held a "lock" while the confirm screen was active until the user had confirmed and didn't allow any other messages in between. This breaks the U2F protocol as explained above. After this commit we instead only hold the "lock" for the transaction (request + response), which is the correct way according to the u2f reference: The application channel that manages to get through the first initialization packet when the device is in idle state will keep the device locked for other channels until the last packet of the response message has been received. The device then returns to idle state, ready to perform another transaction for the same or a different channel. Between two transactions, no state is maintained in the device and a host application must assume that any other process may execute other transactions at any time. --- CHANGELOG.md | 1 + src/firmware.c | 10 ++ src/firmware_main_loop.c | 7 + src/u2f.c | 174 ++++++++----------------- src/u2f/u2f_app.c | 4 +- src/u2f/u2f_packet.c | 6 + src/usb/usb_processing.c | 6 + src/usb/usb_processing.h | 2 + src/workflow/orientation_screen.c | 11 -- test/unit-test/framework/mock_hidapi.c | 5 +- test/unit-test/test_u2f_standard.c | 34 +++-- test/unit-test/u2f/u2f_util_t.c | 1 + 12 files changed, 111 insertions(+), 150 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 66d052427..b484b9d1b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ customers cannot upgrade their bootloader, its changes are recorded separately. ## Firmware ### [Unreleased] +- U2F: fix macos/safari macos/firefox support ### 9.23.0 - Ethereum: add confirmation screen for known networks, change base unit to ETH for Arbitrum and Optimism diff --git a/src/firmware.c b/src/firmware.c index a16ba717f..ff0813dc4 100644 --- a/src/firmware.c +++ b/src/firmware.c @@ -25,8 +25,13 @@ #include "screen.h" #include "ui/screen_stack.h" #include "usb/usb_processing.h" +#include #include +#if APP_U2F == 1 +#include +#endif + uint32_t __stack_chk_guard = 0; int main(void) @@ -44,6 +49,11 @@ int main(void) da14531_protocol_init(); } usb_processing_init(); + // Setup usb_processing handlers + hww_setup(); +#if APP_U2F == 1 + u2f_device_setup(); +#endif firmware_main_loop(); return 0; } diff --git a/src/firmware_main_loop.c b/src/firmware_main_loop.c index 5a461eb59..8282b8d5d 100644 --- a/src/firmware_main_loop.c +++ b/src/firmware_main_loop.c @@ -103,6 +103,11 @@ void firmware_main_loop(void) } if (!u2f_data) { u2f_data = queue_pull(queue_u2f_queue()); + // If USB stack was locked and there is no more messages to send out, time to + // unlock it. + if (!u2f_data && usb_processing_locked(usb_processing_u2f())) { + usb_processing_unlock(); + } } #endif // Do USB Input @@ -120,6 +125,7 @@ void firmware_main_loop(void) } #if APP_U2F == 1 if (!u2f_data && hid_u2f_read(&u2f_frame[0])) { + util_log("u2f data %s", util_dbg_hex((void*)u2f_frame, 16)); u2f_packet_process((const USB_FRAME*)u2f_frame); if (communication_mode_ble_enabled()) { // Enqueue a power down command to the da14531 @@ -152,6 +158,7 @@ void firmware_main_loop(void) #if APP_U2F == 1 if (!communication_mode_ble_enabled() && u2f_data) { if (hid_u2f_write_poll(u2f_data)) { + util_log("u2f wrote %s", util_dbg_hex(u2f_data, 16)); u2f_data = NULL; } } diff --git a/src/u2f.c b/src/u2f.c index e0a2046b8..0cdd4dad3 100644 --- a/src/u2f.c +++ b/src/u2f.c @@ -32,6 +32,7 @@ #include #include #include +#include #include #include @@ -68,19 +69,14 @@ typedef enum { } u2f_auth_state_t; typedef struct { - uint32_t cid; /** - * Command that is currently executing - * (blocking) on the U2F stack. + * CID and last command are used to check that we eventually respond to the right command after + * the user has confirmed on screen. U2F is normally stateless between transactions, but since + * we have a display and Yes/No input we can ask the user on the first request and later respond + * that the user is present only if the user confirmed specifically that app-id. */ + uint32_t cid; uint8_t last_cmd; - /** - * Keeps track of whether there is an outstanding - * U2F operation going on in the background. - * This is not strictly necessary, but it's useful - * to have as a sanity checking mechanism. - */ - bool locked; /** * Keeps track of which part of a registration we're currently in. */ @@ -131,30 +127,6 @@ static void _clear_state(void) { _state.reg = U2F_REGISTER_IDLE; _state.auth = U2F_AUTHENTICATE_IDLE; - _state.locked = false; -} - -/** - * Unlocks the USB stack. Resets the U2F state. - */ -static void _unlock(void) -{ - usb_processing_unlock(); - _clear_state(); -} - -/** - * Locks the USB stack to process the given - * U2F request. - * - * @param apdu U2F packet that will own the lock on the USB stack. No other request - * will be processed by the USB stack until this request completes. - */ -static void _lock(const USB_APDU* apdu) -{ - usb_processing_lock(usb_processing_u2f()); - _state.locked = true; - _state.last_cmd = apdu->ins; } static component_t* _nudge_label = NULL; @@ -183,7 +155,7 @@ static void _start_refresh_webpage_screen(void) // Unfortunately unlocking takes more time than U2F is allowed to take. With the current // architecture of the firmware we cannot run it concurrently to other requests. Therefore // we will call it here, make the user unlock the device and then ask the user to refresh - // the webpage. (refreshing is only needed for some browsers) + // the webpage. (refreshing is only needed for some FIDO Clients) _state.refresh_webpage = _create_refresh_webpage(); ui_screen_stack_push(_state.refresh_webpage); _state.refresh_webpage_timeout = 0; @@ -421,7 +393,7 @@ static void _register_start(const USB_APDU* apdu, Packet* out_packet) const U2F_REGISTER_REQ* reg_request = (const U2F_REGISTER_REQ*)apdu->data; uint16_t req_error = _register_sanity_check_req(apdu); if (req_error) { - _unlock(); + _clear_state(); _error(req_error, out_packet); return; } @@ -462,7 +434,7 @@ static void _register_continue(const USB_APDU* apdu, Packet* out_packet) } /* No more pending operations for U2F register */ - _unlock(); + _clear_state(); if (async_result == ASYNC_OP_FALSE) { _error(U2F_SW_CONDITIONS_NOT_SATISFIED, out_packet); @@ -582,7 +554,7 @@ static void _authenticate_start(const USB_APDU* apdu, Packet* out_packet) { uint16_t req_error = _authenticate_sanity_check_req(apdu); if (req_error) { - _unlock(); + _clear_state(); _error(req_error, out_packet); return; } @@ -596,7 +568,7 @@ static void _authenticate_start(const USB_APDU* apdu, Packet* out_packet) } uint16_t key_error = _authenticate_start_confirm(apdu); if (key_error) { - _unlock(); + _clear_state(); _error(key_error, out_packet); return; } @@ -608,7 +580,7 @@ static void _authenticate_wait_refresh(const USB_APDU* apdu, Packet* out_packet) _assert_unlocked(); uint16_t key_error = _authenticate_start_confirm(apdu); if (key_error) { - _unlock(); + _clear_state(); _error(key_error, out_packet); return; } @@ -624,10 +596,18 @@ static void _authenticate_continue(const USB_APDU* apdu, Packet* out_packet) U2F_AUTHENTICATE_SIG_STR sig_base; uint16_t req_error = _authenticate_sanity_check_req(apdu); if (req_error) { + _clear_state(); _error(req_error, out_packet); return; } + uint16_t key_error = _authenticate_verify_key_valid(apdu); + if (key_error) { + _clear_state(); + _error(key_error, out_packet); + return; + } + const U2F_AUTHENTICATE_REQ* auth_request = (const U2F_AUTHENTICATE_REQ*)apdu->data; async_op_result_t async_result = u2f_app_confirm_retry(U2F_APP_AUTHENTICATE, auth_request->appId); @@ -637,7 +617,7 @@ static void _authenticate_continue(const USB_APDU* apdu, Packet* out_packet) } /* No more blocking operations pending for authentication. */ - _unlock(); + _clear_state(); if (async_result == ASYNC_OP_FALSE) { _error(U2F_SW_CONDITIONS_NOT_SATISFIED, out_packet); @@ -706,6 +686,8 @@ static void _cmd_ping(const Packet* in_packet, Packet* out_packet, const size_t { (void)max_out_len; + usb_processing_lock(usb_processing_u2f()); + // 0 and broadcast are reserved if (in_packet->cid == U2FHID_CID_BROADCAST || in_packet->cid == 0) { _error_hid(in_packet->cid, U2FHID_ERR_INVALID_CID, out_packet); @@ -724,6 +706,8 @@ static void _cmd_wink(const Packet* in_packet, Packet* out_packet, const size_t { (void)max_out_len; + usb_processing_lock(usb_processing_u2f()); + // 0 and broadcast are reserved if (in_packet->cid == U2FHID_CID_BROADCAST || in_packet->cid == 0) { _error_hid(in_packet->cid, U2FHID_ERR_INVALID_CID, out_packet); @@ -750,6 +734,8 @@ static void _cmd_wink(const Packet* in_packet, Packet* out_packet, const size_t */ static void _cmd_init(const Packet* in_packet, Packet* out_packet, const size_t max_out_len) { + usb_processing_lock(usb_processing_u2f()); + if (U2FHID_INIT_RESP_SIZE >= max_out_len) { _error_hid(in_packet->cid, U2FHID_ERR_OTHER, out_packet); return; @@ -787,14 +773,14 @@ static void _cmd_register(const Packet* in_packet, Packet* out_packet) { const USB_APDU* apdu = (const USB_APDU*)in_packet->data_addr; /* Sanity-check our state. */ - if (_state.reg != U2F_REGISTER_IDLE && - (_state.last_cmd != U2F_REGISTER || _state.cid != in_packet->cid)) { - Abort("U2F reg arbitration failed."); + if (_state.reg != U2F_REGISTER_IDLE && _state.last_cmd != U2F_REGISTER) { + util_log("u2f: ERROR register invalid state"); + _clear_state(); + return; } switch (_state.reg) { case U2F_REGISTER_IDLE: - _lock(apdu); _register_start(apdu, out_packet); break; case U2F_REGISTER_UNLOCKING: @@ -811,29 +797,6 @@ static void _cmd_register(const Packet* in_packet, Packet* out_packet) } } -/** - * Abort an existing registration request. - */ -static void _abort_register(void) -{ - switch (_state.reg) { - case U2F_REGISTER_UNLOCKING: - rust_workflow_abort_current(); - _clear_state(); - break; - case U2F_REGISTER_CONFIRMING: - u2f_app_confirm_abort(); - _clear_state(); - break; - case U2F_REGISTER_WAIT_REFRESH: - _stop_refresh_webpage_screen(); - _clear_state(); - break; - default: - Abort("Bad U2F register abort status"); - } -} - /** * Processes an incoming registration request. */ @@ -843,12 +806,13 @@ static void _cmd_authenticate(const Packet* in_packet, Packet* out_packet) /* Sanity-check our state. */ if (_state.auth != U2F_AUTHENTICATE_IDLE && (_state.last_cmd != U2F_AUTHENTICATE || _state.cid != in_packet->cid)) { - Abort("U2F auth arbitration failed."); + util_log("u2f: ERROR authenticate invalid state"); + _clear_state(); + return; } switch (_state.auth) { case U2F_AUTHENTICATE_IDLE: - _lock(apdu); _authenticate_start(apdu, out_packet); break; case U2F_AUTHENTICATE_UNLOCKING: @@ -865,35 +829,13 @@ static void _cmd_authenticate(const Packet* in_packet, Packet* out_packet) } } -/** - * Abort an existing authentication request. - */ -static void _abort_authenticate(void) -{ - switch (_state.auth) { - case U2F_AUTHENTICATE_UNLOCKING: - rust_workflow_abort_current(); - _clear_state(); - break; - case U2F_AUTHENTICATE_CONFIRMING: - u2f_app_confirm_abort(); - _clear_state(); - break; - case U2F_AUTHENTICATE_WAIT_REFRESH: - _stop_refresh_webpage_screen(); - _clear_state(); - break; - default: - Abort("Bad U2F register abort status"); - } -} - /** * Process a U2F message */ static void _cmd_msg(const Packet* in_packet, Packet* out_packet, const size_t max_out_len) { (void)max_out_len; + // By default always use the recieved cid _state.cid = in_packet->cid; @@ -903,6 +845,8 @@ static void _cmd_msg(const Packet* in_packet, Packet* out_packet, const size_t m return; } + usb_processing_lock(usb_processing_u2f()); + if (apdu->cla != 0) { _error(U2F_SW_CLA_NOT_SUPPORTED, out_packet); return; @@ -911,33 +855,27 @@ static void _cmd_msg(const Packet* in_packet, Packet* out_packet, const size_t m switch (apdu->ins) { case U2F_REGISTER: _cmd_register(in_packet, out_packet); + _state.last_cmd = apdu->ins; break; case U2F_AUTHENTICATE: _cmd_authenticate(in_packet, out_packet); + _state.last_cmd = apdu->ins; break; case U2F_VERSION: _version(apdu, out_packet); break; default: _error(U2F_SW_INS_NOT_SUPPORTED, out_packet); - return; + break; } } +// U2F only locks USB stack between a request and a response. A request is always immediately +// followed by a response. New requests must wait until the current one is finished. bool u2f_blocking_request_can_go_through(const Packet* in_packet) { - if (!_state.locked) { - Abort("USB stack thinks we're busy, but we're not."); - } - /* - * Check if this request is the same one we're currently operating on. - * For now, this checks the request type and channel ID only. - * FUTURE: Maybe check that the key handle is maintained between requests? - * so that when we're asking for confirmation, we ask to confirm - * "a particular key handle" instead of "a particular tab". - */ - const USB_APDU* apdu = (const USB_APDU*)in_packet->data_addr; - return apdu->ins == _state.last_cmd && in_packet->cid == _state.cid; + (void)in_packet; + return false; } void u2f_blocked_req_error(Packet* out_packet, const Packet* in_packet) @@ -980,7 +918,7 @@ static void _process_wait_refresh(void) { if (_state.refresh_webpage_timeout == 25000) { _stop_refresh_webpage_screen(); - _unlock(); + _clear_state(); } else { _state.refresh_webpage_timeout++; /* Prevent the USB watchdog from killing this workflow. */ @@ -1024,9 +962,6 @@ static void _process_authenticate(void) void u2f_process(void) { - if (!_state.locked) { - return; - } switch (_state.last_cmd) { case U2F_REGISTER: _process_register(); @@ -1034,6 +969,9 @@ void u2f_process(void) case U2F_AUTHENTICATE: _process_authenticate(); break; + case 0: + // initial value + break; default: Abort("Bad U2F process state."); } @@ -1044,6 +982,8 @@ void u2f_process(void) */ void u2f_device_setup(void) { + _clear_state(); + _state.last_cmd = 0; const CMD_Callback u2f_cmd_callbacks[] = { {U2FHID_PING, _cmd_ping}, {U2FHID_WINK, _cmd_wink}, @@ -1056,17 +996,5 @@ void u2f_device_setup(void) void u2f_abort_outstanding_op(void) { - if (!_state.locked) { - Abort("USB stack thinks U2F is busy, but it's not."); - } - switch (_state.last_cmd) { - case U2F_REGISTER: - _abort_register(); - break; - case U2F_AUTHENTICATE: - _abort_authenticate(); - break; - default: - Abort("Invalid U2F status on abort."); - } + util_log("u2f_abort_outstanding_op"); } diff --git a/src/u2f/u2f_app.c b/src/u2f/u2f_app.c index 269d3a532..4bdb0be5e 100644 --- a/src/u2f/u2f_app.c +++ b/src/u2f/u2f_app.c @@ -219,7 +219,9 @@ void u2f_app_confirm_start(enum u2f_app_confirm_t type, const uint8_t* app_id) async_op_result_t u2f_app_confirm_retry(enum u2f_app_confirm_t type, const uint8_t* app_id) { if (_state.outstanding_confirm != type || !MEMEQ(app_id, _state.app_id, 32)) { - Abort("Arbitration failed for U2F confirmation."); + // CID used a new app_id, clearly invalid + // TODO: Clear u2f state + return ASYNC_OP_NOT_READY; } bool result = false; if (!rust_workflow_confirm_poll(&result)) { diff --git a/src/u2f/u2f_packet.c b/src/u2f/u2f_packet.c index 1b4468965..05d5f34a8 100644 --- a/src/u2f/u2f_packet.c +++ b/src/u2f/u2f_packet.c @@ -19,6 +19,7 @@ #include #include #include +#include #define ERR_NONE 0 @@ -126,6 +127,7 @@ void u2f_packet_timeout_tick(void) void u2f_packet_timeout(uint32_t cid) { + util_log("u2f_packet_timeout"); _timeout_disable(cid); if (cid == _in_state.cid) { _reset_state(); @@ -138,19 +140,23 @@ bool u2f_packet_process(const USB_FRAME* frame) struct usb_processing* ctx = usb_processing_u2f(); switch (usb_frame_process(frame, &_in_state)) { case FRAME_ERR_IGNORE: + util_log("u2f_packet ignore"); // Ignore this frame, i.e. no response. break; case FRAME_ERR_INVALID_SEQ: + util_log("u2f_packet seq"); // Reset the state becuase this error indicates that there is a host application bug _reset_state(); _queue_err(FRAME_ERR_INVALID_SEQ, frame->cid); break; case FRAME_ERR_CHANNEL_BUSY: + util_log("u2f_packet busy"); // We don't reset the state because this error doesn't indicate something wrong with the // "current" connection. _queue_err(FRAME_ERR_CHANNEL_BUSY, frame->cid); break; case FRAME_ERR_INVALID_LEN: + util_log("u2f_packet invalid len"); // Reset the state becuase this error indicates that there is a host application bug _reset_state(); _queue_err(FRAME_ERR_INVALID_LEN, frame->cid); diff --git a/src/usb/usb_processing.c b/src/usb/usb_processing.c index 77904fb55..4045e9d0e 100644 --- a/src/usb/usb_processing.c +++ b/src/usb/usb_processing.c @@ -243,12 +243,14 @@ static void _usb_execute_packet(struct usb_processing* ctx, const Packet* in_pac Packet out_packet; _prepare_out_packet(in_packet, &out_packet); ctx->registered_cmds[i].process_cmd(in_packet, &out_packet, USB_DATA_MAX_LEN); + _enqueue_frames(ctx, (const Packet*)&out_packet); break; } } if (!cmd_valid) { + util_log("usb_processing: No handler"); ctx->manage_invalid_endpoint(ctx->out_queue(), _usb_state.in_packet.cid); } } @@ -438,4 +440,8 @@ void usb_processing_unlock(void) } _usb_state.blocking_ctx = NULL; } +bool usb_processing_locked(struct usb_processing* ctx) +{ + return _usb_state.blocking_ctx == ctx; +} #endif diff --git a/src/usb/usb_processing.h b/src/usb/usb_processing.h index c862ad3e9..d8106d561 100644 --- a/src/usb/usb_processing.h +++ b/src/usb/usb_processing.h @@ -76,6 +76,8 @@ void usb_processing_lock(struct usb_processing* ctx); void usb_processing_timeout_reset(int16_t val); void usb_processing_unlock(void); + +bool usb_processing_locked(struct usb_processing* ctx); #endif #endif diff --git a/src/workflow/orientation_screen.c b/src/workflow/orientation_screen.c index 89065066a..cf3b36a6b 100644 --- a/src/workflow/orientation_screen.c +++ b/src/workflow/orientation_screen.c @@ -21,7 +21,6 @@ #endif #include #include -#include #include #include #include @@ -31,10 +30,6 @@ #include #include -#if APP_U2F -#include -#endif - #ifndef TESTING #define IDLE_PERIOD_MS 1300 @@ -67,12 +62,6 @@ static void _idle_timer_cb(const struct timer_task* const timer_task) { (void)timer_task; - // Setup usb_processing handlers - hww_setup(); -#if APP_U2F - u2f_device_setup(); -#endif - // hww handler in usb_process must be setup before we can allow ble connections if (memory_get_platform() == MEMORY_PLATFORM_BITBOX02_PLUS) { da14531_handler_current_product = (const uint8_t*)DEVICE_MODE; diff --git a/test/unit-test/framework/mock_hidapi.c b/test/unit-test/framework/mock_hidapi.c index bb47b97b7..48412355e 100644 --- a/test/unit-test/framework/mock_hidapi.c +++ b/test/unit-test/framework/mock_hidapi.c @@ -8,7 +8,7 @@ // POSIX #include -#include +#include #include "queue.h" #include "u2f.h" @@ -136,6 +136,9 @@ int hid_read_timeout(hid_device* dev, unsigned char* data, size_t length, int mi return -127; } if (queue_peek(queue_u2f_queue()) == NULL) { + if (usb_processing_locked(usb_processing_u2f())) { + usb_processing_unlock(); + } _have_data = false; } _expect_more = false; diff --git a/test/unit-test/test_u2f_standard.c b/test/unit-test/test_u2f_standard.c index 5c665a586..27e0196ea 100644 --- a/test/unit-test/test_u2f_standard.c +++ b/test/unit-test/test_u2f_standard.c @@ -34,6 +34,7 @@ struct U2Fob* device; U2F_REGISTER_REQ regReq; U2F_REGISTER_RESP regRsp; +U2F_AUTHENTICATE_REQ authReq; static void util_uint8_to_hex(const uint8_t* in_bin, const size_t in_len, char* out) { @@ -126,14 +127,6 @@ static void test_WrongLength_U2F_REGISTER(void) static void test_Enroll(int expectedSW12, int printinfo) { - // pick random origin and challenge. - for (size_t i = 0; i < sizeof(regReq.challenge); ++i) { - regReq.challenge[i] = rand(); - } - for (size_t i = 0; i < sizeof(regReq.appId); ++i) { - regReq.appId[i] = rand(); - } - uint64_t t = 0; U2Fob_deltaTime(&t); @@ -218,12 +211,6 @@ static void test_Enroll(int expectedSW12, int printinfo) #if defined(WITH_HARDWARE) static uint32_t test_Sign(int expectedSW12, bool checkOnly) { - U2F_AUTHENTICATE_REQ authReq; - - // pick random challenge and use registered appId. - for (size_t i = 0; i < sizeof(authReq.challenge); ++i) { - authReq.challenge[i] = rand(); - } memcpy(authReq.appId, regReq.appId, sizeof(authReq.appId)); authReq.keyHandleLength = regRsp.keyHandleLen; memcpy(authReq.keyHandle, regRsp.keyHandleCertSig, authReq.keyHandleLength); @@ -316,6 +303,14 @@ static void run_tests(void) PASS(test_WrongLength_U2F_REGISTER()); PASS(test_BadCLA()); + // pick random origin and challenge. + for (size_t i = 0; i < sizeof(regReq.challenge); ++i) { + regReq.challenge[i] = rand(); + } + for (size_t i = 0; i < sizeof(regReq.appId); ++i) { + regReq.appId[i] = rand(); + } + // Fob with button should need touch. if (arg_hasButton) { // Timeout @@ -328,6 +323,11 @@ static void run_tests(void) WaitForUserPresence(device, arg_hasButton); PASS(test_Enroll(0x9000, 1)); + // pick random challenge and use registered appId. + for (size_t i = 0; i < sizeof(authReq.challenge); ++i) { + authReq.challenge[i] = rand(); + } + // Fob with button should have consumed touch. if (arg_hasButton) { // Timeout @@ -352,6 +352,12 @@ static void run_tests(void) WaitForUserPresence(device, arg_hasButton); PASS(test_Sign(0x6985, true)); + // This triggers the confirmation screen + WaitForUserPresence(device, arg_hasButton); + PASS(test_Sign(0x6985, false)); + + WaitForUserPresence(device, arg_hasButton); + uint32_t ctr1; PASS(ctr1 = test_Sign(0x9000, false)); // < fails // Timeout diff --git a/test/unit-test/u2f/u2f_util_t.c b/test/unit-test/u2f/u2f_util_t.c index 10ef6f76d..a5d853786 100644 --- a/test/unit-test/u2f/u2f_util_t.c +++ b/test/unit-test/u2f/u2f_util_t.c @@ -239,6 +239,7 @@ int U2Fob_init(struct U2Fob* device) res = U2Fob_receiveHidFrame(device, &response, 2.0); if (res == -U2FHID_ERR_MSG_TIMEOUT) { + printf("err timeout\n"); return res; } if (res == -U2FHID_ERR_OTHER) {