[fix] Some plausible yet wrong mnemonic were deemed valid on NBGL devices

Author: lpascal-ledger

src/app_main.c

@@ -77,7 +77,7 @@ unsigned char io_event(unsigned char channel __attribute__((unused))) {
                 UX_REDISPLAY();
             } else {
                 if (G_bolos_ux_context.processing == 1) {
-                    UX_DISPLAYED_EVENT(compare_recovery_phrase(););
+                    UX_DISPLAYED_EVENT(compare_recovery_phrase_and_display_result(););
                 } else {
                     UX_DISPLAYED_EVENT();
                 }

src/bagl/nanos_enter_phrase.c

@@ -213,49 +213,17 @@ const bagl_element_t* screen_onboarding_4_restore_word_keyboard_callback(unsigne
     return &G_ux.tmp_element;
 }
 
-void compare_recovery_phrase(void) {
+void compare_recovery_phrase_and_display_result(void) {
     G_bolos_ux_context.processing = 0;
 
     io_seproxyhal_general_status();
 
-    // convert mnemonic to hex-seed
-    uint8_t buffer[64];
-
-    bolos_ux_mnemonic_to_seed((unsigned char*) G_bolos_ux_context.words_buffer,
-                              G_bolos_ux_context.words_buffer_length,
-                              buffer);
-    PRINTF("Input seed:\n %.*H\n", 64, buffer);
-
-    // get rootkey from hex-seed
-    cx_hmac_sha512_t ctx;
-    const char key[] = "Bitcoin seed";
-
-    LEDGER_ASSERT(cx_hmac_sha512_init_no_throw(&ctx, (const uint8_t*) key, strlen(key)) == CX_OK,
-                  "HMAC init failed");
-    LEDGER_ASSERT(cx_hmac_no_throw((cx_hmac_t*) &ctx, CX_LAST, buffer, 64, buffer, 64) == CX_OK,
-                  "HMAC failed");
-    PRINTF("Root key from input:\n%.*H\n", 64, buffer);
-
-    // get rootkey from device's seed
-    uint8_t buffer_device[64];
-
-    // os_derive_bip32* do not accept NULL path, even with a size of 0, so we provide an empty path
-    const unsigned int empty_path = 0;
-    if (os_derive_bip32_no_throw(CX_CURVE_256K1,
-                                 &empty_path,
-                                 0,
-                                 buffer_device,
-                                 buffer_device + 32) != CX_OK) {
-        PRINTF("An error occurred while comparing the recovery phrase\n");
-        return;
-    }
-    PRINTF("Root key from device: \n%.*H\n", 64, buffer_device);
-
-    // compare both rootkey
-    if (os_secure_memcmp(buffer, buffer_device, 64)) {
-        ux_flow_init(0, flow_final_nomatch, NULL);
-    } else {
+    const bool result = compare_recovery_phrase((uint8_t*) G_bolos_ux_context.words_buffer,
+                                                G_bolos_ux_context.words_buffer_length);
+    if (result) {
         ux_flow_init(0, flow_final_match, NULL);
+    } else {
+        ux_flow_init(0, flow_final_nomatch, NULL);
     }
 }
 

src/bagl/nanox_enter_phrase.c

@@ -399,44 +399,6 @@ const bagl_element_t* screen_onboarding_4_restore_word_before_element_display_ca
     return element;
 }
 
-static uint8_t compare_recovery_phrase(void) {
-    // convert mnemonic to hex-seed
-    uint8_t buffer[64];
-
-    bolos_ux_mnemonic_to_seed((unsigned char*) G_bolos_ux_context.words_buffer,
-                              G_bolos_ux_context.words_buffer_length,
-                              buffer);
-    PRINTF("Input seed:\n %.*H\n", 64, buffer);
-
-    // get rootkey from hex-seed
-    cx_hmac_sha512_t ctx;
-    const char key[] = "Bitcoin seed";
-
-    LEDGER_ASSERT(cx_hmac_sha512_init_no_throw(&ctx, (const uint8_t*) key, strlen(key)) == CX_OK,
-                  "HMAC init failed");
-    LEDGER_ASSERT(cx_hmac_no_throw((cx_hmac_t*) &ctx, CX_LAST, buffer, 64, buffer, 64) == CX_OK,
-                  "HMAC failed");
-    PRINTF("Root key from input:\n%.*H\n", 64, buffer);
-
-    // get rootkey from device's seed
-    uint8_t buffer_device[64];
-
-    // os_derive_bip32* do not accept NULL path, even with a size of 0, so we provide an empty path
-    const unsigned int empty_path = 0;
-    if (os_derive_bip32_no_throw(CX_CURVE_256K1,
-                                 &empty_path,
-                                 0,
-                                 buffer_device,
-                                 buffer_device + 32) != CX_OK) {
-        PRINTF("An error occurred while comparing the recovery phrase\n");
-        return 0;
-    }
-    PRINTF("Root key from device: \n%.*H\n", 64, buffer_device);
-
-    // compare both rootkey
-    return os_secure_memcmp(buffer, buffer_device, 64) ? 0 : 1;
-}
-
 void screen_onboarding_4_restore_word_validate(void) {
     bolos_ux_bip39_idx_strcpy(
         G_bolos_ux_context.onboarding_index + G_bolos_ux_context.hslider3_current,
@@ -472,7 +434,8 @@ void screen_onboarding_4_restore_word_validate(void) {
 
             // Display loading icon to user
             ux_flow_init(0, ux_load_flow, NULL);
-            if (compare_recovery_phrase()) {
+            if (compare_recovery_phrase((unsigned char*) G_bolos_ux_context.words_buffer,
+                                        G_bolos_ux_context.words_buffer_length)) {
                 ux_flow_init(0, ux_succesfull_check_flow, NULL);
             } else {
                 ux_flow_init(0, ux_failed_check_flow, NULL);

src/bagl/ux_nano.h

@@ -17,7 +17,7 @@
 #pragma once
 
 #include <ux.h>
-#include "ux_common/common.h"
+#include "mnemonic_common/common.h"
 
 #if defined(HAVE_BAGL)
 
@@ -83,11 +83,11 @@ void screen_common_keyboard_init(unsigned int stack_slot,
                                  unsigned int nb_elements,
                                  keyboard_callback_t callback);
 
-#include "ux_common/common_bip39.h"
+#include "mnemonic_common/common_bip39.h"
 
 #if defined(TARGET_NANOS)
 extern const bagl_element_t screen_onboarding_word_list_elements[9];
-void compare_recovery_phrase(void);
+void compare_recovery_phrase_and_display_result(void);
 #else
 // to be included into all flow that needs to go back to the dashboard
 extern const ux_flow_step_t ux_ob_goto_dashboard_step;

src/ux_common/common.h src/mnemonic_common/common.h

@@ -20,6 +20,7 @@ unsigned int bolos_ux_bip39_get_word_count_starting_with(const unsigned char *pr
 unsigned int bolos_ux_bip39_get_word_next_letters_starting_with(const unsigned char *prefix,
                                                                 const unsigned int prefixLength,
                                                                 unsigned char *next_letters_buffer);
+bool compare_recovery_phrase(uint8_t *buffer, size_t buffer_size);
 
 #if defined(HAVE_NBGL)
 size_t bolos_ux_bip39_fill_with_candidates(const unsigned char *startingChars,

src/ux_common/common_bip39.h src/mnemonic_common/common_bip39.h

@@ -207,6 +207,46 @@ unsigned int bolos_ux_bip39_get_word_next_letters_starting_with(
     return letter_count;
 }
 
+bool compare_recovery_phrase(uint8_t* mnemonic, size_t mnemonic_length) {
+    // convert mnemonic to hex-seed
+    uint8_t buffer[64];
+
+    bolos_ux_mnemonic_to_seed(mnemonic, mnemonic_length, buffer);
+    PRINTF("Input seed:\n %.*H\n", 64, buffer);
+
+    // get rootkey from hex-seed
+    cx_hmac_sha512_t ctx;
+    const char key[] = "Bitcoin seed";
+
+    LEDGER_ASSERT(cx_hmac_sha512_init_no_throw(&ctx, (const uint8_t*) key, strlen(key)) == CX_OK,
+                  "HMAC init failed");
+    LEDGER_ASSERT(cx_hmac_no_throw((cx_hmac_t*) &ctx, CX_LAST, buffer, 64, buffer, 64) == CX_OK,
+                  "HMAC failed");
+    PRINTF("Root key from input:\n%.*H\n", 64, buffer);
+
+    // get rootkey from device's seed
+    uint8_t buffer_device[64];
+
+    // os_derive_bip32* do not accept NULL path, even with a size of 0, so we provide an empty path
+    const unsigned int empty_path = 0;
+    if (os_derive_bip32_no_throw(CX_CURVE_256K1,
+                                 &empty_path,
+                                 0,
+                                 buffer_device,
+                                 buffer_device + 32) != CX_OK) {
+        PRINTF("An error occurred while comparing the recovery phrase\n");
+        return 0;
+    }
+    PRINTF("Root key from device: \n%.*H\n", 64, buffer_device);
+
+    // compare both rootkey
+    const bool result = os_secure_memcmp(buffer, buffer_device, 64) ? false : true;
+    explicit_bzero(buffer_device, 64);
+    explicit_bzero(buffer, 64);
+
+    return result;
+}
+
 #if defined(HAVE_NBGL)
 #include <nbgl_layout.h>
 

src/ux_common/onboarding_electrum.c src/mnemonic_common/onboarding_electrum.c

@@ -1,8 +1,10 @@
 #include <os.h>
 #include <string.h>
+#include <lcx_hmac.h>
+#include <lcx_rng.h>
 
-#include "../ux_common/common_bip39.h"
 #include "./mnemonic.h"
+#include "../mnemonic_common/common_bip39.h"
 
 #if defined(SCREEN_SIZE_WALLET)
 
@@ -45,7 +47,7 @@ size_t get_current_word_number() {
 }
 
 void reset_mnemonic() {
-    memset(&mnemonic, 0, sizeof(mnemonic));
+    explicit_bzero(&mnemonic, sizeof(mnemonic));
     mnemonic.current_word_index = (size_t) -1;
 }
 
@@ -90,10 +92,15 @@ bool check_mnemonic() {
     PRINTF("Checking the following mnemonic: '%s' (size %ld)\n",
            &mnemonic.buffer[0],
            mnemonic.length);
-    const bool result =
-        bolos_ux_mnemonic_check((unsigned char*) &mnemonic.buffer[0], mnemonic.length);
-    // clearing the mnemonic ASAP
+
+    if (bolos_ux_mnemonic_check((unsigned char*) &mnemonic.buffer[0], mnemonic.length) == false) {
+        reset_mnemonic();
+        return false;
+    }
+
+    const bool result = compare_recovery_phrase((uint8_t*) mnemonic.buffer, mnemonic.length);
     reset_mnemonic();
+
     return result;
 }
 

src/ux_common/onboarding_seed_bip39.c src/mnemonic_common/onboarding_seed_bip39.c

@@ -14,8 +14,8 @@
 #include <nbgl_page.h>
 #include <nbgl_layout.h>
 
-#include "../ux_common/common_bip39.h"
 #include "../ui.h"
+#include "../mnemonic_common/common_bip39.h"
 #include "./mnemonic.h"
 #include "./passphrase_length_screen.h"
 

src/ux_common/onboarding_seed_rom_variables.c src/mnemonic_common/onboarding_seed_rom_variables.c

@@ -4,9 +4,11 @@
 from .utils import format_instructions
 
 
-SPECULOS_MNEMONIC = "glory promote mansion idle axis finger extra " \
-    "february uncover one trip resource lawn turtle enact monster " \
-    "seven myth punch hobby comfort wild raise skin"
+SPECULOS_MNEMONIC = "glory promote mansion idle axis finger extra february uncover one trip resource " \
+    "lawn turtle enact monster seven myth punch hobby comfort wild raise skin"
+
+PLAUSIBLE_MNEMONIC= "feature trigger apart fold answer lend enrich blind foam deny match ecology " \
+    "reform again snow stadium vibrant brain hungry already sadness verify team speed"
 
 
 def test_nominal_full_passphrase_check_ok(navigator: TouchNavigator, functional_test_directory: str):
@@ -40,6 +42,37 @@ def test_nominal_full_passphrase_check_ok(navigator: TouchNavigator, functional_
                                    screen_change_before_first_instruction=True,
                                    screen_change_after_last_instruction=False)
 
+def test_nominal_full_passphrase_check_plausible_but_wrong(navigator: TouchNavigator, functional_test_directory: str):
+    # instructions to go the the keyboard
+    instructions = [
+        CustomNavInsID.HOME_TO_CHECK,
+        CustomNavInsID.LENGTH_CHOOSE_24,
+    ]
+    # instruction to write the words
+    for word in PLAUSIBLE_MNEMONIC.split():
+        instructions += [
+            NavIns(CustomNavInsID.KEYBOARD_WRITE, args=(word[:4], )),
+            NavIns(CustomNavInsID.KEYBOARD_SELECT_SUGGESTION, args=(1, )),
+        ]
+    instructions = format_instructions(instructions)
+    # running the instruction to go to result screen
+    navigator.navigate(instructions,
+                       screen_change_before_first_instruction=False,
+                       screen_change_after_last_instruction=False)
+
+    # now that the 24 words have been written, we check the resulting screen
+    # should be correct
+
+    instructions = format_instructions([
+        CustomNavInsID.RESULT_TO_HOME,
+    ])
+
+    navigator.navigate_and_compare(functional_test_directory,
+                                   "nominal_full_passphrase_check_incorrect",
+                                   instructions,
+                                   screen_change_before_first_instruction=True,
+                                   screen_change_after_last_instruction=False)
+
 
 def test_nominal_full_passphrase_check_error_wrong_passphrase(navigator: TouchNavigator, functional_test_directory: str):
     # instructions to go the the keyboard
@@ -68,7 +101,7 @@ def test_nominal_full_passphrase_check_error_wrong_passphrase(navigator: TouchNa
     ])
 
     navigator.navigate_and_compare(functional_test_directory,
-                                   "nominal_full_passphrase_check_error_wrong_passphrase",
+                                   "nominal_full_passphrase_check_incorrect",
                                    instructions,
                                    screen_change_before_first_instruction=True,
                                    screen_change_after_last_instruction=False)