From 87f23349add57f55f44afd6ff20a99a732a01ad7 Mon Sep 17 00:00:00 2001 From: Alexandre Paillier Date: Thu, 9 Feb 2023 11:30:42 +0100 Subject: [PATCH 1/3] Fix freeze on slow/unresponsive APDU transport --- src_bagl/ui_flow_signMessage.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src_bagl/ui_flow_signMessage.c b/src_bagl/ui_flow_signMessage.c index 9b97d57..e1eed03 100644 --- a/src_bagl/ui_flow_signMessage.c +++ b/src_bagl/ui_flow_signMessage.c @@ -19,6 +19,9 @@ static void dummy_pre_cb(void) { static void dummy_post_cb(void) { if (ui_pos == UI_191_POS_QUESTION) { continue_displaying_message(); + // temporarily disable button clicks, they will be re-enabled as soon as new data + // is received and the page is redrawn with ux_flow_init() + G_ux.stack[0].button_push_callback = NULL; } else // UI_191_END { ui_191_switch_to_message_end(); @@ -55,6 +58,7 @@ UX_STEP_CB( #else nnn, #endif + G_ux.stack[0].button_push_callback = NULL; // disable button clicks skip_rest_of_message(), { #ifndef TARGET_NANOS From 27392c20de4d405c312fd8a8d1a367f580aeb821 Mon Sep 17 00:00:00 2001 From: Alexandre Paillier Date: Thu, 9 Feb 2023 11:44:21 +0100 Subject: [PATCH 2/3] Better context checks when processing EIP-191 APDUs --- src/main.c | 10 ++++++---- src_features/signMessage/cmd_signMessage.c | 23 ++++++++++++++-------- 2 files changed, 21 insertions(+), 12 deletions(-) diff --git a/src/main.c b/src/main.c index be755e5..56a7f6b 100644 --- a/src/main.c +++ b/src/main.c @@ -673,10 +673,12 @@ void handleApdu(unsigned int *flags, unsigned int *tx) { case INS_SIGN_PERSONAL_MESSAGE: memset(tmpCtx.transactionContext.tokenSet, 0, MAX_ITEMS); *flags |= IO_ASYNCH_REPLY; - handleSignPersonalMessage(G_io_apdu_buffer[OFFSET_P1], - G_io_apdu_buffer[OFFSET_P2], - G_io_apdu_buffer + OFFSET_CDATA, - G_io_apdu_buffer[OFFSET_LC]); + if (!handleSignPersonalMessage(G_io_apdu_buffer[OFFSET_P1], + G_io_apdu_buffer[OFFSET_P2], + G_io_apdu_buffer + OFFSET_CDATA, + G_io_apdu_buffer[OFFSET_LC])) { + reset_app_context(); + } break; case INS_SIGN_EIP_712_MESSAGE: diff --git a/src_features/signMessage/cmd_signMessage.c b/src_features/signMessage/cmd_signMessage.c index 1779142..6280ca0 100644 --- a/src_features/signMessage/cmd_signMessage.c +++ b/src_features/signMessage/cmd_signMessage.c @@ -21,6 +21,9 @@ static const char SIGN_MAGIC[] = * @param[in] sw status word */ static void apdu_reply(uint16_t sw) { + if ((sw != APDU_RESPONSE_OK) && states.ui_started) { + ui_idle(); + } G_io_apdu_buffer[0] = (sw >> 8) & 0xff; G_io_apdu_buffer[1] = sw & 0xff; io_exchange(CHANNEL_APDU | IO_RETURN_AFTER_TX, 2); @@ -90,18 +93,18 @@ static void reset_ui_buffer(void) { */ static const uint8_t *first_apdu_data(const uint8_t *data, uint8_t *length) { if (appState != APP_STATE_IDLE) { - reset_app_context(); + apdu_reply(APDU_RESPONSE_CONDITION_NOT_SATISFIED); } appState = APP_STATE_SIGNING_MESSAGE; data = parseBip32(data, length, &tmpCtx.messageSigningContext.bip32); if (data == NULL) { - apdu_reply(0x6a80); + apdu_reply(APDU_RESPONSE_INVALID_DATA); return NULL; } if (*length < sizeof(uint32_t)) { PRINTF("Invalid data\n"); - apdu_reply(0x6a80); + apdu_reply(APDU_RESPONSE_INVALID_DATA); return NULL; } @@ -140,7 +143,7 @@ static bool feed_hash(const uint8_t *const data, const uint8_t length) { PRINTF("Error: Length mismatch ! (%u > %u)!\n", length, tmpCtx.messageSigningContext.remainingLength); - apdu_reply(0x6a80); + apdu_reply(APDU_RESPONSE_INVALID_DATA); return false; } cx_hash((cx_hash_t *) &global_sha3, 0, data, length, NULL, 0); @@ -194,7 +197,7 @@ static void feed_display(void) { } if ((unprocessed_length() == 0) && (tmpCtx.messageSigningContext.remainingLength > 0)) { - apdu_reply(0x9000); + apdu_reply(APDU_RESPONSE_OK); } } @@ -222,7 +225,11 @@ bool handleSignPersonalMessage(uint8_t p1, processed_size = data - payload; } else if (p1 != P1_MORE) { PRINTF("Error: Unexpected P1 (%u)!\n", p1); - apdu_reply(0x6B00); + apdu_reply(APDU_RESPONSE_INVALID_P1_P2); + return false; + } else if (appState != APP_STATE_SIGNING_MESSAGE) { + PRINTF("Error: App not already in signing state!\n"); + apdu_reply(APDU_RESPONSE_INVALID_DATA); return false; } @@ -241,7 +248,7 @@ bool handleSignPersonalMessage(uint8_t p1, ui_191_switch_to_sign(); #endif } else { - apdu_reply(0x9000); + apdu_reply(APDU_RESPONSE_OK); } } return true; @@ -266,7 +273,7 @@ void question_switcher(void) { void skip_rest_of_message(void) { states.sign_state = STATE_191_HASH_ONLY; if (tmpCtx.messageSigningContext.remainingLength > 0) { - apdu_reply(0x9000); + apdu_reply(APDU_RESPONSE_OK); } else { ui_191_switch_to_sign(); } From cf1d21edf96eb18fd57ac57574188da6753c1f3e Mon Sep 17 00:00:00 2001 From: Alexandre Paillier Date: Thu, 9 Feb 2023 11:46:40 +0100 Subject: [PATCH 3/3] Updated the changelog --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7c22116..950ee93 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,7 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](http://keepachangelog.com/) and this project adheres to [Semantic Versioning](http://semver.org/). -## [1.10.2](https://github.com/ledgerhq/app-ethereum/compare/1.10.1...1.10.2) - 2022-02-03 +## [1.10.2](https://github.com/ledgerhq/app-ethereum/compare/1.10.1...1.10.2) - 2022-02-09 ### Added @@ -34,6 +34,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Possible overflow with very large transactions - EnergyWebChain ticker - Arbitrum ticker +- Better error handling on EIP-191 APDUs ## [1.10.1](https://github.com/ledgerhq/app-ethereum/compare/1.10.0...1.10.1) - 2022-11-09