Round 4 security remediation: storage key scrubbing, CTAP2 truncation, OOB read, cipher-rotation bypass

include/keepkey/firmware/app_layout.h

@@ -131,7 +131,7 @@ void layout_cipher(const char* current_word, const char* cipher,
 void layout_address(const char* address, QRSize qr_size);
 void set_leaving_handler(leaving_handler_t leaving_func);
 
-void layoutU2FDialog(bool request, const char* title, const char* body, ...)
+bool layoutU2FDialog(bool request, const char* title, const char* body, ...)
     __attribute__((format(printf, 3, 4)));
 
 #endif

include/keepkey/firmware/recovery_cipher.h

@@ -49,6 +49,9 @@ void recovery_cipher_abort(void);
 #if DEBUG_LINK
 const char* recovery_get_cipher(void);
 const char* recovery_get_auto_completed_word(void);
+const char* recovery_get_decoded_mnemonic(void);
+const char* recovery_get_coded_mnemonic(void);
+void recovery_debugLinkStart(uint32_t _word_count);
 #endif
 
 /// Determine if two strings are exact matches for length passed

lib/firmware/app_layout.c

@@ -18,6 +18,7 @@
  */
 
 #include "keepkey/board/layout.h"
+#include "keepkey/board/confirm_sm.h"
 #include "keepkey/board/draw.h"
 #include "keepkey/board/font.h"
 #include "keepkey/board/keepkey_display.h"
@@ -879,14 +880,24 @@ void layout_address(const char* address, QRSize qr_size) {
   }
 }
 
