storage_commit() erases the only valid storage sector before writing the replacement; an interruption anywhere in that window makes the next boot silently reset the wallet

[BitHighlander]

Finding

Severity: medium · Dimension: storage-bootloader · Location: lib/firmware/storage.c:1875

A user unplugs the KeepKey (or the USB link browns out) while the device is processing a PIN entry. pin_protect() calls storage_increasePinFails() -> storage_commit(), which first erases the sector holding the only copy of the wallet (line 1875), then erases the next sector, then writes ~2.5 KB. On an STM32F2 a 16 KB sector erase alone is on the order of hundreds of milliseconds, so the window in which NO sector carries the "stor" magic is a substantial fraction of a second, and it is opened twice per PIN entry plus on every ApplySettings/ApplyPolicies/U2F-counter update. If power is lost in that window, the next boot has find_active_storage() fail, storage_isActiveSector() return false, and storage_init() takes the fresh-device path: storage_resetUuid() + storage_commit(). The seed is gone, with no prompt and no error — the exact 'wipe when it should not' outcome, reachable without any attacker at all.

Evidence

lib/firmware/storage.c:1875-1891:
  flash_erase_word(storage_location);      /* erase the CURRENTLY ACTIVE sector */
  wear_leveling_shift();
  flash_erase_word(storage_location);      /* erase the next sector */

  /* Write storage data first before writing storage magic  */
  if (!flash_write_word(storage_location, STORAGE_MAGIC_LEN,
                        sizeof(flash_temp) - STORAGE_MAGIC_LEN,
                        (uint8_t*)flash_temp + STORAGE_MAGIC_LEN)) {
    flash_erase_word(storage_location);
    continue;  // Retry
  }
  if (!flash_write_word(storage_location, 0, STORAGE_MAGIC_LEN,
                        (uint8_t*)flash_temp)) {

lib/board/keepkey_flash.c:99-109 — flash_erase_word() erases a whole flash SECTOR via svc_flash_erase_sector().

lib/firmware/storage.c:1591-1609 (storage_init):
  if (!find_active_storage(&storage_location)) {
    storage_location = STORAGE_SECT_DEFAULT;
  }
  ...
  if (!storage_isActiveSector(flash)) {
    storage_resetUuid();
    storage_commit();
    return;
  }

lib/firmware/pin_sm.c:249 and :266 — every PIN entry commits twice:
  storage_increasePinFails();   /* -> storage_commit() */
  ...
  storage_resetPinFails();      /* -> storage_commit() */

Suggested fix

Make the write crash-safe rather than erase-first: write the new record (magic last) into the next sector, verify its CRC, and only then erase the previous sector. That requires breaking find_active_storage()'s 'lowest-numbered sector with the magic wins' rule (lib/board/memory.c:283-294) — add a monotonic generation counter to Metadata and have find_active_storage() pick the highest generation, tie-broken by CRC validity. Until then, at minimum stop committing twice per PIN attempt (storage_increasePinFails/storage_resetPinFails).

Verification

(not independently verified)

---

Found by an adversarial audit of release/7.15 (628b09257). Each finding was independently re-checked by a separate reviewer instructed to refute it by default; this one survived at confidence ?.

[BitHighlander]

Resolved on alpha by #587 (merge 893c93d). The final immutable head e4ec531 passed both complete CI workflow runs.