Bug 92298 - DBus session bus crashes due to specially crafted call of 'dbus-monitor'
Summary: DBus session bus crashes due to specially crafted call of 'dbus-monitor'
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: 1.10
Hardware: x86-64 (AMD64) Linux (All)
: medium normal
Assignee: Simon McVittie
QA Contact: D-Bus Maintainers
URL:
Whiteboard: review? not a security flaw
Keywords: patch
Depends on:
Blocks:
 
Reported: 2015-10-05 20:03 UTC by Thomas
Modified: 2015-10-27 10:56 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
BecomeMonitor: do not overwrite error with another error (1.23 KB, patch)
2015-10-06 11:50 UTC, Simon McVittie
Details | Splinter Review
Add a regression test for invalid BecomeMonitor calls (5.10 KB, patch)
2015-10-06 11:51 UTC, Simon McVittie
Details | Splinter Review
Add a regression test for invalid BecomeMonitor calls (5.13 KB, patch)
2015-10-06 11:57 UTC, Simon McVittie
Details | Splinter Review
Keep cmake build system in sync with autotools (add test-monitor) (1.16 KB, patch)
2015-10-21 11:21 UTC, Ralf Habacker
Details | Splinter Review
Add a regression test for invalid BecomeMonitor calls (5.21 KB, patch)
2015-10-21 11:35 UTC, Simon McVittie
Details | Splinter Review
attachment-24989-0.html (3.61 KB, text/html)
2015-10-26 22:30 UTC, Ralf Habacker
Details

Description Thomas 2015-10-05 20:03:09 UTC
I am able to kill the user's session bus reproducible by running the following command line:

dbus-monitor --profile "interface='org.kde.walletd',member='/modules/kwalletd/org.kde.KWallet/walletOpened'"

in a KDE session (mostly KDE4/Plasma 4) under Arch Linux. Program 'dbus-monitor' is part of Arch Linux's package 'core/dbus' which is installed in version 1.10.0-3.
A bug has been reported in Arch Linux's bug databse (FS#46535).

The command line is most likely wrong, but DBus should still not crash just because of wrongful parameters.

Once the session DBus has crash, the desktop environment is severely affected, such as that one can no longer log out properly using Plasma widgets (e.g. K Menu). My work-around has been to switch to text mode (Ctrl+Alt+Fn), log in as root, and restart the kdm service.

Please test if you can reproduce this crash.
Unfortunately, Arch Linux does not provide debug packages and my attempts to attach gdb to the dbus session bus process right before triggering the crash have not revealed any information (not an expert here).
Please let me know how to proceed.
Comment 1 Thiago Macieira 2015-10-05 21:06:59 UTC
Please recompile dbus in debug mode or at the very least with -rdynamic and no symbol stripping.

A backtrace (by anyone) would be most appreciated.

I cannot reproduce this issue using 1.18.16.

