fix(core): Improve error handling and range checking in modtrezorconfig.

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) {
+  mp_uint_t count = trezor_obj_get_uint(args[2]);
-    if (sectrue != storage_delete(appkey)) {
+  if (count > UINT32_MAX || sectrue != storage_set_counter(appkey, count)) {
-      return mp_const_false;
+    mp_raise_msg(&mp_type_RuntimeError, "Failed to set value in storage.");
-    }
-  } else {
-    uint32_t count = trezor_obj_get_uint(args[2]);
-    if (sectrue != storage_set_counter(appkey, count)) {
-      return mp_const_false;
-    }
   }
-  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);
 }

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.

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:
+def delete(
-    config.delete(app, key, public)
+    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]:
+def next_counter(app: int, key: int, writable_locked: bool = False) -> int:
-    return config.next_counter(app, key, public)
+    return config.next_counter(app, key, writable_locked)
 
 
-def set_counter(app: int, key: int, count: int, public: bool = False) -> None:
+def set_counter(app: int, key: int, count: int, writable_locked: bool = False) -> None:
-    config.set_counter(app, key, count, public)
+    config.set_counter(app, key, count, writable_locked)

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]:
+def next_u2f_counter() -> int:
-    return common.next_counter(_NAMESPACE, U2F_COUNTER, True)  # writable when locked
+    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:

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__':

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()

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.
+// The maximum value of app_id which is the six least significant bits of APP.
-#define FLAGS_APPID 0x3F
+#define MAX_APPID 0x3F
 
 // The PIN value corresponding to an empty PIN.
 extern const uint8_t *PIN_EMPTY;