reduce number of secure chip security events when creating/restoring

Before, `keystore_encrypt_and_store_seed()` (called when
creating/restoring a seed) would always be followed by
`keystore_unlock(<password>)` with the password the user just chose,
so unlock could never fail. The unlocking part costs many secure chip
operations (for stretching the password).

By making the first function already unlock the keystore, we can avoid
calling `keystore_unlock()`, reducing the number of secure chip events
by 5.

This effort is part of mitigating Optiga's throttling mechanism that
kicks in after 133 events - users can run into this by repeatedly
resetting/restoring).

Author: benma

src/keystore.c

@@ -224,86 +224,6 @@ static bool _verify_seed(
     return true;
 }
 
-keystore_error_t keystore_encrypt_and_store_seed(
-    const uint8_t* seed,
-    size_t seed_length,
-    const char* password)
-{
-    if (memory_is_initialized()) {
-        return KEYSTORE_ERR_MEMORY;
-    }
-    keystore_lock();
-    if (!_validate_seed_length(seed_length)) {
-        return KEYSTORE_ERR_SEED_SIZE;
-    }
-    if (securechip_init_new_password(password)) {
-        return KEYSTORE_ERR_SECURECHIP;
-    }
-    uint8_t secret[32] = {0};
-    UTIL_CLEANUP_32(secret);
-    if (securechip_stretch_password(password, secret)) {
-        return KEYSTORE_ERR_SECURECHIP;
-    }
-
-    size_t encrypted_seed_len = seed_length + 64;
-    uint8_t encrypted_seed[encrypted_seed_len];
-    UTIL_CLEANUP_32(encrypted_seed);
-    if (!cipher_aes_hmac_encrypt(seed, seed_length, encrypted_seed, &encrypted_seed_len, secret)) {
-        return KEYSTORE_ERR_ENCRYPT;
-    }
-    if (encrypted_seed_len > 255) { // sanity check, can't happen
-        Abort("keystore_encrypt_and_store_seed");
-    }
-    uint8_t encrypted_seed_len_u8 = (uint8_t)encrypted_seed_len;
-    if (!memory_set_encrypted_seed_and_hmac(encrypted_seed, encrypted_seed_len_u8)) {
-        return KEYSTORE_ERR_MEMORY;
-    }
-    if (!_verify_seed(password, seed, seed_length)) {
-        if (!memory_reset_hww()) {
-            return KEYSTORE_ERR_MEMORY;
-        }
-        return KEYSTORE_ERR_MEMORY;
-    }
-    return KEYSTORE_OK;
-}
-
-keystore_error_t keystore_create_and_store_seed(
-    const char* password,
-    const uint8_t* host_entropy,
-    size_t host_entropy_size)
-{
-    if (host_entropy_size != 16 && host_entropy_size != 32) {
-        return KEYSTORE_ERR_SEED_SIZE;
-    }
-    if (KEYSTORE_MAX_SEED_LENGTH != RANDOM_NUM_SIZE) {
-        Abort("keystore create: size mismatch");
-    }
-    uint8_t seed[KEYSTORE_MAX_SEED_LENGTH];
-    UTIL_CLEANUP_32(seed);
-    random_32_bytes(seed);
-
-    // Mix in Host entropy.
-    for (size_t i = 0; i < host_entropy_size; i++) {
-        seed[i] ^= host_entropy[i];
-    }
-
-    // Mix in entropy derived from the user password.
-    uint8_t password_salted_hashed[KEYSTORE_MAX_SEED_LENGTH] = {0};
-    UTIL_CLEANUP_32(password_salted_hashed);
-    if (!salt_hash_data(
-            (const uint8_t*)password,
-            strlen(password),
-            "keystore_seed_generation",
-            password_salted_hashed)) {
-        return KEYSTORE_ERR_SALT;
-    }
-
-    for (size_t i = 0; i < host_entropy_size; i++) {
-        seed[i] ^= password_salted_hashed[i];
-    }
-    return keystore_encrypt_and_store_seed(seed, host_entropy_size, password);
-}
-
 USE_RESULT static keystore_error_t _retain_seed(const uint8_t* seed, size_t seed_len)
 {
 #ifdef TESTING
@@ -385,6 +305,93 @@ static void _delete_retained_seeds(void)
     _retained_bip39_seed_encrypted_len = 0;
 }
 
