CVE-2019-18672: Improper Validation of Integrity Check Value (CWE-354)
value: Insufficient checks in the finite state machine of the ShapeShift KeepKey hardware wallet before firmware 6.2.2 allow a partial reset of cryptographic secrets to known values via crafted messages. Notably, this breaks the security of U2F for new server registrations and invalidates existing registrations. This vulnerability can be exploited by unauthenticated attackers and the interface is reachable via WebUSB.
CWE description: The product does not validate or incorrectly validates the integrity check values or "checksums" of a message. This may prevent it from detecting if the data has been modified or corrupted in transmission.
CVSS v3 base score: 7.5
Published: 2019-12-06T18:15Z
Repository: https://github.com/keepkey/keepkey-firmware
Language: C
Files changed: lib/firmware/recovery_cipher.c
Commit date: 2019-09-18 08:49:46 -0600
Commit message: firmware: stronger recovery state machine checks

Diff: diff --git a/lib/firmware/recovery_cipher.c b/lib/firmware/recovery_cipher.c
index ace3bb9b8..1ef2501a2 100644
--- a/lib/firmware/recovery_cipher.c
+++ b/lib/firmware/recovery_cipher.c
@@ -34,10 +34,11 @@
 #include <string.h>
 #include <stdio.h>
 
 #define MAX_UNCYPHERED_WORDS (3)
 
+static bool recovery_started = false;
 static bool enforce_wordlist;
 static bool dry_run;
 static bool awaiting_character;
 static CONFIDENTIAL char mnemonic[MNEMONIC_BUF];
 static char english_alphabet[ENGLISH_ALPHABET_BUF] = "abcdefghijklmnopqrstuvwxyz";
@@ -54,10 +55,11 @@ static void get_current_word(char *current_word);
 static void recovery_abort(void) {
     if (!dry_run) {
         storage_reset();
     }
 
+    recovery_started = false;
     awaiting_character = false;
     memzero(mnemonic, sizeof(mnemonic));
     memzero(cipher, sizeof(cipher));
 }
 
@@ -268,10 +270,11 @@ void recovery_cipher_init(bool passphrase_protection, bool pin_protection,
     /* Clear mnemonic */
     memset(mnemonic, 0, sizeof(mnemonic) / sizeof(char));
 
     /* Set to recovery cipher mode and generate and show next cipher */
     awaiting_character = true;
+    recovery_started = true;
     next_character();
 }
 
 /*
  * next_character() - Randomizes cipher and displays it for next character entry
@@ -281,10 +284,17 @@ void recovery_cipher_init(bool passphrase_protection, bool pin_protection,
  * OUTPUT
  *     none
  */
 void next_character(void)
 {
+    if (!recovery_started) {
+        recovery_abort();
+        fsm_sendFailure(FailureType_Failure_UnexpectedMessage, "Not in Recovery mode");
+        layoutHome();
+        return;
+    }
+
     /* Scramble cipher */
     strlcpy(cipher, english_alphabet, ENGLISH_ALPHABET_BUF);
     random_permute_char(cipher, strlen(cipher));
 
     static char CONFIDENTIAL current_word[CURRENT_WORD_BUF];
@@ -339,11 +349,11 @@ void next_character(void)
  * OUTPUT
  *     none
  */
 void recovery_character(const char *character)
 {
-    if (!awaiting_character) {
+    if (!awaiting_character || !recovery_started) {
         recovery_abort();
         fsm_sendFailure(FailureType_Failure_UnexpectedMessage, "Not in Recovery mode");
         layoutHome();
         return;
     }
@@ -428,10 +438,17 @@ void recovery_character(const char *character)
  * OUTPUT
  *     none
  */
 void recovery_delete_character(void)
 {
+    if (!recovery_started) {
+        recovery_abort();
+        fsm_sendFailure(FailureType_Failure_UnexpectedMessage, "Not in Recovery mode");
+        layoutHome();
+        return;
+    }
+
     if(strlen(mnemonic) > 0)
     {
         mnemonic[strlen(mnemonic) - 1] = '\0';
     }
 
@@ -446,10 +463,17 @@ void recovery_delete_character(void)
  * OUTPUT
  *     none
  */
 void recovery_cipher_finalize(void)
 {
+    if (!recovery_started) {
+        recovery_abort();
+        fsm_sendFailure(FailureType_Failure_UnexpectedMessage, "Not in Recovery mode");
+        layoutHome();
+        return;
+    }
+
     static char CONFIDENTIAL new_mnemonic[MNEMONIC_BUF] = "";
     static char CONFIDENTIAL temp_word[CURRENT_WORD_BUF];
     volatile bool auto_completed = true;
 
     /* Attempt to autocomplete each word */
@@ -477,11 +501,11 @@ void recovery_cipher_finalize(void)
         layoutHome();
         return;
     }
 
     /* Truncate additional space at the end */
-    new_mnemonic[strlen(new_mnemonic) - 1] = '\0';
+    new_mnemonic[MAX(0u, strnlen(new_mnemonic, sizeof(new_mnemonic)) - 1)] = '\0';
 
     if (!dry_run && (!enforce_wordlist || mnemonic_check(new_mnemonic))) {
         storage_setMnemonic(new_mnemonic);
         memzero(new_mnemonic, sizeof(new_mnemonic));
         if (!enforce_wordlist) {
@@ -530,10 +554,12 @@ void recovery_cipher_finalize(void)
  * OUTPUT
  *     true/false of whether recovery was aborted
  */
 bool recovery_cipher_abort(void)
 {
+    recovery_started = false;
+
     if (awaiting_character) {
         awaiting_character = false;
         return true;
     }
     return false;

Vulnerable code:     if (!awaiting_character) {
    new_mnemonic[strlen(new_mnemonic) - 1] = '\0';
Fixed code: static bool recovery_started = false;
    recovery_started = false;
    recovery_started = true;
    if (!recovery_started) {
        recovery_abort();
        fsm_sendFailure(FailureType_Failure_UnexpectedMessage, "Not in Recovery mode");
        layoutHome();
        return;
    }

    if (!awaiting_character || !recovery_started) {
    if (!recovery_started) {
        recovery_abort();
        fsm_sendFailure(FailureType_Failure_UnexpectedMessage, "Not in Recovery mode");
        layoutHome();
        return;
    }

    if (!recovery_started) {
        recovery_abort();
        fsm_sendFailure(FailureType_Failure_UnexpectedMessage, "Not in Recovery mode");
        layoutHome();
        return;
    }

    new_mnemonic[MAX(0u, strnlen(new_mnemonic, sizeof(new_mnemonic)) - 1)] = '\0';
    recovery_started = false;