CVE-2014-3477: deliver activation errors correctly, fixing Denial of Service

How it should work:

When a D-Bus message activates a service, LSMs (SELinux or AppArmor) check
whether the message can be delivered after the service has been activated. The
service is considered activated when its well-known name is requested with
org.freedesktop.DBus.RequestName. When the message delivery is denied, the
service stays activated but should not receive the activating message (the
message which triggered the activation). dbus-daemon is supposed to drop the
activating message and reply to the sender with a D-Bus error message.

However, it does not work as expected:

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"

   If the sender and the service are malicious confederates and agree on a
   protocol to insert information in the member name, the sender can leak
   information to the service, even though the LSM attempted to block the
   communication between the sender and the service.

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.

The following changes fix the bug:
- bus_activation_send_pending_auto_activation_messages() only returns an error
  in case of OOM. The prototype is changed to return TRUE, or FALSE on OOM
  (and its only caller sets the OOM error).
- When a client is not allowed to talk to the service, a D-Bus error message
  is pre-allocated to be delivered to the client as part of the transaction.
  The error is not propagated to the caller so RequestName will not fail
  (except on OOM).

[fixed a misleading comment -smcv]

Bug: https://bugs.freedesktop.org/show_bug.cgi?id=78979
Reviewed-by: Simon McVittie <simon.mcvittie@collabora.co.uk>
Reviewed-by: Colin Walters <walters@verbum.org>

3 files changed, 24 insertions, 11 deletions

diff --git a/bus/activation.c b/bus/activation.c
index ea48a26f..280cc017 100644
--- a/bus/activation.c
+++ b/bus/activation.c
@@ -1162,14 +1162,11 @@ bus_activation_service_created (BusActivation  *activation,
 dbus_bool_t
 bus_activation_send_pending_auto_activation_messages (BusActivation  *activation,
                                                       BusService     *service,
-                                                      BusTransaction *transaction,
-                                                      DBusError      *error)
+                                                      BusTransaction *transaction)
 {
   BusPendingActivation *pending_activation;
   DBusList *link;
 
-  _DBUS_ASSERT_ERROR_IS_CLEAR (error);
-
   /* Check if it's a pending activation */
   pending_activation = _dbus_hash_table_lookup_string (activation->pending_activations,
                                                        bus_service_get_name (service));
@@ -1186,6 +1183,9 @@ bus_activation_send_pending_auto_activation_messages (BusActivation  *activation
       if (entry->auto_activation && (entry->connection == NULL || dbus_connection_get_is_connected (entry->connection)))
         {
           DBusConnection *addressed_recipient;
+          DBusError error;
+
+          dbus_error_init (&error);
 
           addressed_recipient = bus_service_get_primary_owners_connection (service);
 
@@ -1193,8 +1193,22 @@ bus_activation_send_pending_auto_activation_messages (BusActivation  *activation
           if (!bus_dispatch_matches (transaction,
                                      entry->connection,
                                      addressed_recipient,
-                                     entry->activation_message, error))
-            goto error;
+                                     entry->activation_message, &error))
+            {
+              /* 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). */
+              if (!bus_transaction_send_error_reply (transaction, entry->connection,
+                                                     &error, entry->activation_message))
+                {
+                  bus_connection_send_oom_error (entry->connection,
+                                                 entry->activation_message);
+                }
+
+              link = next;
+              continue;
+            }
         }
 
       link = next;
@@ -1203,7 +1217,6 @@ bus_activation_send_pending_auto_activation_messages (BusActivation  *activation
   if (!add_restore_pending_to_transaction (transaction, pending_activation))
     {
       _dbus_verbose ("Could not add cancel hook to transaction to revert removing pending activation\n");
-      BUS_SET_OOM (error);
       goto error;
     }
 
diff --git a/bus/activation.h b/bus/activation.h
index 97f25b1f..fc5d426f 100644
--- a/bus/activation.h
+++ b/bus/activation.h
@@ -62,8 +62,7 @@ dbus_bool_t    dbus_activation_systemd_failure (BusActivation     *activation,
 
 dbus_bool_t    bus_activation_send_pending_auto_activation_messages (BusActivation     *activation,
 								     BusService        *service,
-								     BusTransaction    *transaction,
-								     DBusError         *error);
+								     BusTransaction    *transaction);
 
 
 #endif /* BUS_ACTIVATION_H */
diff --git a/bus/services.c b/bus/services.c
index 01a720ed..584485b1 100644
--- a/bus/services.c
+++ b/bus/services.c
@@ -588,8 +588,9 @@ bus_registry_acquire_service (BusRegistry      *registry,
   activation = bus_context_get_activation (registry->context);
   retval = bus_activation_send_pending_auto_activation_messages (activation,
 								 service,
-								 transaction,
-								 error);
+								 transaction);
+  if (!retval)
+    BUS_SET_OOM (error);
   
  out:
   return retval;