From f99cfefe673daedba3de51b5117ce1486f39de4b Mon Sep 17 00:00:00 2001 From: Lucas PASCAL Date: Fri, 26 Jul 2024 10:53:54 +0200 Subject: [PATCH] [fix] Some plausible yet wrong mnemonic were deemed valid on NBGL devices --- src/app_main.c | 2 +- src/bagl/nanos_enter_phrase.c | 44 +++---------------- src/bagl/nanox_enter_phrase.c | 41 +---------------- src/bagl/ux_nano.h | 6 +-- src/{ux_common => mnemonic_common}/common.h | 0 .../common_bip39.h | 1 + .../onboarding_electrum.c | 0 .../onboarding_seed_bip39.c | 40 +++++++++++++++++ .../onboarding_seed_rom_variables.c | 0 .../onboarding_seed_rom_variables.h | 0 src/nbgl/mnemonic.c | 17 ++++--- src/nbgl/ui.c | 2 +- .../00000.png | 0 .../00000.png | 0 tests/functional/test_touch_full.py | 41 +++++++++++++++-- 15 files changed, 103 insertions(+), 91 deletions(-) rename src/{ux_common => mnemonic_common}/common.h (100%) rename src/{ux_common => mnemonic_common}/common_bip39.h (97%) rename src/{ux_common => mnemonic_common}/onboarding_electrum.c (100%) rename src/{ux_common => mnemonic_common}/onboarding_seed_bip39.c (86%) rename src/{ux_common => mnemonic_common}/onboarding_seed_rom_variables.c (100%) rename src/{ux_common => mnemonic_common}/onboarding_seed_rom_variables.h (100%) rename tests/functional/snapshots/flex/{nominal_full_passphrase_check_error_wrong_passphrase => nominal_full_passphrase_check_incorrect}/00000.png (100%) rename tests/functional/snapshots/stax/{nominal_full_passphrase_check_error_wrong_passphrase => nominal_full_passphrase_check_incorrect}/00000.png (100%) diff --git a/src/app_main.c b/src/app_main.c index 2f2b9ce5..17792835 100644 --- a/src/app_main.c +++ b/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(); } diff --git a/src/bagl/nanos_enter_phrase.c b/src/bagl/nanos_enter_phrase.c index c19a925a..5d688039 100644 --- a/src/bagl/nanos_enter_phrase.c +++ b/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); } } diff --git a/src/bagl/nanox_enter_phrase.c b/src/bagl/nanox_enter_phrase.c index b9f41d1d..2ed43328 100644 --- a/src/bagl/nanox_enter_phrase.c +++ b/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); diff --git a/src/bagl/ux_nano.h b/src/bagl/ux_nano.h index 21e2d108..ba607b47 100644 --- a/src/bagl/ux_nano.h +++ b/src/bagl/ux_nano.h @@ -17,7 +17,7 @@ #pragma once #include -#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; diff --git a/src/ux_common/common.h b/src/mnemonic_common/common.h similarity index 100% rename from src/ux_common/common.h rename to src/mnemonic_common/common.h diff --git a/src/ux_common/common_bip39.h b/src/mnemonic_common/common_bip39.h similarity index 97% rename from src/ux_common/common_bip39.h rename to src/mnemonic_common/common_bip39.h index b3682794..b301417f 100644 --- a/src/ux_common/common_bip39.h +++ b/src/mnemonic_common/common_bip39.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, diff --git a/src/ux_common/onboarding_electrum.c b/src/mnemonic_common/onboarding_electrum.c similarity index 100% rename from src/ux_common/onboarding_electrum.c rename to src/mnemonic_common/onboarding_electrum.c diff --git a/src/ux_common/onboarding_seed_bip39.c b/src/mnemonic_common/onboarding_seed_bip39.c similarity index 86% rename from src/ux_common/onboarding_seed_bip39.c rename to src/mnemonic_common/onboarding_seed_bip39.c index e78c28c4..6c5ce668 100644 --- a/src/ux_common/onboarding_seed_bip39.c +++ b/src/mnemonic_common/onboarding_seed_bip39.c @@ -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 diff --git a/src/ux_common/onboarding_seed_rom_variables.c b/src/mnemonic_common/onboarding_seed_rom_variables.c similarity index 100% rename from src/ux_common/onboarding_seed_rom_variables.c rename to src/mnemonic_common/onboarding_seed_rom_variables.c diff --git a/src/ux_common/onboarding_seed_rom_variables.h b/src/mnemonic_common/onboarding_seed_rom_variables.h similarity index 100% rename from src/ux_common/onboarding_seed_rom_variables.h rename to src/mnemonic_common/onboarding_seed_rom_variables.h diff --git a/src/nbgl/mnemonic.c b/src/nbgl/mnemonic.c index 8db63bc2..894eb2c0 100644 --- a/src/nbgl/mnemonic.c +++ b/src/nbgl/mnemonic.c @@ -1,8 +1,10 @@ #include #include +#include +#include -#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; } diff --git a/src/nbgl/ui.c b/src/nbgl/ui.c index 02a88fe6..e95b881f 100644 --- a/src/nbgl/ui.c +++ b/src/nbgl/ui.c @@ -14,8 +14,8 @@ #include #include -#include "../ux_common/common_bip39.h" #include "../ui.h" +#include "../mnemonic_common/common_bip39.h" #include "./mnemonic.h" #include "./passphrase_length_screen.h" diff --git a/tests/functional/snapshots/flex/nominal_full_passphrase_check_error_wrong_passphrase/00000.png b/tests/functional/snapshots/flex/nominal_full_passphrase_check_incorrect/00000.png similarity index 100% rename from tests/functional/snapshots/flex/nominal_full_passphrase_check_error_wrong_passphrase/00000.png rename to tests/functional/snapshots/flex/nominal_full_passphrase_check_incorrect/00000.png diff --git a/tests/functional/snapshots/stax/nominal_full_passphrase_check_error_wrong_passphrase/00000.png b/tests/functional/snapshots/stax/nominal_full_passphrase_check_incorrect/00000.png similarity index 100% rename from tests/functional/snapshots/stax/nominal_full_passphrase_check_error_wrong_passphrase/00000.png rename to tests/functional/snapshots/stax/nominal_full_passphrase_check_incorrect/00000.png diff --git a/tests/functional/test_touch_full.py b/tests/functional/test_touch_full.py index e678c56f..4e399177 100644 --- a/tests/functional/test_touch_full.py +++ b/tests/functional/test_touch_full.py @@ -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)