chore(core): remove double check when changing pin

core/embed/rust/src/trezorhal/storage.rs

@@ -134,20 +134,12 @@ pub fn unlock(pin: &str, salt: Option<&ExternalSalt>) -> bool {
 /// Change PIN and/or external salt.
 /// Returns true if the PIN + salt combination is correct and the change was
 /// successful.
-pub fn change_pin(
-    old_pin: &str,
-    new_pin: &str,
-    old_salt: Option<&ExternalSalt>,
-    new_salt: Option<&ExternalSalt>,
-) -> bool {
+pub fn change_pin(new_pin: &str, new_salt: Option<&ExternalSalt>) -> bool {
     ffi::sectrue
         == unsafe {
             ffi::storage_change_pin(
-                old_pin.as_ptr() as *const _,
-                old_pin.len(),
                 new_pin.as_ptr() as *const _,
                 new_pin.len(),
-                old_salt.map(|s| s.as_ptr()).unwrap_or(ptr::null()),
                 new_salt.map(|s| s.as_ptr()).unwrap_or(ptr::null()),
             )
         }
@@ -333,8 +325,8 @@ mod tests {
 
         assert!(!unlock("1234", None));
 
-        //unlock("", None);
-        assert!(change_pin("", "1234", None, None));
+        unlock("", None);
+        assert!(change_pin("1234", None));
         assert!(has_pin());
 
         lock();

core/embed/sys/smcall/stm32/smcall_dispatch.c

@@ -241,14 +241,10 @@ __attribute((no_stack_protector)) void smcall_handler(uint32_t *args,
     } break;
 
     case SMCALL_STORAGE_CHANGE_PIN: {
-      const uint8_t *oldpin = (const uint8_t *)args[0];
-      size_t oldpin_len = args[1];
-      const uint8_t *newpin = (const uint8_t *)args[2];
-      size_t newpin_len = args[3];
-      const uint8_t *old_ext_salt = (const uint8_t *)args[4];
-      const uint8_t *new_ext_salt = (const uint8_t *)args[5];
-      args[0] = storage_change_pin__verified(
-          oldpin, oldpin_len, newpin, newpin_len, old_ext_salt, new_ext_salt);
+      const uint8_t *newpin = (const uint8_t *)args[0];
+      size_t newpin_len = args[1];
+      const uint8_t *new_ext_salt = (const uint8_t *)args[2];
+      args[0] = storage_change_pin__verified(newpin, newpin_len, new_ext_salt);
     } break;
 
     case SMCALL_STORAGE_ENSURE_NOT_WIPE_CODE: {

core/embed/sys/smcall/stm32/smcall_stubs.c

@@ -247,12 +247,9 @@ uint32_t storage_get_pin_rem(void) {
   return smcall_invoke0(SMCALL_STORAGE_GET_PIN_REM);
 }
 
-secbool storage_change_pin(const uint8_t *oldpin, size_t oldpin_len,
-                           const uint8_t *newpin, size_t newpin_len,
-                           const uint8_t *old_ext_salt,
+secbool storage_change_pin(const uint8_t *newpin, size_t newpin_len,
                            const uint8_t *new_ext_salt) {
-  return (secbool)smcall_invoke6((uint32_t)oldpin, oldpin_len, (uint32_t)newpin,
-                                 newpin_len, (uint32_t)old_ext_salt,
+  return (secbool)smcall_invoke3((uint32_t)newpin, newpin_len,
                                  (uint32_t)new_ext_salt,
                                  SMCALL_STORAGE_CHANGE_PIN);
 }

core/embed/sys/smcall/stm32/smcall_verifiers.c

@@ -279,28 +279,17 @@ secbool storage_unlock__verified(const uint8_t *pin, size_t pin_len,
   return secfalse;
 }
 
-secbool storage_change_pin__verified(const uint8_t *oldpin, size_t oldpin_len,
-                                     const uint8_t *newpin, size_t newpin_len,
-                                     const uint8_t *old_ext_salt,
+secbool storage_change_pin__verified(const uint8_t *newpin, size_t newpin_len,
                                      const uint8_t *new_ext_salt) {
-  if (!probe_read_access(oldpin, oldpin_len)) {
-    goto access_violation;
-  }
-
   if (!probe_read_access(newpin, newpin_len)) {
     goto access_violation;
   }
 
-  if (!probe_read_access(old_ext_salt, EXTERNAL_SALT_SIZE)) {
-    goto access_violation;
-  }
-
   if (!probe_read_access(new_ext_salt, EXTERNAL_SALT_SIZE)) {
     goto access_violation;
   }
 
-  return storage_change_pin(oldpin, oldpin_len, newpin, newpin_len,
-                            old_ext_salt, new_ext_salt);
+  return storage_change_pin(newpin, newpin_len, new_ext_salt);
 
 access_violation:
   apptask_access_violation();

core/embed/sys/smcall/stm32/smcall_verifiers.h

@@ -87,9 +87,7 @@ void storage_setup__verified(PIN_UI_WAIT_CALLBACK callback);
 secbool storage_unlock__verified(const uint8_t *pin, size_t pin_len,
                                  const uint8_t *ext_salt);
 
-secbool storage_change_pin__verified(const uint8_t *oldpin, size_t oldpin_len,
-                                     const uint8_t *newpin, size_t newpin_len,
-                                     const uint8_t *old_ext_salt,
+secbool storage_change_pin__verified(const uint8_t *newpin, size_t newpin_len,
                                      const uint8_t *new_ext_salt);
 
 void storage_ensure_not_wipe_code__verified(const uint8_t *pin, size_t pin_len);

core/embed/sys/syscall/stm32/syscall_dispatch.c

@@ -483,14 +483,10 @@ __attribute((no_stack_protector)) void syscall_handler(uint32_t *args,
     } break;
 
     case SYSCALL_STORAGE_CHANGE_PIN: {
-      const uint8_t *oldpin = (const uint8_t *)args[0];
-      size_t oldpin_len = args[1];
-      const uint8_t *newpin = (const uint8_t *)args[2];
-      size_t newpin_len = args[3];
-      const uint8_t *old_ext_salt = (const uint8_t *)args[4];
-      const uint8_t *new_ext_salt = (const uint8_t *)args[5];
-      args[0] = storage_change_pin__verified(
-          oldpin, oldpin_len, newpin, newpin_len, old_ext_salt, new_ext_salt);
+      const uint8_t *newpin = (const uint8_t *)args[0];
+      size_t newpin_len = args[1];
+      const uint8_t *new_ext_salt = (const uint8_t *)args[2];
+      args[0] = storage_change_pin__verified(newpin, newpin_len, new_ext_salt);
     } break;
 
     case SYSCALL_STORAGE_ENSURE_NOT_WIPE_CODE: {

core/embed/sys/syscall/stm32/syscall_stubs.c

@@ -482,14 +482,11 @@ uint32_t storage_get_pin_rem(void) {
   return syscall_invoke0(SYSCALL_STORAGE_GET_PIN_REM);
 }
 
-secbool storage_change_pin(const uint8_t *oldpin, size_t oldpin_len,
-                           const uint8_t *newpin, size_t newpin_len,
-                           const uint8_t *old_ext_salt,
+secbool storage_change_pin(const uint8_t *newpin, size_t newpin_len,
                            const uint8_t *new_ext_salt) {
-  return (secbool)syscall_invoke6(
-      (uint32_t)oldpin, oldpin_len, (uint32_t)newpin, newpin_len,
-      (uint32_t)old_ext_salt, (uint32_t)new_ext_salt,
-      SYSCALL_STORAGE_CHANGE_PIN);
+  return (secbool)syscall_invoke3((uint32_t)newpin, newpin_len,
+                                  (uint32_t)new_ext_salt,
+                                  SYSCALL_STORAGE_CHANGE_PIN);
 }
 
 void storage_ensure_not_wipe_code(const uint8_t *pin, size_t pin_len) {

core/embed/sys/syscall/stm32/syscall_verifiers.c

@@ -538,28 +538,17 @@ secbool storage_unlock__verified(const uint8_t *pin, size_t pin_len,
   return secfalse;
 }
 
-secbool storage_change_pin__verified(const uint8_t *oldpin, size_t oldpin_len,
-                                     const uint8_t *newpin, size_t newpin_len,
-                                     const uint8_t *old_ext_salt,
+secbool storage_change_pin__verified(const uint8_t *newpin, size_t newpin_len,
                                      const uint8_t *new_ext_salt) {
-  if (!probe_read_access(oldpin, oldpin_len)) {
-    goto access_violation;
-  }
-
   if (!probe_read_access(newpin, newpin_len)) {
     goto access_violation;
   }
 
-  if (!probe_read_access(old_ext_salt, EXTERNAL_SALT_SIZE)) {
-    goto access_violation;
-  }
-
   if (!probe_read_access(new_ext_salt, EXTERNAL_SALT_SIZE)) {
     goto access_violation;
   }
 
-  return storage_change_pin(oldpin, oldpin_len, newpin, newpin_len,
-                            old_ext_salt, new_ext_salt);
+  return storage_change_pin(newpin, newpin_len, new_ext_salt);
 
 access_violation:
   apptask_access_violation();

core/embed/sys/syscall/stm32/syscall_verifiers.h

@@ -143,9 +143,7 @@ void storage_setup__verified(PIN_UI_WAIT_CALLBACK callback);
 secbool storage_unlock__verified(const uint8_t *pin, size_t pin_len,
                                  const uint8_t *ext_salt);
 
-secbool storage_change_pin__verified(const uint8_t *oldpin, size_t oldpin_len,
-                                     const uint8_t *newpin, size_t newpin_len,
-                                     const uint8_t *old_ext_salt,
+secbool storage_change_pin__verified(const uint8_t *newpin, size_t newpin_len,
                                      const uint8_t *new_ext_salt);
 
 void storage_ensure_not_wipe_code__verified(const uint8_t *pin, size_t pin_len);

core/embed/upymod/modtrezorconfig/modtrezorconfig.c

@@ -152,48 +152,35 @@ STATIC MP_DEFINE_CONST_FUN_OBJ_0(mod_trezorconfig_get_pin_rem_obj,
                                  mod_trezorconfig_get_pin_rem);
 
 /// def change_pin(
-///     oldpin: str,
 ///     newpin: str,
-///     old_ext_salt: AnyBytes | None,
 ///     new_ext_salt: AnyBytes | None,
 /// ) -> bool:
 ///     """
 ///     Change PIN and external salt. Returns True on success, False on failure.
+///     Has to be run with unlocked storage.
 ///     """
 STATIC mp_obj_t mod_trezorconfig_change_pin(size_t n_args,
                                             const mp_obj_t *args) {
-  mp_buffer_info_t oldpin = {0};
-  mp_get_buffer_raise(args[0], &oldpin, MP_BUFFER_READ);
-
   mp_buffer_info_t newpin = {0};
-  mp_get_buffer_raise(args[1], &newpin, MP_BUFFER_READ);
+  mp_get_buffer_raise(args[0], &newpin, MP_BUFFER_READ);
 
   mp_buffer_info_t ext_salt_b = {0};
-  const uint8_t *old_ext_salt = NULL;
-  if (args[2] != mp_const_none) {
-    mp_get_buffer_raise(args[2], &ext_salt_b, MP_BUFFER_READ);
-    if (ext_salt_b.len != EXTERNAL_SALT_SIZE)
-      mp_raise_msg(&mp_type_ValueError,
-                   MP_ERROR_TEXT("Invalid length of external salt."));
-    old_ext_salt = ext_salt_b.buf;
-  }
   const uint8_t *new_ext_salt = NULL;
-  if (args[3] != mp_const_none) {
-    mp_get_buffer_raise(args[3], &ext_salt_b, MP_BUFFER_READ);
+  if (args[1] != mp_const_none) {
+    mp_get_buffer_raise(args[1], &ext_salt_b, MP_BUFFER_READ);
     if (ext_salt_b.len != EXTERNAL_SALT_SIZE)
       mp_raise_msg(&mp_type_ValueError,
                    MP_ERROR_TEXT("Invalid length of external salt."));
     new_ext_salt = ext_salt_b.buf;
   }
 
-  if (sectrue != storage_change_pin(oldpin.buf, oldpin.len, newpin.buf,
-                                    newpin.len, old_ext_salt, new_ext_salt)) {
+  if (sectrue != storage_change_pin(newpin.buf, newpin.len, new_ext_salt)) {
     return mp_const_false;
   }
   return mp_const_true;
 }
-STATIC MP_DEFINE_CONST_FUN_OBJ_VAR_BETWEEN(mod_trezorconfig_change_pin_obj, 4,
-                                           4, mod_trezorconfig_change_pin);
+STATIC MP_DEFINE_CONST_FUN_OBJ_VAR_BETWEEN(mod_trezorconfig_change_pin_obj, 2,
+                                           2, mod_trezorconfig_change_pin);
 
 /// def ensure_not_wipe_code(pin: str) -> None:
 ///     """

core/mocks/generated/trezorconfig.pyi

@@ -60,13 +60,12 @@ def get_pin_rem() -> int:
 
 # upymod/modtrezorconfig/modtrezorconfig.c
 def change_pin(
-    oldpin: str,
     newpin: str,
-    old_ext_salt: AnyBytes | None,
     new_ext_salt: AnyBytes | None,
 ) -> bool:
     """
     Change PIN and external salt. Returns True on success, False on failure.
+    Has to be run with unlocked storage.
     """
 
 

core/src/apps/debug/load_device.py

@@ -70,6 +70,6 @@ async def load_device(msg: LoadDevice) -> Success:
     storage_device.set_passphrase_enabled(bool(msg.passphrase_protection))
     storage_device.set_label(msg.label or "")
     if msg.pin:
-        config.change_pin("", msg.pin, None, None)
+        config.change_pin(msg.pin, None)
 
     return Success(message="Device loaded")

core/src/apps/management/change_pin.py

@@ -38,8 +38,8 @@ async def change_pin(msg: ChangePin) -> Success:
     # get old pin
     curpin, salt = await request_pin_and_sd_salt(TR.pin__enter)
 
-    # if changing pin, pre-check the entered pin before getting new pin
-    if curpin and not msg.remove:
+    # check the entered pin before getting new pin
+    if curpin:
         if not config.check_pin(curpin, salt):
             await error_pin_invalid()
 
@@ -50,7 +50,7 @@ async def change_pin(msg: ChangePin) -> Success:
         newpin = ""
 
     # write into storage
-    if not config.change_pin(curpin, newpin, salt, salt):
+    if not config.change_pin(newpin, salt):
         if newpin:
             await error_pin_matches_wipe_code()
         else:

core/src/apps/management/recovery_device/__init__.py

@@ -81,7 +81,7 @@ async def recovery_device(msg: RecoveryDevice) -> Success:
         # set up pin if requested
         if msg.pin_protection:
             newpin = await request_pin_confirm(allow_cancel=False)
-            config.change_pin("", newpin, None, None)
+            config.change_pin(newpin, None)
 
         storage_device.set_passphrase_enabled(bool(msg.passphrase_protection))
 

core/src/apps/management/reset_device/__init__.py

@@ -78,7 +78,7 @@ async def reset_device(msg: ResetDevice) -> Success:
     # request and set new PIN
     if msg.pin_protection:
         newpin = await request_pin_confirm()
-        if not config.change_pin("", newpin, None, None):
+        if not config.change_pin(newpin, None):
             raise ProcessError("Failed to set PIN")
 
     prev_int_entropy = None

core/src/apps/management/sd_protect.py

@@ -75,7 +75,7 @@ async def _sd_protect_enable(msg: SdProtect) -> Success:
     salt, salt_auth_key, salt_tag = _make_salt()
     await _set_salt(salt, salt_tag)
 
-    if not config.change_pin(pin, pin, None, salt):
+    if not config.unlock(pin, None):
         # Wrong PIN. Clean up the prepared salt file.
         try:
             storage_sd_salt.remove_sd_salt()
@@ -85,6 +85,7 @@ async def _sd_protect_enable(msg: SdProtect) -> Success:
             # exception, because primarily we need to raise wire.PinInvalid.
             pass
         await error_pin_invalid()
+    config.change_pin(pin, salt)
 
     storage_device.set_sd_salt_auth_key(salt_auth_key)
 
@@ -106,7 +107,7 @@ async def _sd_protect_disable(msg: SdProtect) -> Success:
     pin, salt = await request_pin_and_sd_salt(TR.pin__enter)
 
     # Check PIN and remove salt.
-    if not config.change_pin(pin, pin, salt, None):
+    if not config.unlock(pin, salt):
         await error_pin_invalid()
 
     storage_device.set_sd_salt_auth_key(None)
@@ -141,8 +142,9 @@ async def _sd_protect_refresh(msg: SdProtect) -> Success:
     new_salt, new_auth_key, new_salt_tag = _make_salt()
     await _set_salt(new_salt, new_salt_tag, stage=True)
 
-    if not config.change_pin(pin, pin, old_salt, new_salt):
+    if not config.unlock(pin, old_salt):
         await error_pin_invalid()
+    config.change_pin(pin, new_salt)
 
     storage_device.set_sd_salt_auth_key(new_auth_key)
 

core/tests/test_trezor.config.py

@@ -100,11 +100,7 @@ def test_change_pin(self):
             with self.assertRaises(ValueError):
                 config.set(PINAPP, PINKEY, b"value")
 
-            self.assertTrue(config.change_pin(old_pin, new_pin, None, None))
-
-            # Old PIN cannot be used to change the current PIN.
-            if old_pin != new_pin:
-                self.assertFalse(config.change_pin(old_pin, "666", None, None))
+            self.assertTrue(config.change_pin(new_pin, None))
 
             # Storage remains unlocked.
             self.assertEqual(config.get(1, 1), b"value")
@@ -139,24 +135,24 @@ def test_change_sd_salt(self):
         config.wipe()
         self.assertTrue(config.unlock("", None))
         config.set(1, 1, b"value")
-        self.assertFalse(config.change_pin("", "", salt1, None))
-        self.assertTrue(config.change_pin("", "000", None, salt1))
+        self.assertFalse(config.unlock("", salt1))
+        self.assertTrue(config.change_pin("000", salt1))
         self.assertEqual(config.get(1, 1), b"value")
 
         # Disable PIN and change SD salt.
         config.init()
         self.assertFalse(config.unlock("000", None))
         self.assertIsNone(config.get(1, 1))
         self.assertTrue(config.unlock("000", salt1))
-        self.assertTrue(config.change_pin("000", "", salt1, salt2))
+        self.assertTrue(config.change_pin("", salt2))
         self.assertEqual(config.get(1, 1), b"value")
 
         # Disable SD salt.
         config.init()
         self.assertFalse(config.unlock("000", salt2))
         self.assertIsNone(config.get(1, 1))
         self.assertTrue(config.unlock("", salt2))
-        self.assertTrue(config.change_pin("", "", salt2, None))
+        self.assertTrue(config.change_pin("", None))
         self.assertEqual(config.get(1, 1), b"value")
 
         # Check that PIN and SD salt are disabled.