From 0062d795bf29301ae054e1826a7189198a2565c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 14 Apr 2020 09:06:53 +0000 Subject: [PATCH] Merge branch 'polkit-ref-count' Upsteam-Status: Backport [https://github.com/systemd/systemd/commit/ea0d0ede03c6f18dbc5036c5e9cccf97e415ccc2] CVE: CVE-2020-1712 Signed-off-by: Wenlin Kang --- TODO | 2 +- man/rules/meson.build | 1 + man/sd_bus_enqueue_for_read.xml | 88 ++++++++++++++++ src/libsystemd/libsystemd.sym | 1 + src/libsystemd/sd-bus/sd-bus.c | 24 +++++ src/shared/bus-util.c | 179 +++++++++++++++++++++----------- src/systemd/sd-bus.h | 1 + 7 files changed, 235 insertions(+), 61 deletions(-) create mode 100644 man/sd_bus_enqueue_for_read.xml diff --git a/TODO b/TODO index c5b5b86057..5c5ea1f568 100644 --- a/TODO +++ b/TODO @@ -184,7 +184,7 @@ Features: * the a-posteriori stopping of units bound to units that disappeared logic should be reworked: there should be a queue of units, and we should only - enqeue stop jobs from a defer event that processes queue instead of + enqueue stop jobs from a defer event that processes queue instead of right-away when we find a unit that is bound to one that doesn't exist anymore. (similar to how the stop-unneeded queue has been reworked the same way) diff --git a/man/rules/meson.build b/man/rules/meson.build index 3b63311d7b..e80ed98c34 100644 --- a/man/rules/meson.build +++ b/man/rules/meson.build @@ -192,6 +192,7 @@ manpages = [ 'sd_bus_open_user_with_description', 'sd_bus_open_with_description'], ''], + ['sd_bus_enqueue_for_read', '3', [], ''], ['sd_bus_error', '3', ['SD_BUS_ERROR_MAKE_CONST', diff --git a/man/sd_bus_enqueue_for_read.xml b/man/sd_bus_enqueue_for_read.xml new file mode 100644 index 0000000000..3318a3031b --- /dev/null +++ b/man/sd_bus_enqueue_for_read.xml @@ -0,0 +1,88 @@ + + + + + + + + sd_bus_enqueue_for_read + systemd + + + + sd_bus_enqueue_for_read + 3 + + + + sd_bus_enqueue_for_read + + Re-enqueue a bus message on a bus connection, for reading. + + + + + #include <systemd/sd-bus.h> + + + int sd_bus_enqueue_for_read + sd_bus *bus + sd_bus_message *message + + + + + + + Description + + sd_bus_enqueue_for_read() may be used to re-enqueue an incoming bus message on + the local read queue, so that it is processed and dispatched locally again, similar to how an incoming + message from the peer is processed. Takes a bus connection object and the message to enqueue. A reference + is taken of the message and the caller's reference thus remains in possession of the caller. The message + is enqueued at the end of the queue, thus will be dispatched after all other already queued messages are + dispatched. + + This call is primarily useful for dealing with incoming method calls that may be processed only + after an additional asynchronous operation completes. One example are PolicyKit authorization requests + that are determined to be necessary to authorize a newly incoming method call: when the PolicyKit response + is received the original method call may be re-enqueued to process it again, this time with the + authorization result known. + + + + Return Value + + On success, this function return 0 or a positive integer. On failure, it returns a negative errno-style + error code. + + + Errors + + Returned errors may indicate the following problems: + + + + -ECHILD + + The bus connection has been created in a different process. + + + + + + + + + See Also + + + systemd1, + sd-bus3, + sd_bus_send3, + + + + diff --git a/src/libsystemd/libsystemd.sym b/src/libsystemd/libsystemd.sym index 5ec42e0f1f..c40f1b7d1a 100644 --- a/src/libsystemd/libsystemd.sym +++ b/src/libsystemd/libsystemd.sym @@ -679,6 +679,7 @@ global: LIBSYSTEMD_243 { global: + sd_bus_enqueue_for_read; sd_bus_object_vtable_format; sd_event_source_disable_unref; } LIBSYSTEMD_241; diff --git a/src/libsystemd/sd-bus/sd-bus.c b/src/libsystemd/sd-bus/sd-bus.c index 026ac8cb94..07bc145f37 100644 --- a/src/libsystemd/sd-bus/sd-bus.c +++ b/src/libsystemd/sd-bus/sd-bus.c @@ -4194,3 +4194,27 @@ _public_ int sd_bus_get_close_on_exit(sd_bus *bus) { return bus->close_on_exit; } + +_public_ int sd_bus_enqueue_for_read(sd_bus *bus, sd_bus_message *m) { + int r; + + assert_return(bus, -EINVAL); + assert_return(bus = bus_resolve(bus), -ENOPKG); + assert_return(m, -EINVAL); + assert_return(m->sealed, -EINVAL); + assert_return(!bus_pid_changed(bus), -ECHILD); + + if (!BUS_IS_OPEN(bus->state)) + return -ENOTCONN; + + /* Re-enqueue a message for reading. This is primarily useful for PolicyKit-style authentication, + * where we accept a message, then determine we need to interactively authenticate the user, and then + * we want to process the message again. */ + + r = bus_rqueue_make_room(bus); + if (r < 0) + return r; + + bus->rqueue[bus->rqueue_size++] = bus_message_ref_queued(m, bus); + return 0; +} diff --git a/src/shared/bus-util.c b/src/shared/bus-util.c index e9b0b8a99d..88cad9cd0a 100644 --- a/src/shared/bus-util.c +++ b/src/shared/bus-util.c @@ -212,6 +212,34 @@ static int check_good_user(sd_bus_message *m, uid_t good_user) { return sender_uid == good_user; } +#if ENABLE_POLKIT +static int bus_message_append_strv_key_value( + sd_bus_message *m, + const char **l) { + + const char **k, **v; + int r; + + assert(m); + + r = sd_bus_message_open_container(m, 'a', "{ss}"); + if (r < 0) + return r; + + STRV_FOREACH_PAIR(k, v, l) { + r = sd_bus_message_append(m, "{ss}", *k, *v); + if (r < 0) + return r; + } + + r = sd_bus_message_close_container(m); + if (r < 0) + return r; + + return r; +} +#endif + int bus_test_polkit( sd_bus_message *call, int capability, @@ -219,7 +247,7 @@ int bus_test_polkit( const char **details, uid_t good_user, bool *_challenge, - sd_bus_error *e) { + sd_bus_error *ret_error) { int r; @@ -242,7 +270,7 @@ int bus_test_polkit( _cleanup_(sd_bus_message_unrefp) sd_bus_message *request = NULL; _cleanup_(sd_bus_message_unrefp) sd_bus_message *reply = NULL; int authorized = false, challenge = false; - const char *sender, **k, **v; + const char *sender; sender = sd_bus_message_get_sender(call); if (!sender) @@ -266,17 +294,7 @@ int bus_test_polkit( if (r < 0) return r; - r = sd_bus_message_open_container(request, 'a', "{ss}"); - if (r < 0) - return r; - - STRV_FOREACH_PAIR(k, v, details) { - r = sd_bus_message_append(request, "{ss}", *k, *v); - if (r < 0) - return r; - } - - r = sd_bus_message_close_container(request); + r = bus_message_append_strv_key_value(request, details); if (r < 0) return r; @@ -284,11 +302,11 @@ int bus_test_polkit( if (r < 0) return r; - r = sd_bus_call(call->bus, request, 0, e, &reply); + r = sd_bus_call(call->bus, request, 0, ret_error, &reply); if (r < 0) { /* Treat no PK available as access denied */ - if (sd_bus_error_has_name(e, SD_BUS_ERROR_SERVICE_UNKNOWN)) { - sd_bus_error_free(e); + if (sd_bus_error_has_name(ret_error, SD_BUS_ERROR_SERVICE_UNKNOWN)) { + sd_bus_error_free(ret_error); return -EACCES; } @@ -319,15 +337,17 @@ int bus_test_polkit( #if ENABLE_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; @@ -339,9 +359,25 @@ static void async_polkit_query_free(AsyncPolkitQuery *q) { sd_bus_message_unref(q->request); 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; @@ -350,21 +386,46 @@ static int async_polkit_callback(sd_bus_message *reply, void *userdata, sd_bus_e 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 = q->callback(q->request, q->userdata, &error_buffer); - r = bus_maybe_reply_error(q->request, r, &error_buffer); + r = sd_bus_enqueue_for_read(sd_bus_message_get_bus(q->request), q->request); + if (r < 0) + goto fail; -finish: - async_polkit_query_free(q); + return 1; +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; } @@ -378,16 +439,14 @@ int bus_verify_polkit_async( bool interactive, uid_t good_user, Hashmap **registry, - sd_bus_error *error) { + sd_bus_error *ret_error) { #if ENABLE_POLKIT _cleanup_(sd_bus_message_unrefp) sd_bus_message *pk = NULL; AsyncPolkitQuery *q; - const char *sender, **k, **v; - sd_bus_message_handler_t callback; - void *userdata; int c; #endif + const char *sender; int r; assert(call); @@ -403,11 +462,17 @@ int bus_verify_polkit_async( if (q) { int authorized, challenge; - /* This is the second invocation of this function, and - * there's already a response from polkit, let's - * process it */ + /* This is the second invocation of this function, and there's already a response from + * polkit, let's process it */ assert(q->reply); + /* If the operation we want to authenticate changed between the first and the second time, + * let's not use this authentication, it might be out of date as the object and context we + * operate on might have changed. */ + if (!streq(q->action, action) || + !strv_equal(q->details, (char**) details)) + return -ESTALE; + if (sd_bus_message_is_method_error(q->reply, NULL)) { const sd_bus_error *e; @@ -418,7 +483,7 @@ int bus_verify_polkit_async( return -EACCES; /* Copy error from polkit reply */ - sd_bus_error_copy(error, e); + sd_bus_error_copy(ret_error, e); return -sd_bus_error_get_errno(e); } @@ -433,7 +498,7 @@ int bus_verify_polkit_async( return 1; if (challenge) - return sd_bus_error_set(error, SD_BUS_ERROR_INTERACTIVE_AUTHORIZATION_REQUIRED, "Interactive authentication required."); + return sd_bus_error_set(ret_error, SD_BUS_ERROR_INTERACTIVE_AUTHORIZATION_REQUIRED, "Interactive authentication required."); return -EACCES; } @@ -445,20 +510,12 @@ int bus_verify_polkit_async( 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; @@ -487,17 +544,7 @@ int bus_verify_polkit_async( if (r < 0) return r; - r = sd_bus_message_open_container(pk, 'a', "{ss}"); - if (r < 0) - return r; - - STRV_FOREACH_PAIR(k, v, details) { - r = sd_bus_message_append(pk, "{ss}", *k, *v); - if (r < 0) - return r; - } - - r = sd_bus_message_close_container(pk); + r = bus_message_append_strv_key_value(pk, details); if (r < 0) return r; @@ -505,13 +552,25 @@ int bus_verify_polkit_async( if (r < 0) return r; - q = new0(AsyncPolkitQuery, 1); + q = new(AsyncPolkitQuery, 1); if (!q) return -ENOMEM; - q->request = sd_bus_message_ref(call); - q->callback = callback; - q->userdata = userdata; + *q = (AsyncPolkitQuery) { + .request = sd_bus_message_ref(call), + }; + + q->action = strdup(action); + if (!q->action) { + async_polkit_query_free(q); + return -ENOMEM; + } + + q->details = strv_copy((char**) details); + if (!q->details) { + async_polkit_query_free(q); + return -ENOMEM; + } r = hashmap_put(*registry, call, q); if (r < 0) { diff --git a/src/systemd/sd-bus.h b/src/systemd/sd-bus.h index 84ceb62dc7..0e5c761f83 100644 --- a/src/systemd/sd-bus.h +++ b/src/systemd/sd-bus.h @@ -201,6 +201,7 @@ int sd_bus_process(sd_bus *bus, sd_bus_message **r); int sd_bus_process_priority(sd_bus *bus, int64_t max_priority, sd_bus_message **r); int sd_bus_wait(sd_bus *bus, uint64_t timeout_usec); int sd_bus_flush(sd_bus *bus); +int sd_bus_enqueue_for_read(sd_bus *bus, sd_bus_message *m); sd_bus_slot* sd_bus_get_current_slot(sd_bus *bus); sd_bus_message* sd_bus_get_current_message(sd_bus *bus); -- 2.23.0