Bug 101567 - bus driver refactoring
Summary: bus driver refactoring
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: git master
Hardware: Other All
: medium enhancement
Assignee: Simon McVittie
QA Contact: D-Bus Maintainers
URL: https://github.com/smcv/dbus/commits/...
Whiteboard: review+
Keywords: patch
Depends on:
Blocks: 101354
  Show dependency treegraph
 
Reported: 2017-06-23 16:09 UTC by Simon McVittie
Modified: 2017-06-29 18:41 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
driver: Share bus_driver_get_conn_helper with other modules (4.94 KB, patch)
2017-06-23 16:10 UTC, Simon McVittie
Details | Splinter Review
driver: Use a data-driven approach to marking methods as privileged (3.39 KB, patch)
2017-06-23 16:10 UTC, Simon McVittie
Details | Splinter Review
driver: Make eavesdropping a privileged action (1.54 KB, patch)
2017-06-23 16:16 UTC, Simon McVittie
Details | Splinter Review
bus_driver_send_ack_reply: Make available to other modules (3.71 KB, patch)
2017-06-23 16:24 UTC, Simon McVittie
Details | Splinter Review
spec: Do not promise match rules with eavesdrop='true' can be added (2.05 KB, patch)
2017-06-29 15:36 UTC, Simon McVittie
Details | Splinter Review
spec: Formally deprecate eavesdropping (5.12 KB, patch)
2017-06-29 15:36 UTC, Simon McVittie
Details | Splinter Review
spec: Document versioning of eavesdrop='true' (1.19 KB, patch)
2017-06-29 15:36 UTC, Simon McVittie
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Simon McVittie 2017-06-23 16:09:27 UTC
This bug collects some bus driver refactoring that I did for Bug #101354.
Comment 1 Simon McVittie 2017-06-23 16:10:24 UTC
Created attachment 132159 [details] [review]
driver: Share bus_driver_get_conn_helper with other  modules

Now that we're starting to implement methods in more places, it makes
sense to share this code. The Stats interface can already benefit.
Comment 2 Simon McVittie 2017-06-23 16:10:37 UTC
Created attachment 132160 [details] [review]
driver: Use a data-driven approach to marking methods  as privileged
Comment 3 Simon McVittie 2017-06-23 16:16:50 UTC
Created attachment 132164 [details] [review]
driver: Make eavesdropping a privileged action

Eavesdropping on unicast messages to other processes is not something
that should be done by processes in containers, or on the system bus
by users other than root or the bus owner. bus/system.conf.in
does not enable eavesdropping, but adding inadvisable configuration
could. This brings it into line with Monitoring.
Comment 4 Simon McVittie 2017-06-23 16:24:37 UTC
Created attachment 132168 [details] [review]
bus_driver_send_ack_reply: Make available to other  modules
Comment 5 Philip Withnall 2017-06-27 18:48:46 UTC
Comment on attachment 132159 [details] [review]
driver: Share bus_driver_get_conn_helper with other  modules

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

++
Comment 6 Philip Withnall 2017-06-27 18:51:31 UTC
Comment on attachment 132160 [details] [review]
driver: Use a data-driven approach to marking methods  as privileged

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

::: bus/driver.c
@@ -1083,4 @@
>  
>    _DBUS_ASSERT_ERROR_IS_CLEAR (error);
>  
> -#ifdef DBUS_UNIX

Is it OK that we’re dropping this DBUS_UNIX check? bus_driver_check_caller_is_privileged() does have code for non-Unix platforms, so this is a behaviour change as far as I can see.
Comment 7 Philip Withnall 2017-06-27 18:53:16 UTC
Comment on attachment 132164 [details] [review]
driver: Make eavesdropping a privileged action

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

This might break some esoteric (and arguably incorrect) configurations, right? Are you OK with that? I am.
Comment 8 Philip Withnall 2017-06-27 18:53:40 UTC
Comment on attachment 132168 [details] [review]
bus_driver_send_ack_reply: Make available to other  modules

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