-void layoutU2FDialog(bool request, const char* title, const char* body, ...) {
+bool layoutU2FDialog(bool request, const char* title, const char* body, ...) {
   char strbuf[BODY_CHAR_MAX];
 
   va_list vl;
   va_start(vl, body);
-  vsnprintf(strbuf, BODY_CHAR_MAX, body, vl);
+  int written = vsnprintf(strbuf, BODY_CHAR_MAX, body, vl);
   va_end(vl);
 
+  // Detect both SOURCE truncation (the formatted body did not fit strbuf)
+  // and RENDER truncation (the body fit strbuf but not the OLED canvas), the
+  // same two checks confirm_helper() runs for every other confirmation
+  // screen. This dialog draws unconditionally either way -- callers that
+  // display attacker-controlled, unbounded-length text (e.g. a CTAP2 rp_id)
+  // must check the return value and refuse rather than proceed on an
+  // approval the user could not fully read.
+  bool fits = written >= 0 && (size_t)written < BODY_CHAR_MAX &&
+              confirm_body_fits(strbuf, BODY_WIDTH);
+
   layout_standard_notification(title, strbuf,
                                request ? NOTIFICATION_REQUEST_NO_ANIMATION
                                        : NOTIFICATION_CONFIRM_ANIMATION);
@@ -896,4 +907,6 @@ void layoutU2FDialog(bool request, const char* title, const char* body, ...) {
     animate();
     display_refresh();
   }
+
+  return fits;
 }

lib/firmware/fsm_msg_crypto.h

@@ -44,13 +44,16 @@ void fsm_msgCipherKeyValue(CipherKeyValue* msg) {
     aes_cbc_encrypt(msg->value.bytes, resp->value.bytes, msg->value.size,
                     ((msg->iv.size == 16) ? (msg->iv.bytes) : (data + 32)),
                     &ctx);
+    memzero(&ctx, sizeof(ctx));
   } else {
     aes_decrypt_ctx ctx;
     aes_decrypt_key256(data, &ctx);
     aes_cbc_decrypt(msg->value.bytes, resp->value.bytes, msg->value.size,
                     ((msg->iv.size == 16) ? (msg->iv.bytes) : (data + 32)),
                     &ctx);
+    memzero(&ctx, sizeof(ctx));
   }
+  memzero(data, sizeof(data));
 
   resp->has_value = true;
   resp->value.size = msg->value.size;

lib/firmware/recovery_cipher.c

@@ -60,13 +60,24 @@ static CONFIDENTIAL char cipher[ENGLISH_ALPHABET_BUF];
 static CONFIDENTIAL char coded_word[12];
 static CONFIDENTIAL char decoded_word[12];
 static CONFIDENTIAL char last_completed_word[12];
+/* Raw cipher bytes for the whole mnemonic entered so far, mirroring
+ * `mnemonic` byte-for-byte (same appends, same truncations, always the same
+ * length) but holding the literal characters the host sent instead of their
+ * decoded plaintext. cipher rotates after every character, so re-deriving a
+ * word's coded form from its decoded form via the CURRENT cipher does not
+ * recover what was actually sent for any earlier position -- only bytes
+ * preserved at the time they were typed do. Backspacing, of any depth across
+ * any number of completed words, is then just "truncate like mnemonic and
+ * re-derive the current word" -- see get_current_coded_word(). */
+static CONFIDENTIAL char coded_mnemonic[MNEMONIC_BUF];
 
 #if DEBUG_LINK
 static char auto_completed_word[CURRENT_WORD_BUF];
 #endif
 
 static uint32_t get_current_word_pos(void);
 static void get_current_word(char* current_word);
+static void get_current_coded_word(char* current_coded_word);
 
 void recovery_cipher_reset(void) {
   awaiting_character = false;
@@ -75,6 +86,7 @@ void recovery_cipher_reset(void) {
   words_entered = 0;
   word_count = 0;
   memzero(mnemonic, sizeof(mnemonic));
+  memzero(coded_mnemonic, sizeof(coded_mnemonic));
   memzero(cipher, sizeof(cipher));
   memzero(coded_word, sizeof(coded_word));
   memzero(decoded_word, sizeof(decoded_word));
@@ -149,6 +161,21 @@ static void get_current_word(char* current_word) {
   }
 }
 
+/// \returns the current word's raw typed cipher bytes by parsing
+/// coded_mnemonic thus far -- mirrors get_current_word() exactly, but reads
+/// the coded form instead of the decoded plaintext.
+/// \param current_coded_word[out]  Array to populate; sized like coded_word.
+static void get_current_coded_word(char* current_coded_word) {
+  char* pos = strrchr(coded_mnemonic, ' ');
+
+  if (pos) {
+    pos++;
+    strlcpy(current_coded_word, pos, sizeof(coded_word));
+  } else {
+    strlcpy(current_coded_word, coded_mnemonic, sizeof(coded_word));
+  }
+}
+
 _Static_assert(BIP39_WORDLIST_PADDED,
                "bip39 wordlist must be padded to 9 characters");
 
@@ -514,7 +541,10 @@ void recovery_character(const char* character) {
     memzero(decoded_word, sizeof(decoded_word));
 
     if (word_count && words_entered == word_count) {
+      // Keep coded_mnemonic's length-per-mnemonic invariant even on this
+      // early-return path -- it skips the shared append below.
       strlcat(mnemonic, " ", MNEMONIC_BUF);
+      strlcat(coded_mnemonic, character, MNEMONIC_BUF);
       recovery_cipher_finalize();
       return;
     }
@@ -530,8 +560,9 @@ void recovery_character(const char* character) {
     }
   }
 
-  // concat to mnemonic
+  // concat to mnemonic, and to its raw-cipher-bytes mirror in lockstep
   strlcat(mnemonic, decoded_character, MNEMONIC_BUF);
+  strlcat(coded_mnemonic, character, MNEMONIC_BUF);
 
   next_character();
 }
@@ -558,23 +589,29 @@ void recovery_delete_character(void) {
     if (mnemonic[len - 1] == ' ') words_entered--;
 
     mnemonic[len - 1] = '\0';
+    // coded_mnemonic is always exactly as long as mnemonic -- every append
+    // to one is paired with an append to the other in recovery_character(),
+    // and CharacterAck.character is nanopb-bounded to one byte -- so the
+    // same truncation applies to both, regardless of how many characters or
+    // word boundaries are being backed up over.
+    coded_mnemonic[len - 1] = '\0';
   }
 
   /* Resync the current-word accumulators with the edited mnemonic so a
    * corrected word is validated on its real value (stale bytes here would
    * fail validation and trigger a storage_reset on a real recovery).
-   * decoded_word is the typed prefix of the current word; coded_word is its
-   * reverse-cipher form (session cipher is fixed, so it is reconstructable). */
+   * decoded_word is plaintext, always safe to rebuild from mnemonic.
+   * coded_word must hold the literal bytes the host actually typed -- cipher
+   * rotates every character, so re-deriving it from decoded_word via the
+   * CURRENT cipher does not recover what was sent for any earlier position.
+   * Re-deriving it from coded_mnemonic the same way decoded_word is
+   * re-derived from mnemonic does, at any backspace depth or word count. */
   char cur[CURRENT_WORD_BUF];
   get_current_word(cur);
   strlcpy(decoded_word, cur, sizeof(decoded_word));
   memzero(cur, sizeof(cur));
-  size_t wlen = strlen(decoded_word);
-  for (size_t i = 0; i < wlen && i + 1 < sizeof(coded_word); i++) {
-    char d = decoded_word[i];
-    coded_word[i] = (d >= 'a' && d <= 'z') ? cipher[d - 'a'] : d;
-  }
-  coded_word[wlen < sizeof(coded_word) ? wlen : sizeof(coded_word) - 1] = '\0';
+
+  get_current_coded_word(coded_word);
 
   next_character();
 }
