PIN-KDF per-device salt is never drawn: it is 32 zero bytes on every device initialized after storage V2 and after every WipeDevice

[BitHighlander]

Finding

Severity: medium · Dimension: entropy-coverage · Location: lib/firmware/storage.c:1675

Factory-new KeepKey running 7.15 (or any device right after WipeDevice). storage_init() finds no active sector, calls storage_reset_impl() which memsets Storage (storage.c:1675) leaving pub.random_salt = 32 x 0x00, then storage_resetUuid()+storage_commit() (storage.c:1606-1607) persists those zeros. On the next boot the record is version 19, so storage_fromFlash dispatches to storage_readV19 -> storage_readStorageV16Plaintext, which memcpy's the stored zeros back (storage.c:1201). storage_readStorageV1 -- the only function that ever draws a salt -- is never executed on this device. When the user sets a PIN, storage_setPin_impl -> storage_deriveWrappingKey builds salt = HW_ENTROPY_DATA[44] || 0x00 x 32 and PBKDF2-stretches the PIN with it. Concrete consequence: the per-installation entropy contributes zero bits. An attacker who has once recovered the device's 12-byte STM32 unique ID and the 32-byte OTP randomness block (both are plain read-only flash/OTP, not PIN-protected) computes ONE PBKDF2 table over the PIN space for that device and reuses it after every wipe and every PIN change for the life of the device, instead of redoing 2 x PIN_ITER_COUNT PBKDF2 work per installation. The defense the comment at storage.c:921-922 claims -- "unpredictability here is what stops a precomputed wrapping-key table" -- is absent on every device that did not migrate from a pre-2020 V1/V2 record.

Evidence

lib/firmware/storage.c:1671-1679 (the only initializer for a fresh/wiped device):
  1671: void storage_reset_impl(SessionState* ss, ConfigFlash* cfg) {
  1675:   memset(&cfg->storage, 0, sizeof(cfg->storage));
  1679:   storage_setPin_impl(ss, &cfg->storage, "");

The ONE and only mint site, inside storage_readStorageV1() (declared at storage.c:867):
  921:   /* PIN-KDF salt: unpredictability here is what stops a precomputed
  922:    * wrapping-key table, so it is key material. */
  923:   storage_drawKeyMaterial(storage->pub.random_salt, 32);

reachable only from the V1/V2 record readers:
  1317:   storage_readStorageV1(ss, &dst->storage, flash + 44, 481);   // storage_readV1
  1324:   storage_readStorageV1(ss, &dst->storage, flash + 44, 481);   // storage_readV2

Every other write to the field is a memcpy back out of flash:
  1059:   memcpy(storage->pub.random_salt, ptr + 341, 32);   // readStorageV11
  1201:   memcpy(storage->pub.random_salt, ptr + 437, 32);   // readStorageV16Plaintext

And it is consumed as the whole per-installation half of the PBKDF2 salt:
  373:     uint8_t salt[HW_ENTROPY_LEN + RANDOM_SALT_LEN];
  390:     memset(salt, 0, sizeof(salt));
  391:     flash_readHWEntropy(salt, sizeof(salt));
  392:     memcpy(salt + HW_ENTROPY_LEN, random_salt, RANDOM_SALT_LEN);
  395:     pbkdf2_hmac_sha256_Init(&ctx, (const uint8_t*)pin, pin_len, salt, sizeof(salt), 1);

The opt-in list nevertheless advertises it as a covered draw, include/keepkey/rand/rng_health.h:95-96:
  ///   - the PIN-KDF salt         storage_readStorageV1(), the V1
  ///                              upgrade path that mints one

Control on the search: `grep -rn 'pub\.random|->random_salt|\.random_salt'` over lib/**.c returns exactly the 11 lines above; no other assignment exists. `git log -S random_salt -- lib/firmware/storage.c` shows the draw has sat in storage_readStorageV1 since 24330a664 (Feb 2020), so this is long-standing, not a 7.15 regression.

Suggested fix

Draw the salt where a new storage record is created, not only on the V1 upgrade path: add storage_drawKeyMaterial(cfg->storage.pub.random_salt, 32); in storage_reset_impl() between lines 1677 and 1679 (before the empty-PIN storage_setPin_impl, which takes the sha512 branch and does not consume the salt). Flash loads overwrite it on initialized devices, so nothing existing is disturbed. Then correct include/keepkey/rand/rng_health.h:95-96, which currently reads as though the PIN-KDF salt is a live covered draw.

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.