Finding
Severity: medium · Dimension: memory-safety · Location: lib/firmware/recovery_cipher.c:650
User is recovering a 24-word seed with the substitution cipher (enforce_wordlist defaults to true; the cipher exists precisely so a compromised host never sees the plaintext words). After the user types 23 complete words and the first two characters of word 24, words_entered == 24 == word_count. The host — which decides message timing — sends CharacterAck{done:true} instead of relaying the remaining keystrokes. fsm_msgCharacterAck calls recovery_cipher_finalize() directly (fsm_msg_common.h:749-750).
The word-count gate at recovery_cipher.c:598-618 passes. The loop at 630-639 auto-completes and appends all 24 tokens into new_mnemonic; the 2-character 24th token is ambiguous, so attempt_auto_complete returns false and auto_completed becomes false. Control takes the branch at 650 and returns at 656.
new_mnemonic now holds 23 fully-expanded, in-order, real BIP-39 words of the user's seed plus a 2-character prefix — enough to brute-force the wallet trivially — and it stays there, in the confidential NOLOAD section, until the next recovery_cipher_finalize() call or a power cycle. (CONFIDENTIAL is only cleared at boot, by the bootloader: tools/bootloader/main.c:81-83; see the same note in lib/firmware/passphrase_sm.c:190-203.) Before this change the same input was wiped at line 697. The host cannot read RAM directly, so this is a defense-in-depth regression rather than direct disclosure — but it is exactly the residue the rest of this file goes out of its way to eliminate.
Evidence
lib/firmware/recovery_cipher.c:620-657 and 697
620: static char CONFIDENTIAL new_mnemonic[MNEMONIC_BUF] = "";
...
630: while (tok) {
631: strlcpy(temp_word, tok, CURRENT_WORD_BUF);
633: auto_completed &= attempt_auto_complete(temp_word);
635: strlcat(new_mnemonic, temp_word, MNEMONIC_BUF); <-- expanded BIP-39 words
636: strlcat(new_mnemonic, " ", MNEMONIC_BUF);
638: tok = strtok(NULL, " ");
639: }
640: memzero(temp_word, sizeof(temp_word));
...
650: if (!auto_completed) {
651: fsm_sendFailure(FailureType_Failure_SyntaxError, "Words were not entered ...");
654: setup_abort();
655: layoutHome();
656: return; <-- skips line 697
657: }
...
697: memzero(new_mnemonic, sizeof(new_mnemonic)); <-- the only wipe
setup_abort() does not reach it: recovery_cipher_reset (recovery_cipher.c:71-82) zeroes mnemonic, cipher, coded_word, decoded_word and last_completed_word only — new_mnemonic is a function-scope static it cannot see.
This is a behaviour change from the base. git show 1af2ffe7de:lib/firmware/recovery_cipher.c gated the same early return on `if (!auto_completed && !enforce_wordlist)`, so with the default enforce_wordlist=true the failure fell through to `else { session_clear(true); fsm_sendFailure("Invalid mnemonic..."); recovery_cipher_abort(); }` and reached the common `memzero(new_mnemonic, ...)`. Widening the condition to `!auto_completed` alone made a previously-wiped case exit early.
The per-word wordlist check that would normally catch this only runs at a space boundary (recovery_cipher.c:489-509, inside the `character[0] == ' '` branch), so a word that is still a partial prefix is never validated.
And the host controls when finalize runs — lib/firmware/fsm_msg_common.h:746-755:
} else if (msg->has_done && msg->done) {
recovery_cipher_finalize();
Suggested fix
Add memzero(new_mnemonic, sizeof(new_mnemonic)); before the return at recovery_cipher.c:656 — or restructure the branch to goto the existing cleanup at 697-700 so no future early exit can skip it. Zeroing new_mnemonic on entry (it is already done at line 624) is not sufficient: the residue must not outlive the failing call.
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: memory-safety · Location:
lib/firmware/recovery_cipher.c:650User is recovering a 24-word seed with the substitution cipher (enforce_wordlist defaults to true; the cipher exists precisely so a compromised host never sees the plaintext words). After the user types 23 complete words and the first two characters of word 24, words_entered == 24 == word_count. The host — which decides message timing — sends CharacterAck{done:true} instead of relaying the remaining keystrokes. fsm_msgCharacterAck calls recovery_cipher_finalize() directly (fsm_msg_common.h:749-750).
The word-count gate at recovery_cipher.c:598-618 passes. The loop at 630-639 auto-completes and appends all 24 tokens into new_mnemonic; the 2-character 24th token is ambiguous, so attempt_auto_complete returns false and auto_completed becomes false. Control takes the branch at 650 and returns at 656.
new_mnemonic now holds 23 fully-expanded, in-order, real BIP-39 words of the user's seed plus a 2-character prefix — enough to brute-force the wallet trivially — and it stays there, in the
confidentialNOLOAD section, until the next recovery_cipher_finalize() call or a power cycle. (CONFIDENTIAL is only cleared at boot, by the bootloader: tools/bootloader/main.c:81-83; see the same note in lib/firmware/passphrase_sm.c:190-203.) Before this change the same input was wiped at line 697. The host cannot read RAM directly, so this is a defense-in-depth regression rather than direct disclosure — but it is exactly the residue the rest of this file goes out of its way to eliminate.Evidence
Suggested fix
Add
memzero(new_mnemonic, sizeof(new_mnemonic));before the return at recovery_cipher.c:656 — or restructure the branch togotothe existing cleanup at 697-700 so no future early exit can skip it. Zeroing new_mnemonic on entry (it is already done at line 624) is not sufficient: the residue must not outlive the failing call.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?.