Attachment 100463 Details for Bug 78979 – CVE-2014-3477: deliver activation errors correctly, fixing Denial of Service

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

0001-CVE-2014-3477-deliver-activation-errors-correctly-fi.patch (text/plain), 6.38 KB, created by Simon McVittie on 2014-06-05 14:31:44 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Simon McVittie

Created: 2014-06-05 14:31:44 UTC

Size: 6.38 KB

patch

obsolete

>From 24c590703ca47eb71ddef453de43126b90954567 Mon Sep 17 00:00:00 2001
>From: Alban Crequy <alban.crequy@collabora.co.uk>
>Date: Tue, 20 May 2014 14:37:37 +0100
>Subject: [PATCH] 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>
>---
> bus/activation.c | 27 ++++++++++++++++++++-------
> bus/activation.h |  3 +--
> bus/services.c   |  5 +++--
> 3 files changed, 24 insertions(+), 11 deletions(-)
>
>diff --git a/bus/activation.c b/bus/activation.c
>index fa6c156..149cca8 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 97f25b1..fc5d426 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 01a720e..584485b 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;
>-- 
>2.0.0
>

Actions: View | Splinter Review

Attachments on bug 78979: 99433 | 100282 | 100463

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.