keystore: display unlock error code

Users reported a panic on the device after unlock: "keystore unlock
failed".

This was a panic because it was never supposed to happen. The expected
error (incorrect password) was caught and handled with a special
message. Since it is happening in for users anyway sometimes, we don't
panic anymore and we pass through granular error codes and display
them to the user, so we can get a better idea of what is going wrong.

The next commit will also display the securechip error code in case
of a securechip failure.

Author: benma

src/keystore.c

@@ -108,7 +108,7 @@ static uint8_t* _get_bip39_seed(void)
     return _retained_bip39_seed;
 }
 
-static bool _stretch_password(const char* password, uint8_t* kdf_out)
+static keystore_error_t _stretch_password(const char* password, uint8_t* kdf_out)
 {
     uint8_t password_salted_hashed[32] = {0};
     UTIL_CLEANUP_32(password_salted_hashed);
@@ -117,7 +117,7 @@ static bool _stretch_password(const char* password, uint8_t* kdf_out)
             strlen(password),
             "keystore_seed_access_in",
             password_salted_hashed)) {
-        return false;
+        return KEYSTORE_ERR_SALT;
     }
 
     uint8_t kdf_in[32] = {0};
@@ -127,13 +127,13 @@ static bool _stretch_password(const char* password, uint8_t* kdf_out)
     // First KDF on SECURECHIP_SLOT_ROLLKEY increments the monotonic
     // counter. Call only once!
     if (!securechip_kdf(SECURECHIP_SLOT_ROLLKEY, kdf_in, 32, kdf_out)) {
-        return false;
+        return KEYSTORE_ERR_SC_KDF;
     }
     // Second KDF does not use the counter and we call it multiple times.
     for (int i = 0; i < KDF_NUM_ITERATIONS; i++) {
         memcpy(kdf_in, kdf_out, 32);
         if (!securechip_kdf(SECURECHIP_SLOT_KDF, kdf_in, 32, kdf_out)) {
-            return false;
+            return KEYSTORE_ERR_SC_KDF;
         }
     }
 
@@ -142,17 +142,18 @@ static bool _stretch_password(const char* password, uint8_t* kdf_out)
             strlen(password),
             "keystore_seed_access_out",
             password_salted_hashed)) {
-        return false;
+        return KEYSTORE_ERR_SALT;
     }
     if (wally_hmac_sha256(
             password_salted_hashed, sizeof(password_salted_hashed), kdf_out, 32, kdf_out, 32) !=
         WALLY_OK) {
-        return false;
+        return KEYSTORE_ERR_HASH;
     }
 
-    return true;
+    return KEYSTORE_OK;
 }
