ResetDevice(no_backup=true) silently drops the no_backup flag: storage_setU2FCounter() commits mid-ceremony and the commit's own backstop memzeroes `setup` before line 179 reads it

[BitHighlander]

Finding

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

Host sends ResetDevice{no_backup:true, strength:128}. The user holds through both "No Backup" WARNING screens (reset.c:236-247), so setup.no_backup=true. reset_entropy() reaches line 371, takes the no-backup branch, and calls setup_commit(). Inside setup_commit, line 178 calls storage_setU2FCounter(), which calls storage_commit(); storage_commit's first statement sees setup.kind == SETUP_RESET (nothing disarmed it yet) and runs setup_abort(), which memzeroes the whole SetupState. Line 179 then reads setup.no_backup as false and never calls storage_setNoBackup(). Result: a wallet whose recovery seed was never displayed and can never be recovered is persisted with pub.no_backup == false, and fsm_msgGetFeatures (lib/firmware/fsm_msg_common.h:125-126) reports no_backup=false to every host forever. The host and the user are told this device has a recoverable backup when it provably does not. Nothing in unittests/ exercises setup_commit, so CI is blind to it.

Evidence

lib/firmware/reset.c:169-189 (setup_commit):
  173   storage_setPin(setup.pin);
  174   storage_setPassphraseProtected(setup.passphrase_protection);
  175   if (setup.has_language) storage_setLanguage(setup.language);
  176   if (setup.has_label) storage_setLabel(setup.label);
  177   storage_setAutoLockDelayMs(setup.auto_lock_delay_ms);
  178   storage_setU2FCounter(setup.u2f_counter);
  179   if (setup.no_backup) storage_setNoBackup();
  ...
  187   setup_abort();
  188   storage_commit();

lib/firmware/storage.c:174-177:
  void storage_setU2FCounter(uint32_t u2f_counter) {
    shadow_config.storage.pub.u2f_counter = u2f_counter;
    storage_commit();
  }

lib/firmware/storage.c:1806 (first statement of storage_commit):
  if (setup_isArmed()) setup_abort();

lib/firmware/reset.c:90-103 (setup_abort):
  memzero(&setup, sizeof(setup));

lib/firmware/reset.c:84: bool setup_isArmed(void) { return setup.kind != SETUP_NONE; }
lib/firmware/reset.c:352: setup_arm(SETUP_RESET);   /* still armed when setup_commit runs */
lib/firmware/reset.c:371-374:
  if (setup.no_backup) {
    setup_commit(temp_mnemonic, /*imported=*/false);

include/keepkey/firmware/reset.h:92-94 documents the intended order:
  /// The ONE place staged settings reach storage. Applies them, stores \a
  /// mnemonic, disarms, then commits to flash.

Suggested fix

Move the disarm to the top of setup_commit() (call setup_abort() into a local copy of the staged settings first, or capture bool no_backup = setup.no_backup; before line 173), so no field of setup is read after any callee can reach storage_commit(). Replacing line 178 with a direct shadow_config write (no inner commit) also removes the redundant erase/write cycle that currently happens in the middle of every wallet creation.

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.