From f8ffaecd1cb5876965a78f81643dc5314feded14 Mon Sep 17 00:00:00 2001 From: Andrew Kozlik Date: Fri, 1 Jul 2022 16:44:23 +0200 Subject: [PATCH] fix(legacy): Abort recovery for invalid words. --- .../firmware/.changelog.d/noissue.security.3 | 1 + legacy/firmware/recovery.c | 66 +++++++++---------- 2 files changed, 31 insertions(+), 36 deletions(-) create mode 100644 legacy/firmware/.changelog.d/noissue.security.3 diff --git a/legacy/firmware/.changelog.d/noissue.security.3 b/legacy/firmware/.changelog.d/noissue.security.3 new file mode 100644 index 000000000..74614fb84 --- /dev/null +++ b/legacy/firmware/.changelog.d/noissue.security.3 @@ -0,0 +1 @@ +Fix potential security issues in recovery workflow. diff --git a/legacy/firmware/recovery.c b/legacy/firmware/recovery.c index 317fb2e20..6aae892ef 100644 --- a/legacy/firmware/recovery.c +++ b/legacy/firmware/recovery.c @@ -37,12 +37,12 @@ /* number of words expected in the new seed */ static uint32_t word_count; -/* recovery mode: - * 0: not recovering - * 1: recover by scrambled plain text words - * 2: recover by matrix entry - */ -static int awaiting_word = 0; +/* recovery mode */ +static enum { + RECOVERY_NONE = 0, // recovery not in progress + RECOVERY_SCRAMBLED = 1, // standard recovery by scrambled plain text words + RECOVERY_MATRIX = 2, // advanced recovery by matrix entry +} recovery_mode = RECOVERY_NONE; /* True if we should not write anything back to config * (can be used for testing seed for correctness). @@ -145,7 +145,7 @@ static void format_number(char *dest, int number) { static void recovery_request(void) { WordRequest resp = {0}; memzero(&resp, sizeof(WordRequest)); - if (awaiting_word == 1) { + if (recovery_mode == RECOVERY_SCRAMBLED) { resp.type = WordRequestType_WordRequestType_Plain; } else if (word_index % 4 == 3) { resp.type = WordRequestType_WordRequestType_Matrix6; @@ -217,7 +217,9 @@ static void recovery_done(void) { fsm_sendFailure(FailureType_Failure_DataError, _("Invalid seed, are words in correct order?")); } - awaiting_word = 0; + memzero(words, sizeof(words)); + word_pincode = 0; + recovery_mode = RECOVERY_NONE; layoutHome(); } @@ -476,6 +478,9 @@ void recovery_init(uint32_t _word_count, bool passphrase_protection, bool _dry_run) { if (_word_count != 12 && _word_count != 18 && _word_count != 24) return; + recovery_mode = RECOVERY_NONE; + word_pincode = 0; + word_index = 0; word_count = _word_count; enforce_wordlist = _enforce_wordlist; dry_run = _dry_run; @@ -504,10 +509,9 @@ void recovery_init(uint32_t _word_count, bool passphrase_protection, config_setU2FCounter(u2f_counter); } + // Prefer matrix recovery if the host supports it. if ((type & RecoveryDeviceType_RecoveryDeviceType_Matrix) != 0) { - awaiting_word = 2; - word_index = 0; - word_pincode = 0; + recovery_mode = RECOVERY_MATRIX; next_matrix(); } else { for (uint32_t i = 0; i < word_count; i++) { @@ -517,8 +521,7 @@ void recovery_init(uint32_t _word_count, bool passphrase_protection, word_order[i] = 0; } random_permute(word_order, 24); - awaiting_word = 1; - word_index = 0; + recovery_mode = RECOVERY_SCRAMBLED; next_word(); } } @@ -527,29 +530,18 @@ static void recovery_scrambledword(const char *word) { int index = -1; if (enforce_wordlist) { // check if word is valid index = mnemonic_find_word(word); - } - if (word_pos == 0) { // fake word - if (strcmp(word, fake_word) != 0) { + if (index < 0) { // not found if (!dry_run) { session_clear(true); } - fsm_sendFailure(FailureType_Failure_ProcessError, - _("Wrong word retyped")); - layoutHome(); + fsm_sendFailure(FailureType_Failure_DataError, + _("Word not found in a wordlist")); + recovery_abort(); return; } - } else { // real word - if (enforce_wordlist) { - if (index < 0) { // not found - if (!dry_run) { - session_clear(true); - } - fsm_sendFailure(FailureType_Failure_DataError, - _("Word not found in a wordlist")); - layoutHome(); - return; - } - } + } + + if (word_pos != 0) { // ignore fake words strlcpy(words[word_pos - 1], word, sizeof(words[word_pos - 1])); } @@ -565,11 +557,11 @@ static void recovery_scrambledword(const char *word) { * for scrambled recovery. */ void recovery_word(const char *word) { - switch (awaiting_word) { - case 2: + switch (recovery_mode) { + case RECOVERY_MATRIX: recovery_digit(word[0]); break; - case 1: + case RECOVERY_SCRAMBLED: recovery_scrambledword(word); break; default: @@ -582,9 +574,11 @@ void recovery_word(const char *word) { /* Abort recovery. */ void recovery_abort(void) { - if (awaiting_word) { + memzero(words, sizeof(words)); + word_pincode = 0; + if (recovery_mode != RECOVERY_NONE) { layoutHome(); - awaiting_word = 0; + recovery_mode = RECOVERY_NONE; } }