Bug 78979

(Filed on behalf of Alban as he couldn't restrict it to security-only. I don't know the first thing about it, and didn't even read what I copy and pasted.)

This bug is not specific to LSMs (SELinux or AppArmor): I manage to trigger it with a D-Bus security policy alone.

My test is the following:

/etc/dbus-1/system.d/org.freedesktop.RealtimeKit1.conf:

<!DOCTYPE busconfig PUBLIC
 "-//freedesktop//DTD D-BUS Bus Configuration 1.0//EN"
 "http://www.freedesktop.org/standards/dbus/1.0/busconfig.dtd">
<busconfig>
  <policy user="rtkit">
    <allow own="org.freedesktop.RealtimeKit1"/>
  </policy>
  <policy user="root">
    <allow own="org.freedesktop.RealtimeKit1"/>
  </policy>
  <policy context="default">
    <!-- XXX: I replaced "allow" by "deny" here for my test! -->
    <deny send_destination="org.freedesktop.RealtimeKit1"/>
  </policy>
  <policy user="root">
    <allow send_destination="org.freedesktop.RealtimeKit1"/>
    <allow receive_sender="org.freedesktop.RealtimeKit1"/>
  </policy>
</busconfig>

For the purpose of the test, I forbid non-root processes (such as pulseaudio) to deliver a message to rtkit. Then, when starting pulseaudio, it attempts to send a message to rtkit: the message is not delivered (that's ok, that's what I wanted) but rtkit couldn't request its well-known name (that's not ok, that's a denial of service):

rtkit-daemon[1167]: Failed to register name on bus: Rejected send message, 2 matched rules; type="method_call", sender=":1.59" (uid=1000 pid=1093 comm="/usr/bin/pulseaudio --start --log-target=syslog ") interface="org.freedesktop.RealtimeKit1" member="MakeThreadRealtime" error name="(unset)" requested_reply="0" destination="org.freedesktop.RealtimeKit1" (uid=0 pid=1167 comm="/usr/lib/rtkit/rtkit-daemon ")

When rtkit-daemon calls RequestName(), it receives the signal "NameAcquired" successfully but RequestName() returns the D-Bus error message above that should have been delivered to Pulseaudio, and it does not receive the real D-Bus reply. So rtkit believes it failed to claim its well-known name and exits.

The patch works by sending the D-Bus error message to pulseaudio immediately in a different context (using local variables "sub_transaction" and "tmp_error") instead of hijacking the context of RequestName (parameters "transaction" and "error").

Adding de facto D-Bus security contacts to Cc

(In reply to comment #0)
> 1. The error message is delivered to the service instead of being delivered
> to
>    the sender. As an example, the error message could be something like:
> 
>      An SELinux policy prevents this sender from sending this
>      message to this recipient, [...] member="MaliciousMethod"
> 
>    So the sender could maliciously leak information to the service by
> inserting
>    the information in the member name, even though the LSM attempted to block
>    the communication between the sender and the service.

I do not consider this sort of compartmentalization to be something that D-Bus should claim to support - it isn't something we test, and isn't necessary for mainstream Linux. In my view, if you have a mixture of "normal" and "top secret" processes and you want it to be impossible for the "top secret" processes to leak information to the "normal" processes, you should be using virtual machines or containers. However, if fans of SELinux do want it, they are welcome to help support it.

So I don't consider (1) to be a vulnerability, as such...

> 2. The error message is delivered as a reply to the RequestName call from
>    service. It means the activated service will believe it cannot request the
>    name and might exit. The sender could activate the service frequently and
>    systemd will give up activating it. Thus the denial of service.

... but (2) is certainly a denial-of-service vulnerability, so this is worth addressing. I'll try to do a detailed review on this soon.

(In reply to comment #4)
> (In reply to comment #0)
> > 1. The error message is delivered to the service instead of being delivered
> > to
> >    the sender. As an example, the error message could be something like:
> > 
> >      An SELinux policy prevents this sender from sending this
> >      message to this recipient, [...] member="MaliciousMethod"
> > 
> >    So the sender could maliciously leak information to the service by
> > inserting
> >    the information in the member name, even though the LSM attempted to block
> >    the communication between the sender and the service.
> 
> I do not consider this sort of compartmentalization to be something that
> D-Bus should claim to support - it isn't something we test, and isn't
> necessary for mainstream Linux.

I don't see how this is leaking any information. You get an error message saying the message you sent was rejected, with some of the details of the message you sent. That is, the malicious sender already knew about "MaliciousMethod" in order to send a call to that method.

Once we fix the destination of the error message, the problem is gone.

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #0)
> > > 1. The error message is delivered to the service instead of being delivered
> > > to
> > >    the sender. As an example, the error message could be something like:
> > > 
> > >      An SELinux policy prevents this sender from sending this
> > >      message to this recipient, [...] member="MaliciousMethod"
> > > 
> > >    So the sender could maliciously leak information to the service by
> > > inserting
> > >    the information in the member name, even though the LSM attempted to block
> > >    the communication between the sender and the service.
> > 
> > I do not consider this sort of compartmentalization to be something that
> > D-Bus should claim to support - it isn't something we test, and isn't
> > necessary for mainstream Linux.
> 
> I don't see how this is leaking any information. You get an error message
> saying the message you sent was rejected, with some of the details of the
> message you sent. That is, the malicious sender already knew about
> "MaliciousMethod" in order to send a call to that method.

It can only leak information if the sender and the service are malicious confederates and agree on a protocol to leak the information, e.g. base64 the secret information and insert the result in the "member" field of the D-Bus message.

> Once we fix the destination of the error message, the problem is gone.

I agree.

Comment on attachment 99433 [details] [review]
resolve the error

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

Code review: looks correct, but could be simpler.

::: bus/activation.c
@@ +1203,5 @@
>                                       addressed_recipient,
> +                                     entry->activation_message, &tmp_error))
> +            {
> +              if (!bus_transaction_send_error_reply (sub_transaction, entry->connection,
> +                                                     &tmp_error, entry->activation_message))

(As a side note, that function is confusingly-named: it does not actually send anything. Instead, it reserves enough memory that sending the message later cannot fail.)

@@ +1207,5 @@
> +                                                     &tmp_error, entry->activation_message))
> +                {
> +                  bus_connection_send_oom_error (entry->connection,
> +                                                 entry->activation_message);
> +                  bus_transaction_cancel_and_free (sub_transaction);

The bug here is really: if bus_activation_send_pending_auto_activation_messages() raises a DBusError, it is sent back to the service requesting the name. That's correct if the error is OOM, but not correct if the intention was to send the error back to the process that originally requested activation instead.

So the function at least deserves a comment

/* Any DBusError raised here will make the RequestName call fail with that error,
 * so do not return a DBusError unless the whole transaction has failed. */

I'm tempted to say it should not have a DBusError parameter at all, so it's more clearly an example of the "return TRUE, or FALSE on OOM" pattern (and its only caller would have to set the OOM error instead).

In addition to fixing that, you're introducing slightly more complex transactions. Your code essentially reads: if there is not enough memory to send the error message(s) to each client that was not allowed to talk to the service, send OOM to the client(s) that hit OOM instead, and ignore those OOMs for the purposes of deciding whether RequestName() was successful or needs to be rolled back.

Not using the sub-transaction, and just using @transaction, would instead result in: if there is not enough memory to send the error message(s) to each client that was not allowed to talk to the service, RequestName() fails with OOM; the service may retry, or just exit (which is theoretically DoS I suppose, but if you're hitting OOM in dbus-daemon then your system is already being denied service).

This is all pretty theoretical - dbus-daemon shouldn't OOM under any normal circumstance - so I'm tempted to ask for the simpler version. Alban, would you mind implementing and testing that simpler version, and we can decide which is better when we can compare both patches?

Proposed advisory text:

----8<----
D-Bus <http://www.freedesktop.org/wiki/Software/dbus/> is an
asynchronous inter-process communication system, commonly used
for system services or within a desktop session on Linux and other
operating systems.

Alban Crequy discovered a denial-of-service flaw in the reference
implementation of dbus-daemon, the D-Bus message bus daemon. Additionally,
in highly unusual environments the same flaw could lead to a side channel
between processes that should not be able to communicate.

Summary:

If a client C1 is prohibited from sending a message to a service S1,
and S1 is not currently running, then C1 can attempt to send a message
to S1's well-known bus name, causing dbus-daemon to start S1 [1].
When S1 has started and obtained its well-known bus name, the
dbus-daemon evaluates its security policy, decides that it will not
deliver the message to S1, and constructs an AccessDenied error.
However, instead of sending that AccessDenied error reply to C1
as a reply to the denied message, dbus-daemon incorrectly sends it to S1
as a reply to the request to obtain its well-known bus name.

Impact 1: denial of service. S1 will fail to initialize, and exit,
denying service to legitimate clients of S1.

Impact 2: in environments where C1 and S1 are untrusted and are
administratively prohibited from communicating, S1 could also use these
incorrectly-directed error messages as a side channel to receive information
from C1.

Mitigations:

Impact 1: if a legitimate client was actively using S1, S1 would already
have been started, so C1 can only deny service to a legitimate client
that only recently became active.

Impact 2: in practice processes sharing a system bus can typically
communicate in other ways (non-D-Bus IPC mechanisms, files in /tmp, etc.),
so impact 2 is not relevant on normal systems. It might be relevant
on systems when an LSM such as SELinux is used in a highly restrictive
configuration.

Footnotes:

[1] This is perhaps unexpected, but the dbus-daemon is behaving as designed:
it cannot necessarily evaluate which security policies it should apply
to S1 until S1 has actually connected back to dbus-daemon, because S1
might change its uid, SELinux context, etc. during startup.
The conceptual model is that activatable services are always running,
and that the dbus-daemon delaying their startup until they are actually
needed is a form of lazy evaluation. As such, the D-Bus maintainers do not
consider this to be a bug or vulnerability.
----8<----

Created attachment 100282 [details] [review]
[PATCH v2] security and activation: deliver error messages correctly and fix Denial of Service

patch updated after smcv's review:
- prototype changed to remove the error parameter and let the caller set the OOM error if it retuns FALSE.
- do not use subtransaction but the same transaction.
- do not cancel, execute or free the transaction: let the caller do it.

I tested the patch in the same way as the previous one and this one works as well for me.

The proposed advisory text looks good to me.

Comment on attachment 100282 [details] [review]
[PATCH v2] security and activation: deliver error messages correctly and fix Denial of Service

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

This looks good to me, but reviews from other D-Bus maintainers would be extremely welcome.

I'll contact distros@ and see whether I can get a CVE ID before releasing.

Colin pointed out that the comment is confusingly wrong now, and suggested this:

               /* Any DBusError raised here will make the RequestName call fail
-               * with that error, so do not return a DBusError unless the
-               * whole transaction has failed. */
+               * with that error; we want to return the error to the original
+               * method invoker. */

I don't think that's really right either: maybe this?

/* If permission is denied, we just want to return the
 * error to the original method invoker; in particular,
 * we don't want to make the RequestName call fail
 * with that error (see fd.o #78979, CVE-2014-3477). */

I'll commit --amend if people are happy with that wording. I need to do that to add the CVE reference anyway.

(In reply to comment #11)

> I don't think that's really right either: maybe this?
> 
> /* If permission is denied, we just want to return the
>  * error to the original method invoker; in particular,
>  * we don't want to make the RequestName call fail
>  * with that error (see fd.o #78979, CVE-2014-3477). */

Yep, this looks good to me.

(In reply to comment #11)

> I'll commit --amend if people are happy with that wording. I need to do that
> to add the CVE reference anyway.

This looks good to me too.

Created attachment 100463 [details] [review]
CVE-2014-3477: deliver activation errors correctly, fixing  Denial of Service

Here is a revised version of Alban's patch with the adjusted comment. This is what I'm going to recommend to vendors, assuming my tests are successful. I'm preparing 1.6.20 and 1.8.4 releases ahead of time so that they're ready for upload on $(date -d '2014-06-10 16:00+0000').

The same patch seems to cherry-pick nicely to 1.6 and 1.4, and to 1.2 with a bit of conflict-fixing (where trailing whitespace was deleted post-1.2).

I do not consider 1.4 or 1.2 to be security-supported upstream any more, but I will push suggested patches to those branches.

(In reply to comment #14)
> Created attachment 100463 [details] [review] [review]
> CVE-2014-3477: deliver activation errors correctly, fixing  Denial of Service
> 
> Here is a revised version of Alban's patch with the adjusted comment. This
> is what I'm going to recommend to vendors, assuming my tests are successful.
> I'm preparing 1.6.20 and 1.8.4 releases ahead of time so that they're ready
> for upload on $(date -d '2014-06-10 16:00+0000').
> 
> The same patch seems to cherry-pick nicely to 1.6 and 1.4, and to 1.2 with a
> bit of conflict-fixing (where trailing whitespace was deleted post-1.2).
> 
> I do not consider 1.4 or 1.2 to be security-supported upstream any more, but
> I will push suggested patches to those branches.

It looks good to me.

I gave the patch another try in the environment I was using to trigger the bug (basically, dbus 1.8.0) and it worked fine for me.

Unembargoing.

Fixed in 1.8.4, 1.6.20. Fixed in git for 1.4 and 1.2 branches.