Bug 101567

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.

Created attachment 132160 [details] [review]
driver: Use a data-driven approach to marking methods  as privileged

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.

Created attachment 132168 [details] [review]
bus_driver_send_ack_reply: Make available to other  modules

Comment on attachment 132159 [details] [review]
driver: Share bus_driver_get_conn_helper with other  modules

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

++

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 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 on attachment 132168 [details] [review]
bus_driver_send_ack_reply: Make available to other  modules

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

r+

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

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

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

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.

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.

Created attachment 132346 [details] [review]
spec: Formally deprecate eavesdropping

---

NEWS already said eavesdropping was deprecated since 1.10, but the spec didn't.

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.

I've pushed the implementation commits for 1.11.14. The follow-up commits to the spec would also be nice to have.

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 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 on attachment 132347 [details] [review]
spec: Document versioning of eavesdrop='true'

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

r+

Thanks for the reviews. These will be in 1.11.14.