diff --git a/include/keepkey/firmware/app_layout.h b/include/keepkey/firmware/app_layout.h index 10e35b891..3f353fc38 100644 --- a/include/keepkey/firmware/app_layout.h +++ b/include/keepkey/firmware/app_layout.h @@ -131,7 +131,7 @@ void layout_cipher(const char* current_word, const char* cipher, void layout_address(const char* address, QRSize qr_size); void set_leaving_handler(leaving_handler_t leaving_func); -void layoutU2FDialog(bool request, const char* title, const char* body, ...) +bool layoutU2FDialog(bool request, const char* title, const char* body, ...) __attribute__((format(printf, 3, 4))); #endif diff --git a/include/keepkey/firmware/recovery_cipher.h b/include/keepkey/firmware/recovery_cipher.h index a332b862c..d47939d1d 100644 --- a/include/keepkey/firmware/recovery_cipher.h +++ b/include/keepkey/firmware/recovery_cipher.h @@ -49,6 +49,9 @@ void recovery_cipher_abort(void); #if DEBUG_LINK const char* recovery_get_cipher(void); const char* recovery_get_auto_completed_word(void); +const char* recovery_get_decoded_mnemonic(void); +const char* recovery_get_coded_mnemonic(void); +void recovery_debugLinkStart(uint32_t _word_count); #endif /// Determine if two strings are exact matches for length passed diff --git a/lib/firmware/app_layout.c b/lib/firmware/app_layout.c index 82bff62b3..21b0dc133 100644 --- a/lib/firmware/app_layout.c +++ b/lib/firmware/app_layout.c @@ -18,6 +18,7 @@ */ #include "keepkey/board/layout.h" +#include "keepkey/board/confirm_sm.h" #include "keepkey/board/draw.h" #include "keepkey/board/font.h" #include "keepkey/board/keepkey_display.h" @@ -879,14 +880,24 @@ void layout_address(const char* address, QRSize qr_size) { } } -void layoutU2FDialog(bool request, const char* title, const char* body, ...) { +bool layoutU2FDialog(bool request, const char* title, const char* body, ...) { char strbuf[BODY_CHAR_MAX]; va_list vl; va_start(vl, body); - vsnprintf(strbuf, BODY_CHAR_MAX, body, vl); + int written = vsnprintf(strbuf, BODY_CHAR_MAX, body, vl); va_end(vl); + // Detect both SOURCE truncation (the formatted body did not fit strbuf) + // and RENDER truncation (the body fit strbuf but not the OLED canvas), the + // same two checks confirm_helper() runs for every other confirmation + // screen. This dialog draws unconditionally either way -- callers that + // display attacker-controlled, unbounded-length text (e.g. a CTAP2 rp_id) + // must check the return value and refuse rather than proceed on an + // approval the user could not fully read. + bool fits = written >= 0 && (size_t)written < BODY_CHAR_MAX && + confirm_body_fits(strbuf, BODY_WIDTH); + layout_standard_notification(title, strbuf, request ? NOTIFICATION_REQUEST_NO_ANIMATION : NOTIFICATION_CONFIRM_ANIMATION); @@ -896,4 +907,6 @@ void layoutU2FDialog(bool request, const char* title, const char* body, ...) { animate(); display_refresh(); } + + return fits; } diff --git a/lib/firmware/fsm_msg_crypto.h b/lib/firmware/fsm_msg_crypto.h index 309ac487a..fc24cf01c 100644 --- a/lib/firmware/fsm_msg_crypto.h +++ b/lib/firmware/fsm_msg_crypto.h @@ -44,13 +44,16 @@ void fsm_msgCipherKeyValue(CipherKeyValue* msg) { aes_cbc_encrypt(msg->value.bytes, resp->value.bytes, msg->value.size, ((msg->iv.size == 16) ? (msg->iv.bytes) : (data + 32)), &ctx); + memzero(&ctx, sizeof(ctx)); } else { aes_decrypt_ctx ctx; aes_decrypt_key256(data, &ctx); aes_cbc_decrypt(msg->value.bytes, resp->value.bytes, msg->value.size, ((msg->iv.size == 16) ? (msg->iv.bytes) : (data + 32)), &ctx); + memzero(&ctx, sizeof(ctx)); } + memzero(data, sizeof(data)); resp->has_value = true; resp->value.size = msg->value.size; diff --git a/lib/firmware/recovery_cipher.c b/lib/firmware/recovery_cipher.c index 8270caf97..0089aee35 100644 --- a/lib/firmware/recovery_cipher.c +++ b/lib/firmware/recovery_cipher.c @@ -60,6 +60,16 @@ static CONFIDENTIAL char cipher[ENGLISH_ALPHABET_BUF]; static CONFIDENTIAL char coded_word[12]; static CONFIDENTIAL char decoded_word[12]; static CONFIDENTIAL char last_completed_word[12]; +/* Raw cipher bytes for the whole mnemonic entered so far, mirroring + * `mnemonic` byte-for-byte (same appends, same truncations, always the same + * length) but holding the literal characters the host sent instead of their + * decoded plaintext. cipher rotates after every character, so re-deriving a + * word's coded form from its decoded form via the CURRENT cipher does not + * recover what was actually sent for any earlier position -- only bytes + * preserved at the time they were typed do. Backspacing, of any depth across + * any number of completed words, is then just "truncate like mnemonic and + * re-derive the current word" -- see get_current_coded_word(). */ +static CONFIDENTIAL char coded_mnemonic[MNEMONIC_BUF]; #if DEBUG_LINK static char auto_completed_word[CURRENT_WORD_BUF]; @@ -67,6 +77,7 @@ static char auto_completed_word[CURRENT_WORD_BUF]; static uint32_t get_current_word_pos(void); static void get_current_word(char* current_word); +static void get_current_coded_word(char* current_coded_word); void recovery_cipher_reset(void) { awaiting_character = false; @@ -75,6 +86,7 @@ void recovery_cipher_reset(void) { words_entered = 0; word_count = 0; memzero(mnemonic, sizeof(mnemonic)); + memzero(coded_mnemonic, sizeof(coded_mnemonic)); memzero(cipher, sizeof(cipher)); memzero(coded_word, sizeof(coded_word)); memzero(decoded_word, sizeof(decoded_word)); @@ -149,6 +161,21 @@ static void get_current_word(char* current_word) { } } +/// \returns the current word's raw typed cipher bytes by parsing +/// coded_mnemonic thus far -- mirrors get_current_word() exactly, but reads +/// the coded form instead of the decoded plaintext. +/// \param current_coded_word[out] Array to populate; sized like coded_word. +static void get_current_coded_word(char* current_coded_word) { + char* pos = strrchr(coded_mnemonic, ' '); + + if (pos) { + pos++; + strlcpy(current_coded_word, pos, sizeof(coded_word)); + } else { + strlcpy(current_coded_word, coded_mnemonic, sizeof(coded_word)); + } +} + _Static_assert(BIP39_WORDLIST_PADDED, "bip39 wordlist must be padded to 9 characters"); @@ -514,7 +541,10 @@ void recovery_character(const char* character) { memzero(decoded_word, sizeof(decoded_word)); if (word_count && words_entered == word_count) { + // Keep coded_mnemonic's length-per-mnemonic invariant even on this + // early-return path -- it skips the shared append below. strlcat(mnemonic, " ", MNEMONIC_BUF); + strlcat(coded_mnemonic, character, MNEMONIC_BUF); recovery_cipher_finalize(); return; } @@ -530,8 +560,9 @@ void recovery_character(const char* character) { } } - // concat to mnemonic + // concat to mnemonic, and to its raw-cipher-bytes mirror in lockstep strlcat(mnemonic, decoded_character, MNEMONIC_BUF); + strlcat(coded_mnemonic, character, MNEMONIC_BUF); next_character(); } @@ -558,23 +589,29 @@ void recovery_delete_character(void) { if (mnemonic[len - 1] == ' ') words_entered--; mnemonic[len - 1] = '\0'; + // coded_mnemonic is always exactly as long as mnemonic -- every append + // to one is paired with an append to the other in recovery_character(), + // and CharacterAck.character is nanopb-bounded to one byte -- so the + // same truncation applies to both, regardless of how many characters or + // word boundaries are being backed up over. + coded_mnemonic[len - 1] = '\0'; } /* Resync the current-word accumulators with the edited mnemonic so a * corrected word is validated on its real value (stale bytes here would * fail validation and trigger a storage_reset on a real recovery). - * decoded_word is the typed prefix of the current word; coded_word is its - * reverse-cipher form (session cipher is fixed, so it is reconstructable). */ + * decoded_word is plaintext, always safe to rebuild from mnemonic. + * coded_word must hold the literal bytes the host actually typed -- cipher + * rotates every character, so re-deriving it from decoded_word via the + * CURRENT cipher does not recover what was sent for any earlier position. + * Re-deriving it from coded_mnemonic the same way decoded_word is + * re-derived from mnemonic does, at any backspace depth or word count. */ char cur[CURRENT_WORD_BUF]; get_current_word(cur); strlcpy(decoded_word, cur, sizeof(decoded_word)); memzero(cur, sizeof(cur)); - size_t wlen = strlen(decoded_word); - for (size_t i = 0; i < wlen && i + 1 < sizeof(coded_word); i++) { - char d = decoded_word[i]; - coded_word[i] = (d >= 'a' && d <= 'z') ? cipher[d - 'a'] : d; - } - coded_word[wlen < sizeof(coded_word) ? wlen : sizeof(coded_word) - 1] = '\0'; + + get_current_coded_word(coded_word); next_character(); } @@ -722,4 +759,37 @@ const char* recovery_get_cipher(void) { return cipher; } const char* recovery_get_auto_completed_word(void) { return auto_completed_word; } + +/* + * recovery_get_decoded_mnemonic() - Gets the plaintext mnemonic decoded so + * far. Test-only: production code never exposes this outside the CONFIDENTIAL + * section. + */ +const char* recovery_get_decoded_mnemonic(void) { return mnemonic; } + +/* + * recovery_get_coded_mnemonic() - Gets the raw cipher bytes typed so far. + * Test-only: lets a test assert directly that backspacing (at any depth, + * across any number of word boundaries) leaves coded_mnemonic holding + * exactly the bytes the host actually sent, not a stale/reconstructed + * mixture. See #584. + */ +const char* recovery_get_coded_mnemonic(void) { return coded_mnemonic; } + +/// Test-only: arms a recovery ceremony and generates the first cipher, +/// bypassing recovery_cipher_init()'s confirm()/PIN gates so +/// recovery_character()/recovery_delete_character() can be driven directly +/// from a unit test. Mirrors the tail of recovery_cipher_init() exactly. +void recovery_debugLinkStart(uint32_t _word_count) { + setup_stage(/*passphrase_protection=*/false, "english", "test", + /*auto_lock_delay_ms=*/0, /*u2f_counter=*/0, /*no_backup=*/false); + word_count = _word_count; + enforce_wordlist = true; + dry_run = true; + memset(mnemonic, 0, sizeof(mnemonic)); + awaiting_character = true; + words_entered = 1; + setup_arm(SETUP_RECOVERY); + next_character(); +} #endif diff --git a/lib/firmware/storage.c b/lib/firmware/storage.c index 0fe9d0ac4..9c7905d57 100644 --- a/lib/firmware/storage.c +++ b/lib/firmware/storage.c @@ -622,6 +622,7 @@ void storage_secMigrate(SessionState* ss, Storage* storage, bool encrypt) { aes_cbc_encrypt((const uint8_t*)scratch, storage->encrypted_sec, sizeof(scratch), iv + 32, &ctx); memzero(&ctx, sizeof(ctx)); + memzero(iv, sizeof(iv)); storage->encrypted_sec_version = STORAGE_VERSION; } else { memzero(&storage->sec, sizeof(storage->sec)); @@ -640,6 +641,7 @@ void storage_secMigrate(SessionState* ss, Storage* storage, bool encrypt) { (uint8_t*)&scratch[0], sizeof(scratch), iv + 32, &ctx); } memzero(iv, sizeof(iv)); + memzero(&ctx, sizeof(ctx)); // De-serialize from scratch. storage_readHDNode(&storage->sec.node, &scratch[0], 129); @@ -721,6 +723,7 @@ static void storage_cipherBlock(bool encrypt, const uint8_t* key, aes_cbc_encrypt((const uint8_t*)plaintextBlock, ciphertextBlock, blockSize, iv + 32, &ctx); memzero(&ctx, sizeof(ctx)); + memzero(iv, sizeof(iv)); } else { // decrypt memcpy(iv, key, sizeof(iv)); @@ -729,6 +732,7 @@ static void storage_cipherBlock(bool encrypt, const uint8_t* key, aes_cbc_decrypt((const uint8_t*)ciphertextBlock, plaintextBlock, blockSize, iv + 32, &ctx); memzero(iv, sizeof(iv)); + memzero(&ctx, sizeof(ctx)); } return; @@ -811,6 +815,8 @@ bool storage_getAuthData(authType* returnData) { sizeof(shadow_config.storage.sec.authBlock)); } else { // encrypted, passphrase not available + memzero(authdataKey, sizeof(authdataKey)); + memzero((void*)&plaintextAuthBlock, sizeof(plaintextAuthBlock)); return false; } } else { @@ -825,11 +831,15 @@ bool storage_getAuthData(authType* returnData) { // authdata if (0 != memcmp(shadow_config.storage.pub.authdata_fingerprint, testFp, sizeof(shadow_config.storage.pub.authdata_fingerprint))) { + memzero(authdataKey, sizeof(authdataKey)); + memzero((void*)&plaintextAuthBlock, sizeof(plaintextAuthBlock)); return false; } memcpy(returnData, plaintextAuthBlock.authData, sizeof(plaintextAuthBlock.authData)); + memzero(authdataKey, sizeof(authdataKey)); + memzero((void*)&plaintextAuthBlock, sizeof(plaintextAuthBlock)); return true; } @@ -856,6 +866,7 @@ void storage_setAuthData(const authType* setData) { (uint8_t*)&shadow_config.storage.sec.authBlock, sizeof(shadow_config.storage.sec.authBlock)); shadow_config.storage.pub.authdata_encrypted = true; + memzero(authdataKey, sizeof(authdataKey)); } else { // not encrypted memcpy((void*)&shadow_config.storage.sec.authBlock, diff --git a/lib/firmware/transaction.c b/lib/firmware/transaction.c index 60e3bde8b..17ce07c32 100644 --- a/lib/firmware/transaction.c +++ b/lib/firmware/transaction.c @@ -546,7 +546,13 @@ int compile_output(const CoinType* coin, const HDNode* root, TxOutputType* in, case OutputScriptType_PAYTOP2SHWITNESS: case OutputScriptType_PAYTOTAPROOT: { char amount_str[32]; - char node_str[NODE_STRING_LENGTH]; + // ADDR_STR_LEN, not NODE_STRING_LENGTH: this buffer is passed to + // txin_dgst_compare()/txin_dgst_save_and_reset(), which unconditionally + // memcpy/strncmp ADDR_STR_LEN (130) bytes -- their real contract, per + // the other call site below which passes a 130-byte protobuf address + // field. A 50-byte NODE_STRING_LENGTH buffer here was an 80-byte OOB + // stack read. + char node_str[ADDR_STR_LEN]; coin_amnt_to_str(coin, in->amount, amount_str, sizeof(amount_str)); memset(node_str, 0, sizeof(node_str)); if (!bip32_node_to_string(node_str, sizeof(node_str), coin, diff --git a/lib/firmware/u2f.c b/lib/firmware/u2f.c index 413945e1a..33f2d7235 100644 --- a/lib/firmware/u2f.c +++ b/lib/firmware/u2f.c @@ -649,9 +649,16 @@ bool u2f_load_credential(const uint8_t app_id[32], const uint8_t key_handle[64], } bool ctap2_request_user_presence(const char* rp_id, bool registration) { - layoutU2FDialog(true, registration ? "Create Passkey" : "Use Passkey", - registration ? "Create a passkey for %s?" : "Sign in to %s?", - rp_id); + bool fits = layoutU2FDialog( + true, registration ? "Create Passkey" : "Use Passkey", + registration ? "Create a passkey for %s?" : "Sign in to %s?", rp_id); + if (!fits) { + // rp_id is host-controlled and can run up to 253 chars; the credential + // is bound to the FULL string (see ctap2's sha256_Raw() over rp_id), so + // an rp_id the user can't read in full must not be silently approved. + layoutHome(); + return false; + } bool saw_button_up = false; for (uint32_t remaining = 10 * U2F_TIMEOUT; remaining > 0; --remaining) { if (reader != NULL && reader->cmd == U2FHID_CANCEL) { diff --git a/unittests/firmware/recovery.cpp b/unittests/firmware/recovery.cpp index 654891c28..d3c0d8936 100644 --- a/unittests/firmware/recovery.cpp +++ b/unittests/firmware/recovery.cpp @@ -1,11 +1,14 @@ extern "C" { #include "keepkey/firmware/recovery_cipher.h" +#include "keepkey/firmware/reset.h" +#include "keepkey/firmware/fsm.h" #include "trezor/crypto/bip39_english.h" } #include "gtest/gtest.h" #include +#include TEST(Recovery, ExactStrMatch) { char LHS[] = "allow\0"; @@ -42,3 +45,282 @@ TEST(Recovery, WordlistLengths) { } } } + +// Regression coverage for #584: the substitution cipher rotates after every +// character, so recovery_delete_character() must reconstruct coded_word (the +// literal raw bytes the host sent) from history actually preserved at typing +// time, not by re-deriving it through whatever cipher happens to be active +// at delete time. These tests drive recovery_character()/ +// recovery_delete_character() directly, via the DEBUG_LINK-only +// recovery_debugLinkStart() hook that arms a ceremony without going through +// recovery_cipher_init()'s confirm()/PIN gate (which blocks on a real button +// press and cannot run headless here). +// unittests/firmware/test_board.cpp -- single guarded board bootstrap for +// the whole binary; see that file for why this must never be called +// directly outside it. +void kk_test_board_init(void); + +namespace { + +class RecoveryCipher : public ::testing::Test { + protected: + void SetUp() override { + kk_test_board_init(); // canvas for next_character()'s layout_cipher() draw + static bool fsm_ready = false; + if (!fsm_ready) { + fsm_init(); + fsm_ready = true; + } + setup_abort(); + } + + void TearDown() override { setup_abort(); } +}; + +// Looks up the raw cipher byte that currently decodes to `plain`. +char CipherCharFor(char plain) { + return recovery_get_cipher()[plain - 'a']; +} + +// Types `word` (at most 4 chars -- recovery_character() rejects a longer +// in-progress word) through the ACTIVE cipher, one character at a time, so +// each byte sent is the raw cipher-encoded form of a real plaintext letter. +void TypeViaCipher(const char *word) { + for (const char *p = word; *p; ++p) { + char buf[2] = {CipherCharFor(*p), '\0'}; + recovery_character(buf); + } +} + +// Types `word` verbatim, unencoded -- simulating a host bypassing the +// substitution cipher and sending real letters directly. +void TypeRaw(const char *word) { + for (const char *p = word; *p; ++p) { + char buf[2] = {*p, '\0'}; + recovery_character(buf); + } +} + +void TypeSpace() { recovery_character(" "); } + +void Backspace(int n) { + for (int i = 0; i < n; ++i) recovery_delete_character(); +} + +} // namespace + +TEST_F(RecoveryCipher, BackspaceWithinWord) { + recovery_debugLinkStart(/*word_count=*/0); + ASSERT_TRUE(setup_isArmedAs(SETUP_RECOVERY)); + + TypeViaCipher("aban"); + EXPECT_STREQ(recovery_get_decoded_mnemonic(), "aban"); + + Backspace(1); + EXPECT_STREQ(recovery_get_decoded_mnemonic(), "aba"); + EXPECT_EQ(strlen(recovery_get_coded_mnemonic()), 3u); + + TypeViaCipher("n"); + EXPECT_STREQ(recovery_get_decoded_mnemonic(), "aban"); + EXPECT_TRUE(setup_isArmedAs(SETUP_RECOVERY)); +} + +TEST_F(RecoveryCipher, BackspaceAcrossOneWordBoundaryRestoresRawBytes) { + recovery_debugLinkStart(/*word_count=*/0); + ASSERT_TRUE(setup_isArmedAs(SETUP_RECOVERY)); + + std::string word1_raw; + for (const char *p = "aban"; *p; ++p) { + char c = CipherCharFor(*p); + word1_raw += c; + char buf[2] = {c, '\0'}; + recovery_character(buf); + } + TypeSpace(); + ASSERT_TRUE(setup_isArmedAs(SETUP_RECOVERY)); + ASSERT_STREQ(recovery_get_decoded_mnemonic(), "aban "); + + Backspace(1); // delete the trailing space + + EXPECT_STREQ(recovery_get_decoded_mnemonic(), "aban"); + EXPECT_STREQ(recovery_get_coded_mnemonic(), word1_raw.c_str()); +} + +// The exact repro from #584: complete two words, then back up across BOTH +// of them (deleting word2 entirely and the space in front of it), landing +// back in the middle of editing word1. coded_mnemonic must hold word1's own +// raw bytes, not word2's -- which is precisely what the single-slot +// last_completed_coded_word fix in PR #582 got wrong. +TEST_F(RecoveryCipher, BackspaceAcrossTwoWordBoundariesRestoresRawBytes) { + recovery_debugLinkStart(/*word_count=*/0); + ASSERT_TRUE(setup_isArmedAs(SETUP_RECOVERY)); + + std::string word1_raw; + for (const char *p = "aban"; *p; ++p) { + char c = CipherCharFor(*p); + word1_raw += c; + char buf[2] = {c, '\0'}; + recovery_character(buf); + } + TypeSpace(); + ASSERT_TRUE(setup_isArmedAs(SETUP_RECOVERY)); + + TypeViaCipher("abil"); // "ability" + TypeSpace(); + ASSERT_TRUE(setup_isArmedAs(SETUP_RECOVERY)); + ASSERT_STREQ(recovery_get_decoded_mnemonic(), "aban abil "); + + // trailing space, all 4 letters of word2, and the space before it: 6 deletes. + Backspace(6); + + EXPECT_STREQ(recovery_get_decoded_mnemonic(), "aban") + << "plaintext should be back to word1 alone"; + EXPECT_STREQ(recovery_get_coded_mnemonic(), word1_raw.c_str()) + << "raw coded history must be word1's OWN bytes, not carried over " + "from word2 (#584)"; + EXPECT_TRUE(setup_isArmedAs(SETUP_RECOVERY)); +} + +// Generalizes the above to 24 words and 23 boundaries. BIP39's wordlist +// guarantees every word's first four letters are a globally unique prefix, +// so wordlist[i]'s first four characters are always a valid, unambiguous +// word to type here. +TEST_F(RecoveryCipher, BackspaceAcross23WordBoundariesRestoresRawBytes) { + recovery_debugLinkStart(/*word_count=*/0); + ASSERT_TRUE(setup_isArmedAs(SETUP_RECOVERY)); + + std::string word1_raw; + for (const char *p = wordlist[0]; p < wordlist[0] + 4; ++p) { + char c = CipherCharFor(*p); + word1_raw += c; + char buf[2] = {c, '\0'}; + recovery_character(buf); + } + TypeSpace(); + ASSERT_TRUE(setup_isArmedAs(SETUP_RECOVERY)); + + // Complete words at indices 1..22 (22 more words: 23 completed words and + // 23 boundaries total, counting word0). recovery_cipher.c enforces a + // 24-word ceremony maximum (words_entered > 24 aborts), so the 24th word + // (index 23) is typed but deliberately left un-space-completed below -- + // completing it would step words_entered to 25, one past the legitimate + // maximum, for a reason unrelated to what this test is checking. + int deletes = 1; // the space just typed after word1 + for (int w = 1; w < 23; w++) { + char prefix[5] = {0}; + memcpy(prefix, wordlist[w], 4); + TypeViaCipher(prefix); + TypeSpace(); + ASSERT_TRUE(setup_isArmedAs(SETUP_RECOVERY)) << "word index " << w; + // Not every BIP39 word has 4+ letters (e.g. "act"), so count what was + // actually typed rather than assuming 4 letters + a space every time. + deletes += static_cast(strlen(prefix)) + 1; + } + + char last_prefix[5] = {0}; + memcpy(last_prefix, wordlist[23], 4); + TypeViaCipher(last_prefix); + ASSERT_TRUE(setup_isArmedAs(SETUP_RECOVERY)); + deletes += static_cast(strlen(last_prefix)); + + Backspace(deletes); + + EXPECT_STREQ(recovery_get_decoded_mnemonic(), std::string(wordlist[0], 4).c_str()); + EXPECT_STREQ(recovery_get_coded_mnemonic(), word1_raw.c_str()) + << "raw coded history must survive backing up over 23 completed words"; + EXPECT_TRUE(setup_isArmedAs(SETUP_RECOVERY)); +} + +// Interleave typing, deleting, and retyping a correction, checking after +// every step that the plaintext mnemonic, the raw coded history, and the +// word count implied by them stay mutually consistent. +TEST_F(RecoveryCipher, RepeatedDeleteRetypeStaysAligned) { + recovery_debugLinkStart(/*word_count=*/0); + ASSERT_TRUE(setup_isArmedAs(SETUP_RECOVERY)); + + TypeViaCipher("aban"); + TypeSpace(); + ASSERT_TRUE(setup_isArmedAs(SETUP_RECOVERY)); + ASSERT_STREQ(recovery_get_decoded_mnemonic(), "aban "); + + TypeViaCipher("abou"); // start typing a wrong word ("about") + ASSERT_STREQ(recovery_get_decoded_mnemonic(), "aban abou"); + + Backspace(4); // realize the mistake, delete all four letters + ASSERT_STREQ(recovery_get_decoded_mnemonic(), "aban "); + ASSERT_EQ(strlen(recovery_get_coded_mnemonic()), + strlen(recovery_get_decoded_mnemonic())); + + TypeViaCipher("abov"); // correct to "above" instead + ASSERT_TRUE(setup_isArmedAs(SETUP_RECOVERY)); + EXPECT_STREQ(recovery_get_decoded_mnemonic(), "aban abov"); + EXPECT_EQ(strlen(recovery_get_coded_mnemonic()), + strlen(recovery_get_decoded_mnemonic())); + + TypeSpace(); + EXPECT_TRUE(setup_isArmedAs(SETUP_RECOVERY)); + EXPECT_STREQ(recovery_get_decoded_mnemonic(), "aban abov "); +} + +// After a multi-boundary backspace, a host sending a real BIP39 prefix +// directly (bypassing the cipher entirely) must still be caught -- proving +// coded_word/coded_mnemonic aren't left holding stale bytes from an earlier, +// already-backed-out-of word that would mask the raw prefix. +TEST_F(RecoveryCipher, RawPrefixAfterMultiBoundaryBackspaceStillCaught) { + recovery_debugLinkStart(/*word_count=*/0); + ASSERT_TRUE(setup_isArmedAs(SETUP_RECOVERY)); + + TypeViaCipher("aban"); + TypeSpace(); + TypeViaCipher("abil"); + TypeSpace(); + ASSERT_TRUE(setup_isArmedAs(SETUP_RECOVERY)); + + Backspace(6); // back into the middle of word1, same as the boundary test above + ASSERT_TRUE(setup_isArmedAs(SETUP_RECOVERY)); + ASSERT_STREQ(recovery_get_decoded_mnemonic(), "aban"); + + Backspace(4); // and all the way out, so the next word starts clean + ASSERT_TRUE(setup_isArmedAs(SETUP_RECOVERY)); + ASSERT_STREQ(recovery_get_decoded_mnemonic(), ""); + ASSERT_STREQ(recovery_get_coded_mnemonic(), ""); + + // Try a few fixed real-word prefixes raw and take whichever one's + // cipher-decoded form ISN'T itself coincidentally a valid prefix too (the + // production code's own comment documents a ~0.4% coincidence rate for + // any single one -- trying several fixed candidates makes the test + // deterministic regardless of this ceremony's randomly generated cipher). + static const char *kCandidates[] = {"aban", "abil", "able", "abou", "abov"}; + bool caught = false; + for (const char *candidate : kCandidates) { + char decoded_probe[8] = {0}; + for (size_t i = 0; i < strlen(candidate); i++) { + const char *pos = strchr(recovery_get_cipher(), candidate[i]); + ASSERT_NE(pos, nullptr); + decoded_probe[i] = "abcdefghijklmnopqrstuvwxyz"[pos - recovery_get_cipher()]; + } + if (attempt_auto_complete(decoded_probe)) { + continue; // this candidate's raw form also happens to decode validly + } + + TypeRaw(candidate); + if (!setup_isArmedAs(SETUP_RECOVERY)) { + caught = true; // mid-word uncyphered-count check aborted it + break; + } + TypeSpace(); + if (!setup_isArmedAs(SETUP_RECOVERY)) { + caught = true; // space-completion wordlist validation aborted it + break; + } + // Accepted outright: re-arm and try the next candidate. setup_abort() + // first -- setup_stage() (inside recovery_debugLinkStart()) refuses to + // restage over an already-armed ceremony. + setup_abort(); + recovery_debugLinkStart(/*word_count=*/0); + } + + EXPECT_TRUE(caught) << "a raw, unenciphered real-word prefix must be " + "rejected, not silently accepted as if the cipher " + "had been used"; +} diff --git a/unittests/firmware/ripple.cpp b/unittests/firmware/ripple.cpp index 155d057aa..d622aaf10 100644 --- a/unittests/firmware/ripple.cpp +++ b/unittests/firmware/ripple.cpp @@ -128,3 +128,44 @@ TEST(Ripple, Serialize) { ASSERT_TRUE(memcmp(serialized, expected, sizeof(serialized)) == 0); } + +/* #553 boundary regression, requested by independent review before PR #557 + * merged (added afterward in round 4's remediation, per #583). An earlier + * fix attempt bounded payment.amount at 1e11 (XRP-sized), which would have + * rejected any legitimate payment over 100,000 XRP; the corrected bound is + * 1e17 DROPS -- XRPL's real protocol maximum of 100,000,000,000 XRP. This + * checks the serializer half of that fix: ripple_serializeAmount()'s own + * assert() uses the same 1e17 ceiling as fsm_msg_ripple.h's runtime check + * (see #557), so the true maximum must serialize without tripping it. */ +TEST(Ripple, SerializerAcceptsMaximumProtocolAmount) { + RippleSignTx tx; + memset(&tx, 0, sizeof(tx)); + + tx.address_n_count = 0; + tx.has_fee = true; + tx.fee = 100000; + tx.has_flags = true; + tx.flags = 0x80000000; + tx.has_sequence = true; + tx.sequence = 1; + + tx.has_payment = true; + tx.payment.has_amount = true; + tx.payment.amount = 100000000000000000ULL; // 1e17 drops = 100B XRP + tx.payment.has_destination = true; + strcpy(tx.payment.destination, "rBKz5MC2iXdoS3XgnNSYmF69K1Yo4NS3Ws"); + + uint8_t serialized[200]; + memset(serialized, 0, sizeof(serialized)); + + const uint8_t *public_key = (const uint8_t*) + "\x02\x13\x1f\xac\xd1\xea\xb7\x48\xd6\xcd\xdc\x49\x2f\x54\xb0\x4e" + "\x8c\x35\x65\x88\x94\xf4\xad\xd2\x23\x2e\xbc\x5a\xfe\x75\x21\xdb\xe4"; + + uint8_t *buf = serialized; + EXPECT_TRUE(ripple_serialize(&buf, buf + sizeof(serialized), &tx, + "rNaqKtKrMSwpwZSzRckPf7S96DkimjkF4H", public_key, + nullptr, 0)) + << "the protocol maximum must serialize, not trip " + "ripple_serializeAmount()'s bound assert"; +} diff --git a/unittests/firmware/solana.cpp b/unittests/firmware/solana.cpp index 397d2edef..0086933e3 100644 --- a/unittests/firmware/solana.cpp +++ b/unittests/firmware/solana.cpp @@ -589,6 +589,54 @@ TEST(Solana, ParseTxTooShort) { EXPECT_FALSE(solana_parseTx(raw, sizeof(raw), &tx)); } +/* #550 boundary regressions, requested by independent review before PR #557 + * merged (and only added afterward, in round 4's remediation, per #583). + * tx->accounts[] has exactly SOL_MAX_ACCOUNTS(32) entries; num_accounts must + * be rejected -- MALFORMED, fully closed -- before it's ever stored or used + * as a loop bound, whether it's a small overage (33..255, which would read + * past accounts[31] as a uint16_t loop bound) or a value that silently wraps + * to a small/zero uint8_t (256, 512, ...), which would have reintroduced the + * original signer-check bypass this fix closed. */ +TEST(Solana, RejectsThirtyThreeAccounts) { + uint8_t raw[4]; + size_t pos = 0; + raw[pos++] = 1; /* num_required_sigs */ + raw[pos++] = 0; /* num_readonly_signed */ + raw[pos++] = 1; /* num_readonly_unsigned */ + raw[pos++] = 33; /* compact-u16 num_accounts: 33 > SOL_MAX_ACCOUNTS(32) */ + + SolanaParsedTx tx; + EXPECT_EQ(solana_inspectTx(raw, pos, &tx), SOL_TX_REVIEW_MALFORMED); +} + +TEST(Solana, RejectsAccountCountWrapAt256) { + uint8_t raw[5]; + size_t pos = 0; + raw[pos++] = 1; + raw[pos++] = 0; + raw[pos++] = 1; + /* compact-u16 for 256: byte0 = (256 & 0x7F) | 0x80, byte1 = 256 >> 7 */ + raw[pos++] = 0x80; + raw[pos++] = 0x02; + + SolanaParsedTx tx; + EXPECT_EQ(solana_inspectTx(raw, pos, &tx), SOL_TX_REVIEW_MALFORMED); +} + +TEST(Solana, RejectsAccountCountWrapAt512) { + uint8_t raw[5]; + size_t pos = 0; + raw[pos++] = 1; + raw[pos++] = 0; + raw[pos++] = 1; + /* compact-u16 for 512: byte0 = (512 & 0x7F) | 0x80, byte1 = 512 >> 7 */ + raw[pos++] = 0x80; + raw[pos++] = 0x04; + + SolanaParsedTx tx; + EXPECT_EQ(solana_inspectTx(raw, pos, &tx), SOL_TX_REVIEW_MALFORMED); +} + TEST(Solana, RejectsTrailingBytes) { /* Build a valid 1-instruction system transfer, then append extra bytes */ uint8_t raw[256];