From 4aea5220c5065a5dd86d9fef850cc8d43d7f4799 Mon Sep 17 00:00:00 2001 From: Marko Bencun Date: Sat, 24 Dec 2022 13:15:47 +0100 Subject: [PATCH 1/4] keystore: split retaining/deleting seeds into functions Intermediate commit for clarity. These two functions can then be changed to not keep the seed in RAM in plantext, but to encrypt them. --- src/keystore.c | 59 ++++++++++++++++++++++++++++++++------------------ src/keystore.h | 14 ++++++------ 2 files changed, 45 insertions(+), 28 deletions(-) diff --git a/src/keystore.c b/src/keystore.c index 1260a8b3f..339f22fca 100644 --- a/src/keystore.c +++ b/src/keystore.c @@ -47,21 +47,6 @@ static bool _is_unlocked_bip39 = false; // Must be defined if _is_unlocked is true. ONLY ACCESS THIS WITH _copy_bip39_seed(). static uint8_t _retained_bip39_seed[64] = {0}; -#ifdef TESTING -void keystore_mock_unlocked(const uint8_t* seed, size_t seed_len, const uint8_t* bip39_seed) -{ - _is_unlocked_device = seed != NULL; - if (seed != NULL) { - _seed_length = seed_len; - memcpy(_retained_seed, seed, seed_len); - } - _is_unlocked_bip39 = bip39_seed != NULL; - if (bip39_seed != NULL) { - memcpy(_retained_bip39_seed, bip39_seed, sizeof(_retained_bip39_seed)); - } -} -#endif - /** * We allow seeds of 16, 24 or 32 bytes. */ @@ -319,6 +304,25 @@ static void _free_string(char** str) wally_free_string(*str); } +static void _retain_seed(const uint8_t* seed, size_t seed_len) +{ + memcpy(_retained_seed, seed, seed_len); + _seed_length = seed_len; +} + +USE_RESULT static bool _retain_bip39_seed(const uint8_t* bip39_seed) +{ + memcpy(_retained_bip39_seed, bip39_seed, sizeof(_retained_bip39_seed)); + return true; +} + +static void _delete_retained_seeds(void) +{ + _seed_length = 0; + util_zero(_retained_seed, sizeof(_retained_seed)); + util_zero(_retained_bip39_seed, sizeof(_retained_bip39_seed)); +} + keystore_error_t keystore_unlock( const char* password, uint8_t* remaining_attempts_out, @@ -354,8 +358,7 @@ keystore_error_t keystore_unlock( Abort("Seed has suddenly changed. This should never happen."); } } else { - memcpy(_retained_seed, seed, seed_len); - _seed_length = seed_len; + _retain_seed(seed, seed_len); _is_unlocked_device = true; } bitbox02_smarteeprom_reset_unlock_attempts(); @@ -396,7 +399,9 @@ bool keystore_unlock_bip39(const char* mnemonic_passphrase) mnemonic, mnemonic_passphrase, bip39_seed, sizeof(bip39_seed), NULL) != WALLY_OK) { return false; } - memcpy(_retained_bip39_seed, bip39_seed, sizeof(bip39_seed)); + if (!_retain_bip39_seed(bip39_seed)) { + return false; + } _is_unlocked_bip39 = true; return true; } @@ -405,9 +410,7 @@ void keystore_lock(void) { _is_unlocked_device = false; _is_unlocked_bip39 = false; - _seed_length = 0; - util_zero(_retained_seed, sizeof(_retained_seed)); - util_zero(_retained_bip39_seed, sizeof(_retained_bip39_seed)); + _delete_retained_seeds(); } bool keystore_is_locked(void) @@ -789,3 +792,17 @@ bool keystore_secp256k1_schnorr_bip86_sign( } return secp256k1_schnorrsig_verify(ctx, sig64_out, msg32, 32, &pubkey) == 1; } + +#ifdef TESTING +void keystore_mock_unlocked(const uint8_t* seed, size_t seed_len, const uint8_t* bip39_seed) +{ + _is_unlocked_device = seed != NULL; + if (seed != NULL) { + _retain_seed(seed, seed_len); + } + _is_unlocked_bip39 = bip39_seed != NULL; + if (bip39_seed != NULL) { + memcpy(_retained_bip39_seed, bip39_seed, sizeof(_retained_bip39_seed)); + } +} +#endif diff --git a/src/keystore.h b/src/keystore.h index 3dc4f82c9..7b036bb41 100644 --- a/src/keystore.h +++ b/src/keystore.h @@ -45,13 +45,6 @@ typedef enum { KEYSTORE_ERR_ENCRYPT, } keystore_error_t; -#ifdef TESTING -/** - * convenience to mock the keystore state (locked, seed) in tests. - */ -void keystore_mock_unlocked(const uint8_t* seed, size_t seed_len, const uint8_t* bip39_seed); -#endif - /** * Copies the retained seed into the given buffer. The caller must * zero the seed with util_zero once it is no longer needed. @@ -284,4 +277,11 @@ USE_RESULT bool keystore_secp256k1_schnorr_bip86_sign( const uint8_t* msg32, uint8_t* sig64_out); +#ifdef TESTING +/** + * convenience to mock the keystore state (locked, seed) in tests. + */ +void keystore_mock_unlocked(const uint8_t* seed, size_t seed_len, const uint8_t* bip39_seed); +#endif + #endif From be929a5d3e06405a571a04b871ca2ffaa4b805af Mon Sep 17 00:00:00 2001 From: Marko Bencun Date: Sun, 25 Dec 2022 21:35:04 +0100 Subject: [PATCH 2/4] keystore: encrypt seed in RAM Until now, after keystore_unlock(), the seed was present in RAM in plaintext. We instead keep the seed in RAM in encrypted form. The enryption key is stretched using the secure chip, much like the device unlock password itself is stretched to encrypt the seed for storage in flash memory. The idea is to keep the decrypted seed in memory for the least amount of time possible, so: decrypt, use, discard. This reduces the attack surface and mitigates potential attacks where the RAM could be accessed by an attacker. The unstretched key in this case is not the device password, but a randomly generated key stored in RAM. We cannot use the device password as we want to be able to decrypt the seed on demand without prompting the user for the password. We use the KDF secure chip slot. It is not attached to the monotonic counter, so it can be used without limitations. One KDF key stretch (used in encryption and decryption) takes around 100ms. Alternatives discarded: - Use a data slot on the securechip to store the encryption key. Surprisingly, an encrypted read of this slot is slower than KDF, likely because an encrypted read, in addition to an auth call (consisting of NONCE an CHECKMAC commands), do a READ, NONCE, GENDIG and again READ, while KDF after auth only is one KDF call. Possibly the many roundtrips to the chip make it slower overall. - Use SHA instead of KDF: our secure chip configuration does not permit SHA operations. This commit performs encryption and on-demand decryption for the seed. The next commit will do the same for the bip39 seed. The changed backup.rs unit test is to move mock_unlocked() before mock_memory(), as mock_memory() overwrites the salt root, which is needed in the key stretching. The changed signtx.rs unit test is to move random::mock_reset() after mock_unlocked(), as the P2TR signature uses random_32_bytes(), which in unit tests is a deterministic PRNG, which was influenced by calling random_32_bytes() when creating the seed encryption key in mock_unlocked(). --- CHANGELOG.md | 1 + CMakeLists.txt | 4 +- src/keystore.c | 104 +++++++++++++++--- src/keystore.h | 3 + src/rust/bitbox02-rust/src/hww/api/backup.rs | 9 +- .../src/hww/api/bitcoin/signtx.rs | 3 +- test/unit-test/test_keystore.c | 71 +++++++++--- test/unit-test/test_keystore_functional.c | 1 + 8 files changed, 160 insertions(+), 36 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 628b3f263..d2f679645 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ customers cannot upgrade their bootloader, its changes are recorded separately. ## Firmware ### [Unreleased] +- Improved security: keep seed encrypted in RAM ### 9.14.0 - Improved touch button positional accuracy in noisy environments diff --git a/CMakeLists.txt b/CMakeLists.txt index 797ca70fb..4a54f8980 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -88,8 +88,8 @@ endif() # # Versions MUST contain three parts and start with lowercase 'v'. # Example 'v1.0.0'. They MUST not contain a pre-release label such as '-beta'. -set(FIRMWARE_VERSION "v9.14.0") -set(FIRMWARE_BTC_ONLY_VERSION "v9.14.0") +set(FIRMWARE_VERSION "v9.14.1") +set(FIRMWARE_BTC_ONLY_VERSION "v9.14.1") set(BOOTLOADER_VERSION "v1.0.5") find_package(PythonInterp 3.6 REQUIRED) diff --git a/src/keystore.c b/src/keystore.c index 339f22fca..88898b7f4 100644 --- a/src/keystore.c +++ b/src/keystore.c @@ -36,11 +36,10 @@ // Change this ONLY via keystore_unlock() or keystore_lock() static bool _is_unlocked_device = false; -// Must be defined if is_unlocked is true. Length of the seed store in `_retained_seed`. See also: -// `_validate_seed_length()`. -static size_t _seed_length = 0; +static uint8_t _unstretched_retained_seed_encryption_key[32] = {0}; // Must be defined if is_unlocked is true. ONLY ACCESS THIS WITH keystore_copy_seed(). -static uint8_t _retained_seed[KEYSTORE_MAX_SEED_LENGTH] = {0}; +static uint8_t _retained_seed_encrypted[KEYSTORE_MAX_SEED_LENGTH + 64] = {0}; +static size_t _retained_seed_encrypted_len = 0; // Change this ONLY via keystore_unlock_bip39(). static bool _is_unlocked_bip39 = false; @@ -55,13 +54,56 @@ static bool _validate_seed_length(size_t seed_len) return seed_len == 16 || seed_len == 24 || seed_len == 32; } +USE_RESULT static keystore_error_t _stretch_retained_seed_encryption_key( + const uint8_t* encryption_key, + const char* purpose_in, + const char* purpose_out, + uint8_t* out) +{ + uint8_t salted_hashed[32] = {0}; + UTIL_CLEANUP_32(salted_hashed); + if (!salt_hash_data(encryption_key, 32, purpose_in, salted_hashed)) { + return KEYSTORE_ERR_SALT; + } + if (securechip_kdf(SECURECHIP_SLOT_KDF, salted_hashed, 32, out)) { + return KEYSTORE_ERR_SECURECHIP; + } + if (!salt_hash_data(encryption_key, 32, purpose_out, salted_hashed)) { + return KEYSTORE_ERR_SALT; + } + if (wally_hmac_sha256(salted_hashed, sizeof(salted_hashed), out, 32, out, 32) != WALLY_OK) { + return KEYSTORE_ERR_HASH; + } + return KEYSTORE_OK; +} + bool keystore_copy_seed(uint8_t* seed_out, size_t* length_out) { if (!_is_unlocked_device) { return false; } - memcpy(seed_out, _retained_seed, _seed_length); - *length_out = _seed_length; + + uint8_t retained_seed_encryption_key[32] = {0}; + UTIL_CLEANUP_32(retained_seed_encryption_key); + if (_stretch_retained_seed_encryption_key( + _unstretched_retained_seed_encryption_key, + "keystore_retained_seed_access_in", + "keystore_retained_seed_access_out", + retained_seed_encryption_key) != KEYSTORE_OK) { + return false; + } + size_t len = _retained_seed_encrypted_len - 48; + bool password_correct = cipher_aes_hmac_decrypt( + _retained_seed_encrypted, + _retained_seed_encrypted_len, + seed_out, + &len, + retained_seed_encryption_key); + if (!password_correct) { + // Should never happen. + return false; + } + *length_out = len; return true; } @@ -304,10 +346,26 @@ static void _free_string(char** str) wally_free_string(*str); } -static void _retain_seed(const uint8_t* seed, size_t seed_len) +USE_RESULT static keystore_error_t _retain_seed(const uint8_t* seed, size_t seed_len) { - memcpy(_retained_seed, seed, seed_len); - _seed_length = seed_len; + random_32_bytes(_unstretched_retained_seed_encryption_key); + uint8_t retained_seed_encryption_key[32] = {0}; + UTIL_CLEANUP_32(retained_seed_encryption_key); + keystore_error_t result = _stretch_retained_seed_encryption_key( + _unstretched_retained_seed_encryption_key, + "keystore_retained_seed_access_in", + "keystore_retained_seed_access_out", + retained_seed_encryption_key); + if (result != KEYSTORE_OK) { + return result; + } + size_t len = seed_len + 64; + if (!cipher_aes_hmac_encrypt( + seed, seed_len, _retained_seed_encrypted, &len, retained_seed_encryption_key)) { + return KEYSTORE_ERR_ENCRYPT; + } + _retained_seed_encrypted_len = len; + return KEYSTORE_OK; } USE_RESULT static bool _retain_bip39_seed(const uint8_t* bip39_seed) @@ -318,8 +376,10 @@ USE_RESULT static bool _retain_bip39_seed(const uint8_t* bip39_seed) static void _delete_retained_seeds(void) { - _seed_length = 0; - util_zero(_retained_seed, sizeof(_retained_seed)); + util_zero( + _unstretched_retained_seed_encryption_key, + sizeof(_unstretched_retained_seed_encryption_key)); + util_zero(_retained_seed_encrypted, sizeof(_retained_seed_encrypted)); util_zero(_retained_bip39_seed, sizeof(_retained_bip39_seed)); } @@ -354,11 +414,19 @@ keystore_error_t keystore_unlock( if (result == KEYSTORE_OK) { if (_is_unlocked_device) { // Already unlocked. Fail if the seed changed under our feet (should never happen). - if (seed_len != _seed_length || !MEMEQ(_retained_seed, seed, _seed_length)) { + uint8_t current_seed[KEYSTORE_MAX_SEED_LENGTH] = {0}; + size_t current_seed_len = 0; + if (!keystore_copy_seed(current_seed, ¤t_seed_len)) { + return KEYSTORE_ERR_DECRYPT; + } + if (seed_len != current_seed_len || !MEMEQ(current_seed, seed, current_seed_len)) { Abort("Seed has suddenly changed. This should never happen."); } } else { - _retain_seed(seed, seed_len); + keystore_error_t retain_seed_result = _retain_seed(seed, seed_len); + if (retain_seed_result != KEYSTORE_OK) { + return retain_seed_result; + } _is_unlocked_device = true; } bitbox02_smarteeprom_reset_unlock_attempts(); @@ -798,11 +866,19 @@ void keystore_mock_unlocked(const uint8_t* seed, size_t seed_len, const uint8_t* { _is_unlocked_device = seed != NULL; if (seed != NULL) { - _retain_seed(seed, seed_len); + if (_retain_seed(seed, seed_len) != KEYSTORE_OK) { + Abort("couldn't retain seed"); + } } _is_unlocked_bip39 = bip39_seed != NULL; if (bip39_seed != NULL) { memcpy(_retained_bip39_seed, bip39_seed, sizeof(_retained_bip39_seed)); } } + +const uint8_t* keystore_test_get_retained_seed_encrypted(size_t* len_out) +{ + *len_out = _retained_seed_encrypted_len; + return _retained_seed_encrypted; +} #endif diff --git a/src/keystore.h b/src/keystore.h index 7b036bb41..fe3c36747 100644 --- a/src/keystore.h +++ b/src/keystore.h @@ -43,6 +43,7 @@ typedef enum { KEYSTORE_ERR_SALT, KEYSTORE_ERR_HASH, KEYSTORE_ERR_ENCRYPT, + KEYSTORE_ERR_DECRYPT, } keystore_error_t; /** @@ -282,6 +283,8 @@ USE_RESULT bool keystore_secp256k1_schnorr_bip86_sign( * convenience to mock the keystore state (locked, seed) in tests. */ void keystore_mock_unlocked(const uint8_t* seed, size_t seed_len, const uint8_t* bip39_seed); + +const uint8_t* keystore_test_get_retained_seed_encrypted(size_t* len_out); #endif #endif diff --git a/src/rust/bitbox02-rust/src/hww/api/backup.rs b/src/rust/bitbox02-rust/src/hww/api/backup.rs index 151fe4b62..3c343f952 100644 --- a/src/rust/bitbox02-rust/src/hww/api/backup.rs +++ b/src/rust/bitbox02-rust/src/hww/api/backup.rs @@ -180,8 +180,8 @@ mod tests { ..Default::default() }); mock_sd(); - mock_unlocked(); mock_memory(); + mock_unlocked(); assert_eq!( block_on(create(&pb::CreateBackupRequest { timestamp: EXPECTED_TIMESTMAP, @@ -229,11 +229,11 @@ mod tests { ..Default::default() }); mock_sd(); + mock_memory(); mock_unlocked_using_mnemonic( "memory raven era cave phone system dice come mechanic split moon repeat", "", ); - mock_memory(); // Create the three files using the a fixture in the directory with the backup ID of the // above seed. @@ -279,8 +279,9 @@ mod tests { ui_confirm_create: Some(Box::new(|_params| true)), ..Default::default() }); - mock_unlocked_using_mnemonic("purity concert above invest pigeon category peace tuition hazard vivid latin since legal speak nation session onion library travel spell region blast estate stay", ""); mock_memory(); + mock_unlocked_using_mnemonic("purity concert above invest pigeon category peace tuition hazard vivid latin since legal speak nation session onion library travel spell region blast estate stay", ""); + bitbox02::memory::set_device_name(DEVICE_NAME_1).unwrap(); assert!(block_on(create(&pb::CreateBackupRequest { timestamp: EXPECTED_TIMESTAMP, @@ -305,8 +306,8 @@ mod tests { ui_confirm_create: Some(Box::new(|_params| true)), ..Default::default() }); - mock_unlocked_using_mnemonic("goddess item rack improve shaft occur actress rib emerge salad rich blame model glare lounge stable electric height scrub scrub oyster now dinner oven", ""); mock_memory(); + mock_unlocked_using_mnemonic("goddess item rack improve shaft occur actress rib emerge salad rich blame model glare lounge stable electric height scrub scrub oyster now dinner oven", ""); bitbox02::memory::set_device_name(DEVICE_NAME_2).unwrap(); assert!(block_on(create(&pb::CreateBackupRequest { timestamp: EXPECTED_TIMESTAMP, diff --git a/src/rust/bitbox02-rust/src/hww/api/bitcoin/signtx.rs b/src/rust/bitbox02-rust/src/hww/api/bitcoin/signtx.rs index 5f339f6f0..5f8692ace 100644 --- a/src/rust/bitbox02-rust/src/hww/api/bitcoin/signtx.rs +++ b/src/rust/bitbox02-rust/src/hww/api/bitcoin/signtx.rs @@ -1736,8 +1736,6 @@ mod tests { /// Test signing if all inputs are of type P2TR. #[test] pub fn test_script_type_p2tr() { - bitbox02::random::mock_reset(); - let transaction = alloc::rc::Rc::new(core::cell::RefCell::new(Transaction::new(pb::BtcCoin::Btc))); for input in transaction.borrow_mut().inputs.iter_mut() { @@ -1763,6 +1761,7 @@ mod tests { mock_default_ui(); mock_unlocked(); + bitbox02::random::mock_reset(); let mut init_request = transaction.borrow().init_request(); init_request.script_configs[0] = pb::BtcScriptConfigWithKeypath { script_config: Some(pb::BtcScriptConfig { diff --git a/test/unit-test/test_keystore.c b/test/unit-test/test_keystore.c index ecc78b818..e717cb502 100644 --- a/test/unit-test/test_keystore.c +++ b/test/unit-test/test_keystore.c @@ -57,6 +57,10 @@ static uint8_t _mock_bip39_seed[64] = { 0x33, 0x33, 0x33, 0x33, 0x33, 0x33, 0x33, 0x33, 0x44, 0x44, 0x44, 0x44, 0x44, 0x44, 0x44, 0x44, }; +static uint8_t _unstretched_retained_seed_encryption_key[32] = + "\xfe\x09\x76\x01\x14\x52\xa7\x22\x12\xe4\xb8\xbd\x57\x2b\x5b\xe3\x01\x41\xa3\x56\xf1\x13\x37" + "\xd2\x9d\x35\xea\x8f\xf9\x97\xbe\xfc"; + static const uint32_t _keypath[] = { 44 + BIP32_INITIAL_HARDENED_CHILD, 0 + BIP32_INITIAL_HARDENED_CHILD, @@ -70,6 +74,10 @@ static const uint8_t _expected_seckey[32] = { 0x58, 0x92, 0x32, 0x9d, 0x67, 0xdf, 0xd4, 0xad, 0x05, 0xe9, 0xc3, 0xd0, 0x6e, 0xdf, 0x74, 0xfb, }; +static uint8_t _expected_retained_seed_secret[32] = + "\xb1\x56\xbe\x41\x65\x30\xc6\xfc\x00\x01\x88\x44\x16\x17\x74\xa3\x54\x6a\x53\xac\x6d\xd4\xa0" + "\x46\x26\x08\x83\x8e\x21\x60\x08\xf7"; + static uint8_t _expected_secret[32] = "\xa8\xf4\xfe\x54\x33\x0e\x1a\xb7\xa0\xe3\xbe\x8a\x8d\x75\xd2\x22\xb2\xae\xc2\xb3\xab\x41\xca" "\x2a\x04\x0e\xa0\x08\x60\x6b\xaf\xce"; @@ -124,6 +132,19 @@ void __wrap_random_32_bytes(uint8_t* buf) memcpy(buf, (const void*)mock(), 32); } +static void _expect_retain_seed(void) +{ + will_return(__wrap_random_32_bytes, _unstretched_retained_seed_encryption_key); +} + +void _mock_unlocked(const uint8_t* seed, size_t seed_len, const uint8_t* bip39_seed) +{ + if (seed != NULL) { + _expect_retain_seed(); + } + keystore_mock_unlocked(seed, seed_len, bip39_seed); +} + static bool _pubkeys_equal( const secp256k1_context* ctx, const secp256k1_pubkey* pubkey1, @@ -145,11 +166,11 @@ static void _test_keystore_get_xpub(void** state) struct ext_key xpub = {0}; - keystore_mock_unlocked(NULL, 0, NULL); + _mock_unlocked(NULL, 0, NULL); // fails because keystore is locked assert_false(keystore_get_xpub(_keypath, sizeof(_keypath) / sizeof(uint32_t), &xpub)); - keystore_mock_unlocked(_mock_seed, sizeof(_mock_seed), _mock_bip39_seed); + _mock_unlocked(_mock_seed, sizeof(_mock_seed), _mock_bip39_seed); assert_true(keystore_get_xpub(_keypath, sizeof(_keypath) / sizeof(uint32_t), &xpub)); secp256k1_pubkey expected_pubkey; @@ -180,7 +201,7 @@ static void _test_keystore_secp256k1_nonce_commit(void** state) memset(host_commitment, 0xAB, sizeof(host_commitment)); { - keystore_mock_unlocked(NULL, 0, NULL); + _mock_unlocked(NULL, 0, NULL); // fails because keystore is locked assert_false(keystore_secp256k1_nonce_commit( _keypath, @@ -190,7 +211,7 @@ static void _test_keystore_secp256k1_nonce_commit(void** state) client_commitment)); } { - keystore_mock_unlocked(_mock_seed, sizeof(_mock_seed), _mock_bip39_seed); + _mock_unlocked(_mock_seed, sizeof(_mock_seed), _mock_bip39_seed); assert_true(keystore_secp256k1_nonce_commit( _keypath, sizeof(_keypath) / sizeof(uint32_t), @@ -219,13 +240,13 @@ static void _test_keystore_secp256k1_sign(void** state) memset(host_nonce, 0x56, sizeof(host_nonce)); { - keystore_mock_unlocked(NULL, 0, NULL); + _mock_unlocked(NULL, 0, NULL); // fails because keystore is locked assert_false(keystore_secp256k1_sign( _keypath, sizeof(_keypath) / sizeof(uint32_t), msg, host_nonce, sig, NULL)); } { - keystore_mock_unlocked(_mock_seed, sizeof(_mock_seed), _mock_bip39_seed); + _mock_unlocked(_mock_seed, sizeof(_mock_seed), _mock_bip39_seed); _sign_expected_seckey = _expected_seckey; _sign_expected_msg = msg; @@ -277,6 +298,7 @@ static void _test_keystore_create_and_unlock_twice(void** state) _smarteeprom_reset(); will_return(__wrap_memory_is_seeded, true); + _expect_retain_seed(); assert_int_equal(KEYSTORE_OK, keystore_unlock(PASSWORD, &remaining_attempts, NULL)); // Create new (different) seed. @@ -284,6 +306,7 @@ static void _test_keystore_create_and_unlock_twice(void** state) assert_int_equal(keystore_encrypt_and_store_seed(_mock_seed_2, 32, PASSWORD), KEYSTORE_OK); will_return(__wrap_memory_is_seeded, true); + _expect_retain_seed(); assert_int_equal(KEYSTORE_OK, keystore_unlock(PASSWORD, &remaining_attempts, NULL)); } @@ -292,6 +315,23 @@ static void _expect_seeded(bool seeded) uint8_t seed[KEYSTORE_MAX_SEED_LENGTH]; size_t len; assert_int_equal(seeded, keystore_copy_seed(seed, &len)); + if (seeded) { + assert_memory_equal(seed, _mock_seed, sizeof(_mock_seed)); + // Also check that the retained seed was encrypted with the expected encryption key. + size_t encrypted_len = 0; + const uint8_t* retained_seed_encrypted = + keystore_test_get_retained_seed_encrypted(&encrypted_len); + size_t decrypted_len = encrypted_len - 48; + uint8_t out[decrypted_len]; + assert_true(cipher_aes_hmac_decrypt( + retained_seed_encrypted, + encrypted_len, + out, + &decrypted_len, + _expected_retained_seed_secret)); + assert_int_equal(decrypted_len, 32); + assert_memory_equal(out, _mock_seed, decrypted_len); + } } static void _perform_some_unlocks(void) @@ -301,6 +341,9 @@ static void _perform_some_unlocks(void) for (int i = 0; i < 3; i++) { _reset_reset_called = false; will_return(__wrap_memory_is_seeded, true); + if (i == 0) { + _expect_retain_seed(); + } assert_int_equal(KEYSTORE_OK, keystore_unlock(PASSWORD, &remaining_attempts, NULL)); assert_int_equal(remaining_attempts, MAX_UNLOCK_ATTEMPTS); assert_false(_reset_reset_called); @@ -311,7 +354,7 @@ static void _perform_some_unlocks(void) static void _test_keystore_unlock(void** state) { _smarteeprom_reset(); - keystore_mock_unlocked(NULL, 0, NULL); // reset to locked + _mock_unlocked(NULL, 0, NULL); // reset to locked uint8_t remaining_attempts; @@ -349,11 +392,11 @@ static void _test_keystore_unlock(void** state) static void _test_keystore_lock(void** state) { - keystore_mock_unlocked(NULL, 0, NULL); + _mock_unlocked(NULL, 0, NULL); assert_true(keystore_is_locked()); - keystore_mock_unlocked(_mock_seed, sizeof(_mock_seed), NULL); + _mock_unlocked(_mock_seed, sizeof(_mock_seed), NULL); assert_true(keystore_is_locked()); - keystore_mock_unlocked(_mock_seed, sizeof(_mock_seed), _mock_bip39_seed); + _mock_unlocked(_mock_seed, sizeof(_mock_seed), _mock_bip39_seed); assert_false(keystore_is_locked()); keystore_lock(); assert_true(keystore_is_locked()); @@ -362,13 +405,13 @@ static void _test_keystore_lock(void** state) static void _test_keystore_get_bip39_mnemonic(void** state) { char mnemonic[300]; - keystore_mock_unlocked(NULL, 0, NULL); + _mock_unlocked(NULL, 0, NULL); assert_false(keystore_get_bip39_mnemonic(mnemonic, sizeof(mnemonic))); - keystore_mock_unlocked(_mock_seed, sizeof(_mock_seed), NULL); + _mock_unlocked(_mock_seed, sizeof(_mock_seed), NULL); assert_false(keystore_get_bip39_mnemonic(mnemonic, sizeof(mnemonic))); - keystore_mock_unlocked(_mock_seed, sizeof(_mock_seed), _mock_bip39_seed); + _mock_unlocked(_mock_seed, sizeof(_mock_seed), _mock_bip39_seed); assert_true(keystore_get_bip39_mnemonic(mnemonic, sizeof(mnemonic))); const char* expected_mnemonic = "baby mass dust captain baby mass mass dust captain baby mass dutch creek office smoke " @@ -430,7 +473,7 @@ static void _mock_with_mnemonic(const char* mnemonic, const char* passphrase) size_t seed_len; assert_true(keystore_bip39_mnemonic_to_seed(mnemonic, seed, &seed_len)); - keystore_mock_unlocked(seed, seed_len, NULL); + _mock_unlocked(seed, seed_len, NULL); assert_true(keystore_unlock_bip39(passphrase)); } diff --git a/test/unit-test/test_keystore_functional.c b/test/unit-test/test_keystore_functional.c index e9789975f..993a83403 100644 --- a/test/unit-test/test_keystore_functional.c +++ b/test/unit-test/test_keystore_functional.c @@ -90,6 +90,7 @@ static void _test_seeds(void** state) } assert_true(keystore_copy_seed(read_seed, &read_seed_len)); assert_int_equal(seed_size, read_seed_len); + assert_memory_equal(read_seed, _seed, seed_size); keystore_lock(); } } From edeefe95e2d46f1da2aa08a2c5c324525538db57 Mon Sep 17 00:00:00 2001 From: Marko Bencun Date: Mon, 26 Dec 2022 01:18:41 +0100 Subject: [PATCH 3/4] keystore: encrypt the bip39 seed in RAM This encrypts the bip39 seed in RAM in the same way as the seed is encrypted, using the secure chip for key stretching. Since decryption (key stretching) takes around 100ms, this has a performance impact on every operation that needs the an xprv or xpub: - computing xpubs - getting private keys to sign something - etc. This has an impact especially in Bitcoin transaction signing, as we need the xpub at every transaction input during the first round of input streaming and for every change output. The xprv is needed twice per input when signing the input (once for the antiklepto nonce commit, then for signing). The performance impact was mitigated previously by 463b1f657eeec11978085c2e716ab2f378234334, which sped up the first round of input streaming by caching xpubs - only one xpub per script type is needed for the whole transaction for single sig, where we only support three script types (p2wpkh, p2wpkh-p2sh, p2tr). In multisig, it is only one xpub. Here are some performance numbers: If you had a 50 inputs tx signed with antiklepto enabled, my experiment resulted in: - loading tx (stream inputs): ~30s, before xpub caching was activated - loading tx (stream inputs): ~6s, after xpub caching was activated - signing tx: 50*2*100ms: ~10s. I.e. Xpub caching saved more than seed decrypting added. Another place where this has a performance impact: retrieving xpubs. When the user connects the BitBox02 for the first time to the BitBoxApp, the BitBoxApp gets 3 xpubs right away (one per single sig script type) for Bitcoin, 2 xpubs for Litecoin, 1 for Ethereum. Each xpub computation is also performed twice (`see _get_xprv_twice()`) to avoid catch potential bitflips. With 100ms per access this means a delay of ~1.2s, which is noticable. Maybe a progress animation can/should be added there. --- src/keystore.c | 70 +++++++++++++++++++++++++++++++--- src/keystore.h | 1 + test/unit-test/test_keystore.c | 48 +++++++++++++++++++++++ 3 files changed, 113 insertions(+), 6 deletions(-) diff --git a/src/keystore.c b/src/keystore.c index 88898b7f4..b223b524d 100644 --- a/src/keystore.c +++ b/src/keystore.c @@ -43,8 +43,10 @@ static size_t _retained_seed_encrypted_len = 0; // Change this ONLY via keystore_unlock_bip39(). static bool _is_unlocked_bip39 = false; +static uint8_t _unstretched_retained_bip39_seed_encryption_key[32] = {0}; // Must be defined if _is_unlocked is true. ONLY ACCESS THIS WITH _copy_bip39_seed(). -static uint8_t _retained_bip39_seed[64] = {0}; +static uint8_t _retained_bip39_seed_encrypted[64 + 64] = {0}; +static size_t _retained_bip39_seed_encrypted_len = 0; /** * We allow seeds of 16, 24 or 32 bytes. @@ -119,13 +121,37 @@ static bool _copy_bip39_seed(uint8_t* bip39_seed_out) if (!_is_unlocked_bip39) { return false; } + + uint8_t retained_bip39_seed_encryption_key[32] = {0}; + UTIL_CLEANUP_32(retained_bip39_seed_encryption_key); + if (_stretch_retained_seed_encryption_key( + _unstretched_retained_bip39_seed_encryption_key, + "keystore_retained_bip39_seed_access_in", + "keystore_retained_bip39_seed_access_out", + retained_bip39_seed_encryption_key) != KEYSTORE_OK) { + return false; + } + size_t len = _retained_bip39_seed_encrypted_len - 48; + bool password_correct = cipher_aes_hmac_decrypt( + _retained_bip39_seed_encrypted, + _retained_bip39_seed_encrypted_len, + bip39_seed_out, + &len, + retained_bip39_seed_encryption_key); + if (!password_correct) { + // Should never happen. + return false; + } + if (len != 64) { + // Should never happen. + return false; + } // sanity check uint8_t zero[64] = {0}; util_zero(zero, 64); - if (MEMEQ(_retained_bip39_seed, zero, sizeof(_retained_bip39_seed))) { + if (MEMEQ(bip39_seed_out, zero, 64)) { return false; } - memcpy(bip39_seed_out, _retained_bip39_seed, sizeof(_retained_bip39_seed)); return true; } @@ -370,7 +396,26 @@ USE_RESULT static keystore_error_t _retain_seed(const uint8_t* seed, size_t seed USE_RESULT static bool _retain_bip39_seed(const uint8_t* bip39_seed) { - memcpy(_retained_bip39_seed, bip39_seed, sizeof(_retained_bip39_seed)); + random_32_bytes(_unstretched_retained_bip39_seed_encryption_key); + uint8_t retained_bip39_seed_encryption_key[32] = {0}; + UTIL_CLEANUP_32(retained_bip39_seed_encryption_key); + if (_stretch_retained_seed_encryption_key( + _unstretched_retained_bip39_seed_encryption_key, + "keystore_retained_bip39_seed_access_in", + "keystore_retained_bip39_seed_access_out", + retained_bip39_seed_encryption_key) != KEYSTORE_OK) { + return false; + } + size_t len = sizeof(_retained_bip39_seed_encrypted); + if (!cipher_aes_hmac_encrypt( + bip39_seed, + 64, + _retained_bip39_seed_encrypted, + &len, + retained_bip39_seed_encryption_key)) { + return false; + } + _retained_bip39_seed_encrypted_len = len; return true; } @@ -380,7 +425,12 @@ static void _delete_retained_seeds(void) _unstretched_retained_seed_encryption_key, sizeof(_unstretched_retained_seed_encryption_key)); util_zero(_retained_seed_encrypted, sizeof(_retained_seed_encrypted)); - util_zero(_retained_bip39_seed, sizeof(_retained_bip39_seed)); + _retained_seed_encrypted_len = 0; + util_zero( + _unstretched_retained_bip39_seed_encryption_key, + sizeof(_unstretched_retained_seed_encryption_key)); + util_zero(_retained_bip39_seed_encrypted, sizeof(_retained_bip39_seed_encrypted)); + _retained_bip39_seed_encrypted_len = 0; } keystore_error_t keystore_unlock( @@ -872,7 +922,9 @@ void keystore_mock_unlocked(const uint8_t* seed, size_t seed_len, const uint8_t* } _is_unlocked_bip39 = bip39_seed != NULL; if (bip39_seed != NULL) { - memcpy(_retained_bip39_seed, bip39_seed, sizeof(_retained_bip39_seed)); + if (!_retain_bip39_seed(bip39_seed)) { + Abort("couldn't retain bip39 seed"); + } } } @@ -881,4 +933,10 @@ const uint8_t* keystore_test_get_retained_seed_encrypted(size_t* len_out) *len_out = _retained_seed_encrypted_len; return _retained_seed_encrypted; } + +const uint8_t* keystore_test_get_retained_bip39_seed_encrypted(size_t* len_out) +{ + *len_out = _retained_bip39_seed_encrypted_len; + return _retained_bip39_seed_encrypted; +} #endif diff --git a/src/keystore.h b/src/keystore.h index fe3c36747..54ef8844d 100644 --- a/src/keystore.h +++ b/src/keystore.h @@ -285,6 +285,7 @@ USE_RESULT bool keystore_secp256k1_schnorr_bip86_sign( void keystore_mock_unlocked(const uint8_t* seed, size_t seed_len, const uint8_t* bip39_seed); const uint8_t* keystore_test_get_retained_seed_encrypted(size_t* len_out); +const uint8_t* keystore_test_get_retained_bip39_seed_encrypted(size_t* len_out); #endif #endif diff --git a/test/unit-test/test_keystore.c b/test/unit-test/test_keystore.c index e717cb502..852a78ba1 100644 --- a/test/unit-test/test_keystore.c +++ b/test/unit-test/test_keystore.c @@ -61,6 +61,10 @@ static uint8_t _unstretched_retained_seed_encryption_key[32] = "\xfe\x09\x76\x01\x14\x52\xa7\x22\x12\xe4\xb8\xbd\x57\x2b\x5b\xe3\x01\x41\xa3\x56\xf1\x13\x37" "\xd2\x9d\x35\xea\x8f\xf9\x97\xbe\xfc"; +static uint8_t _unstretched_retained_bip39_seed_encryption_key[32] = + "\x9b\x44\xc7\x04\x88\x93\xfa\xaf\x6e\x2d\x76\x25\xd1\x3d\x8f\x1c\xab\x07\x65\xfd\x61\xf1\x59" + "\xd9\x71\x3e\x08\x15\x5d\x06\x71\x7c"; + static const uint32_t _keypath[] = { 44 + BIP32_INITIAL_HARDENED_CHILD, 0 + BIP32_INITIAL_HARDENED_CHILD, @@ -78,6 +82,10 @@ static uint8_t _expected_retained_seed_secret[32] = "\xb1\x56\xbe\x41\x65\x30\xc6\xfc\x00\x01\x88\x44\x16\x17\x74\xa3\x54\x6a\x53\xac\x6d\xd4\xa0" "\x46\x26\x08\x83\x8e\x21\x60\x08\xf7"; +const uint8_t _expected_retained_bip39_seed_secret[32] = + "\x85\x6d\x9a\x8c\x1e\xa4\x2a\x69\xae\x76\x32\x42\x44\xac\xe6\x74\x39\x7f\xf1\x36\x0a\x4b\xa4" + "\xc8\x5f\xfb\xd4\x2c\xee\x8a\x7f\x29"; + static uint8_t _expected_secret[32] = "\xa8\xf4\xfe\x54\x33\x0e\x1a\xb7\xa0\xe3\xbe\x8a\x8d\x75\xd2\x22\xb2\xae\xc2\xb3\xab\x41\xca" "\x2a\x04\x0e\xa0\x08\x60\x6b\xaf\xce"; @@ -137,11 +145,19 @@ static void _expect_retain_seed(void) will_return(__wrap_random_32_bytes, _unstretched_retained_seed_encryption_key); } +static void _expect_retain_bip39_seed(void) +{ + will_return(__wrap_random_32_bytes, _unstretched_retained_bip39_seed_encryption_key); +} + void _mock_unlocked(const uint8_t* seed, size_t seed_len, const uint8_t* bip39_seed) { if (seed != NULL) { _expect_retain_seed(); } + if (bip39_seed != NULL) { + _expect_retain_bip39_seed(); + } keystore_mock_unlocked(seed, seed_len, bip39_seed); } @@ -390,6 +406,36 @@ static void _test_keystore_unlock(void** state) assert_true(_reset_reset_called); } +static void _test_keystore_unlock_bip39(void** state) +{ + keystore_lock(); + assert_false(keystore_unlock_bip39("")); + + _mock_unlocked(_mock_seed, sizeof(_mock_seed), NULL); + assert_true(keystore_is_locked()); + + _expect_retain_bip39_seed(); + assert_true(keystore_unlock_bip39("foo")); + // Check that the retained bip39 seed was encrypted with the expected encryption key. + size_t encrypted_len = 0; + const uint8_t* retained_bip39_seed_encrypted = + keystore_test_get_retained_bip39_seed_encrypted(&encrypted_len); + size_t decrypted_len = encrypted_len - 48; + uint8_t out[decrypted_len]; + assert_true(cipher_aes_hmac_decrypt( + retained_bip39_seed_encrypted, + encrypted_len, + out, + &decrypted_len, + _expected_retained_bip39_seed_secret)); + assert_int_equal(decrypted_len, 64); + const uint8_t expected_bip39_seed[64] = + "\x2b\x3c\x63\xde\x86\xf0\xf2\xb1\x3c\xc6\xa3\x6c\x1b\xa2\x31\x4f\xbc\x1b\x40\xc7\x7a\xb9" + "\xcb\x64\xe9\x6b\xa4\xd5\xc6\x2f\xc2\x04\x74\x8c\xa6\x62\x6a\x9f\x03\x5e\x7d\x43\x1b\xce" + "\x8c\x92\x10\xec\x0b\xdf\xfc\x2e\x7d\xb8\x73\xde\xe5\x6c\x8a\xc2\x15\x3e\xee\x9a"; + assert_memory_equal(out, expected_bip39_seed, decrypted_len); +} + static void _test_keystore_lock(void** state) { _mock_unlocked(NULL, 0, NULL); @@ -474,6 +520,7 @@ static void _mock_with_mnemonic(const char* mnemonic, const char* passphrase) assert_true(keystore_bip39_mnemonic_to_seed(mnemonic, seed, &seed_len)); _mock_unlocked(seed, seed_len, NULL); + _expect_retain_bip39_seed(); assert_true(keystore_unlock_bip39(passphrase)); } @@ -691,6 +738,7 @@ int main(void) cmocka_unit_test(_test_keystore_encrypt_and_store_seed), cmocka_unit_test(_test_keystore_create_and_unlock_twice), cmocka_unit_test(_test_keystore_unlock), + cmocka_unit_test(_test_keystore_unlock_bip39), cmocka_unit_test(_test_keystore_lock), cmocka_unit_test(_test_keystore_get_bip39_mnemonic), cmocka_unit_test(_test_keystore_create_and_store_seed), From 4f04a35149f125d82ed5ee342f9e76ddc915d325 Mon Sep 17 00:00:00 2001 From: Marko Bencun Date: Thu, 18 May 2023 05:29:10 +0200 Subject: [PATCH 4/4] keystore: add some helpful comments --- src/keystore.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/keystore.c b/src/keystore.c index b223b524d..e56ed95f0 100644 --- a/src/keystore.c +++ b/src/keystore.c @@ -36,15 +36,20 @@ // Change this ONLY via keystore_unlock() or keystore_lock() static bool _is_unlocked_device = false; +// Stores a random key after unlock which, after stretching, is used to encrypt the retained seed. static uint8_t _unstretched_retained_seed_encryption_key[32] = {0}; // Must be defined if is_unlocked is true. ONLY ACCESS THIS WITH keystore_copy_seed(). +// Stores the encrypted seed after unlock. static uint8_t _retained_seed_encrypted[KEYSTORE_MAX_SEED_LENGTH + 64] = {0}; static size_t _retained_seed_encrypted_len = 0; // Change this ONLY via keystore_unlock_bip39(). static bool _is_unlocked_bip39 = false; +// Stores a random keyy after bip39-unlock which, after stretching, is used to encrypt the retained +// bip39 seed. static uint8_t _unstretched_retained_bip39_seed_encryption_key[32] = {0}; // Must be defined if _is_unlocked is true. ONLY ACCESS THIS WITH _copy_bip39_seed(). +// Stores the encrypted BIP-39 seed after bip39-unlock. static uint8_t _retained_bip39_seed_encrypted[64 + 64] = {0}; static size_t _retained_bip39_seed_encrypted_len = 0;