-static bool _get_and_decrypt_seed(
+
+static keystore_error_t _get_and_decrypt_seed(
     const char* password,
     uint8_t* decrypted_seed_out,
     size_t* decrypted_seed_len_out,
@@ -162,12 +163,13 @@ static bool _get_and_decrypt_seed(
     UTIL_CLEANUP_32(encrypted_seed_and_hmac);
     uint8_t encrypted_len;
     if (!memory_get_encrypted_seed_and_hmac(encrypted_seed_and_hmac, &encrypted_len)) {
-        return false;
+        return KEYSTORE_ERR_MEMORY;
     }
     uint8_t secret[32];
     UTIL_CLEANUP_32(secret);
-    if (!_stretch_password(password, secret)) {
-        return false;
+    keystore_error_t result = _stretch_password(password, secret);
+    if (result != KEYSTORE_OK) {
+        return result;
     }
     if (encrypted_len < 49) {
         Abort("_get_and_decrypt_seed: underflow / zero size");
@@ -179,13 +181,13 @@ static bool _get_and_decrypt_seed(
     if (*password_correct_out) {
         if (!_validate_seed_length(decrypted_len)) {
             util_zero(decrypted, sizeof(decrypted));
-            return false;
+            return KEYSTORE_ERR_SEED_SIZE;
         }
         *decrypted_seed_len_out = decrypted_len;
         memcpy(decrypted_seed_out, decrypted, decrypted_len);
     }
     util_zero(decrypted, sizeof(decrypted));
-    return true;
+    return KEYSTORE_OK;
 }
 
 static bool _verify_seed(
@@ -197,7 +199,8 @@ static bool _verify_seed(
     size_t seed_len;
     UTIL_CLEANUP_32(decrypted_seed);
     bool password_correct = false;
-    if (!_get_and_decrypt_seed(password, decrypted_seed, &seed_len, &password_correct)) {
+    if (_get_and_decrypt_seed(password, decrypted_seed, &seed_len, &password_correct) !=
+        KEYSTORE_OK) {
         return false;
     }
     if (!password_correct) {
@@ -232,7 +235,7 @@ bool keystore_encrypt_and_store_seed(
     }
     uint8_t secret[32] = {0};
     UTIL_CLEANUP_32(secret);
-    if (!_stretch_password(password, secret)) {
+    if (_stretch_password(password, secret) != KEYSTORE_OK) {
         return false;
     }
 
@@ -302,7 +305,7 @@ static void _free_string(char** str)
 keystore_error_t keystore_unlock(const char* password, uint8_t* remaining_attempts_out)
 {
     if (!memory_is_seeded()) {
-        return KEYSTORE_ERR_GENERIC;
+        return KEYSTORE_ERR_UNSEEDED;
     }
     uint8_t failed_attempts = bitbox02_smarteeprom_get_unlock_attempts();
     if (failed_attempts >= MAX_UNLOCK_ATTEMPTS) {
@@ -320,8 +323,9 @@ keystore_error_t keystore_unlock(const char* password, uint8_t* remaining_attemp
     UTIL_CLEANUP_32(seed);
     size_t seed_len;
     bool password_correct = false;
-    if (!_get_and_decrypt_seed(password, seed, &seed_len, &password_correct)) {
-        return KEYSTORE_ERR_GENERIC;
+    keystore_error_t result = _get_and_decrypt_seed(password, seed, &seed_len, &password_correct);
+    if (result != KEYSTORE_OK) {
+        return result;
     }
     if (password_correct) {
         if (_is_unlocked_device) {

src/keystore.h

@@ -36,7 +36,12 @@ typedef enum {
     KEYSTORE_OK,
     KEYSTORE_ERR_INCORRECT_PASSWORD,
     KEYSTORE_ERR_MAX_ATTEMPTS_EXCEEDED,
-    KEYSTORE_ERR_GENERIC,
+    KEYSTORE_ERR_UNSEEDED,
+    KEYSTORE_ERR_MEMORY,
+    KEYSTORE_ERR_SC_KDF,
+    KEYSTORE_ERR_SEED_SIZE,
+    KEYSTORE_ERR_SALT,
+    KEYSTORE_ERR_HASH,
 } keystore_error_t;
 
 #ifdef TESTING
@@ -91,10 +96,7 @@ USE_RESULT bool keystore_create_and_store_seed(
  * If zero, the keystore is locked until the device is reset.
  * @return
  * - KEYSTORE_OK if they keystore was successfully unlocked
- * - KEYSTORE_ERR_INCORRECT_PASSWORD if the password was wrong
- * - KEYSTORE_ERR_MAX_ATTEMPTS_EXCEEDED if there were too many unsuccessful attempts. The device is
- *   reset in this case.
- * - KEYSTORE_ERR_GENERIC if there was a fatal memory error.
+ * - KEYSTORE_ERR_* if unsuccessful.
  * Only call this if memory_is_seeded() returns true.
  */
 USE_RESULT keystore_error_t keystore_unlock(const char* password, uint8_t* remaining_attempts_out);

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

@@ -14,6 +14,8 @@
 
 use crate::pb;
 
+use crate::workflow::unlock::UnlockError;
+
 #[allow(dead_code)]
 #[derive(Debug, PartialEq)]
 pub enum Error {
@@ -67,11 +69,11 @@ impl core::convert::From<crate::workflow::verify_message::Error> for Error {
     }
 }
 
-impl core::convert::From<crate::workflow::unlock::UnlockError> for Error {
-    fn from(error: crate::workflow::unlock::UnlockError) -> Self {
+impl core::convert::From<UnlockError> for Error {
+    fn from(error: UnlockError) -> Self {
         match error {
-            crate::workflow::unlock::UnlockError::UserAbort => Error::UserAbort,
-            crate::workflow::unlock::UnlockError::IncorrectPassword => Error::Generic,
+            UnlockError::UserAbort => Error::UserAbort,
+            UnlockError::IncorrectPassword | UnlockError::Generic => Error::Generic,
         }
     }
 }

src/rust/bitbox02-rust/src/workflow/unlock.rs

@@ -53,6 +53,7 @@ async fn confirm_mnemonic_passphrase(passphrase: &str) -> Result<(), confirm::Us
 pub enum UnlockError {
     UserAbort,
     IncorrectPassword,
+    Generic,
 }
 
 impl core::convert::From<super::cancel::Error> for UnlockError {
@@ -84,7 +85,11 @@ pub async fn unlock_keystore(
             status(&msg, false).await;
             Err(UnlockError::IncorrectPassword)
         }
-        _ => panic!("keystore unlock failed"),
+        Err(err) => {
+            let msg = format!("keystore unlock failed\n{:?}", err);
+            status(&msg, false).await;
+            Err(UnlockError::Generic)
+        }
     }
 }
 

src/rust/bitbox02/src/keystore.rs

@@ -35,6 +35,13 @@ pub fn is_locked() -> bool {
 pub enum Error {
     CannotUnlockBIP39,
     IncorrectPassword { remaining_attempts: u8 },
+    MaxAttemptsExceeded,
+    Unseeded,
+    Memory,
+    ScKdf,
+    Salt,
+    Hash,
+    SeedSize,
     Unknown,
 }
 
@@ -45,8 +52,13 @@ pub fn unlock(password: &SafeInputString) -> Result<(), Error> {
         keystore_error_t::KEYSTORE_ERR_INCORRECT_PASSWORD => {
             Err(Error::IncorrectPassword { remaining_attempts })
         }
-        keystore_error_t::KEYSTORE_ERR_MAX_ATTEMPTS_EXCEEDED => Err(Error::Unknown),
-        keystore_error_t::KEYSTORE_ERR_GENERIC => Err(Error::Unknown),
+        keystore_error_t::KEYSTORE_ERR_MAX_ATTEMPTS_EXCEEDED => Err(Error::MaxAttemptsExceeded),
+        keystore_error_t::KEYSTORE_ERR_UNSEEDED => Err(Error::Unseeded),
+        keystore_error_t::KEYSTORE_ERR_MEMORY => Err(Error::Memory),
+        keystore_error_t::KEYSTORE_ERR_SEED_SIZE => Err(Error::SeedSize),
+        keystore_error_t::KEYSTORE_ERR_SC_KDF => Err(Error::ScKdf),
+        keystore_error_t::KEYSTORE_ERR_SALT => Err(Error::Salt),
+        keystore_error_t::KEYSTORE_ERR_HASH => Err(Error::Hash),
     }
 }
 

test/unit-test/test_keystore.c

@@ -417,7 +417,7 @@ static void _test_keystore_unlock(void** state)
     uint8_t remaining_attempts;
 
     will_return(__wrap_memory_is_seeded, false);
-    assert_int_equal(KEYSTORE_ERR_GENERIC, keystore_unlock(PASSWORD, &remaining_attempts));
+    assert_int_equal(KEYSTORE_ERR_UNSEEDED, keystore_unlock(PASSWORD, &remaining_attempts));
     _expect_encrypt_and_store_seed();
     assert_true(keystore_encrypt_and_store_seed(_mock_seed, 32, PASSWORD));
     _expect_seeded(false);