@@ -722,4 +759,37 @@ const char* recovery_get_cipher(void) { return cipher; }
 const char* recovery_get_auto_completed_word(void) {
   return auto_completed_word;
 }
+
+/*
+ * recovery_get_decoded_mnemonic() - Gets the plaintext mnemonic decoded so
+ * far. Test-only: production code never exposes this outside the CONFIDENTIAL
+ * section.
+ */
+const char* recovery_get_decoded_mnemonic(void) { return mnemonic; }
+
+/*
+ * recovery_get_coded_mnemonic() - Gets the raw cipher bytes typed so far.
+ * Test-only: lets a test assert directly that backspacing (at any depth,
+ * across any number of word boundaries) leaves coded_mnemonic holding
+ * exactly the bytes the host actually sent, not a stale/reconstructed
+ * mixture. See #584.
+ */
+const char* recovery_get_coded_mnemonic(void) { return coded_mnemonic; }
+
+/// Test-only: arms a recovery ceremony and generates the first cipher,
+/// bypassing recovery_cipher_init()'s confirm()/PIN gates so
+/// recovery_character()/recovery_delete_character() can be driven directly
+/// from a unit test. Mirrors the tail of recovery_cipher_init() exactly.
+void recovery_debugLinkStart(uint32_t _word_count) {
+  setup_stage(/*passphrase_protection=*/false, "english", "test",
+              /*auto_lock_delay_ms=*/0, /*u2f_counter=*/0, /*no_backup=*/false);
+  word_count = _word_count;
+  enforce_wordlist = true;
+  dry_run = true;
+  memset(mnemonic, 0, sizeof(mnemonic));
+  awaiting_character = true;
+  words_entered = 1;
+  setup_arm(SETUP_RECOVERY);
+  next_character();
+}
 #endif

lib/firmware/storage.c

@@ -622,6 +622,7 @@ void storage_secMigrate(SessionState* ss, Storage* storage, bool encrypt) {
     aes_cbc_encrypt((const uint8_t*)scratch, storage->encrypted_sec,
                     sizeof(scratch), iv + 32, &ctx);
     memzero(&ctx, sizeof(ctx));
+    memzero(iv, sizeof(iv));
     storage->encrypted_sec_version = STORAGE_VERSION;
   } else {
     memzero(&storage->sec, sizeof(storage->sec));
@@ -640,6 +641,7 @@ void storage_secMigrate(SessionState* ss, Storage* storage, bool encrypt) {
                       (uint8_t*)&scratch[0], sizeof(scratch), iv + 32, &ctx);
     }
     memzero(iv, sizeof(iv));
+    memzero(&ctx, sizeof(ctx));
 
     // De-serialize from scratch.
     storage_readHDNode(&storage->sec.node, &scratch[0], 129);
