Merge branch 'unlock'

Author: benma

CHANGELOG.md

@@ -1,6 +1,7 @@
 # Changelog
 
 ## [Unreleased]
+- Display granular error codes when unlock fails unexpectedly
 
 ## 9.6.0 [released 2021-05-20]
 - Attempt to fix flaky SD behavior

CMakeLists.txt

@@ -89,9 +89,9 @@ 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.6.0")
-set(FIRMWARE_BTC_ONLY_VERSION "v9.6.0")
-set(FIRMWARE_BITBOXBASE_VERSION "v9.6.0")
+set(FIRMWARE_VERSION "v9.6.1")
+set(FIRMWARE_BTC_ONLY_VERSION "v9.6.1")
+set(FIRMWARE_BITBOXBASE_VERSION "v9.6.1")
 set(BOOTLOADER_VERSION "v1.0.3")
 
 find_package(PythonInterp 3.6 REQUIRED)

src/keystore.c

@@ -108,16 +108,27 @@ static uint8_t* _get_bip39_seed(void)
     return _retained_bip39_seed;
 }
 
-static bool _stretch_password(const char* password, uint8_t* kdf_out)
+/**
+ * Stretch the user password using the securechip, putting the result in `kdf_out`, which must be 32
+ * bytes. `securechip_result_out`, if not NULL, will contain the error code from `securechip_kdf()`
+ * if there was a secure chip error, and 0 otherwise.
+ */
+static keystore_error_t _stretch_password(
+    const char* password,
+    uint8_t* kdf_out,
+    int* securechip_result_out)
 {
+    if (securechip_result_out != NULL) {
+        *securechip_result_out = 0;
+    }
     uint8_t password_salted_hashed[32] = {0};
     UTIL_CLEANUP_32(password_salted_hashed);
     if (!salt_hash_data(
             (const uint8_t*)password,
             strlen(password),
             "keystore_seed_access_in",
             password_salted_hashed)) {
-        return false;
+        return KEYSTORE_ERR_SALT;
     }
 
     uint8_t kdf_in[32] = {0};
@@ -126,14 +137,22 @@ 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;
+    int securechip_result = securechip_kdf(SECURECHIP_SLOT_ROLLKEY, kdf_in, 32, kdf_out);
+    if (securechip_result) {
+        if (securechip_result_out != NULL) {
+            *securechip_result_out = securechip_result;
+        }
+        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;
+        securechip_result = securechip_kdf(SECURECHIP_SLOT_KDF, kdf_in, 32, kdf_out);
+        if (securechip_result) {
+            if (securechip_result_out != NULL) {
+                *securechip_result_out = securechip_result;
+            }
+            return KEYSTORE_ERR_SC_KDF;
         }
     }
 
@@ -142,50 +161,59 @@ 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(
+
+/**
+ * Retrieves the encrypted seed and attempts to decrypt it using the password.
+ *
+ * `securechip_result_out`, if not NULL, will contain the error code from `securechip_kdf()` if
+ * there was a secure chip error, and 0 otherwise.
+ */
+static keystore_error_t _get_and_decrypt_seed(
     const char* password,
     uint8_t* decrypted_seed_out,
     size_t* decrypted_seed_len_out,
-    bool* password_correct_out)
+    int* securechip_result_out)
 {
     uint8_t encrypted_seed_and_hmac[96];
     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, securechip_result_out);
+    if (result != KEYSTORE_OK) {
+        return result;
     }
     if (encrypted_len < 49) {
         Abort("_get_and_decrypt_seed: underflow / zero size");
     }
     size_t decrypted_len = encrypted_len - 48;
     uint8_t decrypted[decrypted_len];
-    *password_correct_out = cipher_aes_hmac_decrypt(
+    bool password_correct = cipher_aes_hmac_decrypt(
         encrypted_seed_and_hmac, encrypted_len, decrypted, &decrypted_len, secret);
-    if (*password_correct_out) {
-        if (!_validate_seed_length(decrypted_len)) {
-            util_zero(decrypted, sizeof(decrypted));
-            return false;
-        }
-        *decrypted_seed_len_out = decrypted_len;
-        memcpy(decrypted_seed_out, decrypted, decrypted_len);
+    if (!password_correct) {
+        return KEYSTORE_ERR_INCORRECT_PASSWORD;
     }
-    util_zero(decrypted, sizeof(decrypted));
-    return true;
+    if (!_validate_seed_length(decrypted_len)) {
+        util_zero(decrypted, sizeof(decrypted));
+        return KEYSTORE_ERR_SEED_SIZE;
+    }
+    *decrypted_seed_len_out = decrypted_len;
+    memcpy(decrypted_seed_out, decrypted, decrypted_len);
+
+    return KEYSTORE_OK;
 }
 
 static bool _verify_seed(
@@ -196,11 +224,7 @@ static bool _verify_seed(
     uint8_t decrypted_seed[KEYSTORE_MAX_SEED_LENGTH] = {0};
     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)) {
-        return false;
-    }
-    if (!password_correct) {
+    if (_get_and_decrypt_seed(password, decrypted_seed, &seed_len, NULL) != KEYSTORE_OK) {
         return false;
     }
     if (expected_seed_len != seed_len) {
@@ -232,7 +256,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, NULL) != KEYSTORE_OK) {
         return false;
     }
 
@@ -299,10 +323,13 @@ static void _free_string(char** str)
     wally_free_string(*str);
 }
 
-keystore_error_t keystore_unlock(const char* password, uint8_t* remaining_attempts_out)
+keystore_error_t keystore_unlock(
+    const char* password,
+    uint8_t* remaining_attempts_out,
+    int* securechip_result_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) {
@@ -319,11 +346,12 @@ keystore_error_t keystore_unlock(const char* password, uint8_t* remaining_attemp
     uint8_t seed[KEYSTORE_MAX_SEED_LENGTH] = {0};
     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, securechip_result_out);
+    if (result != KEYSTORE_OK && result != KEYSTORE_ERR_INCORRECT_PASSWORD) {
+        return result;
     }
-    if (password_correct) {
+    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)) {
@@ -346,7 +374,7 @@ keystore_error_t keystore_unlock(const char* password, uint8_t* remaining_attemp
     }
 
     *remaining_attempts_out = MAX_UNLOCK_ATTEMPTS - failed_attempts;
-    return password_correct ? KEYSTORE_OK : KEYSTORE_ERR_INCORRECT_PASSWORD;
+    return result;
 }
 
 bool keystore_unlock_bip39(const char* mnemonic_passphrase)

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
@@ -89,15 +94,15 @@ USE_RESULT bool keystore_create_and_store_seed(
  * If it is false, the keystore is not unlocked.
  * @param[out] remaining_attempts_out will have the number of remaining attempts.
  * If zero, the keystore is locked until the device is reset.
+ * @param[out] securechip_result_out, if not NULL, will contain the error code from
+ * `securechip_kdf()` if there was a secure chip error, and 0 otherwise.
  * @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);
+USE_RESULT keystore_error_t
+keystore_unlock(const char* password, uint8_t* remaining_attempts_out, int* securechip_result_out);
 
 /** Unlocks the bip39 seed.
  * @param[in] mnemonic_passphrase bip39 passphrase used in the derivation. Use the

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,18 +35,38 @@ pub fn is_locked() -> bool {
 pub enum Error {
     CannotUnlockBIP39,
     IncorrectPassword { remaining_attempts: u8 },
+    MaxAttemptsExceeded,
+    Unseeded,
+    Memory,
+    // Securechip error with the error code from securechip.c
+    ScKdf(i32),
+    Salt,
+    Hash,
+    SeedSize,
     Unknown,
 }
 
 pub fn unlock(password: &SafeInputString) -> Result<(), Error> {
     let mut remaining_attempts: u8 = 0;
-    match unsafe { bitbox02_sys::keystore_unlock(password.as_cstr(), &mut remaining_attempts) } {
+    let mut securechip_result: i32 = 0;
+    match unsafe {
+        bitbox02_sys::keystore_unlock(
+            password.as_cstr(),
+            &mut remaining_attempts,
+            &mut securechip_result,
+        )
+    } {
         keystore_error_t::KEYSTORE_OK => Ok(()),
         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(securechip_result)),
+        keystore_error_t::KEYSTORE_ERR_SALT => Err(Error::Salt),
+        keystore_error_t::KEYSTORE_ERR_HASH => Err(Error::Hash),
     }
 }
 

src/securechip/securechip.c

@@ -513,18 +513,18 @@ bool securechip_update_keys(void)
     return _update_kdf_key() == ATCA_SUCCESS;
 }
 
-bool securechip_kdf(securechip_slot_t slot, const uint8_t* msg, size_t len, uint8_t* kdf_out)
+int securechip_kdf(securechip_slot_t slot, const uint8_t* msg, size_t len, uint8_t* kdf_out)
 {
     if (len > 127 || (slot != SECURECHIP_SLOT_ROLLKEY && slot != SECURECHIP_SLOT_KDF)) {
-        return false;
+        return SC_ERR_INVALID_ARGS;
     }
     if (msg == kdf_out) {
-        return false;
+        return SC_ERR_INVALID_ARGS;
     }
 
     ATCA_STATUS result = _authorize_key();
     if (result != ATCA_SUCCESS) {
-        return false;
+        return result;
     }
 
     uint8_t nonce_out[32] = {0};
@@ -555,7 +555,7 @@ bool securechip_kdf(securechip_slot_t slot, const uint8_t* msg, size_t len, uint
     /*     kdf_out, */
     /*     nonce_out); */
     if (result != ATCA_SUCCESS) {
-        return false;
+        return result;
     }
     // Output is encrypted with the io protection key.
     uint8_t io_protection_key[32] = {0};
@@ -567,11 +567,7 @@ bool securechip_kdf(securechip_slot_t slot, const uint8_t* msg, size_t len, uint
         .data = kdf_out,
         .data_size = 32,
     };
-    result = atcah_io_decrypt(&io_dec_params);
-    if (result != ATCA_SUCCESS) {
-        return false;
-    }
-    return true;
+    return atcah_io_decrypt(&io_dec_params);
 }
 
 bool securechip_gen_attestation_key(uint8_t* pubkey_out)