+keystore_error_t keystore_encrypt_and_store_seed(
+    const uint8_t* seed,
+    size_t seed_length,
+    const char* password)
+{
+    if (memory_is_initialized()) {
+        return KEYSTORE_ERR_MEMORY;
+    }
+    keystore_lock();
+    if (!_validate_seed_length(seed_length)) {
+        return KEYSTORE_ERR_SEED_SIZE;
+    }
+    if (securechip_init_new_password(password)) {
+        return KEYSTORE_ERR_SECURECHIP;
+    }
+    uint8_t secret[32] = {0};
+    UTIL_CLEANUP_32(secret);
+    if (securechip_stretch_password(password, secret)) {
+        return KEYSTORE_ERR_SECURECHIP;
+    }
+
+    size_t encrypted_seed_len = seed_length + 64;
+    uint8_t encrypted_seed[encrypted_seed_len];
+    UTIL_CLEANUP_32(encrypted_seed);
+    if (!cipher_aes_hmac_encrypt(seed, seed_length, encrypted_seed, &encrypted_seed_len, secret)) {
+        return KEYSTORE_ERR_ENCRYPT;
+    }
+    if (encrypted_seed_len > 255) { // sanity check, can't happen
+        Abort("keystore_encrypt_and_store_seed");
+    }
+    uint8_t encrypted_seed_len_u8 = (uint8_t)encrypted_seed_len;
+    if (!memory_set_encrypted_seed_and_hmac(encrypted_seed, encrypted_seed_len_u8)) {
+        return KEYSTORE_ERR_MEMORY;
+    }
+    if (!_verify_seed(password, seed, seed_length)) {
+        if (!memory_reset_hww()) {
+            return KEYSTORE_ERR_MEMORY;
+        }
+        return KEYSTORE_ERR_MEMORY;
+    }
+
+    keystore_error_t retain_seed_result = _retain_seed(seed, seed_length);
+    if (retain_seed_result != KEYSTORE_OK) {
+        return retain_seed_result;
+    }
+    _is_unlocked_device = true;
+
+    return KEYSTORE_OK;
+}
+
+keystore_error_t keystore_create_and_store_seed(
+    const char* password,
+    const uint8_t* host_entropy,
+    size_t host_entropy_size)
+{
+    if (host_entropy_size != 16 && host_entropy_size != 32) {
+        return KEYSTORE_ERR_SEED_SIZE;
+    }
+    if (KEYSTORE_MAX_SEED_LENGTH != RANDOM_NUM_SIZE) {
+        Abort("keystore create: size mismatch");
+    }
+    uint8_t seed[KEYSTORE_MAX_SEED_LENGTH];
+    UTIL_CLEANUP_32(seed);
+    random_32_bytes(seed);
+
+    // Mix in Host entropy.
+    for (size_t i = 0; i < host_entropy_size; i++) {
+        seed[i] ^= host_entropy[i];
+    }
+
+    // Mix in entropy derived from the user password.
+    uint8_t password_salted_hashed[KEYSTORE_MAX_SEED_LENGTH] = {0};
+    UTIL_CLEANUP_32(password_salted_hashed);
+    if (!salt_hash_data(
+            (const uint8_t*)password,
+            strlen(password),
+            "keystore_seed_generation",
+            password_salted_hashed)) {
+        return KEYSTORE_ERR_SALT;
+    }
+
+    for (size_t i = 0; i < host_entropy_size; i++) {
+        seed[i] ^= password_salted_hashed[i];
+    }
+    return keystore_encrypt_and_store_seed(seed, host_entropy_size, password);
+}
+
 keystore_error_t keystore_unlock(
     const char* password,
     uint8_t* remaining_attempts_out,

src/keystore.h

@@ -63,7 +63,7 @@ USE_RESULT bool keystore_copy_seed(uint8_t* seed_out, size_t* length_out);
 USE_RESULT bool keystore_copy_bip39_seed(uint8_t* bip32_seed_out);
 
 /**
- * Restores a seed.
+ * Restores a seed. This also unlocks the keystore with this seed.
  * @param[in] seed The seed that is to be restored.
  * @param[in] seed_length The length of the seed (max. 32 bytes).
  * @param[in] password The password with which we encrypt the seed.
@@ -75,6 +75,7 @@ keystore_encrypt_and_store_seed(const uint8_t* seed, size_t seed_length, const c
    Generates the seed, mixes it with host_entropy, and stores it encrypted with the
    password. The size of the host entropy determines the size of the seed. Can be either 16 or 32
    bytes, resulting in 12 or 24 BIP39 recovery words.
+   This also unlocks the keystore with the new seed.
    @param[in] host_entropy bytes of entropy to be mixed in.
    @param[in] host_entropy_size must be 16 or 32.
 */

src/rust/bitbox02-rust/src/hww/api/restore.rs

@@ -17,7 +17,6 @@ use crate::pb;
 
 use pb::response::Response;
 
-use crate::general::abort;
 use crate::hal::Ui;
 use crate::workflow::{confirm, mnemonic, password, unlock};
 
@@ -84,9 +83,6 @@ pub async fn from_file(
     }
 
     bitbox02::memory::set_initialized().or(Err(Error::Memory))?;
-    if bitbox02::keystore::unlock(&password).is_err() {
-        abort("restore_from_file: unlock failed");
-    };
 
     // Ignore non-critical error.
     let _ = bitbox02::memory::set_device_name(&metadata.name);
@@ -160,10 +156,6 @@ pub async fn from_mnemonic(
     }
 
     bitbox02::memory::set_initialized().or(Err(Error::Memory))?;
-    // This should never fail.
-    if bitbox02::keystore::unlock(&password).is_err() {
-        abort("restore_from_mnemonic: unlock failed");
-    };
 
     unlock::unlock_bip39(hal).await;
     Ok(Response::Success(pb::Success {}))
@@ -207,7 +199,7 @@ mod tests {
             )),
             Ok(Response::Success(pb::Success {}))
         );
