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().

Author: benma

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

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)

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, &current_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

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

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,

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 {