From 2836bfc64c4b6f97e7843d019947e8adac6f97de Mon Sep 17 00:00:00 2001 From: Andrew Kozlik Date: Wed, 10 Mar 2021 14:43:21 +0100 Subject: [PATCH] fix(core): Improve error handling and range checking in modtrezorconfig. --- .../extmod/modtrezorconfig/modtrezorconfig.c | 60 ++++++++++++------- core/mocks/generated/trezorconfig.pyi | 8 ++- core/src/storage/common.py | 14 +++-- core/src/storage/device.py | 6 +- core/tests/test_storage.py | 2 - core/tests/test_trezor.config.py | 24 +++++++- storage/storage.h | 4 +- 7 files changed, 79 insertions(+), 39 deletions(-) diff --git a/core/embed/extmod/modtrezorconfig/modtrezorconfig.c b/core/embed/extmod/modtrezorconfig/modtrezorconfig.c index 44bb8c9be..2031734e4 100644 --- a/core/embed/extmod/modtrezorconfig/modtrezorconfig.c +++ b/core/embed/extmod/modtrezorconfig/modtrezorconfig.c @@ -259,7 +259,10 @@ STATIC MP_DEFINE_CONST_FUN_OBJ_VAR_BETWEEN( /// value fails. /// """ STATIC mp_obj_t mod_trezorconfig_get(size_t n_args, const mp_obj_t *args) { - uint8_t app = trezor_obj_get_uint8(args[0]) & FLAGS_APPID; + uint8_t app = trezor_obj_get_uint8(args[0]); + if (app == 0 || app > MAX_APPID) { + mp_raise_msg(&mp_type_ValueError, "Invalid app ID."); + } uint8_t key = trezor_obj_get_uint8(args[1]); if (n_args > 2 && args[2] == mp_const_true) { app |= FLAG_PUBLIC; @@ -288,7 +291,10 @@ STATIC MP_DEFINE_CONST_FUN_OBJ_VAR_BETWEEN(mod_trezorconfig_get_obj, 2, 3, /// Sets a value of given key for given app. /// """ STATIC mp_obj_t mod_trezorconfig_set(size_t n_args, const mp_obj_t *args) { - uint8_t app = trezor_obj_get_uint8(args[0]) & FLAGS_APPID; + uint8_t app = trezor_obj_get_uint8(args[0]); + if (app == 0 || app > MAX_APPID) { + mp_raise_msg(&mp_type_ValueError, "Invalid app ID."); + } uint8_t key = trezor_obj_get_uint8(args[1]); if (n_args > 3 && args[3] == mp_const_true) { app |= FLAG_PUBLIC; @@ -296,7 +302,8 @@ STATIC mp_obj_t mod_trezorconfig_set(size_t n_args, const mp_obj_t *args) { uint16_t appkey = (app << 8) | key; mp_buffer_info_t value; mp_get_buffer_raise(args[2], &value, MP_BUFFER_READ); - if (sectrue != storage_set(appkey, value.buf, value.len)) { + if (value.len > UINT16_MAX || + sectrue != storage_set(appkey, value.buf, value.len)) { mp_raise_msg(&mp_type_RuntimeError, "Could not save value"); } return mp_const_none; @@ -304,34 +311,48 @@ STATIC mp_obj_t mod_trezorconfig_set(size_t n_args, const mp_obj_t *args) { STATIC MP_DEFINE_CONST_FUN_OBJ_VAR_BETWEEN(mod_trezorconfig_set_obj, 3, 4, mod_trezorconfig_set); -/// def delete(app: int, key: int, public: bool = False) -> bool: +/// def delete( +/// app: int, key: int, public: bool = False, writable_locked: bool = False +/// ) -> bool: /// """ /// Deletes the given key of the given app. /// """ STATIC mp_obj_t mod_trezorconfig_delete(size_t n_args, const mp_obj_t *args) { - uint8_t app = trezor_obj_get_uint8(args[0]) & FLAGS_APPID; + uint8_t app = trezor_obj_get_uint8(args[0]); + if (app == 0 || app > MAX_APPID) { + mp_raise_msg(&mp_type_ValueError, "Invalid app ID."); + } uint8_t key = trezor_obj_get_uint8(args[1]); if (n_args > 2 && args[2] == mp_const_true) { app |= FLAG_PUBLIC; } + if (n_args > 3 && args[3] == mp_const_true) { + app |= FLAGS_WRITE; + if (args[2] != mp_const_true) { + mp_raise_msg(&mp_type_ValueError, "Writable entry must be public."); + } + } uint16_t appkey = (app << 8) | key; if (sectrue != storage_delete(appkey)) { return mp_const_false; } return mp_const_true; } -STATIC MP_DEFINE_CONST_FUN_OBJ_VAR_BETWEEN(mod_trezorconfig_delete_obj, 2, 3, +STATIC MP_DEFINE_CONST_FUN_OBJ_VAR_BETWEEN(mod_trezorconfig_delete_obj, 2, 4, mod_trezorconfig_delete); /// def set_counter( /// app: int, key: int, count: int, writable_locked: bool = False -/// ) -> bool: +/// ) -> None: /// """ /// Sets the given key of the given app as a counter with the given value. /// """ STATIC mp_obj_t mod_trezorconfig_set_counter(size_t n_args, const mp_obj_t *args) { - uint8_t app = trezor_obj_get_uint8(args[0]) & FLAGS_APPID; + uint8_t app = trezor_obj_get_uint8(args[0]); + if (app == 0 || app > MAX_APPID) { + mp_raise_msg(&mp_type_ValueError, "Invalid app ID."); + } uint8_t key = trezor_obj_get_uint8(args[1]); if (n_args > 3 && args[3] == mp_const_true) { app |= FLAGS_WRITE; @@ -339,31 +360,28 @@ STATIC mp_obj_t mod_trezorconfig_set_counter(size_t n_args, app |= FLAG_PUBLIC; } uint16_t appkey = (app << 8) | key; - if (args[2] == mp_const_none) { - if (sectrue != storage_delete(appkey)) { - return mp_const_false; - } - } else { - uint32_t count = trezor_obj_get_uint(args[2]); - if (sectrue != storage_set_counter(appkey, count)) { - return mp_const_false; - } + mp_uint_t count = trezor_obj_get_uint(args[2]); + if (count > UINT32_MAX || sectrue != storage_set_counter(appkey, count)) { + mp_raise_msg(&mp_type_RuntimeError, "Failed to set value in storage."); } - return mp_const_true; + return mp_const_none; } STATIC MP_DEFINE_CONST_FUN_OBJ_VAR_BETWEEN(mod_trezorconfig_set_counter_obj, 3, 4, mod_trezorconfig_set_counter); /// def next_counter( /// app: int, key: int, writable_locked: bool = False, -/// ) -> Optional[int]: +/// ) -> int: /// """ /// Increments the counter stored under the given key of the given app and /// returns the new value. /// """ STATIC mp_obj_t mod_trezorconfig_next_counter(size_t n_args, const mp_obj_t *args) { - uint8_t app = trezor_obj_get_uint8(args[0]) & FLAGS_APPID; + uint8_t app = trezor_obj_get_uint8(args[0]); + if (app == 0 || app > MAX_APPID) { + mp_raise_msg(&mp_type_ValueError, "Invalid app ID."); + } uint8_t key = trezor_obj_get_uint8(args[1]); if (n_args > 2 && args[2] == mp_const_true) { app |= FLAGS_WRITE; @@ -373,7 +391,7 @@ STATIC mp_obj_t mod_trezorconfig_next_counter(size_t n_args, uint16_t appkey = (app << 8) | key; uint32_t count = 0; if (sectrue != storage_next_counter(appkey, &count)) { - return mp_const_none; + mp_raise_msg(&mp_type_RuntimeError, "Failed to set value in storage."); } return mp_obj_new_int_from_uint(count); } diff --git a/core/mocks/generated/trezorconfig.pyi b/core/mocks/generated/trezorconfig.pyi index e0060b3cf..130313732 100644 --- a/core/mocks/generated/trezorconfig.pyi +++ b/core/mocks/generated/trezorconfig.pyi @@ -109,7 +109,9 @@ def set(app: int, key: int, value: bytes, public: bool = False) -> None: # extmod/modtrezorconfig/modtrezorconfig.c -def delete(app: int, key: int, public: bool = False) -> bool: +def delete( + app: int, key: int, public: bool = False, writable_locked: bool = False +) -> bool: """ Deletes the given key of the given app. """ @@ -118,7 +120,7 @@ def delete(app: int, key: int, public: bool = False) -> bool: # extmod/modtrezorconfig/modtrezorconfig.c def set_counter( app: int, key: int, count: int, writable_locked: bool = False -) -> bool: +) -> None: """ Sets the given key of the given app as a counter with the given value. """ @@ -127,7 +129,7 @@ def set_counter( # extmod/modtrezorconfig/modtrezorconfig.c def next_counter( app: int, key: int, writable_locked: bool = False, -) -> Optional[int]: +) -> int: """ Increments the counter stored under the given key of the given app and returns the new value. diff --git a/core/src/storage/common.py b/core/src/storage/common.py index 4425c337c..2d6e746b4 100644 --- a/core/src/storage/common.py +++ b/core/src/storage/common.py @@ -28,8 +28,10 @@ def get(app: int, key: int, public: bool = False) -> Optional[bytes]: return config.get(app, key, public) -def delete(app: int, key: int, public: bool = False) -> None: - config.delete(app, key, public) +def delete( + app: int, key: int, public: bool = False, writable_locked: bool = False +) -> None: + config.delete(app, key, public, writable_locked) def set_true_or_delete(app: int, key: int, value: bool) -> None: @@ -72,9 +74,9 @@ def get_uint16(app: int, key: int) -> Optional[int]: return int.from_bytes(val, "big") -def next_counter(app: int, key: int, public: bool = False) -> Optional[int]: - return config.next_counter(app, key, public) +def next_counter(app: int, key: int, writable_locked: bool = False) -> int: + return config.next_counter(app, key, writable_locked) -def set_counter(app: int, key: int, count: int, public: bool = False) -> None: - config.set_counter(app, key, count, public) +def set_counter(app: int, key: int, count: int, writable_locked: bool = False) -> None: + config.set_counter(app, key, count, writable_locked) diff --git a/core/src/storage/device.py b/core/src/storage/device.py index 55e7faf61..1e3bc4fd4 100644 --- a/core/src/storage/device.py +++ b/core/src/storage/device.py @@ -243,12 +243,12 @@ def set_autolock_delay_ms(delay_ms: int) -> None: common.set(_NAMESPACE, _AUTOLOCK_DELAY_MS, delay_ms.to_bytes(4, "big")) -def next_u2f_counter() -> Optional[int]: - return common.next_counter(_NAMESPACE, U2F_COUNTER, True) # writable when locked +def next_u2f_counter() -> int: + return common.next_counter(_NAMESPACE, U2F_COUNTER, writable_locked=True) def set_u2f_counter(count: int) -> None: - common.set_counter(_NAMESPACE, U2F_COUNTER, count, True) # writable when locked + common.set_counter(_NAMESPACE, U2F_COUNTER, count, writable_locked=True) def set_slip39_identifier(identifier: int) -> None: diff --git a/core/tests/test_storage.py b/core/tests/test_storage.py index c0b4023e7..f7a936ba8 100644 --- a/core/tests/test_storage.py +++ b/core/tests/test_storage.py @@ -15,8 +15,6 @@ class TestConfig(unittest.TestCase): self.assertEqual(device.next_u2f_counter(), i) device.set_u2f_counter(0) self.assertEqual(device.next_u2f_counter(), 1) - device.set_u2f_counter(None) - self.assertEqual(device.next_u2f_counter(), 0) if __name__ == '__main__': diff --git a/core/tests/test_trezor.config.py b/core/tests/test_trezor.config.py index 14e95e03c..340c0c04a 100644 --- a/core/tests/test_trezor.config.py +++ b/core/tests/test_trezor.config.py @@ -92,7 +92,7 @@ class TestConfig(unittest.TestCase): # The APP namespace which is reserved for storage related values is inaccessible even # when unlocked. - with self.assertRaises(RuntimeError): + with self.assertRaises(ValueError): config.set(PINAPP, PINKEY, b'value') self.assertTrue(config.change_pin(old_pin, new_pin, None, None)) @@ -106,7 +106,8 @@ class TestConfig(unittest.TestCase): # The APP namespace which is reserved for storage related values is inaccessible even # when unlocked. - self.assertEqual(config.get(PINAPP, PINKEY), None) + with self.assertRaises(ValueError): + config.get(PINAPP, PINKEY) # Old PIN cannot be used to unlock storage. if old_pin != new_pin: @@ -169,6 +170,25 @@ class TestConfig(unittest.TestCase): value2 = config.get(appid, key) self.assertEqual(value, value2) + # Test value deletion. + self.assertTrue(config.delete(appid, key)) + self.assertIsNone(config.get(appid, key)) + self.assertFalse(config.delete(appid, key)) + + # Test get/set for APP out ouf range. + + with self.assertRaises(ValueError): + config.set(0, 1, b'test') + + with self.assertRaises(ValueError): + config.get(0, 1) + + with self.assertRaises(ValueError): + config.set(192, 1, b'test') + + with self.assertRaises(ValueError): + config.get(192, 1) + def test_compact(self): config.init() config.wipe() diff --git a/storage/storage.h b/storage/storage.h index 7ff837481..2c8965982 100644 --- a/storage/storage.h +++ b/storage/storage.h @@ -34,8 +34,8 @@ // can be written even when the storage is locked. #define FLAGS_WRITE 0xC0 -// Mask for extracting the "real" app_id. -#define FLAGS_APPID 0x3F +// The maximum value of app_id which is the six least significant bits of APP. +#define MAX_APPID 0x3F // The PIN value corresponding to an empty PIN. extern const uint8_t *PIN_EMPTY;