GNOME Bugzilla – Bug 728733
insufficient input validation in GDBusMenuModel
Last modified: 2018-05-24 16:28:00 UTC
https://errors.ubuntu.com/problem/648b0ee396d66bc960c224a5794f5c22850b51ed shows some crashes caused by insufficient input validation on GDBusMenuModel. This code was never written with security in mind but it's pretty clear that invalid input from across D-Bus should not cause crashes. It's also scary that we plan to use this code (some day) inside of the shell to speak with sandboxed applications that want to publish app menus... In this particular case, we don't adequately inspect the position/removed/added fields on changed signals for sanity. We actually make sure that position+removed is in range, but when position is -1 and removed is 1, that's 0...
the crux of the trace (which is apparently behind a mandatory login):
+ Trace 233510
There's an additional (I think related) crash here (http://paste.ubuntu.com/7307883/) where the index looks more sane. In this case it says that a new item as been added at position 7. But when we ask the local copy of the menu for the menu item at index 7, it fails to find the item's property table (asserts code should not be reached).
Obviously we should be bounds checking indexes in the client (and we will be from now on), I'm just puzzled why the local copy of the exported menu hasn't successfully updated to include the new item.
Created attachment 275298 [details] [review] gdbusmenumodel: validate input from dbus calls
Is it worth adding a validation check for the removed parameter, too?
Sorry, I meant the added parameter.
Created attachment 275309 [details] [review] gmenumodel: disallow exporting large menus on the bus
> Is it worth adding a validation check for the added parameter, too? The added parameter is a variant list containing the items to add. But you make a good point. Allowing arbitrarily sized menus is a bit crazy. I've updated the patch to disallow sections with more than 1000 items. That makes the validation checks easier, as there's no need to check for overflow.
That sounds like an excellent idea. The other question I have, is it possible to push back errors to the misbehaving GTK app somehow? i.e. so the application developer / user can see that it is doing something screwy with menus. (I have to assume at this point that it's a GTK app causing this, rather than someone who's reverse engineered the menumodel DBus protocol)
(In reply to comment #9) > The other question I have, is it possible to push back errors to the > misbehaving GTK app somehow? i.e. so the application developer / user can see > that it is doing something screwy with menus. Not really, because we're doing these checks in response to a D-Bus signal and after calling a method ourselves. However, this patch makes use of (and is blocked by) g_dbus_warning(), which will print a descriptive message including the sender and its PID.
(In reply to comment #10) > (In reply to comment #9) > > The other question I have, is it possible to push back errors to the > > misbehaving GTK app somehow? i.e. so the application developer / user can see > > that it is doing something screwy with menus. > > Not really, because we're doing these checks in response to a D-Bus signal and > after calling a method ourselves. However, this patch makes use of (and is > blocked by) g_dbus_warning(), which will print a descriptive message including > the sender and its PID. I understand that, I more just meant, should we be adding the same validation on the server end? With g_warning / g_error whatever is appropriate. The idea being not to stop malicious code, but to help prevent mistakes being made.
It does print those warnings on the server side :)
Yay! Okay, color me happy then!
Looks like a useful patch - sadly it doesn't compile here: gdbusmenumodel.c:604:7: error: implicit declaration of function 'g_dbus_warning' [-Werror=implicit-function-declaration] g_dbus_warning ("invalid arguments");
(In reply to Matthias Clasen from comment #14) > Looks like a useful patch - sadly it doesn't compile here: That's because we never got around to merging g_dbus_warning (bug #728734).
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/861.