-        assert_eq!(bitbox02::securechip::fake_event_counter(), 19);
+        assert_eq!(bitbox02::securechip::fake_event_counter(), 14);
         drop(mock_hal); // to remove mutable borrow of counter
         assert_eq!(counter, 2);
         assert!(!keystore::is_locked());

src/rust/bitbox02-rust/src/hww/api/set_password.rs

@@ -40,9 +40,6 @@ pub async fn process(
         hal.ui().status(&format!("Error\n{:?}", err), false).await;
         return Err(Error::Generic);
     }
-    if keystore::unlock(&password).is_err() {
-        panic!("Unexpected error during restore: unlock failed.");
-    }
     unlock::unlock_bip39(hal).await;
     Ok(Response::Success(pb::Success {}))
 }
@@ -83,7 +80,7 @@ mod tests {
             )),
             Ok(Response::Success(pb::Success {}))
         );
-        assert_eq!(bitbox02::securechip::fake_event_counter(), 19);
+        assert_eq!(bitbox02::securechip::fake_event_counter(), 14);
         drop(mock_hal); // to remove mutable borrow of counter
         assert_eq!(counter, 2);
         assert!(!keystore::is_locked());

src/rust/bitbox02-rust/src/keystore.rs

@@ -457,14 +457,7 @@ mod tests {
 
             bitbox02::securechip::fake_event_counter_reset();
             assert!(keystore::encrypt_and_store_seed(seed, "foo").is_ok());
-            assert_eq!(bitbox02::securechip::fake_event_counter(), 11);
-
-            assert!(keystore::unlock_bip39(test.mnemonic_passphrase).is_err());
-            assert!(keystore::is_locked());
-
-            bitbox02::securechip::fake_event_counter_reset();
-            assert!(keystore::unlock("foo").is_ok());
-            assert_eq!(bitbox02::securechip::fake_event_counter(), 6);
+            assert_eq!(bitbox02::securechip::fake_event_counter(), 12);
 
             assert!(keystore::is_locked());
 

src/rust/bitbox02/src/keystore.rs

@@ -398,7 +398,6 @@ mod tests {
         let seed = hex::decode("cb33c20cea62a5c277527e2002da82e6e2b37450a755143a540a54cea8da9044")
             .unwrap();
         assert!(encrypt_and_store_seed(&seed, "password").is_ok());
-        assert!(unlock("password").is_ok());
         assert!(is_locked()); // still locked, it is only unlocked after unlock_bip39.
         assert!(unlock_bip39("foo").is_ok());
         assert!(!is_locked());
@@ -422,6 +421,7 @@ mod tests {
         crate::memory::set_salt_root(mock_salt_root.as_slice().try_into().unwrap()).unwrap();
 
         assert!(encrypt_and_store_seed(&seed, "password").is_ok());
+        lock();
 
         // Loop to check that unlocking works while unlocked.
         for _ in 0..3 {
@@ -486,7 +486,6 @@ mod tests {
         crate::memory::set_salt_root(mock_salt_root.as_slice().try_into().unwrap()).unwrap();
 
         assert!(encrypt_and_store_seed(&seed, "password").is_ok());
-        assert!(unlock("password").is_ok());
         assert!(unlock_bip39("foo").is_ok());
 
         let expected_bip39_seed = hex::decode("2b3c63de86f0f2b13cc6a36c1ba2314fbc1b40c77ab9cb64e96ba4d5c62fc204748ca6626a9f035e7d431bce8c9210ec0bdffc2e7db873dee56c8ac2153eee9a").unwrap();
@@ -559,7 +558,6 @@ mod tests {
             lock();
 
             assert!(create_and_store_seed("password", &host_entropy[..size]).is_ok());
-            assert!(unlock("password").is_ok());
             assert_eq!(copy_seed().unwrap().as_slice(), &expected_seed[..size]);
             // Check the seed has been stored encrypted with the expected encryption key.
             // Decrypt and check seed.
@@ -590,14 +588,12 @@ mod tests {
         let seed2 = hex::decode("c28135734876aff9ccf4f1d60df8d19a0a38fd02085883f65fc608eb769a635d")
             .unwrap();
         assert!(encrypt_and_store_seed(&seed, "password").is_ok());
-        assert!(unlock("password").is_ok());
         // Create new (different) seed.
         assert!(encrypt_and_store_seed(&seed2, "password").is_ok());
-        assert!(unlock("password").is_ok());
         assert_eq!(copy_seed().unwrap().as_slice(), &seed2);
     }
 
-    // Functional test to store seeds, unlock, retrieve seed.
+    // Functional test to store seeds, lock/unlock, retrieve seed.
     #[test]
     fn test_seeds() {
         let seed = hex::decode("cb33c20cea62a5c277527e2002da82e6e2b37450a755143a540a54cea8da9044")
@@ -611,6 +607,12 @@ mod tests {
             for _ in 0..2 {
                 assert!(encrypt_and_store_seed(&seed[..seed_size], "foo").is_ok());
             }
+            // Also unlocks, so we can get the retained seed.
+            assert_eq!(copy_seed().unwrap().as_slice(), &seed[..seed_size]);
+
+            lock();
+            // Can't get seed before unlock.
+            assert!(copy_seed().is_err());
 
             // Wrong password.
             assert!(matches!(
@@ -620,8 +622,6 @@ mod tests {
                 })
             ));
 
-            // Can't get seed before unlock.
-            assert!(copy_seed().is_err());
             // Correct password. First time: unlock. After unlock, it becomes a password check.
             for _ in 0..3 {
                 assert!(unlock("foo").is_ok());