r+
Comment 9 Simon McVittie 2017-06-29 11:56:34 UTC
(In reply to Philip Withnall from comment #6)
> Is it OK that we’re dropping this DBUS_UNIX check?
> bus_driver_check_caller_is_privileged() does have code for non-Unix
> platforms, so this is a behaviour change as far as I can see.

I think this is fine.

The only non-Unix platform that we make any attempt to support is Windows (DBUS_UNIX really means "not Windows").

On Windows, the only bus we support is the session bus, which is only accessible by the same SID (uid-equivalent) as the dbus-daemon anyway.

The code that later became bus_driver_check_caller_is_privileged() was added to mitigate issues like CVE-2014-8148 <http://www.openwall.com/lists/oss-security/2015/01/05/2>. It looks as though I probably only added this #ifdef because it was a minimal security-hardening patch being done under embargo, and I didn't want to write untested Windows code without public review. When I factored out bus_driver_check_caller_is_privileged() in Bug #88810, I added the Windows code path.
Comment 10 Simon McVittie 2017-06-29 12:08:27 UTC
(In reply to Philip Withnall from comment #7)
> driver: Make eavesdropping a privileged action
...
> This might break some esoteric (and arguably incorrect) configurations,
> right? Are you OK with that? I am.

Yes. Now that we have BecomeMonitor(), I consider legacy eavesdropping to be A Bad Thing. Bug #46787 explains some of the ways in which it is bizarre and broken, particularly on buses where this change would matter (it matters on the system bus, but not the session bus).

Also, we broke legacy eavesdropping more seriously than this in 1.5.x for for Bug #37890, and nobody complained very loudly.
Comment 11 Philip Withnall 2017-06-29 12:20:22 UTC
(In reply to Simon McVittie from comment #9)
> (In reply to Philip Withnall from comment #6)
> > Is it OK that we’re dropping this DBUS_UNIX check?
> > bus_driver_check_caller_is_privileged() does have code for non-Unix
> > platforms, so this is a behaviour change as far as I can see.
> 
> I think this is fine.
> 
> The only non-Unix platform that we make any attempt to support is Windows
> (DBUS_UNIX really means "not Windows").
> 
> On Windows, the only bus we support is the session bus, which is only
> accessible by the same SID (uid-equivalent) as the dbus-daemon anyway.
> 
> The code that later became bus_driver_check_caller_is_privileged() was added
> to mitigate issues like CVE-2014-8148
> <http://www.openwall.com/lists/oss-security/2015/01/05/2>. It looks as
> though I probably only added this #ifdef because it was a minimal
> security-hardening patch being done under embargo, and I didn't want to
> write untested Windows code without public review. When I factored out
> bus_driver_check_caller_is_privileged() in Bug #88810, I added the Windows
> code path.

r+

(In reply to Simon McVittie from comment #10)
> (In reply to Philip Withnall from comment #7)
> > driver: Make eavesdropping a privileged action
> ...
> > This might break some esoteric (and arguably incorrect) configurations,
> > right? Are you OK with that? I am.
> 
> Yes. Now that we have BecomeMonitor(), I consider legacy eavesdropping to be
> A Bad Thing. Bug #46787 explains some of the ways in which it is bizarre and
> broken, particularly on buses where this change would matter (it matters on
> the system bus, but not the session bus).
> 
> Also, we broke legacy eavesdropping more seriously than this in 1.5.x for
> for Bug #37890, and nobody complained very loudly.

r+, let’s break things. Probably best to mention this in the NEWS file.
Comment 12 Simon McVittie 2017-06-29 15:26:13 UTC
I realised after pushing them that I put the wrong bug reference in the patches for Bug #101566. So, for any future maintainers looking up the history of a commit and wondering what this bug has to do with unix:dir=foo or test/loopback: please see Bug #101566 for those.
Comment 13 Simon McVittie 2017-06-29 15:36:06 UTC
Created attachment 132345 [details] [review]
spec: Do not promise match rules with eavesdrop='true'  can be added

This is no longer true, and it seems less misleading to raise an
error than to obey the letter of the spec by quietly ignoring calls
from an inappropriate caller.
Comment 14 Simon McVittie 2017-06-29 15:36:38 UTC
Created attachment 132346 [details] [review]
spec: Formally deprecate eavesdropping

---

NEWS already said eavesdropping was deprecated since 1.10, but the spec didn't.
Comment 15 Simon McVittie 2017-06-29 15:36:55 UTC
Created attachment 132347 [details] [review]
spec: Document versioning of eavesdrop='true'

The wording and formatting used here is consistent with other
semi-recently-added match keys.
Comment 16 Simon McVittie 2017-06-29 15:37:43 UTC
I've pushed the implementation commits for 1.11.14. The follow-up commits to the spec would also be nice to have.
Comment 17 Philip Withnall 2017-06-29 16:01:27 UTC
Comment on attachment 132345 [details] [review]
spec: Do not promise match rules with eavesdrop='true'  can be added

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

r+
Comment 18 Philip Withnall 2017-06-29 16:04:30 UTC
Comment on attachment 132346 [details] [review]
spec: Formally deprecate eavesdropping

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

A couple of nitpicks, but r+ otherwise. Feel free to push with (or without, if you don’t care) those fixes applied.

::: doc/dbus-specification.xml
@@ +4361,5 @@
>          </para>
>  
>          <para>
> +          Eavesdropping interacts poorly with buses with non-trivial
> +          access control restrictions, and is deprecated. The BecomeMonitor

Nitpick: Add a <literal> around BecomeMonitor.

@@ +4405,5 @@
>          <para>
>            Match rules can also be used for eavesdropping
>            (see <xref linkend="message-bus-routing-eavesdropping"/>),
> +          if the security policy of the message bus allows it, but this
> +          usage is deprecated in favour of the BecomeMonitor method

Nitpick: Add a <literal> around BecomeMonitor.

@@ +4632,5 @@
> +                    </para>
> +                    <para>
> +                      Use of <literal>eavesdrop='true'</literal> is
> +                      deprecated. Monitors should prefer to use the
> +                      BecomeMonitor method (see

Nitpick: Add a <literal> around BecomeMonitor.
Comment 19 Philip Withnall 2017-06-29 16:04:59 UTC
Comment on attachment 132347 [details] [review]
spec: Document versioning of eavesdrop='true'

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

r+
Comment 20 Simon McVittie 2017-06-29 18:41:56 UTC
Thanks for the reviews. These will be in 1.11.14.


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.