Merge pull request LedgerHQ#402 from LedgerHQ/bugfix/apa/eip191_freeze

Better EIP-191 error handling
jnicoulaud-ledger · Feb 9, 2023 · f6717ae · f6717ae
2 parents 3e724ef + cf1d21e
commit f6717ae
Show file tree

Hide file tree

Showing 4 changed files with 27 additions and 13 deletions.
diff --git 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
 

diff --git 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_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

diff --git 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();
     }