Finding
Severity: medium · Dimension: entropy-coverage · Location: lib/firmware/pin_sm.c:163
Threat model is a malicious host. Suppose random32() degenerates after the boot gate has latched RNG_PASSED -- the exact post-gate degradation rng_health.h:113-116 says the continuous test exists to catch, except neither of these paths is folded into that state. random_uniform() (deps/crypto/.../rand.c:67-72) then returns a fixed value for each n, so Fisher-Yates in RANDOM_PERMUTE (rng.c:129-134) yields a fixed permutation that the attacker computes offline from the firmware image. (a) PIN entry: pin_matrix is that fixed permutation; the host receives the grid positions the user pressed in PinMatrixAck and decode_pin() (pin_sm.c:141-150) is the public mapping, so the host reads the PIN it was never supposed to see. (b) Scrambled recovery: recovery_cipher.c:343-344 produces a predictable substitution over the 26-letter alphabet, and the host receives the ciphered characters the user types -- it decodes the BIP-39 mnemonic in cleartext during the recovery it was supposed to be blind to. Nothing in either path consults rng_health_check(), refuses, or degrades gracefully; both silently produce a permutation and continue. Note the gate correctly disclaims coverage here (rng_health.h:101-102), so the defect is not a false claim of coverage -- it is that the two draws whose predictability hands a secret straight to the host are absent from both the covered list AND the RC28 accepted-raw list, so the release's risk accounting does not name them at all.
Evidence
lib/firmware/pin_sm.c:161-163 -- the matrix the host never sees:
161: /* Init and randomize pin matrix */
162: strlcpy(pin_matrix, "123456789", PIN_BUF);
163: random_permute_char(pin_matrix, 9);
lib/firmware/pin_sm.c:141-150 -- the host sends POSITIONS, the device maps them:
141: static void decode_pin(PINInfo* pin_info) {
142: for (uint32_t i = 0; i < strlen(pin_info->pin); i++) {
143: int32_t j = pin_info->pin[i] - '1';
145: if (j >= 0 && (uint32_t)j < strlen(pin_matrix)) {
146: pin_info->pin[i] = pin_matrix[j];
lib/firmware/recovery_cipher.c:342-344 -- the same construction over the alphabet:
342: /* Scramble cipher */
343: strlcpy(cipher, english_alphabet, ENGLISH_ALPHABET_BUF);
344: random_permute_char(cipher, strlen(cipher));
Both run on the UNCHECKED generator. lib/rand/rng.c:127-137:
127: #define RANDOM_PERMUTE(BUFF, COUNT) \
129: for (size_t i = (COUNT) - 1; i >= 1; i--) { \
130: size_t j = random_uniform(i + 1); \
137: void random_permute_char(char* str, size_t len) { RANDOM_PERMUTE(str, len); }
deps/crypto/trezor-firmware/crypto/rand.c:67-72:
67: uint32_t random_uniform(uint32_t n) {
68: uint32_t x = 0, max = 0xFFFFFFFF - (0xFFFFFFFF % n);
69: while ((x = random32()) >= max)
71: return x / (max / n);
include/keepkey/rand/rng_health.h:101-102 confirms neither is gated:
/// NOT covered: everything else in the tree and in deps/, because plain
/// random_buffer() and random32() are unchecked exactly as on develop.
And the release's own enumeration of deliberately-raw draws omits them -- docs/security/rc28-open-findings-handoff.md:167-172:
`random32_raw()` / `random_buffer_raw()` are the named opt-outs, each
justified at its call site: the health gate itself ..., `GetEntropy` ...,
stack canaries in firmware and bootloader, timer jitter, U2F channel ids,
compare decoys, and `drbg_init`'s seeding.
Exhaustiveness control: an unfiltered `grep -rni rand` over lib/, include/ and tools/ (no --include filters) enumerates every draw outside deps/; pin_sm.c:163 and recovery_cipher.c:344 are the only two whose output is a host-visible secret mapping.
Suggested fix
Add a checked permutation entry point (e.g. random_permute_char_checked() built on the same latched verdict and folding its bytes into rng_continuous) and use it at pin_sm.c:163 and recovery_cipher.c:344; both call sites have somewhere to go on failure -- pin_request() can fail PIN entry and next_character() already has recovery_cipher_abort(). At minimum, add both draws to the documented uncovered list in rng_health.h and to the RC28 accepted-raw enumeration with an explicit rationale, so a future reviewer does not have to rediscover them.
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 ?.
Finding
Severity: medium · Dimension: entropy-coverage · Location:
lib/firmware/pin_sm.c:163Threat model is a malicious host. Suppose random32() degenerates after the boot gate has latched RNG_PASSED -- the exact post-gate degradation rng_health.h:113-116 says the continuous test exists to catch, except neither of these paths is folded into that state. random_uniform() (deps/crypto/.../rand.c:67-72) then returns a fixed value for each n, so Fisher-Yates in RANDOM_PERMUTE (rng.c:129-134) yields a fixed permutation that the attacker computes offline from the firmware image. (a) PIN entry: pin_matrix is that fixed permutation; the host receives the grid positions the user pressed in PinMatrixAck and decode_pin() (pin_sm.c:141-150) is the public mapping, so the host reads the PIN it was never supposed to see. (b) Scrambled recovery: recovery_cipher.c:343-344 produces a predictable substitution over the 26-letter alphabet, and the host receives the ciphered characters the user types -- it decodes the BIP-39 mnemonic in cleartext during the recovery it was supposed to be blind to. Nothing in either path consults rng_health_check(), refuses, or degrades gracefully; both silently produce a permutation and continue. Note the gate correctly disclaims coverage here (rng_health.h:101-102), so the defect is not a false claim of coverage -- it is that the two draws whose predictability hands a secret straight to the host are absent from both the covered list AND the RC28 accepted-raw list, so the release's risk accounting does not name them at all.
Evidence
Suggested fix
Add a checked permutation entry point (e.g. random_permute_char_checked() built on the same latched verdict and folding its bytes into rng_continuous) and use it at pin_sm.c:163 and recovery_cipher.c:344; both call sites have somewhere to go on failure -- pin_request() can fail PIN entry and next_character() already has recovery_cipher_abort(). At minimum, add both draws to the documented uncovered list in rng_health.h and to the RC28 accepted-raw enumeration with an explicit rationale, so a future reviewer does not have to rediscover them.
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?.