The daemon replies with error "org.freedesktop.DBus.Error.MatchRuleInvalid" and message "Member name '/modules/kwalletd/org.kde.KWallet/walletOpened' is invalid"
Comment 2 Simon McVittie 2015-10-06 11:49:49 UTC
This is a bug in the new "monitoring" code, which mostly supersedes the old "eavesdropping" in D-Bus 1.10. (Bug #46787)
Comment 3 Simon McVittie 2015-10-06 11:50:17 UTC
Created attachment 118708 [details] [review]
BecomeMonitor: do not overwrite error with another error

If the user gave us a syntactically invalid error name, we'd
overwrite the MatchRuleInvalid error with NoMemory, causing an
assertion failure (crash) in the dbus-daemon.

This is not a denial-of-service vulnerability on the system bus,
because monitoring is a privileged action, and root privilege
is checked before this code is reached. However, it's an annoying
bug on the session bus.
Comment 4 Simon McVittie 2015-10-06 11:51:58 UTC
Created attachment 118709 [details] [review]
Add a regression test for invalid BecomeMonitor calls

---

This reproduces a dbus-daemon crash, unless Attachment #118708 [details] is applied first.

This is a simplified version of the bug reporter's steps-to-reproduce. I also added a test for passing nonzero flags to BecomeMonitor, since that's the other major way for the method implementation to fail (it takes a flags parameter, but no flags are defined yet, so nonzero flags are always InvalidArgument).
Comment 5 Simon McVittie 2015-10-06 11:57:06 UTC
Created attachment 118710 [details] [review]
Add a regression test for invalid BecomeMonitor calls

---

Superseding previous patch with a couple of minor fixes:

* use the same syntactically invalid match rule that the bug reporter quoted
* remove a leftover call to dbus_error_free()
Comment 6 Simon McVittie 2015-10-06 12:00:14 UTC
(In reply to Thomas from comment #0)
> The command line is most likely wrong

Yes: I think you said "member" when you meant "path". A member is a method or signal name, like GetNameOwner or NameOwnerChanged.

> but DBus should still not crash just because of wrongful parameters

Indeed. I believe the first patch on this bug should fix it.

If this had also affected the system bus, it would have been a minor security vulnerability (denial of service); but as far as I can see, it doesn't.
Comment 7 Simon McVittie 2015-10-16 17:55:53 UTC
Any reviews on this? I'd like to get this into 1.10.2 fairly soon, and the change is minimal, so I'll merge this unreviewed if there's no feedback.
Comment 8 Ralf Habacker 2015-10-16 19:19:55 UTC
Comment on attachment 118708 [details] [review]
BecomeMonitor: do not overwrite error with another error

Review of attachment 118708 [details] [review]:
-----------------------------------------------------------------

looks good
Comment 9 Ralf Habacker 2015-10-16 20:51:15 UTC
Comment on attachment 118710 [details] [review]
Add a regression test for invalid BecomeMonitor calls

Review of attachment 118710 [details] [review]:
-----------------------------------------------------------------

Running this test after adding the missing " on the end of line 592 returns on windows using wine:
/monitor/invalid: **
ERROR:../../dbus/test/monitor.c:572:test_invalid: assertion failed (dbus_message_get_error_name (m) == DBUS_ERROR_INVALID_ARGS): ("org.freedesktop.DBus.Error.UnknownInterface" == "org.freedesktop.DBus.Error.InvalidArgs")

abnormal program termination

Is this an expected test result ?

::: test/monitor.c
@@ +588,5 @@
> +  if (!dbus_message_iter_open_container (&appender, DBUS_TYPE_ARRAY, "s",
> +        &array_appender))
> +    g_error ("OOM");
> +
> +  s = "interface='org.kde.walletd',member='/modules/kwalletd/org.kde.KWallet/walletOpened';

../../dbus/test/monitor.c: In function 'test_invalid':
../../dbus/test/monitor.c:592:7: warning: missing terminating " character
   s = "interface='org.kde.walletd',member='/modules/kwalletd/org.kde.KWallet/walletOpened';
       ^
../../dbus/test/monitor.c:592:3: error: missing terminating " character
   s = "interface='org.kde.walletd',member='/modules/kwalletd/org.kde.KWallet/walletOpened';
   ^
../../dbus/test/monitor.c:594:3: error: expected expression before 'if'
   if (!dbus_message_iter_append_basic (&array_appender, DBUS_TYPE_STRING,
   ^
Comment 10 Philip Withnall 2015-10-16 21:52:02 UTC
Comment on attachment 118710 [details] [review]
Add a regression test for invalid BecomeMonitor calls

Review of attachment 118710 [details] [review]:
-----------------------------------------------------------------

::: test/monitor.c
@@ +546,5 @@
> +    g_error ("OOM");
> +
> +  if (!dbus_message_iter_close_container (&appender, &array_appender) ||
> +      !dbus_message_iter_append_basic (&appender, DBUS_TYPE_UINT32,
> +        &invalid_flags))

You could combine these two `if` statements.

@@ +595,5 @@
> +        &s))
> +    g_error ("OOM");
> +
> +  if (!dbus_message_iter_close_container (&appender, &array_appender) ||
> +      !dbus_message_iter_append_basic (&appender, DBUS_TYPE_UINT32, &zero))

Could combine these two too.
Comment 11 Philip Withnall 2015-10-16 21:54:08 UTC
Comment on attachment 118708 [details] [review]
BecomeMonitor: do not overwrite error with another error

Review of attachment 118708 [details] [review]:
-----------------------------------------------------------------

Looks good to me.
Comment 12 Simon McVittie 2015-10-19 15:12:59 UTC
(In reply to Ralf Habacker from comment #9)
> ../../dbus/test/monitor.c:592:7: warning: missing terminating " character

Oops, yes. I thought I'd tested this version of the patch, but apparently I didn't.

> (dbus_message_get_error_name (m) == DBUS_ERROR_INVALID_ARGS):
> ("org.freedesktop.DBus.Error.UnknownInterface" ==
> "org.freedesktop.DBus.Error.InvalidArgs")
...
> Is this an expected test result ?

No, this is a test failure. However, it doesn't indicate that the patch is wrong, either; it indicates that you are testing against a pre-1.10 dbus-daemon that didn't have the new monitoring API. I think this is because some of the GLib-based tests default to using "autolaunch:" which is global to a machine.

I'll open a separate bug for these tests not working correctly in Windows builds and under Wine.
Comment 13 Simon McVittie 2015-10-19 15:49:52 UTC
(In reply to Simon McVittie from comment #12)
> I'll open a separate bug for these tests not working correctly in Windows
> builds and under Wine.

Bug #92538
Comment 14 Simon McVittie 2015-10-19 16:15:15 UTC
Comment on attachment 118708 [details] [review]
BecomeMonitor: do not overwrite error with another error

Pushed to dbus-1.10. Fixed in git for 1.10.2 and 1.11.0.

I'll leave this bug open until the regression test gets in.
Comment 15 freedesk.apriori 2015-10-19 17:58:12 UTC
I’ve run into something similar when accidentally pressing return after entering «dbus monitor interface=». So, my command-line was «dbus monitor interface=», and this promptly brought down both the user and the system dbus (and oddly, all Gtk+ application windows vanished; out of curiosity: why does that happen?). Can you advise whether this is already covered here, or should I file this separately?
Comment 16 Simon McVittie 2015-10-19 19:31:30 UTC
(In reply to freedesk.apriori from comment #15)
> I’ve run into something similar when accidentally pressing return after
> entering «dbus monitor interface=». So, my command-line was «dbus monitor
> interface=», and this promptly brought down both the user and the system
> dbus

The system part of this report is unexpected.

With dbus 1.10.0 on a non-Debian-derived distribution, I would expect this to bring down the session bus and your graphical login session; that's this bug, and does not need to be reported again. I'll release 1.10.2 soon to fix that.

dbus on Debian and its derivatives is configured to try to tolerate assertion failures, mostly for historical reasons; that patch mitigates this. I'm tempted to apply the same thing upstream, either just for dbus-daemon or for all applications, but I'll open a separate bug to discuss that.

I would not expect this to do anything to the system instance of dbus-daemon (typically running under uid "messagebus" or "dbus"). Please check your system log (syslog or systemd Journal) for log messages from the system instance of dbus-daemon when this happened. If the log confirms that the system instance of dbus-daemon crashed at the same time, please open a separate issue, with at least this information:

* the distribution you are using
* the version of dbus you had at the time of the crash, including the
  upstream version and the version of your distribution's packaging
  (for instance "Debian's dbus 1.10.0-3")
* where I can find the patches and configuration that your distribution uses
* system log messages from around the time of the crash

> and oddly, all Gtk+ application windows vanished

Applications that use the D-Bus session or system bus are expected to exit when that bus goes away, and many Gtk applications use D-Bus (via GApplication or GtkApplication, or directly), so this is not unexpected.
Comment 17 Simon McVittie 2015-10-19 19:58:47 UTC
(In reply to Simon McVittie from comment #16)
> dbus on Debian and its derivatives is configured to try to tolerate
> assertion failures, mostly for historical reasons; that patch mitigates
> this. I'm tempted to apply the same thing upstream, either just for
> dbus-daemon or for all applications, but I'll open a separate bug to discuss
> that.

Bug #92546
Comment 18 Ralf Habacker 2015-10-21 11:21:25 UTC
Created attachment 119026 [details] [review]
Keep cmake build system in sync with autotools (add test-monitor)
Comment 19 Simon McVittie 2015-10-21 11:24:41 UTC
Comment on attachment 119026 [details] [review]
Keep cmake build system in sync with autotools (add test-monitor)

Review of attachment 119026 [details] [review]:
-----------------------------------------------------------------

Fine, please apply
Comment 20 Simon McVittie 2015-10-21 11:35:30 UTC
Created attachment 119027 [details] [review]
Add a regression test for invalid BecomeMonitor calls

---

This isn't expected to work on Windows until Bug #92538 is fixed.
Comment 21 Simon McVittie 2015-10-21 11:35:43 UTC
Comment on attachment 118710 [details] [review]
Add a regression test for invalid BecomeMonitor calls

superseded
Comment 22 Ralf Habacker 2015-10-21 12:44:38 UTC
(In reply to Simon McVittie from comment #20)
> Created attachment 119027 [details] [review] [review]
> Add a regression test for invalid BecomeMonitor calls
> 
> ---
> 
> This isn't expected to work on Windows until Bug #92538 is fixed.

I appled all patches from this bug, tried to run test-monitor.exe and got

xxx@yyyy:~/src/dbus-1-cmake-cross-x86-build> DBUS_VERBOSE=1 bin/dbus-daemon.exe --session --address=tcp:host=localhost --print-address
....
Failed to start message bus: Failed to open "S:/xxx/src/dbus-1-cmake-cross-x86-build/share\dbus-1\session.conf": Pfad nicht gefunden.

The reason is that caused by commit 36d864e4697bc6e1eeffb32092bd78b108362ab5   session.conf and friends need to be created in the <build-root>/share/dbus-1/ instead of <build-root>/bus.
Comment 23 Ralf Habacker 2015-10-21 12:52:25 UTC
(In reply to Ralf Habacker from comment #22)
> (In reply to Simon McVittie from comment #20)
> > Created attachment 119027 [details] [review] [review] [review]
> > Add a regression test for invalid BecomeMonitor calls
> > 
> > ---
> > 
> > This isn't expected to work on Windows until Bug #92538 is fixed.
> 
> I appled all patches from this bug, tried to run test-monitor.exe and got
> 
> xxx@yyyy:~/src/dbus-1-cmake-cross-x86-build> DBUS_VERBOSE=1
> bin/dbus-daemon.exe --session --address=tcp:host=localhost --print-address
> ....
> Failed to start message bus: Failed to open
> "S:/xxx/src/dbus-1-cmake-cross-x86-build/share\dbus-1\session.conf": Pfad
> nicht gefunden.
> 
> The reason is that caused by commit 36d864e4697bc6e1eeffb32092bd78b108362ab5
> session.conf and friends need to be created in the
> <build-root>/share/dbus-1/ instead of <build-root>/bus.

From the cmake build root I'm able to run test-monitor with the following command line:

DBUS_TEST_DAEMON=$PWD/bin/dbus-daemon.exe DBUS_TEST_DATA=z:$PWD/test/data   bin/test-monitor.exe 
/monitor/invalid: OK
/monitor/become: OK
/monitor/broadcast: OK
/monitor/forbidden-broadcast: OK
/monitor/unicast-signal: OK
/monitor/forbidden: OK
/monitor/method-call: OK
/monitor/forbidden-method: OK
/monitor/dbus-daemon: OK
/monitor/selective: OK
/monitor/wildcard: OK
/monitor/no-rule: OK
/monitor/eavesdrop: OK
/monitor/no-eavesdrop: OK

I still got one message indicating a potential problem:
 
/monitor/become: Connection :1.0 (pid=86 comm=""sid="S-1-5-21-0-0-0-1000" ) became a monitor.
Monitoring connection :1.0 (pid=86 comm=""sid="S-1-5-21-0-0-0-1000" ) is not allowed to send messages; closing it. Please fix the monitor to not do that. (message type="method_call" interface="org.freedesktop.DBus" member="AddMatch" error name="(unset)" destination="org.freedesktop.DBus")
Monitoring connection :1.0 closed.
Comment 24 Simon McVittie 2015-10-21 14:34:10 UTC
(In reply to Ralf Habacker from comment #23)
> /monitor/become: Connection :1.0 (pid=86 comm=""sid="S-1-5-21-0-0-0-1000" )
> became a monitor.
> Monitoring connection :1.0 (pid=86 comm=""sid="S-1-5-21-0-0-0-1000" ) is not
> allowed to send messages; closing it. Please fix the monitor to not do that.
> (message type="method_call" interface="org.freedesktop.DBus"
> member="AddMatch" error name="(unset)" destination="org.freedesktop.DBus")
> Monitoring connection :1.0 closed.

That isn't a problem. One of the pieces of functionality that is exercised by /monitor/become is "after a connection becomes a monitor, it is not allowed to send any more messages" - those messages are a side-effect of the dbus-daemon under test responding to that.
Comment 25 Simon McVittie 2015-10-22 16:45:21 UTC
(In reply to Ralf Habacker from comment #23)
> From the cmake build root I'm able to run test-monitor with the following
> command line:
> 
> DBUS_TEST_DAEMON=$PWD/bin/dbus-daemon.exe DBUS_TEST_DATA=z:$PWD/test/data  
> bin/test-monitor.exe 

That's correct. Just running the test executable in the built tree with no special arguments is not necessarily expected to work: the tests are primarily designed to run under "make check" (which sets environment variables via TESTS_ENVIRONMENT in the Makefile.am), or after installation.
Comment 26 Simon McVittie 2015-10-26 12:51:51 UTC
(In reply to Simon McVittie from comment #20)
> Created attachment 119027 [details] [review]
> Add a regression test for invalid BecomeMonitor calls

I've applied this unreviewed, because it doesn't affect the non-test parts of D-Bus, nobody has said no, and test > no test.

Fixed in git for 1.10.2, 1.11.0.
Comment 27 Ralf Habacker 2015-10-26 22:30:54 UTC
Created attachment 119208 [details]
attachment-24989-0.html

Am 26.10.2015 um 13:51 schrieb bugzilla-daemon@freedesktop.org:
> Simon McVittie <mailto:simon.mcvittie@collabora.co.uk> changed bug
> 92298 <https://bugs.freedesktop.org/show_bug.cgi?id=92298>
> What 	Removed 	Added
> Status 	ASSIGNED 	RESOLVED
> Resolution 	--- 	FIXED
>
> *Comment # 26 <https://bugs.freedesktop.org/show_bug.cgi?id=92298#c26>
> on bug 92298 <https://bugs.freedesktop.org/show_bug.cgi?id=92298> from
> Simon McVittie <mailto:simon.mcvittie@collabora.co.uk> *
> (In reply to Simon McVittie from comment #20 <show_bug.cgi?id=92298#c20>)
> > Created attachment 119027 [details] [review] <attachment.cgi?id=119027> [details]
> <attachment.cgi?id=119027&action=edit> [review]
> <page.cgi?id=splinter.html&bug=92298&attachment=119027> > Add a
> regression test for invalid BecomeMonitor calls
>
> I've applied this unreviewed, because it doesn't affect the non-test parts of
> D-Bus, nobody has said no, and test > no test.
Unfortunally you applied the outdated patch, which results into compile
errors.  The correct patch is
https://bugs.freedesktop.org/attachment.cgi?id=119027

Regards
 Ralf
Comment 28 Ralf Habacker 2015-10-27 00:03:46 UTC
(In reply to Simon McVittie from comment #20)
> Created attachment 119027 [details] [review] [review]
> Add a regression test for invalid BecomeMonitor calls
> 
> ---
> 
> This isn't expected to work on Windows until Bug #92538 is fixed.

The patches in Bug #92538 are already committed ?
Comment 29 Ralf Habacker 2015-10-27 06:31:15 UTC
(In reply to Ralf Habacker from comment #27)
> Created attachment 119208 [details]
> attachment-24989-0.html
> 
> Am 26.10.2015 um 13:51 schrieb bugzilla-daemon@freedesktop.org:
> > Simon McVittie <mailto:simon.mcvittie@collabora.co.uk> changed bug
> > 92298 <https://bugs.freedesktop.org/show_bug.cgi?id=92298>
> > What 	Removed 	Added
> > Status 	ASSIGNED 	RESOLVED
> > Resolution 	--- 	FIXED
> >
> > *Comment # 26 <https://bugs.freedesktop.org/show_bug.cgi?id=92298#c26>
> > on bug 92298 <https://bugs.freedesktop.org/show_bug.cgi?id=92298> from
> > Simon McVittie <mailto:simon.mcvittie@collabora.co.uk> *
> > (In reply to Simon McVittie from comment #20 <show_bug.cgi?id=92298#c20>)
> > > Created attachment 119027 [details] [review] [review] <attachment.cgi?id=119027> [details]
> > <attachment.cgi?id=119027&action=edit> [review]
> > <page.cgi?id=splinter.html&bug=92298&attachment=119027> > Add a
> > regression test for invalid BecomeMonitor calls
> >
> > I've applied this unreviewed, because it doesn't affect the non-test parts of
> > D-Bus, nobody has said no, and test > no test.
> Unfortunally you applied the outdated patch, which results into compile
> errors.  The correct patch is
> https://bugs.freedesktop.org/attachment.cgi?id=119027
Forget this comment. I was misleaded because the patch commit id in the git repo (dc8bb270996a79be8f83c0c1dab286884b526aa)  is not the same of this patch (is91c41d0ed16b2fff51170426a42f3c326eadd67a).
Comment 30 Simon McVittie 2015-10-27 10:56:06 UTC
(In reply to Ralf Habacker from comment #28)
> The patches in Bug #92538 are already committed ?

Not all of them, but enough that test-monitor should be able to pass in 1.10.2.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.