diff --git a/core/embed/rust/build.rs b/core/embed/rust/build.rs index 2bbe003858..3616a8dada 100644 --- a/core/embed/rust/build.rs +++ b/core/embed/rust/build.rs @@ -358,6 +358,10 @@ fn generate_trezorhal_bindings() { .allowlist_function("translations_write") .allowlist_function("translations_erase") .allowlist_function("translations_area_bytesize") + .allowlist_type("storage_unlock_result_t") + .rustified_enum("storage_unlock_result_t") + .allowlist_type("storage_pin_change_result_t") + .rustified_enum("storage_pin_change_result_t") // display .allowlist_function("display_refresh") .allowlist_function("display_set_backlight") diff --git a/core/embed/rust/src/trezorhal/storage.rs b/core/embed/rust/src/trezorhal/storage.rs index f790f2a56b..31c541e819 100644 --- a/core/embed/rust/src/trezorhal/storage.rs +++ b/core/embed/rust/src/trezorhal/storage.rs @@ -128,29 +128,22 @@ pub fn lock() { /// Returns true if the PIN + salt combination is correct. pub fn unlock(pin: &str, salt: Option<&ExternalSalt>) -> bool { let salt = salt.map(|s| s.as_ptr()).unwrap_or(ptr::null()); - ffi::sectrue == unsafe { ffi::storage_unlock(pin.as_ptr() as *const _, pin.len(), salt) } + let result = unsafe { ffi::storage_unlock(pin.as_ptr() as *const _, pin.len(), salt) }; + matches!(result, ffi::storage_unlock_result_t::UNLOCK_OK) } /// 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 { - 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()), - ) - } +pub fn change_pin(new_pin: &str, new_salt: Option<&ExternalSalt>) -> bool { + let result = unsafe { + ffi::storage_change_pin( + new_pin.as_ptr() as *const _, + new_pin.len(), + new_salt.map(|s| s.as_ptr()).unwrap_or(ptr::null()), + ) + }; + matches!(result, ffi::storage_pin_change_result_t::PIN_CHANGE_OK) } /// Check if storage has PIN set. @@ -333,8 +326,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(); diff --git a/core/embed/sys/smcall/stm32/smcall_dispatch.c b/core/embed/sys/smcall/stm32/smcall_dispatch.c index 97e5042e50..43a744c140 100644 --- a/core/embed/sys/smcall/stm32/smcall_dispatch.c +++ b/core/embed/sys/smcall/stm32/smcall_dispatch.c @@ -254,14 +254,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: { diff --git a/core/embed/sys/smcall/stm32/smcall_stubs.c b/core/embed/sys/smcall/stm32/smcall_stubs.c index 96da37855a..0f08735635 100644 --- a/core/embed/sys/smcall/stm32/smcall_stubs.c +++ b/core/embed/sys/smcall/stm32/smcall_stubs.c @@ -240,10 +240,10 @@ secbool storage_is_unlocked(void) { void storage_lock(void) { smcall_invoke0(SMCALL_STORAGE_LOCK); } -secbool storage_unlock(const uint8_t *pin, size_t pin_len, - const uint8_t *ext_salt) { - return (secbool)smcall_invoke3((uint32_t)pin, pin_len, (uint32_t)ext_salt, - SMCALL_STORAGE_UNLOCK); +storage_unlock_result_t storage_unlock(const uint8_t *pin, size_t pin_len, + const uint8_t *ext_salt) { + return (storage_unlock_result_t)smcall_invoke3( + (uint32_t)pin, pin_len, (uint32_t)ext_salt, SMCALL_STORAGE_UNLOCK); } secbool storage_has_pin(void) { @@ -257,14 +257,12 @@ 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, - 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, - (uint32_t)new_ext_salt, - SMCALL_STORAGE_CHANGE_PIN); +storage_pin_change_result_t storage_change_pin(const uint8_t *newpin, + size_t newpin_len, + const uint8_t *new_ext_salt) { + return (storage_pin_change_result_t)smcall_invoke3( + (uint32_t)newpin, newpin_len, (uint32_t)new_ext_salt, + SMCALL_STORAGE_CHANGE_PIN); } void storage_ensure_not_wipe_code(const uint8_t *pin, size_t pin_len) { diff --git a/core/embed/sys/smcall/stm32/smcall_verifiers.c b/core/embed/sys/smcall/stm32/smcall_verifiers.c index d7c7d7442a..459ee15b0d 100644 --- a/core/embed/sys/smcall/stm32/smcall_verifiers.c +++ b/core/embed/sys/smcall/stm32/smcall_verifiers.c @@ -262,8 +262,9 @@ access_violation: apptask_access_violation(); } -secbool storage_unlock__verified(const uint8_t *pin, size_t pin_len, - const uint8_t *ext_salt) { +storage_unlock_result_t storage_unlock__verified(const uint8_t *pin, + size_t pin_len, + const uint8_t *ext_salt) { if (!probe_read_access(pin, pin_len)) { goto access_violation; } @@ -276,35 +277,24 @@ secbool storage_unlock__verified(const uint8_t *pin, size_t pin_len, access_violation: apptask_access_violation(); - return secfalse; + return UNLOCK_ACCESS_VIOLATION; } -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, - const uint8_t *new_ext_salt) { - if (!probe_read_access(oldpin, oldpin_len)) { - goto access_violation; - } - +storage_pin_change_result_t storage_change_pin__verified( + const uint8_t *newpin, size_t newpin_len, const uint8_t *new_ext_salt) { 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(); - return secfalse; + return PIN_CHANGE_ACCESS_VIOLATION; } void storage_ensure_not_wipe_code__verified(const uint8_t *pin, diff --git a/core/embed/sys/smcall/stm32/smcall_verifiers.h b/core/embed/sys/smcall/stm32/smcall_verifiers.h index 5a919e8fd9..0f685a4d07 100644 --- a/core/embed/sys/smcall/stm32/smcall_verifiers.h +++ b/core/embed/sys/smcall/stm32/smcall_verifiers.h @@ -84,13 +84,12 @@ secbool secret_key_delegated_identity__verified( 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); +storage_unlock_result_t 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, - const uint8_t *new_ext_salt); +storage_pin_change_result_t 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); diff --git a/core/embed/sys/syscall/stm32/syscall_dispatch.c b/core/embed/sys/syscall/stm32/syscall_dispatch.c index 84ccefac19..7c1951cb5e 100644 --- a/core/embed/sys/syscall/stm32/syscall_dispatch.c +++ b/core/embed/sys/syscall/stm32/syscall_dispatch.c @@ -545,14 +545,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: { diff --git a/core/embed/sys/syscall/stm32/syscall_stubs.c b/core/embed/sys/syscall/stm32/syscall_stubs.c index 93273d8a4d..0eebc1071a 100644 --- a/core/embed/sys/syscall/stm32/syscall_stubs.c +++ b/core/embed/sys/syscall/stm32/syscall_stubs.c @@ -536,10 +536,10 @@ secbool storage_is_unlocked(void) { void storage_lock(void) { syscall_invoke0(SYSCALL_STORAGE_LOCK); } -secbool storage_unlock(const uint8_t *pin, size_t pin_len, - const uint8_t *ext_salt) { - return (secbool)syscall_invoke3((uint32_t)pin, pin_len, (uint32_t)ext_salt, - SYSCALL_STORAGE_UNLOCK); +storage_unlock_result_t storage_unlock(const uint8_t *pin, size_t pin_len, + const uint8_t *ext_salt) { + return (storage_unlock_result_t)syscall_invoke3( + (uint32_t)pin, pin_len, (uint32_t)ext_salt, SYSCALL_STORAGE_UNLOCK); } secbool storage_has_pin(void) { @@ -553,13 +553,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, - 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, +storage_pin_change_result_t storage_change_pin(const uint8_t *newpin, + size_t newpin_len, + const uint8_t *new_ext_salt) { + return (storage_pin_change_result_t)syscall_invoke3( + (uint32_t)newpin, newpin_len, (uint32_t)new_ext_salt, SYSCALL_STORAGE_CHANGE_PIN); } diff --git a/core/embed/sys/syscall/stm32/syscall_verifiers.c b/core/embed/sys/syscall/stm32/syscall_verifiers.c index d82974c3b2..4db27545cc 100644 --- a/core/embed/sys/syscall/stm32/syscall_verifiers.c +++ b/core/embed/sys/syscall/stm32/syscall_verifiers.c @@ -639,8 +639,9 @@ access_violation: apptask_access_violation(); } -secbool storage_unlock__verified(const uint8_t *pin, size_t pin_len, - const uint8_t *ext_salt) { +storage_unlock_result_t storage_unlock__verified(const uint8_t *pin, + size_t pin_len, + const uint8_t *ext_salt) { if (!probe_read_access(pin, pin_len)) { goto access_violation; } @@ -653,35 +654,24 @@ secbool storage_unlock__verified(const uint8_t *pin, size_t pin_len, access_violation: apptask_access_violation(); - return secfalse; + return UNLOCK_ACCESS_VIOLATION; } -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, - const uint8_t *new_ext_salt) { - if (!probe_read_access(oldpin, oldpin_len)) { - goto access_violation; - } - +storage_pin_change_result_t storage_change_pin__verified( + const uint8_t *newpin, size_t newpin_len, const uint8_t *new_ext_salt) { 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(); - return secfalse; + return PIN_CHANGE_ACCESS_VIOLATION; } void storage_ensure_not_wipe_code__verified(const uint8_t *pin, diff --git a/core/embed/sys/syscall/stm32/syscall_verifiers.h b/core/embed/sys/syscall/stm32/syscall_verifiers.h index f8b98a1dc5..9e58b3f8de 100644 --- a/core/embed/sys/syscall/stm32/syscall_verifiers.h +++ b/core/embed/sys/syscall/stm32/syscall_verifiers.h @@ -180,13 +180,12 @@ bool telemetry_get__verified(telemetry_data_t *out); 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); +storage_unlock_result_t 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, - const uint8_t *new_ext_salt); +storage_pin_change_result_t 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); diff --git a/core/embed/upymod/modtrezorconfig/modtrezorconfig.c b/core/embed/upymod/modtrezorconfig/modtrezorconfig.c index cea36e8744..dad0b2d3dc 100644 --- a/core/embed/upymod/modtrezorconfig/modtrezorconfig.c +++ b/core/embed/upymod/modtrezorconfig/modtrezorconfig.c @@ -85,10 +85,57 @@ STATIC mp_obj_t mod_trezorconfig_unlock(mp_obj_t pin, mp_obj_t ext_salt) { MP_ERROR_TEXT("Invalid length of external salt.")); } - if (sectrue != storage_unlock(pin_b.buf, pin_b.len, ext_salt_b.buf)) { - return mp_const_false; + switch (storage_unlock(pin_b.buf, pin_b.len, ext_salt_b.buf)) { + case UNLOCK_OK: + return mp_const_true; + case UNLOCK_NOT_INITIALIZED: + mp_raise_msg(&mp_type_RuntimeError, + MP_ERROR_TEXT("Device is not initialized.")); + case UNLOCK_NO_PIN: + mp_raise_msg(&mp_type_RuntimeError, MP_ERROR_TEXT("No PIN is set.")); + case UNLOCK_PIN_GET_FAILS_FAILED: + mp_raise_msg(&mp_type_RuntimeError, + MP_ERROR_TEXT("Failed to get PIN failure counter.")); + case UNLOCK_TOO_MANY_FAILS: + mp_raise_msg( + &mp_type_RuntimeError, + MP_ERROR_TEXT("Too many incorrect PIN attempts; storage wiped.")); + case UNLOCK_UI_CANCELLED: + mp_raise_msg(&mp_type_RuntimeError, MP_ERROR_TEXT("UI cancelled.")); + case UNLOCK_INCREASE_FAILS_FAILED: + mp_raise_msg(&mp_type_RuntimeError, + MP_ERROR_TEXT("Failed to increase PIN failure counter.")); + case UNLOCK_INCORRECT_PIN: + return mp_const_false; + case UNLOCK_WRONG_STORAGE_VERSION: + mp_raise_msg(&mp_type_RuntimeError, + MP_ERROR_TEXT("Wrong storage version.")); + case UNLOCK_OPTIGA_GET_HMAC_RESET_KEY_FAILED: + mp_raise_msg(&mp_type_RuntimeError, + MP_ERROR_TEXT("OPTIGA get HMAC reset failed.")); + case UNLOCK_OPTIGA_HMAC_COUNTER_RESET_FAILED: + mp_raise_msg(&mp_type_RuntimeError, + MP_ERROR_TEXT("OPTIGA HMAC counter reset failed.")); + case UNLOCK_GET_TROPIC_MAC_AND_DESTROY_RESET_KEY_FAILED: + mp_raise_msg( + &mp_type_RuntimeError, + MP_ERROR_TEXT("get Tropic MAC and destroy reset key failed.")); + case UNLOCK_TROPIC_RESET_SLOTS_FAILED: + mp_raise_msg(&mp_type_RuntimeError, + MP_ERROR_TEXT("Tropic slots reset failed.")); + case UNLOCK_PIN_RESET_FAILS_FAILED: + mp_raise_msg(&mp_type_RuntimeError, + MP_ERROR_TEXT("Failed to reset PIN failure counter.")); + case UNLOCK_ACCESS_VIOLATION: + mp_raise_msg(&mp_type_RuntimeError, + MP_ERROR_TEXT("Access violation during unlock.")); + case UNLOCK_UNKNOWN: + mp_raise_msg(&mp_type_RuntimeError, + MP_ERROR_TEXT("Something went wrong during unlock.")); + default: + mp_raise_msg(&mp_type_RuntimeError, + MP_ERROR_TEXT("Something went wrong during unlock.")); } - return mp_const_true; } STATIC MP_DEFINE_CONST_FUN_OBJ_2(mod_trezorconfig_unlock_obj, mod_trezorconfig_unlock); @@ -152,48 +199,54 @@ 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. +/// Change PIN and external salt. Returns True on success, False on entering +/// the wipe code. 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)) { - return mp_const_false; + switch (storage_change_pin(newpin.buf, newpin.len, new_ext_salt)) { + case PIN_CHANGE_OK: + return mp_const_true; + case PIN_CHANGE_WIPE_CODE: + return mp_const_false; + case PIN_CHANGE_STORAGE_LOCKED: + mp_raise_msg(&mp_type_RuntimeError, MP_ERROR_TEXT("Storage is locked.")); + case PIN_CHANGE_WRONG_ARGUMENT: + mp_raise_msg(&mp_type_ValueError, + MP_ERROR_TEXT("Wrong argument provided.")); + case PIN_CHANGE_NOT_INITIALIZED: + mp_raise_msg(&mp_type_RuntimeError, + MP_ERROR_TEXT("Device is not initialized.")); + case PIN_CHANGE_CANNOT_SET_PIN: + mp_raise_msg(&mp_type_RuntimeError, MP_ERROR_TEXT("Cannot set the PIN.")); + case PIN_CHANGE_ACCESS_VIOLATION: + mp_raise_msg(&mp_type_RuntimeError, + MP_ERROR_TEXT("Access violation during PIN change.")); + case PIN_CHANGE_UNKNOWN: + mp_raise_msg(&mp_type_RuntimeError, MP_ERROR_TEXT("Change PIN failed.")); + default: + mp_raise_msg(&mp_type_RuntimeError, MP_ERROR_TEXT("Change PIN failed.")); } - 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: /// """ diff --git a/core/mocks/generated/trezorconfig.pyi b/core/mocks/generated/trezorconfig.pyi index f7ab806f5b..3a9f987bb3 100644 --- a/core/mocks/generated/trezorconfig.pyi +++ b/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. + Change PIN and external salt. Returns True on success, False on entering + the wipe code. Has to be run with unlocked storage. """ diff --git a/core/src/apps/debug/load_device.py b/core/src/apps/debug/load_device.py index c1da1b9472..3cc6ff94ae 100644 --- a/core/src/apps/debug/load_device.py +++ b/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") diff --git a/core/src/apps/management/change_pin.py b/core/src/apps/management/change_pin.py index b82a690e05..cc82ac7c53 100644 --- a/core/src/apps/management/change_pin.py +++ b/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 config.has_pin(): if not config.check_pin(curpin, salt): await error_pin_invalid() @@ -50,11 +50,8 @@ async def change_pin(msg: ChangePin) -> Success: newpin = "" # write into storage - if not config.change_pin(curpin, newpin, salt, salt): - if newpin: - await error_pin_matches_wipe_code() - else: - await error_pin_invalid() + if not config.change_pin(newpin, salt): + await error_pin_matches_wipe_code() utils.notify_send(utils.NOTIFY_PIN_CHANGE) diff --git a/core/src/apps/management/recovery_device/__init__.py b/core/src/apps/management/recovery_device/__init__.py index e1c3b03ca4..908493b8d3 100644 --- a/core/src/apps/management/recovery_device/__init__.py +++ b/core/src/apps/management/recovery_device/__init__.py @@ -81,7 +81,8 @@ 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) + if not config.change_pin(newpin, None): + raise wire.ProcessError("Failed to set PIN") storage_device.set_passphrase_enabled(bool(msg.passphrase_protection)) diff --git a/core/src/apps/management/reset_device/__init__.py b/core/src/apps/management/reset_device/__init__.py index ff2fb3dfd8..1074b5e655 100644 --- a/core/src/apps/management/reset_device/__init__.py +++ b/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 diff --git a/core/src/apps/management/sd_protect.py b/core/src/apps/management/sd_protect.py index 74717fea02..cceaad0f6f 100644 --- a/core/src/apps/management/sd_protect.py +++ b/core/src/apps/management/sd_protect.py @@ -2,7 +2,7 @@ from typing import TYPE_CHECKING import storage.device as storage_device import storage.sd_salt as storage_sd_salt -from trezor import TR, config +from trezor import TR, config, wire from trezor.enums import SdProtectOperationType from trezor.messages import Success from trezor.ui.layouts import show_success @@ -65,26 +65,33 @@ async def _sd_protect_enable(msg: SdProtect) -> Success: # Make sure SD card is present. await ensure_sdcard() - # Get the current PIN. + # Get and check the current PIN. if config.has_pin(): pin = await request_pin(TR.pin__enter, config.get_pin_rem()) + if not config.unlock(pin, None): + await error_pin_invalid() else: pin = "" - # Check PIN and prepare salt file. + # Generate the salt and store it on the SD card first, because SD card operations are more likely to fail than MCU flash operations. salt, salt_auth_key, salt_tag = _make_salt() + # Store the salt on the SD card. await _set_salt(salt, salt_tag) - if not config.change_pin(pin, pin, None, salt): - # Wrong PIN. Clean up the prepared salt file. + try: + # Set the salt in storage + config.change_pin(pin, salt) + # Cannot return false, because the device was unlocked by the PIN. + except RuntimeError: + # Failed to set salt in storage. Clean up the prepared salt file. try: storage_sd_salt.remove_sd_salt() except Exception: # The cleanup is not necessary for the correct functioning of # SD-protection. If it fails for any reason, we suppress the - # exception, because primarily we need to raise wire.PinInvalid. + # exception. pass - await error_pin_invalid() + raise wire.ProcessError("Failed to set salt in storage") storage_device.set_sd_salt_auth_key(salt_auth_key) @@ -105,9 +112,16 @@ async def _sd_protect_disable(msg: SdProtect) -> Success: # Get the current PIN and salt from the SD card. 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): - await error_pin_invalid() + if config.has_pin(): + # Check PIN. + if not config.unlock(pin, salt): + await error_pin_invalid() + + # Remove salt from storage. + try: + config.change_pin(pin, None) + except RuntimeError: + raise wire.FirmwareError("Failed to remove SD salt") storage_device.set_sd_salt_auth_key(None) @@ -137,12 +151,19 @@ async def _sd_protect_refresh(msg: SdProtect) -> Success: # Get the current PIN and salt from the SD card. pin, old_salt = await request_pin_and_sd_salt(TR.pin__enter) - # Check PIN and change salt. + if config.has_pin(): + # Check PIN. + if not config.unlock(pin, old_salt): + await error_pin_invalid() + + # Change salt. 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): - await error_pin_invalid() + try: + config.change_pin(pin, new_salt) + except RuntimeError: + raise wire.FirmwareError("Failed to set new salt in storage") storage_device.set_sd_salt_auth_key(new_auth_key) diff --git a/core/tests/test_trezor.config.py b/core/tests/test_trezor.config.py index 4eb1dfc34f..483d3192e5 100644 --- a/core/tests/test_trezor.config.py +++ b/core/tests/test_trezor.config.py @@ -100,11 +100,11 @@ class TestConfig(unittest.TestCase): with self.assertRaises(ValueError): config.set(PINAPP, PINKEY, b"value") - self.assertTrue(config.change_pin(old_pin, new_pin, None, None)) + self.assertTrue(config.change_pin(new_pin, None)) - # Old PIN cannot be used to change the current PIN. + # Old PIN cannot be used to unlock the device. if old_pin != new_pin: - self.assertFalse(config.change_pin(old_pin, "666", None, None)) + self.assertFalse(config.unlock(old_pin, None)) # Storage remains unlocked. self.assertEqual(config.get(1, 1), b"value") @@ -139,8 +139,8 @@ class TestConfig(unittest.TestCase): 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. @@ -148,7 +148,7 @@ class TestConfig(unittest.TestCase): 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. @@ -156,7 +156,7 @@ class TestConfig(unittest.TestCase): 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. diff --git a/legacy/firmware/config.c b/legacy/firmware/config.c index 7c495c861c..22463708c6 100644 --- a/legacy/firmware/config.c +++ b/legacy/firmware/config.c @@ -322,8 +322,7 @@ static secbool config_upgrade_v10(void) { if (config.has_pin) { size_t pin_len = MIN(strnlen(config.pin, sizeof(config.pin)), (size_t)MAX_PIN_LEN); - storage_change_pin(PIN_EMPTY, PIN_EMPTY_LEN, (const uint8_t *)config.pin, - pin_len, NULL, NULL); + storage_change_pin((const uint8_t *)config.pin, pin_len, NULL); } while (pin_wait != 0) { @@ -505,7 +504,7 @@ void config_loadDevice(const LoadDevice *msg) { msg->passphrase_protection); if (msg->has_pin) { - config_changePin("", msg->pin); + config_changePin(msg->pin); } if (msg->mnemonics_count) { @@ -809,23 +808,22 @@ bool config_containsMnemonic(const char *mnemonic) { */ bool config_unlock(const char *pin) { char oldTiny = usbTiny(1); - secbool ret = + storage_unlock_result_t ret = storage_unlock((const uint8_t *)pin, strnlen(pin, MAX_PIN_LEN), NULL); usbTiny(oldTiny); - return sectrue == ret; + return UNLOCK_OK == ret; } bool config_hasPin(void) { return sectrue == storage_has_pin(); } -bool config_changePin(const char *old_pin, const char *new_pin) { +bool config_changePin(const char *new_pin) { char oldTiny = usbTiny(1); - secbool ret = storage_change_pin( - (const uint8_t *)old_pin, strnlen(old_pin, MAX_PIN_LEN), - (const uint8_t *)new_pin, strnlen(new_pin, MAX_PIN_LEN), NULL, NULL); + storage_pin_change_result_t ret = storage_change_pin( + (const uint8_t *)new_pin, strnlen(new_pin, MAX_PIN_LEN), NULL); usbTiny(oldTiny); #if DEBUG_LINK - if (sectrue == ret) { + if (PIN_CHANGE_OK == ret) { if (new_pin[0] != '\0') { storage_set(KEY_DEBUG_LINK_PIN, new_pin, strnlen(new_pin, MAX_PIN_LEN)); } else { @@ -834,7 +832,7 @@ bool config_changePin(const char *old_pin, const char *new_pin) { } #endif - return sectrue == ret; + return PIN_CHANGE_OK == ret; } #if DEBUG_LINK diff --git a/legacy/firmware/config.h b/legacy/firmware/config.h index fe523ada48..12d9dbe609 100644 --- a/legacy/firmware/config.h +++ b/legacy/firmware/config.h @@ -139,7 +139,7 @@ bool config_getPin(char *dest, uint16_t dest_size); bool config_unlock(const char *pin); bool config_hasPin(void); -bool config_changePin(const char *old_pin, const char *new_pin); +bool config_changePin(const char *new_pin); bool session_isUnlocked(void); bool config_hasWipeCode(void); diff --git a/legacy/firmware/protect.c b/legacy/firmware/protect.c index 74e6d9d9ea..0408c9810e 100644 --- a/legacy/firmware/protect.c +++ b/legacy/firmware/protect.c @@ -236,7 +236,6 @@ bool protectPin(bool use_cached) { } bool protectChangePin(bool removal) { - static CONFIDENTIAL char old_pin[MAX_PIN_LEN + 1] = ""; static CONFIDENTIAL char new_pin[MAX_PIN_LEN + 1] = ""; const char *pin = NULL; @@ -248,25 +247,19 @@ bool protectChangePin(bool removal) { return false; } - // If removing, defer the check to config_changePin(). - if (!removal) { - usbTiny(1); - bool ret = config_unlock(pin); - usbTiny(0); - if (ret == false) { - fsm_sendFailure(FailureType_Failure_PinInvalid, NULL); - return false; - } + usbTiny(1); + bool ret = config_unlock(pin); + usbTiny(0); + if (ret == false) { + fsm_sendFailure(FailureType_Failure_PinInvalid, NULL); + return false; } - - strlcpy(old_pin, pin, sizeof(old_pin)); } if (!removal) { pin = requestPin(PinMatrixRequestType_PinMatrixRequestType_NewFirst, _("Please enter new PIN:")); if (pin == NULL || pin[0] == '\0') { - memzero(old_pin, sizeof(old_pin)); fsm_sendFailure(FailureType_Failure_PinCancelled, NULL); return false; } @@ -275,30 +268,23 @@ bool protectChangePin(bool removal) { pin = requestPin(PinMatrixRequestType_PinMatrixRequestType_NewSecond, _("Please re-enter new PIN:")); if (pin == NULL) { - memzero(old_pin, sizeof(old_pin)); memzero(new_pin, sizeof(new_pin)); fsm_sendFailure(FailureType_Failure_PinCancelled, NULL); return false; } if (strncmp(new_pin, pin, sizeof(new_pin)) != 0) { - memzero(old_pin, sizeof(old_pin)); memzero(new_pin, sizeof(new_pin)); fsm_sendFailure(FailureType_Failure_PinMismatch, NULL); return false; } } - bool ret = config_changePin(old_pin, new_pin); - memzero(old_pin, sizeof(old_pin)); + bool ret = config_changePin(new_pin); memzero(new_pin, sizeof(new_pin)); if (ret == false) { - if (removal) { - fsm_sendFailure(FailureType_Failure_PinInvalid, NULL); - } else { - fsm_sendFailure(FailureType_Failure_ProcessError, - _("The new PIN must be different from your wipe code.")); - } + fsm_sendFailure(FailureType_Failure_ProcessError, + _("The new PIN must be different from your wipe code.")); } return ret; } diff --git a/storage/storage.c b/storage/storage.c index c5c07d9d00..8c61b7c249 100644 --- a/storage/storage.c +++ b/storage/storage.c @@ -509,11 +509,6 @@ static uint32_t ui_estimate_time_ms(storage_pin_op_t op) { unlock_time(pin_index, &time_ms, &optiga_sec, &optiga_last_time_decreased_ms); break; - case STORAGE_PIN_OP_CHANGE: - set_pin_time(&time_ms, &optiga_sec, &optiga_last_time_decreased_ms); - unlock_time(pin_index, &time_ms, &optiga_sec, - &optiga_last_time_decreased_ms); - break; default: assert(false); } @@ -1173,8 +1168,8 @@ static uint32_t get_backoff_time_ms(uint32_t fail_ctr) { return 1000 * ((1 << fail_ctr) - 1); } -static secbool unlock(const uint8_t *pin, size_t pin_len, - const uint8_t *ext_salt) { +static storage_unlock_result_t unlock(const uint8_t *pin, size_t pin_len, + const uint8_t *ext_salt) { const uint8_t *unlock_pin = pin; size_t unlock_pin_len = pin_len; uint32_t legacy_pin = 0; @@ -1196,7 +1191,7 @@ static secbool unlock(const uint8_t *pin, size_t pin_len, uint32_t ctr = 0; if (sectrue != pin_get_fails(&ctr)) { memzero(&legacy_pin, sizeof(legacy_pin)); - return secfalse; + return UNLOCK_PIN_GET_FAILS_FAILED; } // Wipe storage if too many failures @@ -1204,7 +1199,7 @@ static secbool unlock(const uint8_t *pin, size_t pin_len, if (ctr >= PIN_MAX_TRIES) { storage_wipe(); show_pin_too_many_screen(); - return secfalse; + return UNLOCK_TOO_MANY_FAILS; } ui_progress(); @@ -1214,7 +1209,7 @@ static secbool unlock(const uint8_t *pin, size_t pin_len, while (hal_ticks_ms() - begin < get_backoff_time_ms(ctr)) { if (sectrue == ui_progress()) { memzero(&legacy_pin, sizeof(legacy_pin)); - return secfalse; + return UNLOCK_UI_CANCELLED; } hal_delay(100); } @@ -1223,14 +1218,14 @@ static secbool unlock(const uint8_t *pin, size_t pin_len, // PIN. If the PIN is correct, we reset the counter afterwards. If not, we // check if this is the last allowed attempt. if (sectrue != storage_pin_fails_increase()) { - return secfalse; + return UNLOCK_INCREASE_FAILS_FAILED; } // Check that the PIN fail counter was incremented. uint32_t ctr_ck = 0; if (sectrue != pin_get_fails(&ctr_ck) || ctr + 1 != ctr_ck) { handle_fault("PIN counter increment"); - return secfalse; + return UNLOCK_PIN_GET_FAILS_FAILED; } // Check whether the entered PIN is correct. @@ -1247,12 +1242,12 @@ static secbool unlock(const uint8_t *pin, size_t pin_len, while (hal_ticks_ms() - ui_begin < ui_total) { ui_message = WRONG_PIN_MSG; if (sectrue == ui_progress()) { - return secfalse; + return UNLOCK_UI_CANCELLED; } hal_delay(100); } - return secfalse; + return UNLOCK_INCORRECT_PIN; } memzero(&legacy_pin, sizeof(legacy_pin)); @@ -1263,7 +1258,7 @@ static secbool unlock(const uint8_t *pin, size_t pin_len, // storage_get_encrypted() which calls auth_get(). if (sectrue != storage_upgrade_unlocked(pin, pin_len, ext_salt) || sectrue != check_storage_version()) { - return secfalse; + return UNLOCK_WRONG_STORAGE_VERSION; } unlocked = sectrue; @@ -1276,11 +1271,11 @@ static secbool unlock(const uint8_t *pin, size_t pin_len, sizeof(optiga_hmac_reset_key), &optiga_hmac_reset_key_len) != sectrue || optiga_hmac_reset_key_len != SHA256_DIGEST_LENGTH) { - return secfalse; + return UNLOCK_OPTIGA_GET_HMAC_RESET_KEY_FAILED; } if (!optiga_pin_reset_hmac_counter(ui_progress, optiga_hmac_reset_key)) { memzero(optiga_hmac_reset_key, sizeof(optiga_hmac_reset_key)); - return secfalse; + return UNLOCK_OPTIGA_HMAC_COUNTER_RESET_FAILED; } memzero(optiga_hmac_reset_key, sizeof(optiga_hmac_reset_key)); } @@ -1295,20 +1290,24 @@ static secbool unlock(const uint8_t *pin, size_t pin_len, &tropic_mac_and_destroy_reset_key_len) != sectrue || tropic_mac_and_destroy_reset_key_len != sizeof(tropic_mac_and_destroy_reset_key)) { - return secfalse; + return UNLOCK_GET_TROPIC_MAC_AND_DESTROY_RESET_KEY_FAILED; } if (!tropic_pin_reset_slots(ui_progress, ctr, tropic_mac_and_destroy_reset_key)) { memzero(tropic_mac_and_destroy_reset_key, sizeof(tropic_mac_and_destroy_reset_key)); - return secfalse; + return UNLOCK_TROPIC_RESET_SLOTS_FAILED; } memzero(tropic_mac_and_destroy_reset_key, sizeof(tropic_mac_and_destroy_reset_key)); #endif // Finally set the counter to 0 to indicate success. - return pin_fails_reset(); + if (sectrue == pin_fails_reset()) { + return UNLOCK_OK; + } else { + return UNLOCK_PIN_RESET_FAILS_FAILED; + } } void unlock_time(uint16_t pin_index, uint32_t *time_ms, uint8_t *optiga_sec, @@ -1362,10 +1361,14 @@ void unlock_time(uint16_t pin_index, uint32_t *time_ms, uint8_t *optiga_sec, *time_ms += time_estimate_clock_cycles_ms(13500000); } -secbool storage_unlock(const uint8_t *pin, size_t pin_len, - const uint8_t *ext_salt) { - if (sectrue != initialized || pin == NULL) { - return secfalse; +storage_unlock_result_t storage_unlock(const uint8_t *pin, size_t pin_len, + const uint8_t *ext_salt) { + if (sectrue != initialized) { + return UNLOCK_NOT_INITIALIZED; + } + + if (pin == NULL) { + return UNLOCK_NO_PIN; } mpu_mode_t mpu_mode = mpu_reconfig(MPU_MODE_STORAGE); @@ -1381,7 +1384,7 @@ secbool storage_unlock(const uint8_t *pin, size_t pin_len, ui_message = VERIFYING_PIN_MSG; } - secbool ret = unlock(pin, pin_len, ext_salt); + storage_unlock_result_t ret = unlock(pin, pin_len, ext_salt); mpu_restore(mpu_mode); ui_progress_finish(); @@ -1702,32 +1705,40 @@ end: return rem_mcu; } -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, - const uint8_t *new_ext_salt) { - if (sectrue != initialized || oldpin == NULL || newpin == NULL) { - return secfalse; +storage_pin_change_result_t storage_change_pin(const uint8_t *newpin, + size_t newpin_len, + const uint8_t *new_ext_salt) { + if (sectrue != initialized) { + return PIN_CHANGE_NOT_INITIALIZED; } + if (newpin == NULL) { + return PIN_CHANGE_WRONG_ARGUMENT; + } + + storage_pin_change_result_t ret = PIN_CHANGE_UNKNOWN; + mpu_mode_t mpu_mode = mpu_reconfig(MPU_MODE_STORAGE); - ui_progress_init(STORAGE_PIN_OP_CHANGE); - ui_message = - (oldpin_len != 0 && newpin_len == 0) ? VERIFYING_PIN_MSG : PROCESSING_MSG; + ui_progress_init(STORAGE_PIN_OP_SET); + ui_message = PROCESSING_MSG; - secbool ret = unlock(oldpin, oldpin_len, old_ext_salt); - if (sectrue != ret) { + if (sectrue != storage_is_unlocked()) { + ret = PIN_CHANGE_STORAGE_LOCKED; goto end; } // Fail if the new PIN is the same as the wipe code. - ret = is_not_wipe_code(newpin, newpin_len); - if (sectrue != ret) { + if (sectrue != is_not_wipe_code(newpin, newpin_len)) { + ret = PIN_CHANGE_WIPE_CODE; goto end; } - ret = set_pin(newpin, newpin_len, new_ext_salt); + if (sectrue == set_pin(newpin, newpin_len, new_ext_salt)) { + ret = PIN_CHANGE_OK; + } else { + ret = PIN_CHANGE_CANNOT_SET_PIN; + } end: mpu_restore(mpu_mode); @@ -1783,8 +1794,11 @@ secbool storage_change_wipe_code(const uint8_t *pin, size_t pin_len, ui_message = (pin_len != 0 && wipe_code_len == 0) ? VERIFYING_PIN_MSG : PROCESSING_MSG; - secbool ret = unlock(pin, pin_len, ext_salt); - if (sectrue != ret) { + secbool ret; + if (UNLOCK_OK == unlock(pin, pin_len, ext_salt)) { + ret = sectrue; + } else { + ret = secfalse; goto end; } diff --git a/storage/storage.h b/storage/storage.h index 524a6c053d..5e200f5f9a 100644 --- a/storage/storage.h +++ b/storage/storage.h @@ -79,9 +79,38 @@ enum storage_ui_message_t { typedef enum { STORAGE_PIN_OP_SET = 0, STORAGE_PIN_OP_VERIFY, - STORAGE_PIN_OP_CHANGE, } storage_pin_op_t; +typedef enum { + UNLOCK_OK = 0, + UNLOCK_NOT_INITIALIZED = 1, + UNLOCK_NO_PIN, + UNLOCK_PIN_GET_FAILS_FAILED, + UNLOCK_TOO_MANY_FAILS, + UNLOCK_UI_CANCELLED, + UNLOCK_INCREASE_FAILS_FAILED, + UNLOCK_INCORRECT_PIN, + UNLOCK_WRONG_STORAGE_VERSION, + UNLOCK_OPTIGA_GET_HMAC_RESET_KEY_FAILED, + UNLOCK_OPTIGA_HMAC_COUNTER_RESET_FAILED, + UNLOCK_GET_TROPIC_MAC_AND_DESTROY_RESET_KEY_FAILED, + UNLOCK_TROPIC_RESET_SLOTS_FAILED, + UNLOCK_PIN_RESET_FAILS_FAILED, + UNLOCK_ACCESS_VIOLATION, + UNLOCK_UNKNOWN, +} storage_unlock_result_t; + +typedef enum { + PIN_CHANGE_OK = 0, + PIN_CHANGE_WIPE_CODE = 1, + PIN_CHANGE_STORAGE_LOCKED, + PIN_CHANGE_WRONG_ARGUMENT, + PIN_CHANGE_NOT_INITIALIZED, + PIN_CHANGE_CANNOT_SET_PIN, + PIN_CHANGE_ACCESS_VIOLATION, + PIN_CHANGE_UNKNOWN, +} storage_pin_change_result_t; + typedef secbool (*PIN_UI_WAIT_CALLBACK)(uint32_t wait, uint32_t progress, enum storage_ui_message_t message); @@ -90,15 +119,14 @@ void storage_init(PIN_UI_WAIT_CALLBACK callback, const uint8_t *salt, void storage_wipe(void); secbool storage_is_unlocked(void); void storage_lock(void); -secbool storage_unlock(const uint8_t *pin, size_t pin_len, - const uint8_t *ext_salt); +storage_unlock_result_t storage_unlock(const uint8_t *pin, size_t pin_len, + const uint8_t *ext_salt); secbool storage_has_pin(void); secbool storage_pin_fails_increase(void); uint32_t storage_get_pin_rem(void); -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, - const uint8_t *new_ext_salt); +storage_pin_change_result_t storage_change_pin(const uint8_t *newpin, + size_t newpin_len, + const uint8_t *new_ext_salt); void storage_ensure_not_wipe_code(const uint8_t *pin, size_t pin_len); secbool storage_has_wipe_code(void); secbool storage_change_wipe_code(const uint8_t *pin, size_t pin_len, diff --git a/storage/tests/c/storage.py b/storage/tests/c/storage.py index 16c08edcf7..a5bf63a7f2 100644 --- a/storage/tests/c/storage.py +++ b/storage/tests/c/storage.py @@ -11,6 +11,8 @@ import consts EXTERNAL_SALT_LEN = 32 sectrue = -1431655766 # 0xAAAAAAAAA +UNLOCK_OK = 0 +PIN_CHANGE_OK = 0 class Storage: @@ -32,7 +34,7 @@ class Storage: def unlock(self, pin: str, ext_salt: bytes = None) -> bool: if ext_salt is not None and len(ext_salt) != EXTERNAL_SALT_LEN: raise ValueError - return sectrue == self.lib.storage_unlock(pin.encode(), len(pin), ext_salt) + return UNLOCK_OK == self.lib.storage_unlock(pin.encode(), len(pin), ext_salt) def lock(self) -> None: self.lib.storage_lock() @@ -45,21 +47,14 @@ class Storage: def change_pin( self, - oldpin: str, newpin: str, - old_ext_salt: bytes = None, new_ext_salt: bytes = None, ) -> bool: - if old_ext_salt is not None and len(old_ext_salt) != EXTERNAL_SALT_LEN: - raise ValueError if new_ext_salt is not None and len(new_ext_salt) != EXTERNAL_SALT_LEN: raise ValueError - return sectrue == self.lib.storage_change_pin( - oldpin.encode(), - len(oldpin), + return PIN_CHANGE_OK == self.lib.storage_change_pin( newpin.encode(), len(newpin), - old_ext_salt, new_ext_salt, ) diff --git a/storage/tests/python/src/storage.py b/storage/tests/python/src/storage.py index 39a3d13356..1f5fedfebb 100644 --- a/storage/tests/python/src/storage.py +++ b/storage/tests/python/src/storage.py @@ -113,11 +113,12 @@ class Storage: def get_pin_rem(self) -> int: return consts.PIN_MAX_TRIES - self.pin_log.get_failures_count() - def change_pin(self, oldpin: str, newpin: str) -> bool: + def change_pin(self, newpin: str, oldpin: str | None = None) -> bool: if not self.initialized or not self.unlocked: return False - if not self.check_pin(oldpin): - return False + if oldpin is not None: + if not self.check_pin(oldpin): + return False self._set_pin(newpin) return True diff --git a/storage/tests/tests/storage_model.py b/storage/tests/tests/storage_model.py index f11f9b3e1d..73fd6698c4 100644 --- a/storage/tests/tests/storage_model.py +++ b/storage/tests/tests/storage_model.py @@ -37,8 +37,11 @@ class StorageModel: def get_pin_rem(self) -> int: return self.pin_rem - def change_pin(self, oldpin: str, newpin: str) -> bool: - if self.unlocked and self.unlock(oldpin): + def change_pin(self, newpin: str, oldpin: str | None = None) -> bool: + if self.unlocked: + if oldpin is not None: + if not self.unlock(oldpin): + return False self.pin = newpin return True else: diff --git a/storage/tests/tests/test_pin.py b/storage/tests/tests/test_pin.py index ac5247a868..21aaca5d62 100644 --- a/storage/tests/tests/test_pin.py +++ b/storage/tests/tests/test_pin.py @@ -19,11 +19,11 @@ def test_init_pin(nc_class): def test_change_pin(nc_class): sc, sp = common.init(nc_class, unlock=True) for s in (sc, sp): - assert s.change_pin("", "222") - assert not s.change_pin("9999", "") # invalid PIN + assert s.change_pin("222") + assert not s.unlock("9999") # invalid PIN assert s.unlock("222") - assert s.change_pin("222", "99999") - assert s.change_pin("99999", "Trezor") + assert s.change_pin("99999") + assert s.change_pin("Trezor") assert s.unlock("Trezor") assert not s.unlock("9999") # invalid PIN assert not s.unlock("99999") # invalid old PIN @@ -38,9 +38,9 @@ def test_has_pin(nc_class): assert not s.has_pin() assert s.unlock("") assert not s.has_pin() - assert s.change_pin("", "22") + assert s.change_pin("22") assert s.has_pin() - assert s.change_pin("22", "") + assert s.change_pin("") assert not s.has_pin() @@ -48,7 +48,7 @@ def test_has_pin(nc_class): def test_wipe_after_max_pin(nc_class): sc, sp = common.init(nc_class, unlock=True) for s in (sc, sp): - assert s.change_pin("", "222") + assert s.change_pin("222") assert s.unlock("222") s.set(0x0202, b"Hello") diff --git a/storage/tests/tests/test_random.py b/storage/tests/tests/test_random.py index 47e9d003b7..dd84889f20 100644 --- a/storage/tests/tests/test_random.py +++ b/storage/tests/tests/test_random.py @@ -55,7 +55,8 @@ class StorageComparison(RuleBasedStateMachine): @rule(oldpin=pins, newpin=pins) def change_pin(self, oldpin, newpin): - assert len(set(s.change_pin(oldpin, newpin) for s in self.storages)) == 1 + assert len(set(s.unlock(oldpin) for s in self.storages)) == 1 + assert len(set(s.change_pin(newpin) for s in self.storages)) == 1 self.ensure_unlocked() @rule() diff --git a/storage/tests/tests/test_random_upgrade.py b/storage/tests/tests/test_random_upgrade.py index 5383483e18..5bea4f6253 100644 --- a/storage/tests/tests/test_random_upgrade.py +++ b/storage/tests/tests/test_random_upgrade.py @@ -50,7 +50,9 @@ class StorageUpgrade(RuleBasedStateMachine): @rule(oldpin=pins, newpin=pins) def change_pin(self, oldpin, newpin): - assert self.sm.change_pin(oldpin, newpin) == self.sc.change_pin(oldpin, newpin) + assert self.sm.change_pin(oldpin=oldpin, newpin=newpin) == self.sc.change_pin( + oldpin, newpin + ) self.ensure_unlocked() @invariant() diff --git a/storage/tests/tests/test_set_get.py b/storage/tests/tests/test_set_get.py index f98f88cce0..729d80302d 100644 --- a/storage/tests/tests/test_set_get.py +++ b/storage/tests/tests/test_set_get.py @@ -83,8 +83,8 @@ def test_set_get(nc_class): assert common.memory_equals(sc, sp) for s in (sc, sp): - s.change_pin("", "222") - s.change_pin("222", "99") + s.change_pin("222") + s.change_pin("99") s.set(0xAAAA, b"something else") assert common.memory_equals(sc, sp) diff --git a/storage/tests/tests/test_upgrade.py b/storage/tests/tests/test_upgrade.py index 1438fc61a6..870fa247eb 100644 --- a/storage/tests/tests/test_upgrade.py +++ b/storage/tests/tests/test_upgrade.py @@ -21,12 +21,12 @@ def set_values(s): s.set(0xBEEF, b"Hello") s.set(0xCAFE, b"world! ") s.set(0xDEAD, b"How\n") - s.change_pin("", "222") + s.change_pin(oldpin="", newpin="222") s.set(0xAAAA, b"are") s.set(0x0901, b"you?") s.set(0x0902, b"Lorem") s.set(0x0903, b"ipsum") - s.change_pin("222", "99") + s.change_pin(oldpin="222", newpin="99") s.set(0xDEAD, b"A\n") s.set(0xDEAD, b"AAAAAAAAAAA") s.set(0x2200, b"BBBB")