From b9632ec42f58366efe629683c4c351865272c2df Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 28 Feb 2024 17:06:11 +0100 Subject: [PATCH 1/3] polkit: allow checking if we already acquired some action This adds a new helper that basically just wraps async_polkit_query_have_action() and allows calling this without actually triggering a PK authentication operation: it just checks if we aleady have acquired an action or not. --- src/shared/bus-polkit.c | 39 +++++++++++++++++++++++++++++++++++---- src/shared/bus-polkit.h | 2 ++ 2 files changed, 37 insertions(+), 4 deletions(-) diff --git a/src/shared/bus-polkit.c b/src/shared/bus-polkit.c index 707f33acae..865f895d39 100644 --- a/src/shared/bus-polkit.c +++ b/src/shared/bus-polkit.c @@ -388,6 +388,21 @@ static int async_polkit_callback(sd_bus_message *reply, void *userdata, sd_bus_e return r; } +static bool async_polkit_query_have_action( + AsyncPolkitQuery *q, + const char *action, + const char **details) { + + assert(q); + assert(action); + + LIST_FOREACH(authorized, a, q->authorized_actions) + if (streq(a->action, action) && strv_equal(a->details, (char**) details)) + return true; + + return false; +} + static int async_polkit_query_check_action( AsyncPolkitQuery *q, const char *action, @@ -397,9 +412,8 @@ static int async_polkit_query_check_action( assert(q); assert(action); - LIST_FOREACH(authorized, a, q->authorized_actions) - if (streq(a->action, action) && strv_equal(a->details, (char**) details)) - return 1; /* Allow! */ + if (async_polkit_query_have_action(q, action, details)) + return 1; /* Allow! */ if (q->error_action && streq(q->error_action->action, action)) return sd_bus_error_copy(ret_error, &q->error); @@ -409,7 +423,6 @@ static int async_polkit_query_check_action( return 0; } - #endif /* bus_verify_polkit_async() handles verification of D-Bus calls with polkit. Because the polkit API @@ -820,3 +833,21 @@ int varlink_verify_polkit_async_full( return -EACCES; } + +bool varlink_has_polkit_action(Varlink *link, const char *action, const char **details, Hashmap **registry) { + assert(link); + assert(action); + assert(registry); + + /* Checks if we already have acquired some action previously */ + +#if ENABLE_POLKIT + AsyncPolkitQuery *q = hashmap_get(*registry, link); + if (!q) + return false; + + return async_polkit_query_have_action(q, action, details); +#else + return false; +#endif +} diff --git a/src/shared/bus-polkit.h b/src/shared/bus-polkit.h index 4e2c1d3551..3394c23a2b 100644 --- a/src/shared/bus-polkit.h +++ b/src/shared/bus-polkit.h @@ -30,3 +30,5 @@ static inline int varlink_verify_polkit_async(Varlink *link, sd_bus *bus, const .name = "allowInteractiveAuthentication", \ .type = JSON_VARIANT_BOOLEAN, \ } + +bool varlink_has_polkit_action(Varlink *link, const char *action, const char **details, Hashmap **registry); From f5a12ceaedf4d490a9dc82e9460dd6fd97acc942 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 28 Feb 2024 21:31:53 +0100 Subject: [PATCH 2/3] polkit: add new POLKIT_ALWAYS_QUERY flag When this flag is set we'll disable the local shortcut that skips polkit checks for clients that are privileged, and assumes they are authenticated. Or in other words: if this flag is set, we'll query PK not matter what, regardless if it's root we talk about or any other user. --- src/shared/bus-polkit.c | 21 +++++++++++++-------- src/shared/bus-polkit.h | 1 + 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/src/shared/bus-polkit.c b/src/shared/bus-polkit.c index 865f895d39..2255ef128e 100644 --- a/src/shared/bus-polkit.c +++ b/src/shared/bus-polkit.c @@ -539,11 +539,14 @@ int bus_verify_polkit_async_full( } #endif - r = sd_bus_query_sender_privilege(call, -1); - if (r < 0) - return r; - if (r > 0) - return 1; + if (!FLAGS_SET(flags, POLKIT_ALWAYS_QUERY)) { + /* Don't query PK if client is privileged */ + r = sd_bus_query_sender_privilege(call, /* capability= */ -1); + if (r < 0) + return r; + if (r > 0) + return 1; + } #if ENABLE_POLKIT bool interactive = FLAGS_SET(flags, POLKIT_ALLOW_INTERACTIVE); @@ -742,9 +745,11 @@ int varlink_verify_polkit_async_full( if (r != 0) return r; - r = varlink_check_peer_privilege(link); - if (r != 0) - return r; + if (!FLAGS_SET(flags, POLKIT_ALWAYS_QUERY)) { + r = varlink_check_peer_privilege(link); + if (r != 0) + return r; + } #if ENABLE_POLKIT _cleanup_(async_polkit_query_unrefp) AsyncPolkitQuery *q = NULL; diff --git a/src/shared/bus-polkit.h b/src/shared/bus-polkit.h index 3394c23a2b..9fb5d83f0c 100644 --- a/src/shared/bus-polkit.h +++ b/src/shared/bus-polkit.h @@ -9,6 +9,7 @@ typedef enum PolkitFLags { POLKIT_ALLOW_INTERACTIVE = 1 << 0, /* Allow interactive auth (typically not required, because can be derived from bus message/link automatically) */ + POLKIT_ALWAYS_QUERY = 1 << 1, /* Query polkit even if client is privileged */ } PolkitFlags; int bus_test_polkit(sd_bus_message *call, const char *action, const char **details, uid_t good_user, bool *_challenge, sd_bus_error *e); From 91180a1eec6f864de8e559a1d87dc33476a601b1 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 28 Feb 2024 21:56:55 +0100 Subject: [PATCH 3/3] polkit: add another flag that controls how to treat the PK absent case Typically if PK is not present we want to treat this as "denied". But sometimes it makes sense to treat this case as "allowed". In particular the combination POLKIT_ALWAYS_QUERY and POLKIT_DEFAULT_ALLOW makes a lot of sense: it means we can enable PK logic for actions where we so far bypassed the checks for root. With the new combination we can have a default policy of allowing some operation but still provide an effective hook to disable it. Also add some debug logging about PK operations and results as they are ongoing. --- src/shared/bus-polkit.c | 82 ++++++++++++++++++++++++++--------------- src/shared/bus-polkit.h | 1 + 2 files changed, 54 insertions(+), 29 deletions(-) diff --git a/src/shared/bus-polkit.c b/src/shared/bus-polkit.c index 2255ef128e..e0e9e87954 100644 --- a/src/shared/bus-polkit.c +++ b/src/shared/bus-polkit.c @@ -185,20 +185,21 @@ DEFINE_TRIVIAL_CLEANUP_FUNC(AsyncPolkitQueryAction*, async_polkit_query_action_f typedef struct AsyncPolkitQuery { unsigned n_ref; - AsyncPolkitQueryAction *action; + AsyncPolkitQueryAction *action; /* action currently being processed */ sd_bus *bus; - sd_bus_message *request; /* the original bus method call that triggered the polkit auth, NULL in case of varlink */ + sd_bus_message *request; /* the original bus method call that triggered the polkit auth, NULL in case of varlink */ sd_bus_slot *slot; - Varlink *link; /* the original varlink method call that triggered the polkit auth, NULL in case of bus */ + Varlink *link; /* the original varlink method call that triggered the polkit auth, NULL in case of bus */ Hashmap *registry; sd_event_source *defer_event_source; - LIST_HEAD(AsyncPolkitQueryAction, authorized_actions); - AsyncPolkitQueryAction *denied_action; - AsyncPolkitQueryAction *error_action; - sd_bus_error error; + LIST_HEAD(AsyncPolkitQueryAction, authorized_actions); /* actions we successfully were authorized for */ + AsyncPolkitQueryAction *denied_action; /* if we received denial for an action, it's this one */ + AsyncPolkitQueryAction *absent_action; /* If polkit was absent for some action, it's this one */ + AsyncPolkitQueryAction *error_action; /* if we encountered any other error, it's this one */ + sd_bus_error error; /* the precise error, in case error_action is set */ } AsyncPolkitQuery; static AsyncPolkitQuery *async_polkit_query_free(AsyncPolkitQuery *q) { @@ -226,6 +227,7 @@ static AsyncPolkitQuery *async_polkit_query_free(AsyncPolkitQuery *q) { LIST_CLEAR(authorized, q->authorized_actions, async_polkit_query_action_free); async_polkit_query_action_free(q->denied_action); + async_polkit_query_action_free(q->absent_action); async_polkit_query_action_free(q->error_action); sd_bus_error_free(&q->error); @@ -265,28 +267,34 @@ static int async_polkit_read_reply(sd_bus_message *reply, AsyncPolkitQuery *q) { /* Processing of a PolicyKit checks is canceled on the first auth. error. */ assert(!q->denied_action); + assert(!q->absent_action); assert(!q->error_action); assert(!sd_bus_error_is_set(&q->error)); - assert(q->action); - a = TAKE_PTR(q->action); + a = ASSERT_PTR(TAKE_PTR(q->action)); if (sd_bus_message_is_method_error(reply, NULL)) { const sd_bus_error *e; e = sd_bus_message_get_error(reply); - if (bus_error_is_unknown_service(e) || - sd_bus_error_has_names( - e, - "org.freedesktop.PolicyKit1.Error.Failed", - "org.freedesktop.PolicyKit1.Error.Cancelled", - "org.freedesktop.PolicyKit1.Error.NotAuthorized")) - /* Treat no PK available as access denied. Also treat some of the well-known PK errors as such. */ + if (bus_error_is_unknown_service(e)) { + /* If PK is absent, then store this away, as it depends on the callers flags whether + * this means deny or allow */ + log_debug("Polkit found to be unavailable while trying to authorize action '%s'.", a->action); + q->absent_action = TAKE_PTR(a); + } else if (sd_bus_error_has_names( + e, + "org.freedesktop.PolicyKit1.Error.Failed", + "org.freedesktop.PolicyKit1.Error.Cancelled", + "org.freedesktop.PolicyKit1.Error.NotAuthorized")) { + /* Treat some of the well-known PK errors as denial. */ + log_debug("Polkit authorization for action '%s' failed with an polkit error: %s", a->action, e->name); q->denied_action = TAKE_PTR(a); - else { + } else { /* Save error from polkit reply, so it can be returned when the same authorization * is attempted for second time */ + log_debug("Polkit authorization for action '%s' failed with an unexpected error: %s", a->action, e->name); q->error_action = TAKE_PTR(a); r = sd_bus_error_copy(&q->error, e); if (r == -ENOMEM) @@ -302,13 +310,17 @@ static int async_polkit_read_reply(sd_bus_message *reply, AsyncPolkitQuery *q) { if (r < 0) return r; - if (authorized) + if (authorized) { + log_debug("Polkit authorization for action '%s' succeeded.", a->action); LIST_PREPEND(authorized, q->authorized_actions, TAKE_PTR(a)); - else if (challenge) { + } else if (challenge) { + log_debug("Polkit authorization for action requires '%s' interactive authentication, which we didn't allow.", a->action); q->error_action = TAKE_PTR(a); sd_bus_error_set_const(&q->error, SD_BUS_ERROR_INTERACTIVE_AUTHORIZATION_REQUIRED, "Interactive authentication required."); - } else + } else { + log_debug("Polkit authorization for action '%s' denied.", a->action); q->denied_action = TAKE_PTR(a); + } return 0; } @@ -407,6 +419,7 @@ static int async_polkit_query_check_action( AsyncPolkitQuery *q, const char *action, const char **details, + PolkitFlags flags, sd_bus_error *ret_error) { assert(q); @@ -419,9 +432,12 @@ static int async_polkit_query_check_action( return sd_bus_error_copy(ret_error, &q->error); if (q->denied_action && streq(q->denied_action->action, action)) - return -EACCES; + return -EACCES; /* Deny! */ - return 0; + if (q->absent_action) + return FLAGS_SET(flags, POLKIT_DEFAULT_ALLOW) ? 1 /* Allow! */ : -EACCES /* Deny! */; + + return 0; /* no reply yet */ } #endif @@ -522,6 +538,8 @@ int bus_verify_polkit_async_full( assert(registry); assert(ret_error); + log_debug("Trying to acquire polkit authentication for '%s'.", action); + r = bus_message_check_good_user(call, good_user); if (r != 0) return r; @@ -533,9 +551,11 @@ int bus_verify_polkit_async_full( /* This is a repeated invocation of this function, hence let's check if we've already got * a response from polkit for this action */ if (q) { - r = async_polkit_query_check_action(q, action, details, ret_error); - if (r != 0) + r = async_polkit_query_check_action(q, action, details, flags, ret_error); + if (r != 0) { + log_debug("Found matching previous polkit authentication for '%s'.", action); return r; + } } #endif @@ -601,9 +621,9 @@ int bus_verify_polkit_async_full( TAKE_PTR(q); return 0; +#else + return FLAGS_SET(flags, POLKIT_DEFAULT_ALLOW) ? 1 : -EACCES; #endif - - return -EACCES; } static int varlink_check_good_user(Varlink *link, uid_t good_user) { @@ -738,6 +758,8 @@ int varlink_verify_polkit_async_full( assert(link); assert(registry); + log_debug("Trying to acquire polkit authentication for '%s'.", action); + /* This is the same as bus_verify_polkit_async_full(), but authenticates the peer of a varlink * connection rather than the sender of a bus message. */ @@ -759,7 +781,9 @@ int varlink_verify_polkit_async_full( * a response from polkit for this action */ if (q) { _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; - r = async_polkit_query_check_action(q, action, details, &error); + r = async_polkit_query_check_action(q, action, details, flags, &error); + if (r != 0) + log_debug("Found matching previous polkit authentication for '%s'.", action); if (r < 0) { /* Reply with a nice error */ if (sd_bus_error_has_name(&error, SD_BUS_ERROR_INTERACTIVE_AUTHORIZATION_REQUIRED)) @@ -834,9 +858,9 @@ int varlink_verify_polkit_async_full( TAKE_PTR(q); return 0; +#else + return FLAGS_SET(flags, POLKIT_DEFAULT_ALLOW) ? 1 : -EACCES; #endif - - return -EACCES; } bool varlink_has_polkit_action(Varlink *link, const char *action, const char **details, Hashmap **registry) { diff --git a/src/shared/bus-polkit.h b/src/shared/bus-polkit.h index 9fb5d83f0c..15c621dfa3 100644 --- a/src/shared/bus-polkit.h +++ b/src/shared/bus-polkit.h @@ -10,6 +10,7 @@ typedef enum PolkitFLags { POLKIT_ALLOW_INTERACTIVE = 1 << 0, /* Allow interactive auth (typically not required, because can be derived from bus message/link automatically) */ POLKIT_ALWAYS_QUERY = 1 << 1, /* Query polkit even if client is privileged */ + POLKIT_DEFAULT_ALLOW = 1 << 2, /* If polkit is not around, assume "allow" rather than the usual "deny" */ } PolkitFlags; int bus_test_polkit(sd_bus_message *call, const char *action, const char **details, uid_t good_user, bool *_challenge, sd_bus_error *e);