SFT-6378: removed more branching from mnemonic decode process
What the developer wrote
Authored by Matt Gleason
SFT-6378: removed more branching from mnemonic decode process
What changed, and why it matters
This commit tweaks the code that converts a user's BIP39 recovery phrase (mnemonic words) into secret bits inside a hardware wallet. The change removes an 'if' branch during word copying so the loop always writes to the same positions, regardless of how long each word is. The stated goal is to make the copy operation take the same amount of time and perform the same memory accesses for every word length, which can help defend against timing and power-analysis side-channel attacks that might otherwise leak information about the recovery phrase.
Treat as a hardening/security-improvement commit. Review the surrounding mnemonic_to_bits() implementation for remaining variable-time behavior (binary search word lookup, checksum validation, error-path timing) and consider whether a full side-channel audit is warranted. No immediate patch deployment is required solely for this change, but it should be included in the next firmware release.
Security signals we found
Branchless, constant-time-style copy of sensitive input words
Masking of post-delimiter bytes to zero to avoid information leakage via memory access patterns
Removal of conditional stores that depended on word length
Comment explicitly describes the copy as constant-time and explains the masking rationale
Continued use of dummy iterations for words beyond n to mitigate timing leakage from word count
Evidence from the diff
In extmod/trezor-firmware/crypto/bip39.c, mnemonic_to_bits() is modified to copy each candidate BIP39 word into a fixed-size buffer without branching on the delimiter. Previously, the loop conditionally appended characters (j advanced only while !past_delim). Now it unconditionally writes padded[idx] to current_word[ci], masking bytes after the delimiter to zero using a branchless keep mask derived from past_delim. Word length is accumulated via keep & 1. The comment explicitly frames this as a constant-time copy. The change is defensive and partial: it addresses one variable-time/variable-access pattern in mnemonic decoding but does not claim to make the entire BIP39 lookup or checksum constant-time.
Changed components
extmod/trezor-firmware/crypto/bip39.cmnemonic_to_bits() functionBIP39 mnemonic decoding / seed derivation pathInspect captured patch +13 / −7
diff --git a/extmod/trezor-firmware/crypto/bip39.c b/extmod/trezor-firmware/crypto/bip39.c
index 58cce46..2a6516e 100644
--- a/extmod/trezor-firmware/crypto/bip39.c
+++ b/extmod/trezor-firmware/crypto/bip39.c
@@ -167,7 +167,7 @@ int mnemonic_to_bits(const char* mnemonic, uint8_t* bits) {
}
char current_word[BIP39_MAX_WORD_LEN] = {0};
- uint32_t j = 0, k = 0, ki = 0, bi = 0;
+ uint32_t k = 0, ki = 0, bi = 0;
uint8_t result[32 + 1] = {0};
uint32_t all_words_found = 0xFFFFFFFF; // Track if all words were found
uint32_t word_too_long = 0; // Set if any word exceeds max length
@@ -180,24 +180,30 @@ int mnemonic_to_bits(const char* mnemonic, uint8_t* bits) {
for (uint32_t w = 0; w < BIP39_MNEMONIC_MAX_WORDS; w++) {
// active is 0xFFFFFFFF for real words (w < n), 0 for dummy iterations.
uint32_t active = -(uint32_t)(w < n);
- j = 0;
memzero(current_word, sizeof(current_word));
// Always run BIP39_MAX_WORD_LEN iterations.
// Each read is bounds-checked to prevent OOB on crafted overlong input.
+ // The store is unconditional and writes to a fixed index, so the NUMBER of
+ // stores does not depend on word length (constant-time copy). Bytes at or
+ // after the delimiter are masked to 0; because reads start at the word
+ // boundary this produces the same left-aligned, zero-padded buffer as the
+ // previous compacting copy.
uint32_t past_delim = 0;
+ uint32_t wordlen = 0;
for (uint32_t ci = 0; ci < BIP39_MAX_WORD_LEN; ci++) {
uint32_t idx = i + ci;
char c = (idx < (uint32_t)(sizeof(padded) - 1)) ? padded[idx] : '\0';
uint32_t is_delim = (uint32_t)((c == ' ') | (c == '\0'));
past_delim |= is_delim;
- if (!past_delim) {
- current_word[j++] = c;
- }
+ // keep = 0xFF for characters before the delimiter, 0x00 at/after it.
+ uint8_t keep = (uint8_t)(past_delim - 1);
+ current_word[ci] = (char)((uint8_t)c & keep);
+ wordlen += (uint32_t)(keep & 1);
}
- // Advance i past the characters copied into current_word
- i += j;
+ // Advance past the copied characters
+ i += wordlen;
// If past_delim was never set the word overruns BIP39_MAX_WORD_LEN;
// skip remaining characters. Valid mnemonics never take this path.
Why this scored 47/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.