Merge branch '861-dbus-menu-model-robustness' into 'main'

gmenumodel: disallow exporting large menus on the bus

Closes #861

See merge request GNOME/glib!3133

docs/reference/gio/gio-sections-common.txt

@@ -4328,6 +4328,7 @@ g_power_profile_level_get_type
 
 <SECTION>
 <FILE>gmenuexporter</FILE>
+G_MENU_EXPORTER_MAX_SECTION_SIZE
 g_dbus_connection_export_menu_model
 g_dbus_connection_unexport_menu_model
 </SECTION>

gio/gdbusmenumodel.c

@@ -23,6 +23,7 @@
 
 #include "gdbusmenumodel.h"
 
+#include "gmenuexporter.h"
 #include "gmenumodel.h"
 
 /* Prelude {{{1 */
@@ -580,6 +581,8 @@ g_dbus_menu_group_deactivate (GDBusMenuGroup *group)
     }
 }
 
+/* @menu_id, @position, @removed and @added are all untrusted since they can
+ * come from an external process. */
 static void
 g_dbus_menu_group_changed (GDBusMenuGroup *group,
                            guint           menu_id,
@@ -593,6 +596,20 @@ g_dbus_menu_group_changed (GDBusMenuGroup *group,
   GSequence *items;
   GVariant *item;
   gint n_added;
+  gint n_items;
+
+  /* Caller has to check this. */
+  g_assert (g_variant_is_of_type (added, G_VARIANT_TYPE ("aa{sv}")));
+
+  n_added = g_variant_n_children (added);
+
+  if (position < 0 || position >= G_MENU_EXPORTER_MAX_SECTION_SIZE ||
+      removed < 0 || removed >= G_MENU_EXPORTER_MAX_SECTION_SIZE ||
+      n_added >= G_MENU_EXPORTER_MAX_SECTION_SIZE)
+    {
+      g_warning ("invalid arguments");
+      return;
+    }
 
   /* We could have signals coming to us when we're not active (due to
    * some other process having subscribed to this group) or when we're
@@ -611,9 +628,17 @@ g_dbus_menu_group_changed (GDBusMenuGroup *group,
       g_hash_table_insert (group->menus, GINT_TO_POINTER (menu_id), items);
     }
 
-  point = g_sequence_get_iter_at_pos (items, position + removed);
+  /* Don’t need to worry about overflow due to the low value of
+   * %G_MENU_EXPORTER_MAX_SECTION_SIZE. */
+  n_items = g_sequence_get_length (items);
+  if (position + removed > n_items ||
+      n_items - removed + n_added > G_MENU_EXPORTER_MAX_SECTION_SIZE)
+    {
+      g_warning ("invalid arguments");
+      return;
+    }
 
-  g_return_if_fail (point != NULL);
+  point = g_sequence_get_iter_at_pos (items, position + removed);
 
   if (removed)
     {
@@ -623,7 +648,7 @@ g_dbus_menu_group_changed (GDBusMenuGroup *group,
       g_sequence_remove_range (start, point);
     }
 
-  n_added = g_variant_iter_init (&iter, added);
+  g_variant_iter_init (&iter, added);
   while (g_variant_iter_loop (&iter, "@a{sv}", &item))
     g_sequence_insert_before (point, g_dbus_menu_group_create_item (item));
 

gio/gmenuexporter.c

@@ -252,10 +252,17 @@ g_menu_exporter_menu_items_changed (GMenuModel *model,
   GMenuExporterMenu *menu = user_data;
   GSequenceIter *point;
   gint i;
+  gint n_items;
 
   g_assert (menu->model == model);
   g_assert (menu->item_links != NULL);
-  g_assert (position + removed <= g_sequence_get_length (menu->item_links));
+
+  n_items = g_sequence_get_length (menu->item_links);
+  g_assert (position >= 0 && position < G_MENU_EXPORTER_MAX_SECTION_SIZE);
+  g_assert (removed >= 0 && removed < G_MENU_EXPORTER_MAX_SECTION_SIZE);
+  g_assert (added < G_MENU_EXPORTER_MAX_SECTION_SIZE);
+  g_assert (position + removed <= n_items);
+  g_assert (n_items - removed + added < G_MENU_EXPORTER_MAX_SECTION_SIZE);
 
   point = g_sequence_get_iter_at_pos (menu->item_links, position + removed);
   g_sequence_remove_range (g_sequence_get_iter_at_pos (menu->item_links, position), point);
@@ -770,6 +777,10 @@ g_menu_exporter_method_call (GDBusConnection       *connection,
  * constraint is violated, the export will fail and 0 will be
  * returned (with @error set accordingly).
  *
+ * Exporting menus with sections containing more than
+ * %G_MENU_EXPORTER_MAX_SECTION_SIZE items is not supported and results in
+ * undefined behavior.
+ *
  * You can unexport the menu model using
  * g_dbus_connection_unexport_menu_model() with the return value of
  * this function.

gio/gmenuexporter.h

@@ -27,6 +27,19 @@
 
 G_BEGIN_DECLS
 
+/**
+ * G_MENU_EXPORTER_MAX_SECTION_SIZE:
+ *
+ * The maximum number of entries in a menu section supported by
+ * g_dbus_connection_export_menu_model().
+ *
+ * The exact value of the limit may change in future GLib versions.
+ *
+ * Since: 2.76
+ */
+#define G_MENU_EXPORTER_MAX_SECTION_SIZE 1000 \
+  GIO_AVAILABLE_MACRO_IN_2_76
+
 GIO_AVAILABLE_IN_2_32
 guint                   g_dbus_connection_export_menu_model             (GDBusConnection  *connection,
                                                                          const gchar      *object_path,

gio/tests/gmenumodel.c

@@ -1500,6 +1500,149 @@ test_menuitem (void)
   g_object_unref (submenu);
 }
 
+static GDBusInterfaceInfo *
+org_gtk_Menus_get_interface (void)
+{
+  static GDBusInterfaceInfo *interface_info;
+
+  if (interface_info == NULL)
+    {
+      GError *error = NULL;
+      GDBusNodeInfo *info;
+
+      info = g_dbus_node_info_new_for_xml ("<node>"
+                                           "  <interface name='org.gtk.Menus'>"
+                                           "    <method name='Start'>"
+                                           "      <arg type='au' name='groups' direction='in'/>"
+                                           "      <arg type='a(uuaa{sv})' name='content' direction='out'/>"
+                                           "    </method>"
+                                           "    <method name='End'>"
+                                           "      <arg type='au' name='groups' direction='in'/>"
+                                           "    </method>"
+                                           "    <signal name='Changed'>"
+                                           "      arg type='a(uuuuaa{sv})' name='changes'/>"
+                                           "    </signal>"
+                                           "  </interface>"
+                                           "</node>", &error);
+      if (info == NULL)
+        g_error ("%s\n", error->message);
+      interface_info = g_dbus_node_info_lookup_interface (info, "org.gtk.Menus");
+      g_assert (interface_info != NULL);
+      g_dbus_interface_info_ref (interface_info);
+      g_dbus_node_info_unref (info);
+    }
+
+  return interface_info;
+}
+
+static void
+g_menu_exporter_method_call (GDBusConnection       *connection,
+                             const gchar           *sender,
+                             const gchar           *object_path,
+                             const gchar           *interface_name,
+                             const gchar           *method_name,
+                             GVariant              *parameters,
+                             GDBusMethodInvocation *invocation,
+                             gpointer               user_data)
+{
+  const struct {
+    guint position;
+    guint removed;
+  } data[] = {
+      { -2, 4 },
+      { 0, 3 },
+      { 4, 1 }
+  };
+  gsize i;
+  GError *error = NULL;
+
+  g_dbus_method_invocation_return_value (invocation, g_variant_new_parsed ("@(a(uuaa{sv})) ([(0, 0, [{ 'label': <'test'> }])],)"));
+
+  /* invalid signatures */
+  g_dbus_connection_emit_signal (connection, sender, "/", "org.gtk.Menus", "Changed",
+                                 g_variant_new_parsed ("([(1, 2, 3)],)"), &error);
+  g_assert_no_error (error);
+
+  /* add an item at an invalid position */
+  g_test_expect_message (G_LOG_DOMAIN, G_LOG_LEVEL_WARNING, "*invalid*");
+  g_dbus_connection_emit_signal (connection, sender, "/", "org.gtk.Menus", "Changed",
+                                 g_variant_new_parsed ("@(a(uuuuaa{sv})) ([(%u, %u, %u, %u, [{ 'label': <'test'> }])],)", 0, 0, 2, 0),
+                                 &error);
+  g_assert_no_error (error);
+
+  for (i = 0; i < G_N_ELEMENTS (data); i++)
+    {
+      GVariant *params;
+
+      g_test_expect_message (G_LOG_DOMAIN, G_LOG_LEVEL_WARNING, "*invalid*");
+      params = g_variant_new_parsed ("@(a(uuuuaa{sv})) ([(%u, %u, %u, %u, [])],)", 0, 0, data[i].position, data[i].removed);
+      g_dbus_connection_emit_signal (connection, sender, "/", "org.gtk.Menus", "Changed", params, &error);
+      g_assert_no_error (error);
+    }
+}
+
+static void
+menu_changed (GMenuModel *menu,
+             gint        position,
+              gint        removed,
+              gint        added,
+              gpointer    user_data)
+{
+  unsigned int *counter = user_data;
+
+  *counter += 1;
+}
+
+static void
+test_input_validation (void)
+{
+  const GDBusInterfaceVTable vtable = {
+    g_menu_exporter_method_call, NULL, NULL, { NULL, }
+  };
+  GError *error = NULL;
+  GDBusConnection *bus;
+  GDBusMenuModel *proxy;
+  guint id;
+  const gchar *bus_name;
+  GMainLoop *loop;
+  unsigned int n_signal_emissions = 0;
+  gulong signal_id;
+
+  g_test_bug ("https://gitlab.gnome.org/GNOME/glib/-/issues/861");
+
+  bus = g_bus_get_sync (G_BUS_TYPE_SESSION, NULL, &error);
+  g_assert_no_error (error);
+
+  id = g_dbus_connection_register_object (bus, "/", org_gtk_Menus_get_interface (),
+                                          &vtable, NULL, NULL, &error);
+  g_assert_no_error (error);
+
+  bus_name = g_dbus_connection_get_unique_name (bus);
+  proxy = g_dbus_menu_model_get (bus, bus_name, "/");
+
+  signal_id = g_signal_connect (proxy, "items-changed", G_CALLBACK (menu_changed), &n_signal_emissions);
+
+  /* get over laziness */
+  g_menu_model_get_n_items (G_MENU_MODEL (proxy));
+
+  loop = g_main_loop_new (NULL, FALSE);
+  g_timeout_add (100, stop_loop, loop);
+  g_main_loop_run (loop);
+
+  /* "items-changed" should only be emitted for the initial contents of
+   * the menu. Subsequent calls are all invalid.
+   */
+  g_assert_cmpuint (n_signal_emissions, ==, 1);
+
+  g_test_assert_expected_messages ();
+
+  g_main_loop_unref (loop);
+  g_dbus_connection_unregister_object (bus, id);
+  g_signal_handler_disconnect (proxy, signal_id);
+  g_object_unref (proxy);
+  g_object_unref (bus);
+}
+
 /* Epilogue {{{1 */
 int
 main (int argc, char **argv)
@@ -1523,6 +1666,7 @@ main (int argc, char **argv)
   g_test_add_func ("/gmenu/mutable", test_mutable);
   g_test_add_func ("/gmenu/convenience", test_convenience);
   g_test_add_func ("/gmenu/menuitem", test_menuitem);
+  g_test_add_func ("/gmenu/input-validation", test_input_validation);
 
   ret = g_test_run ();