@@ -721,6 +723,7 @@ static void storage_cipherBlock(bool encrypt, const uint8_t* key,
     aes_cbc_encrypt((const uint8_t*)plaintextBlock, ciphertextBlock, blockSize,
                     iv + 32, &ctx);
     memzero(&ctx, sizeof(ctx));
+    memzero(iv, sizeof(iv));
   } else {
     // decrypt
     memcpy(iv, key, sizeof(iv));
@@ -729,6 +732,7 @@ static void storage_cipherBlock(bool encrypt, const uint8_t* key,
     aes_cbc_decrypt((const uint8_t*)ciphertextBlock, plaintextBlock, blockSize,
                     iv + 32, &ctx);
     memzero(iv, sizeof(iv));
+    memzero(&ctx, sizeof(ctx));
   }
 
   return;
@@ -811,6 +815,8 @@ bool storage_getAuthData(authType* returnData) {
                           sizeof(shadow_config.storage.sec.authBlock));
     } else {
       // encrypted, passphrase not available
+      memzero(authdataKey, sizeof(authdataKey));
+      memzero((void*)&plaintextAuthBlock, sizeof(plaintextAuthBlock));
       return false;
     }
   } else {
@@ -825,11 +831,15 @@ bool storage_getAuthData(authType* returnData) {
   // authdata
   if (0 != memcmp(shadow_config.storage.pub.authdata_fingerprint, testFp,
                   sizeof(shadow_config.storage.pub.authdata_fingerprint))) {
+    memzero(authdataKey, sizeof(authdataKey));
+    memzero((void*)&plaintextAuthBlock, sizeof(plaintextAuthBlock));
     return false;
   }
 
   memcpy(returnData, plaintextAuthBlock.authData,
          sizeof(plaintextAuthBlock.authData));
+  memzero(authdataKey, sizeof(authdataKey));
+  memzero((void*)&plaintextAuthBlock, sizeof(plaintextAuthBlock));
   return true;
 }
 
@@ -856,6 +866,7 @@ void storage_setAuthData(const authType* setData) {
                         (uint8_t*)&shadow_config.storage.sec.authBlock,
                         sizeof(shadow_config.storage.sec.authBlock));
     shadow_config.storage.pub.authdata_encrypted = true;
+    memzero(authdataKey, sizeof(authdataKey));
   } else {
     // not encrypted
     memcpy((void*)&shadow_config.storage.sec.authBlock,

lib/firmware/transaction.c

@@ -546,7 +546,13 @@ int compile_output(const CoinType* coin, const HDNode* root, TxOutputType* in,
       case OutputScriptType_PAYTOP2SHWITNESS:
       case OutputScriptType_PAYTOTAPROOT: {
         char amount_str[32];
-        char node_str[NODE_STRING_LENGTH];
+        // ADDR_STR_LEN, not NODE_STRING_LENGTH: this buffer is passed to
+        // txin_dgst_compare()/txin_dgst_save_and_reset(), which unconditionally
+        // memcpy/strncmp ADDR_STR_LEN (130) bytes -- their real contract, per
+        // the other call site below which passes a 130-byte protobuf address
+        // field. A 50-byte NODE_STRING_LENGTH buffer here was an 80-byte OOB
+        // stack read.
+        char node_str[ADDR_STR_LEN];
         coin_amnt_to_str(coin, in->amount, amount_str, sizeof(amount_str));
         memset(node_str, 0, sizeof(node_str));
         if (!bip32_node_to_string(node_str, sizeof(node_str), coin,

lib/firmware/u2f.c

@@ -649,9 +649,16 @@ bool u2f_load_credential(const uint8_t app_id[32], const uint8_t key_handle[64],
 }
 
 bool ctap2_request_user_presence(const char* rp_id, bool registration) {
-  layoutU2FDialog(true, registration ? "Create Passkey" : "Use Passkey",
-                  registration ? "Create a passkey for %s?" : "Sign in to %s?",
-                  rp_id);
+  bool fits = layoutU2FDialog(
+      true, registration ? "Create Passkey" : "Use Passkey",
+      registration ? "Create a passkey for %s?" : "Sign in to %s?", rp_id);
+  if (!fits) {
+    // rp_id is host-controlled and can run up to 253 chars; the credential
+    // is bound to the FULL string (see ctap2's sha256_Raw() over rp_id), so
+    // an rp_id the user can't read in full must not be silently approved.
+    layoutHome();
+    return false;
+  }
   bool saw_button_up = false;
   for (uint32_t remaining = 10 * U2F_TIMEOUT; remaining > 0; --remaining) {
     if (reader != NULL && reader->cmd == U2FHID_CANCEL) {