Adress review comments

- Modified change_password in bitbox02.py to raise BitBox02Exception on failure
- The api call is now only allowed if the device is initialized and unlocked
- Extra check in removed in process(...) in change_password.rs
- Changed ui status to show more info when re_encrypt_seed fails
- corrected error type in re_encrypt_seed(...) in keystore.rs
- Added retain_bip39_seed(...) to re-use code
- Added unit tests for re_encrypt_seed(...) and retain_bip39_seed(...)
- Removed outdated comment
- Changed check in re_encrypt_seed(...) to is_seeded() instead of is_initialzed()
- Adapted change_password.rs due to merge conflict

Author: cedwies

py/bitbox02/bitbox02/bitbox02/bitbox02.py

@@ -212,22 +212,16 @@ def set_password(self, entropy_size: int = 32) -> bool:
             raise
         return True
 
-    def change_password(self) -> bool:
+    def change_password(self) -> None:
         """
         Returns True if the user changes password successfully by
         unlocking with old password and entering and confirming new password.
-        Returns False otherwise.
+        Raises a Bitbox02Exception on failure.
         """
         # pylint: disable=no-member
         request = hww.Request()
         request.change_password.CopyFrom(bitbox02_system.ChangePasswordRequest())
-        try:
-            self._msg_query(request, expected_response="success")
-        except Bitbox02Exception as err:
-            if err.code == ERR_GENERIC:
-                return False
-            raise
-        return True
+        self._msg_query(request, expected_response="success")
 
     def create_backup(self) -> bool:
         """

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

@@ -139,7 +139,7 @@ fn can_call(request: &Request) -> bool {
         Request::SetPassword(_) | Request::RestoreBackup(_) | Request::RestoreFromMnemonic(_) => {
             matches!(state, State::Uninitialized | State::Seeded)
         }
-        Request::CreateBackup(_) | Request::ShowMnemonic(_) | Request::ChangePassword(_) => {
+        Request::CreateBackup(_) | Request::ShowMnemonic(_) => {
             matches!(state, State::Seeded | State::InitializedAndUnlocked)
         }
         Request::Fingerprint(_)
@@ -152,7 +152,8 @@ fn can_call(request: &Request) -> bool {
         | Request::Eth(_)
         | Request::Reset(_)
         | Request::Cardano(_)
-        | Request::Bip85(_) => {
+        | Request::Bip85(_)
+        | Request::ChangePassword(_) => {
             matches!(state, State::InitializedAndUnlocked)
         }
         // These are streamed asynchronously using the `next_request()` primitive in

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

@@ -8,19 +8,14 @@ use crate::keystore;
 use crate::workflow::{password, unlock};
 
 pub async fn process(hal: &mut impl crate::hal::Hal) -> Result<Response, Error> {
-    // Must be initialized
-    if !bitbox02::memory::is_initialized() {
-        return Err(Error::Generic);
-    }
-
     // Unlock with old password
     let seed = unlock::unlock_keystore(hal, "Unlock device", unlock::CanCancel::Yes).await?;
     // Enter and confirm new password
     let new_password = password::enter_twice(hal).await?;
 
     // Re-encrypt seed with new password
-    if keystore::re_encrypt_seed(hal, &seed, &new_password).is_err() {
-        hal.ui().status("Could not change\npassword", false).await;
+    if let Err(err) = keystore::re_encrypt_seed(hal, &seed, &new_password) {
+        hal.ui().status(&format!("Error\n{:?}", err), false).await;
         return Err(Error::Generic);
     }
 
@@ -73,7 +68,7 @@ mod tests {
             }
         }));
         // reset the chip counter
-        bitbox02::securechip::fake_event_counter_reset();
+        hal.securechip.event_counter_reset();
         // call process
         let result = block_on(process(&mut hal));
         // assert success
@@ -87,9 +82,9 @@ mod tests {
                 success: true,
             }]
         );
-        drop(hal);
 
-        let securechip_events = bitbox02::securechip::fake_event_counter();
+        let securechip_events = hal.securechip.get_event_counter();
+        drop(hal);
         // We expect 14 secure chip events. This is intentionally brittle to catch
         // unintended changes in the number of securechip operations during password change.
         // If this fails after a legitimate change, update the expected count.
@@ -113,18 +108,6 @@ mod tests {
         );
     }
 
-    // Test that we fail if the device is not initialized
-    #[test]
-    fn test_process_device_not_initialized() {
-        mock_memory();
-        let mut hal = TestingHal::new();
-        assert!(!bitbox02::memory::is_initialized());
-        bitbox02::securechip::fake_event_counter_reset();
-        assert_eq!(block_on(process(&mut hal)), Err(Error::Generic));
-        assert!(hal.ui.screens.is_empty());
-        assert_eq!(bitbox02::securechip::fake_event_counter(), 0);
-    }
-
     // Test that we fail if the unlock fails
     #[test]
     fn test_process_unlock_failure() {
@@ -146,7 +129,7 @@ mod tests {
             Ok("wrong_password".into())
         }));
 
-        bitbox02::securechip::fake_event_counter_reset();
+        hal.securechip.event_counter_reset();
         let result = block_on(process(&mut hal));
 
         assert_eq!(result, Err(Error::Generic));
@@ -158,7 +141,7 @@ mod tests {
             }]
         );
         // We expect 5 secure chip events (sensitive to code changes)
-        assert_eq!(bitbox02::securechip::fake_event_counter(), 5);
+        assert_eq!(hal.securechip.get_event_counter(), 5);
 
         drop(hal);
         assert_eq!(prompt_counter, 1);

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

@@ -192,6 +192,15 @@ fn retain_seed(random: &mut impl crate::hal::Random, seed: &[u8]) -> Result<(),
     Ok(())
 }
 
+fn retain_bip39_seed(random: &mut impl crate::hal::Random, bip39_seed: &[u8]) -> Result<(), Error> {
+    RETAINED_BIP39_SEED.write(Some(RetainedEncryptedBuffer::from_buffer(
+        random,
+        bip39_seed,
+        "keystore_retained_bip39_seed_access",
+    )?));
+    Ok(())
+}
+
 // Internal helper to encrypt a seed with a password and store it on flash
 // For setting password (initilization) we clear memory first, to guarantee clean RAM
 // For changing password we do not clear memory to retain the BIP39 seed
@@ -243,7 +252,6 @@ pub fn encrypt_and_store_seed(
     if bitbox02::memory::is_initialized() {
         return Err(Error::Memory);
     }
-    // We are in setup phase, so we clear memory first to guarantee clean RAM
     encrypt_and_store_seed_internal(hal, seed, password)
 }
 
@@ -253,24 +261,20 @@ pub fn re_encrypt_seed(
     seed: &[u8],
     new_password: &str,
 ) -> Result<(), Error> {
-    if !bitbox02::memory::is_initialized() {
+    if !bitbox02::memory::is_seeded() {
         return Err(Error::Unseeded);
     }
 
     // Store the bip39 seed before re-encryption because:
     // 1. The secure chip's internal keys are regenerated with the new password
     // 2. encrypt_and_store_seed_internal calls lock() which clears RAM including BIP39 seed
     // 3. We want to avoid forcing the user to re-enter their BIP39 passphrase
-    let bip39_seed = copy_bip39_seed().map_err(|_| Error::CannotUnlockBIP39)?;
+    let bip39_seed = copy_bip39_seed().map_err(|_| Error::Memory)?;
 
     encrypt_and_store_seed_internal(hal, seed, new_password)?;
 
     // Re-retain the bip39 seed
-    RETAINED_BIP39_SEED.write(Some(RetainedEncryptedBuffer::from_buffer(
-        hal.random(),
-        bip39_seed.as_slice(),
-        "keystore_retained_bip39_seed_access",
-    )?));
+    retain_bip39_seed(hal.random(), bip39_seed.as_slice())?;
 
     Ok(())
 }
@@ -377,11 +381,7 @@ pub async fn unlock_bip39(
         return Err(Error::Memory);
     }
 
-    RETAINED_BIP39_SEED.write(Some(RetainedEncryptedBuffer::from_buffer(
-        random,
-        bip39_seed.as_slice(),
-        "keystore_retained_bip39_seed_access",
-    )?));
+    retain_bip39_seed(random, bip39_seed.as_slice())?;
 
     // Store root fingerprint.
     ROOT_FINGERPRINT.write(Some(root_fingerprint));
@@ -880,6 +880,193 @@ mod tests {
         }
     }
 
+    #[test]
+    fn test_re_encrypt_seed() {
+        mock_memory();
+        lock();
+
+        let mut mock_hal = TestingHal::new();
+        let seed = hex!("cb33c20cea62a5c277527e2002da82e6e2b37450a755143a540a54cea8da9044");
+
+        // Try to re-encrypt without seeding first
+        assert!(matches!(
+            re_encrypt_seed(&mut mock_hal, &seed, "new_password"),
+            Err(Error::Unseeded)
+        ));
+    }
+
+    #[test]
+    fn test_re_encrypt_seed_changes_password() {
+        mock_memory();
+        lock();
+
+        let mut mock_hal = TestingHal::new();
+        let seed = hex!("cb33c20cea62a5c277527e2002da82e6e2b37450a755143a540a54cea8da9044");
+
+        // Step 1: Set up device with initial password
+        assert!(encrypt_and_store_seed(&mut mock_hal, &seed, "old_password").is_ok());
+
+        // Step 2: Unlock with initial password and set up BIP39
+        let unlocked_seed = unlock(&mut mock_hal, "old_password").unwrap();
+        assert_eq!(unlocked_seed.as_slice(), seed.as_slice());
+
+        let mut random = crate::hal::testing::TestingRandom::new();
+        assert!(block_on(unlock_bip39(&mut random, &seed, "", async || {})).is_ok());
+
+        // Step 3: Re-encrypt with new password
+        assert!(re_encrypt_seed(&mut mock_hal, &seed, "new_password").is_ok());
+
+        // Step 4: Lock and verify old password no longer works
+        lock();
+        assert!(matches!(
+            unlock(&mut mock_hal, "old_password"),
+            Err(Error::IncorrectPassword)
+        ));
+
+        // Step 5: Verify new password works
+        let unlocked_seed_new = unlock(&mut mock_hal, "new_password").unwrap();
+        assert_eq!(unlocked_seed_new.as_slice(), seed.as_slice());
+    }
+
+    #[test]
+    fn test_re_encrypt_seed_preserves_seed() {
+        mock_memory();
+        lock();
+
+        let mut mock_hal = TestingHal::new();
+        let seed = hex!("cb33c20cea62a5c277527e2002da82e6e2b37450a755143a540a54cea8da9044");
+
+        // Initial setup
+        assert!(encrypt_and_store_seed(&mut mock_hal, &seed, "password1").is_ok());
+        let seed_copy1 = copy_seed().unwrap();
+
+        // Re-encrypt multiple times
+        for (i, new_password) in ["password2", "password3", "password4"].iter().enumerate() {
+            // Need to be unlocked with BIP39 before re-encrypting
+            if i > 0 {
+                let prev_password = match i {
+                    1 => "password2",
+                    2 => "password3",
+                    _ => unreachable!(),
+                };
+                lock();
+                unlock(&mut mock_hal, prev_password).unwrap();
+            }
+
+            let mut random = crate::hal::testing::TestingRandom::new();
+            assert!(block_on(unlock_bip39(&mut random, &seed, "", async || {})).is_ok());
+            assert!(re_encrypt_seed(&mut mock_hal, &seed, new_password).is_ok());
+
+            // Seed should remain the same
+            assert_eq!(copy_seed().unwrap().as_slice(), seed_copy1.as_slice());
+        }
+    }
+
+    #[test]
+    fn test_re_encrypt_seed_preserves_bip39_seed() {
+        mock_memory();
+        lock();
+
+        let mut mock_hal = TestingHal::new();
+        let seed = hex!("cb33c20cea62a5c277527e2002da82e6e2b37450a755143a540a54cea8da9044");
+        let passphrase = "my_secret_passphrase";
+
+        // Initial setup with password
+        assert!(encrypt_and_store_seed(&mut mock_hal, &seed, "old_password").is_ok());
+        unlock(&mut mock_hal, "old_password").unwrap();
+
+        // Unlock BIP39 with passphrase
+        let mut random = crate::hal::testing::TestingRandom::new();
+        assert!(block_on(unlock_bip39(&mut random, &seed, passphrase, async || {})).is_ok());
+
+        // Store the BIP39 seed before password change
+        let bip39_seed_before = copy_bip39_seed().unwrap();
+
+        // Re-encrypt with new password
+        assert!(re_encrypt_seed(&mut mock_hal, &seed, "new_password").is_ok());
+
+        // BIP39 seed should still be available (not cleared by lock() inside re_encrypt_seed)
+        let bip39_seed_after = copy_bip39_seed().unwrap();
+        assert_eq!(bip39_seed_before.as_slice(), bip39_seed_after.as_slice());
+
+        // Lock and unlock with new password
+        lock();
+        unlock(&mut mock_hal, "new_password").unwrap();
+
+        // BIP39 should be locked now (we'd need to call unlock_bip39 again)
+        assert!(copy_bip39_seed().is_err());
+    }
+
+    #[test]
+    fn test_re_encrypt_seed_invalid_seed_size() {
+        mock_memory();
+        lock();
+
+        let mut mock_hal = TestingHal::new();
+        let seed = hex!("cb33c20cea62a5c277527e2002da82e6e2b37450a755143a540a54cea8da9044");
+
+        // Initial setup
+        assert!(encrypt_and_store_seed(&mut mock_hal, &seed, "password").is_ok());
+        unlock(&mut mock_hal, "password").unwrap();
+
+        let mut random = crate::hal::testing::TestingRandom::new();
+        assert!(block_on(unlock_bip39(&mut random, &seed, "", async || {})).is_ok());
+
+        // Try to re-encrypt with invalid seed size
+        assert!(matches!(
+            re_encrypt_seed(&mut mock_hal, &[0u8; 31], "new_password"),
+            Err(Error::SeedSize)
+        ));
+    }
+
+    #[test]
+    fn test_retain_bip39_seed() {
+        mock_memory();
+        lock();
+
+        let mut random = crate::hal::testing::TestingRandom::new();
+        let bip39_seed = hex!(
+            "2b3c63de86f0f2b13cc6a36c1ba2314fbc1b40c77ab9cb64e96ba4d5c62fc204748ca6626a9f035e7d431bce8c9210ec0bdffc2e7db873dee56c8ac2153eee9a"
+        );
+
+        // Before retention, should not be available
+        assert!(copy_bip39_seed().is_err());
+
+        // Retain the BIP39 seed
+        assert!(retain_bip39_seed(&mut random, &bip39_seed).is_ok());
+
+        // Should now be available
+        let retrieved = copy_bip39_seed().unwrap();
+        assert_eq!(retrieved.as_slice(), bip39_seed.as_slice());
+    }
+
+    #[test]
+    fn test_retain_bip39_seed_overwrites_previous() {
+        mock_memory();
+        lock();
+        let mut random = crate::hal::testing::TestingRandom::new();
+        let bip39_seed1 = hex!(
+            "1111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111"
+        );
+        let bip39_seed2 = hex!(
+            "2222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222222"
+        );
+
+        // Retain first seed
+        assert!(retain_bip39_seed(&mut random, &bip39_seed1).is_ok());
+        assert_eq!(
+            copy_bip39_seed().unwrap().as_slice(),
+            bip39_seed1.as_slice()
+        );
+
+        // Retain second seed (should overwrite)
+        assert!(retain_bip39_seed(&mut random, &bip39_seed2).is_ok());
+        assert_eq!(
+            copy_bip39_seed().unwrap().as_slice(),
+            bip39_seed2.as_slice()
+        );
+    }
+
     // This tests that you can create a keystore, unlock it, and then do this again. This is an
     // expected workflow for when the wallet setup process is restarted after seeding and unlocking,
     // but before creating a backup, in which case a new seed is created.