CVE-2020-1712: Use After Free (CWE-416)
value: A heap use-after-free vulnerability was found in systemd before version v245-rc1, where asynchronous Polkit queries are performed while handling dbus messages. A local unprivileged attacker can abuse this flaw to crash systemd services or potentially execute code and elevate their privileges, by sending specially crafted dbus messages.
CWE description: The product reuses or references memory after it has been freed. At some point afterward, the memory may be allocated again and saved in another pointer, while the original pointer references a location somewhere within the new allocation. Any operations using the original pointer are no longer valid because the memory "belongs" to the code that operates on the new pointer.
CVSS v3 base score: 7.8
Published: 2020-03-31T17:15Z
Repository: https://github.com/systemd/systemd
Language: C
Files changed: src/shared/bus-polkit.c
Commit date: 2020-02-04 18:47:32 +0100
Commit message: polkit: when authorizing via PK let's re-resolve callback/userdata instead of caching it

Previously, when doing an async PK query we'd store the original
callback/userdata pair and call it again after the PK request is
complete. This is problematic, since PK queries might be slow and in the
meantime the userdata might be released and re-acquired. Let's avoid
this by always traversing through the message handlers so that we always
re-resolve the callback and userdata pair and thus can be sure it's
up-to-date and properly valid.

Diff: diff --git a/src/shared/bus-polkit.c b/src/shared/bus-polkit.c
index c42c39afea5..bd88e337b7b 100644
--- a/src/shared/bus-polkit.c
+++ b/src/shared/bus-polkit.c
@@ -157,18 +157,17 @@ int bus_test_polkit(
 typedef struct AsyncPolkitQuery {
         char *action;
         char **details;
 
         sd_bus_message *request, *reply;
-        sd_bus_message_handler_t callback;
-        void *userdata;
         sd_bus_slot *slot;
+
         Hashmap *registry;
+        sd_event_source *defer_event_source;
 } AsyncPolkitQuery;
 
 static void async_polkit_query_free(AsyncPolkitQuery *q) {
-
         if (!q)
                 return;
 
         sd_bus_slot_unref(q->slot);
 
@@ -179,36 +178,74 @@ static void async_polkit_query_free(AsyncPolkitQuery *q) {
         sd_bus_message_unref(q->reply);
 
         free(q->action);
         strv_free(q->details);
 
+        sd_event_source_disable_unref(q->defer_event_source);
         free(q);
 }
 
+static int async_polkit_defer(sd_event_source *s, void *userdata) {
+        AsyncPolkitQuery *q = userdata;
+
+        assert(s);
+
+        /* This is called as idle event source after we processed the async polkit reply, hopefully after the
+         * method call we re-enqueued has been properly processed. */
+
+        async_polkit_query_free(q);
+        return 0;
+}
+
 static int async_polkit_callback(sd_bus_message *reply, void *userdata, sd_bus_error *error) {
         _cleanup_(sd_bus_error_free) sd_bus_error error_buffer = SD_BUS_ERROR_NULL;
         AsyncPolkitQuery *q = userdata;
         int r;
 
         assert(reply);
         assert(q);
 
+        assert(q->slot);
         q->slot = sd_bus_slot_unref(q->slot);
+
+        assert(!q->reply);
         q->reply = sd_bus_message_ref(reply);
 
+        /* Now, let's dispatch the original message a second time be re-enqueing. This will then traverse the
+         * whole message processing again, and thus re-validating and re-retrieving the "userdata" field
+         * again.
+         *
+         * We install an idle event loop event to clean-up the PolicyKit request data when we are idle again,
+         * i.e. after the second time the message is processed is complete. */
+
+        assert(!q->defer_event_source);
+        r = sd_event_add_defer(sd_bus_get_event(sd_bus_message_get_bus(reply)), &q->defer_event_source, async_polkit_defer, q);
+        if (r < 0)
+                goto fail;
+
+        r = sd_event_source_set_priority(q->defer_event_source, SD_EVENT_PRIORITY_IDLE);
+        if (r < 0)
+                goto fail;
+
+        r = sd_event_source_set_enabled(q->defer_event_source, SD_EVENT_ONESHOT);
+        if (r < 0)
+                goto fail;
+
         r = sd_bus_message_rewind(q->request, true);
-        if (r < 0) {
-                r = sd_bus_reply_method_errno(q->request, r, NULL);
-                goto finish;
-        }
+        if (r < 0)
+                goto fail;
+
+        r = sd_bus_enqeue_for_read(sd_bus_message_get_bus(q->request), q->request);
+        if (r < 0)
+                goto fail;
 
-        r = q->callback(q->request, q->userdata, &error_buffer);
-        r = bus_maybe_reply_error(q->request, r, &error_buffer);
+        return 1;
 
-finish:
+fail:
+        log_debug_errno(r, "Processing asynchronous PolicyKit reply failed, ignoring: %m");
+        (void) sd_bus_reply_method_errno(q->request, r, NULL);
         async_polkit_query_free(q);
-
         return r;
 }
 
 #endif
 
@@ -223,15 +260,13 @@ int bus_verify_polkit_async(
                 sd_bus_error *ret_error) {
 
 #if ENABLE_POLKIT
         _cleanup_(sd_bus_message_unrefp) sd_bus_message *pk = NULL;
         AsyncPolkitQuery *q;
-        const char *sender;
-        sd_bus_message_handler_t callback;
-        void *userdata;
         int c;
 #endif
+        const char *sender;
         int r;
 
         assert(call);
         assert(action);
         assert(registry);
@@ -291,24 +326,15 @@ int bus_verify_polkit_async(
         if (r < 0)
                 return r;
         else if (r > 0)
                 return 1;
 
-#if ENABLE_POLKIT
-        if (sd_bus_get_current_message(call->bus) != call)
-                return -EINVAL;
-
-        callback = sd_bus_get_current_handler(call->bus);
-        if (!callback)
-                return -EINVAL;
-
-        userdata = sd_bus_get_current_userdata(call->bus);
-
         sender = sd_bus_message_get_sender(call);
         if (!sender)
                 return -EBADMSG;
 
+#if ENABLE_POLKIT
         c = sd_bus_message_get_allow_interactive_authorization(call);
         if (c < 0)
                 return c;
         if (c > 0)
                 interactive = true;
@@ -347,12 +373,10 @@ int bus_verify_polkit_async(
         if (!q)
                 return -ENOMEM;
 
         *q = (AsyncPolkitQuery) {
                 .request = sd_bus_message_ref(call),
-                .callback = callback,
-                .userdata = userdata,
         };
 
         q->action = strdup(action);
         if (!q->action) {
                 async_polkit_query_free(q);

Vulnerable code:         sd_bus_message_handler_t callback;
        void *userdata;

        if (r < 0) {
                r = sd_bus_reply_method_errno(q->request, r, NULL);
                goto finish;
        }
        r = q->callback(q->request, q->userdata, &error_buffer);
        r = bus_maybe_reply_error(q->request, r, &error_buffer);
finish:

        const char *sender;
        sd_bus_message_handler_t callback;
        void *userdata;
#if ENABLE_POLKIT
        if (sd_bus_get_current_message(call->bus) != call)
                return -EINVAL;

        callback = sd_bus_get_current_handler(call->bus);
        if (!callback)
                return -EINVAL;

        userdata = sd_bus_get_current_userdata(call->bus);

                .callback = callback,
                .userdata = userdata,
Fixed code: 
        sd_event_source *defer_event_source;
        sd_event_source_disable_unref(q->defer_event_source);
static int async_polkit_defer(sd_event_source *s, void *userdata) {
        AsyncPolkitQuery *q = userdata;

        assert(s);

        /* This is called as idle event source after we processed the async polkit reply, hopefully after the
         * method call we re-enqueued has been properly processed. */

        async_polkit_query_free(q);
        return 0;
}

        assert(q->slot);

        assert(!q->reply);
        /* Now, let's dispatch the original message a second time be re-enqueing. This will then traverse the
         * whole message processing again, and thus re-validating and re-retrieving the "userdata" field
         * again.
         *
         * We install an idle event loop event to clean-up the PolicyKit request data when we are idle again,
         * i.e. after the second time the message is processed is complete. */

        assert(!q->defer_event_source);
        r = sd_event_add_defer(sd_bus_get_event(sd_bus_message_get_bus(reply)), &q->defer_event_source, async_polkit_defer, q);
        if (r < 0)
                goto fail;

        r = sd_event_source_set_priority(q->defer_event_source, SD_EVENT_PRIORITY_IDLE);
        if (r < 0)
                goto fail;

        r = sd_event_source_set_enabled(q->defer_event_source, SD_EVENT_ONESHOT);
        if (r < 0)
                goto fail;

        if (r < 0)
                goto fail;

        r = sd_bus_enqeue_for_read(sd_bus_message_get_bus(q->request), q->request);
        if (r < 0)
                goto fail;
        return 1;
fail:
        log_debug_errno(r, "Processing asynchronous PolicyKit reply failed, ignoring: %m");
        (void) sd_bus_reply_method_errno(q->request, r, NULL);
        const char *sender;
#if ENABLE_POLKIT