From a3f3243613b01903f5850a250c793d48d16ebb95 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 23 Nov 2023 18:06:05 +0100 Subject: [PATCH 01/11] pidref: add helpers for waiting for pidref processes A simple test case is added in a follow-up commit. --- src/basic/missing_wait.h | 8 ++++++++ src/basic/pidref.c | 39 +++++++++++++++++++++++++++++++++++++++ src/basic/pidref.h | 14 +++++++++++++- 3 files changed, 60 insertions(+), 1 deletion(-) create mode 100644 src/basic/missing_wait.h diff --git a/src/basic/missing_wait.h b/src/basic/missing_wait.h new file mode 100644 index 0000000000..a24779d977 --- /dev/null +++ b/src/basic/missing_wait.h @@ -0,0 +1,8 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +#pragma once + +#include + +#ifndef P_PIDFD +#define P_PIDFD 3 +#endif diff --git a/src/basic/pidref.c b/src/basic/pidref.c index cf1c165b60..972853bbd6 100644 --- a/src/basic/pidref.c +++ b/src/basic/pidref.c @@ -3,6 +3,7 @@ #include "errno-util.h" #include "fd-util.h" #include "missing_syscall.h" +#include "missing_wait.h" #include "parse-util.h" #include "pidref.h" #include "process-util.h" @@ -302,6 +303,44 @@ bool pidref_is_self(const PidRef *pidref) { return pidref->pid == getpid_cached(); } +int pidref_wait(const PidRef *pidref, siginfo_t *ret, int options) { + int r; + + if (!pidref_is_set(pidref)) + return -ESRCH; + + if (pidref->pid == 1 || pidref->pid == getpid_cached()) + return -ECHILD; + + siginfo_t si = {}; + + if (pidref->fd >= 0) { + r = RET_NERRNO(waitid(P_PIDFD, pidref->fd, &si, options)); + if (r >= 0) { + if (ret) + *ret = si; + return r; + } + if (r != -EINVAL) /* P_PIDFD was added in kernel 5.4 only */ + return r; + } + + r = RET_NERRNO(waitid(P_PID, pidref->pid, &si, options)); + if (r >= 0 && ret) + *ret = si; + return r; +} + +int pidref_wait_for_terminate(const PidRef *pidref, siginfo_t *ret) { + int r; + + for (;;) { + r = pidref_wait(pidref, ret, WEXITED); + if (r != -EINTR) + return r; + } +} + static void pidref_hash_func(const PidRef *pidref, struct siphash *state) { siphash24_compress_typesafe(pidref->pid, state); } diff --git a/src/basic/pidref.h b/src/basic/pidref.h index a01d4cc85b..0fbffb3320 100644 --- a/src/basic/pidref.h +++ b/src/basic/pidref.h @@ -55,7 +55,19 @@ int pidref_new_from_pid(pid_t pid, PidRef **ret); int pidref_kill(const PidRef *pidref, int sig); int pidref_kill_and_sigcont(const PidRef *pidref, int sig); -int pidref_sigqueue(const PidRef *pidfref, int sig, int value); +int pidref_sigqueue(const PidRef *pidref, int sig, int value); + +int pidref_wait(const PidRef *pidref, siginfo_t *siginfo, int options); +int pidref_wait_for_terminate(const PidRef *pidref, siginfo_t *ret); + +static inline void pidref_done_sigkill_wait(PidRef *pidref) { + if (!pidref_is_set(pidref)) + return; + + (void) pidref_kill(pidref, SIGKILL); + (void) pidref_wait_for_terminate(pidref, NULL); + pidref_done(pidref); +} int pidref_verify(const PidRef *pidref); From f17132260f36e3db863afcb8bbbbec7ebd49bfb1 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 23 Nov 2023 18:07:44 +0100 Subject: [PATCH 02/11] process-util: add pidref_safe_fork() helper This combines safe_fork() with pidref_set_pid(). Eventually we really should switch this to use CLONE_PIDFD, but as that is not wrapped by glibc yet, it's hard. But this is not crucial anyway, as a child we just forked off can always safely be referenced also by PID, given the reaping is under our own control. A simple test case is added in a follow-up commit. --- src/basic/process-util.c | 24 ++++++++++++++++++++++++ src/basic/process-util.h | 12 ++++++++++++ 2 files changed, 36 insertions(+) diff --git a/src/basic/process-util.c b/src/basic/process-util.c index 201c5596ae..664222ec84 100644 --- a/src/basic/process-util.c +++ b/src/basic/process-util.c @@ -1650,6 +1650,30 @@ int safe_fork_full( return 0; } +int pidref_safe_fork_full( + const char *name, + const int stdio_fds[3], + const int except_fds[], + size_t n_except_fds, + ForkFlags flags, + PidRef *ret_pid) { + + pid_t pid; + int r, q; + + assert(!FLAGS_SET(flags, FORK_WAIT)); + + r = safe_fork_full(name, stdio_fds, except_fds, n_except_fds, flags, &pid); + if (r < 0) + return r; + + q = pidref_set_pid(ret_pid, pid); + if (q < 0) /* Let's not fail for this, no matter what, the process exists after all, and that's key */ + *ret_pid = PIDREF_MAKE_FROM_PID(pid); + + return r; +} + int namespace_fork( const char *outer_name, const char *inner_name, diff --git a/src/basic/process-util.h b/src/basic/process-util.h index af6cba13eb..83ebda6fab 100644 --- a/src/basic/process-util.h +++ b/src/basic/process-util.h @@ -191,6 +191,18 @@ static inline int safe_fork(const char *name, ForkFlags flags, pid_t *ret_pid) { return safe_fork_full(name, NULL, NULL, 0, flags, ret_pid); } +int pidref_safe_fork_full( + const char *name, + const int stdio_fds[3], + const int except_fds[], + size_t n_except_fds, + ForkFlags flags, + PidRef *ret_pid); + +static inline int pidref_safe_fork(const char *name, ForkFlags flags, PidRef *ret_pid) { + return pidref_safe_fork_full(name, NULL, NULL, 0, flags, ret_pid); +} + int namespace_fork(const char *outer_name, const char *inner_name, const int except_fds[], size_t n_except_fds, ForkFlags flags, int pidns_fd, int mntns_fd, int netns_fd, int userns_fd, int root_fd, pid_t *ret_pid); int set_oom_score_adjust(int value); From 3dee63b762805817fd7a1af02556e8bf44d558e1 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 23 Nov 2023 18:09:30 +0100 Subject: [PATCH 03/11] process-util: add new pid{ref,}_get_start_time() helper This also adds a test case that test pidref_safe_fork(), pidref_wait() and related calls. --- src/basic/process-util.c | 76 ++++++++++++++++++++++++++++++++++++ src/basic/process-util.h | 2 + src/test/test-process-util.c | 21 ++++++++++ 3 files changed, 99 insertions(+) diff --git a/src/basic/process-util.c b/src/basic/process-util.c index 664222ec84..63b2a12815 100644 --- a/src/basic/process-util.c +++ b/src/basic/process-util.c @@ -730,6 +730,82 @@ int get_process_ppid(pid_t pid, pid_t *ret) { return 0; } +int pid_get_start_time(pid_t pid, uint64_t *ret) { + _cleanup_free_ char *line = NULL; + const char *p; + int r; + + assert(pid >= 0); + + p = procfs_file_alloca(pid, "stat"); + r = read_one_line_file(p, &line); + if (r == -ENOENT) + return -ESRCH; + if (r < 0) + return r; + + /* Let's skip the pid and comm fields. The latter is enclosed in () but does not escape any () in its + * value, so let's skip over it manually */ + + p = strrchr(line, ')'); + if (!p) + return -EIO; + + p++; + + unsigned long llu; + + if (sscanf(p, " " + "%*c " /* state */ + "%*u " /* ppid */ + "%*u " /* pgrp */ + "%*u " /* session */ + "%*u " /* tty_nr */ + "%*u " /* tpgid */ + "%*u " /* flags */ + "%*u " /* minflt */ + "%*u " /* cminflt */ + "%*u " /* majflt */ + "%*u " /* cmajflt */ + "%*u " /* utime */ + "%*u " /* stime */ + "%*u " /* cutime */ + "%*u " /* cstime */ + "%*i " /* priority */ + "%*i " /* nice */ + "%*u " /* num_threads */ + "%*u " /* itrealvalue */ + "%lu ", /* starttime */ + &llu) != 1) + return -EIO; + + if (ret) + *ret = llu; + + return 0; +} + +int pidref_get_start_time(const PidRef *pid, uint64_t *ret) { + uint64_t t; + int r; + + if (!pidref_is_set(pid)) + return -ESRCH; + + r = pid_get_start_time(pid->pid, ret ? &t : NULL); + if (r < 0) + return r; + + r = pidref_verify(pid); + if (r < 0) + return r; + + if (ret) + *ret = t; + + return 0; +} + int get_process_umask(pid_t pid, mode_t *ret) { _cleanup_free_ char *m = NULL; const char *p; diff --git a/src/basic/process-util.h b/src/basic/process-util.h index 83ebda6fab..de6a2bd203 100644 --- a/src/basic/process-util.h +++ b/src/basic/process-util.h @@ -54,6 +54,8 @@ int get_process_cwd(pid_t pid, char **ret); int get_process_root(pid_t pid, char **ret); int get_process_environ(pid_t pid, char **ret); int get_process_ppid(pid_t pid, pid_t *ret); +int pid_get_start_time(pid_t pid, uint64_t *ret); +int pidref_get_start_time(const PidRef* pid, uint64_t *ret); int get_process_umask(pid_t pid, mode_t *ret); int container_get_leader(const char *machine, pid_t *pid); diff --git a/src/test/test-process-util.c b/src/test/test-process-util.c index 957e2141ef..027b2a401e 100644 --- a/src/test/test-process-util.c +++ b/src/test/test-process-util.c @@ -946,6 +946,27 @@ TEST(is_reaper_process) { } } +TEST(pid_get_start_time) { + _cleanup_(pidref_done) PidRef pidref = PIDREF_NULL; + + assert_se(pidref_set_self(&pidref) >= 0); + + uint64_t start_time; + assert_se(pidref_get_start_time(&pidref, &start_time) >= 0); + log_info("our starttime: %" PRIu64, start_time); + + _cleanup_(pidref_done_sigkill_wait) PidRef child = PIDREF_NULL; + + assert_se(pidref_safe_fork("(stub)", FORK_RESET_SIGNALS|FORK_CLOSE_ALL_FDS, &child) >= 0); + + uint64_t start_time2; + assert_se(pidref_get_start_time(&child, &start_time2) >= 0); + + log_info("child starttime: %" PRIu64, start_time2); + + assert_se(start_time2 >= start_time); +} + static int intro(void) { log_show_color(true); return EXIT_SUCCESS; From da5e0c442bc1f0e1e47ab92b0196e99e7c4ffafd Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 23 Nov 2023 17:58:01 +0100 Subject: [PATCH 04/11] socket-util: add helper for getting peer pidfd --- src/basic/missing_socket.h | 4 ++++ src/basic/socket-util.c | 15 +++++++++++++++ src/basic/socket-util.h | 1 + 3 files changed, 20 insertions(+) diff --git a/src/basic/missing_socket.h b/src/basic/missing_socket.h index 30ac297e17..3333cf18e7 100644 --- a/src/basic/missing_socket.h +++ b/src/basic/missing_socket.h @@ -32,6 +32,10 @@ struct sockaddr_vm { #define SO_PEERGROUPS 59 #endif +#ifndef SO_PEERPIDFD +#define SO_PEERPIDFD 77 +#endif + #ifndef SO_BINDTOIFINDEX #define SO_BINDTOIFINDEX 62 #endif diff --git a/src/basic/socket-util.c b/src/basic/socket-util.c index 86472c8847..98133a2ecd 100644 --- a/src/basic/socket-util.c +++ b/src/basic/socket-util.c @@ -956,6 +956,21 @@ int getpeergroups(int fd, gid_t **ret) { return (int) n; } +int getpeerpidfd(int fd) { + socklen_t n = sizeof(int); + int pidfd = -EBADF; + + assert(fd >= 0); + + if (getsockopt(fd, SOL_SOCKET, SO_PEERPIDFD, &pidfd, &n) < 0) + return -errno; + + if (n != sizeof(int)) + return -EIO; + + return pidfd; +} + ssize_t send_many_fds_iov_sa( int transport_fd, int *fds_array, size_t n_fds_array, diff --git a/src/basic/socket-util.h b/src/basic/socket-util.h index 9a11df834d..032d73857e 100644 --- a/src/basic/socket-util.h +++ b/src/basic/socket-util.h @@ -152,6 +152,7 @@ bool address_label_valid(const char *p); int getpeercred(int fd, struct ucred *ucred); int getpeersec(int fd, char **ret); int getpeergroups(int fd, gid_t **ret); +int getpeerpidfd(int fd); ssize_t send_many_fds_iov_sa( int transport_fd, From 0eccf7259ec9e891347adecf4326401a619b8848 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 23 Nov 2023 18:12:44 +0100 Subject: [PATCH 05/11] varlink: add new helper varlink_get_peer_pidref() for getting PidRef of peer --- src/shared/varlink.c | 53 ++++++++++++++++++++++++++++++++++++++++++++ src/shared/varlink.h | 2 ++ 2 files changed, 55 insertions(+) diff --git a/src/shared/varlink.c b/src/shared/varlink.c index 12a27fcb8d..4664d6b2a6 100644 --- a/src/shared/varlink.c +++ b/src/shared/varlink.c @@ -171,6 +171,7 @@ struct Varlink { JsonVariant *current; VarlinkSymbol *current_method; + int peer_pidfd; struct ucred ucred; bool ucred_acquired:1; @@ -361,6 +362,8 @@ static int varlink_new(Varlink **ret) { .timeout = VARLINK_DEFAULT_TIMEOUT_USEC, .af = -1, + + .peer_pidfd = -EBADF, }; *ret = v; @@ -638,6 +641,8 @@ static void varlink_clear(Varlink *v) { sigterm_wait(v->exec_pid); v->exec_pid = 0; } + + v->peer_pidfd = safe_close(v->peer_pidfd); } static Varlink* varlink_destroy(Varlink *v) { @@ -2591,6 +2596,54 @@ int varlink_get_peer_pid(Varlink *v, pid_t *ret) { return 0; } +static int varlink_acquire_pidfd(Varlink *v) { + assert(v); + + if (v->peer_pidfd >= 0) + return 0; + + v->peer_pidfd = getpeerpidfd(v->fd); + if (v->peer_pidfd < 0) + return v->peer_pidfd; + + return 0; +} + +int varlink_get_peer_pidref(Varlink *v, PidRef *ret) { + int r; + + assert_return(v, -EINVAL); + assert_return(ret, -EINVAL); + + /* Returns r > 0 if we acquired the pidref via SO_PEERPIDFD (i.e. if we can use it for + * authentication). Returns == 0 if we didn't, and the pidref should not be used for + * authentication. */ + + r = varlink_acquire_pidfd(v); + if (r < 0) + return r; + + if (v->peer_pidfd < 0) { + pid_t pid; + + r = varlink_get_peer_pid(v, &pid); + if (r < 0) + return r; + + r = pidref_set_pid(ret, pid); + if (r < 0) + return r; + + return 0; /* didn't get pidfd securely */ + } + + r = pidref_set_pidfd(ret, v->peer_pidfd); + if (r < 0) + return r; + + return 1; /* got pidfd securely */ +} + int varlink_set_relative_timeout(Varlink *v, usec_t timeout) { assert_return(v, -EINVAL); assert_return(timeout > 0, -EINVAL); diff --git a/src/shared/varlink.h b/src/shared/varlink.h index c60f695be7..ad8e9cc46d 100644 --- a/src/shared/varlink.h +++ b/src/shared/varlink.h @@ -4,6 +4,7 @@ #include "sd-event.h" #include "json.h" +#include "pidref.h" #include "time-util.h" #include "varlink-idl.h" @@ -139,6 +140,7 @@ void* varlink_get_userdata(Varlink *v); int varlink_get_peer_uid(Varlink *v, uid_t *ret); int varlink_get_peer_pid(Varlink *v, pid_t *ret); +int varlink_get_peer_pidref(Varlink *v, PidRef *ret); int varlink_set_relative_timeout(Varlink *v, usec_t usec); From 35793c71e4276eceeac690680732f38e1a50475e Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 23 Nov 2023 18:17:44 +0100 Subject: [PATCH 06/11] varlink: add two helpers for delayed processing of method calls When we want to do Polkit authentication we want to temporarily pause handling of a method call until we have the Polkit reply, and then start again. Let's add some glue to make that easy. This adds two helpers: varlink_dispatch_again() allows to ask for redispatching of the currently queued incoming message. Usecase is this: if we don't process a methd right away, we can come back later, and ask it to be processed again with this function, in which case our handlers will be called a 2nd time, exactly like on the first time. varlink_get_current_message() provides access to the currently processed method call. With this the polkit logic can look into the current message, do its thing, and then restart the method handling. --- src/shared/varlink.c | 42 ++++++++++++++++++++++++++++++++++++++++++ src/shared/varlink.h | 6 ++++++ 2 files changed, 48 insertions(+) diff --git a/src/shared/varlink.c b/src/shared/varlink.c index 4664d6b2a6..b6dc0d8590 100644 --- a/src/shared/varlink.c +++ b/src/shared/varlink.c @@ -1482,6 +1482,48 @@ finish: return r; } +int varlink_dispatch_again(Varlink *v) { + int r; + + assert_return(v, -EINVAL); + + /* If a method call handler could not process the method call just yet (for example because it needed + * some Polkit authentication first), then it can leave the call unanswered, do its thing, and then + * ask to be dispatched a second time, via this call. It will then be called again, for the same + * message */ + + if (v->state == VARLINK_DISCONNECTED) + return varlink_log_errno(v, SYNTHETIC_ERRNO(ENOTCONN), "Not connected."); + if (v->state != VARLINK_PENDING_METHOD) + return varlink_log_errno(v, SYNTHETIC_ERRNO(EBUSY), "Connection has no pending method."); + + varlink_set_state(v, VARLINK_IDLE_SERVER); + + r = sd_event_source_set_enabled(v->defer_event_source, SD_EVENT_ON); + if (r < 0) + return varlink_log_errno(v, r, "Failed to enable deferred event source: %m"); + + return 0; +} + +int varlink_get_current_parameters(Varlink *v, JsonVariant **ret) { + JsonVariant *p; + + assert_return(v, -EINVAL); + + if (!v->current) + return -ENODATA; + + p = json_variant_by_key(v->current, "parameters"); + if (!p) + return -ENODATA; + + if (ret) + *ret = json_variant_ref(p); + + return 0; +} + static void handle_revents(Varlink *v, int revents) { assert(v); diff --git a/src/shared/varlink.h b/src/shared/varlink.h index ad8e9cc46d..bca4fab05d 100644 --- a/src/shared/varlink.h +++ b/src/shared/varlink.h @@ -117,6 +117,12 @@ int varlink_error_errno(Varlink *v, int error); int varlink_notify(Varlink *v, JsonVariant *parameters); int varlink_notifyb(Varlink *v, ...); +/* Ask for the current message to be dispatched again */ +int varlink_dispatch_again(Varlink *v); + +/* Get the currently processed incoming message */ +int varlink_get_current_parameters(Varlink *v, JsonVariant **ret); + /* Parsing incoming data via json_dispatch() and generate a nice error on parse errors */ int varlink_dispatch(Varlink *v, JsonVariant *parameters, const JsonDispatch table[], void *userdata); From d04c1a1c8e7c95daa483d8d52d5fc4c25fbc67f2 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 23 Nov 2023 18:21:21 +0100 Subject: [PATCH 07/11] bus-polkit: add support for authenticating varlink peers via polkit This extends our current polkit logic, so that we can in a very similar fashion as we already can authenticate dbus peers authenticate varlink connection peers. polkit natively speaks dbus and can authentication dbus peers. To get the same level of support for varlink we'll use authentication by pidfd+uid. This requires polkit v124, and if that's not available it will fallback to authorizing root only as before. Co-authored-by: Luca Boccassi --- src/shared/bus-polkit.c | 286 ++++++++++++++++++++++++++++++++++++---- src/shared/bus-polkit.h | 11 ++ src/shared/varlink.h | 3 + 3 files changed, 278 insertions(+), 22 deletions(-) diff --git a/src/shared/bus-polkit.c b/src/shared/bus-polkit.c index 9f923372a4..928b2c3d5b 100644 --- a/src/shared/bus-polkit.c +++ b/src/shared/bus-polkit.c @@ -4,10 +4,11 @@ #include "bus-message.h" #include "bus-polkit.h" #include "bus-util.h" +#include "process-util.h" #include "strv.h" #include "user-util.h" -static int check_good_user(sd_bus_message *m, uid_t good_user) { +static int bus_message_check_good_user(sd_bus_message *m, uid_t good_user) { _cleanup_(sd_bus_creds_unrefp) sd_bus_creds *creds = NULL; uid_t sender_uid; int r; @@ -15,7 +16,7 @@ static int check_good_user(sd_bus_message *m, uid_t good_user) { assert(m); if (good_user == UID_INVALID) - return 0; + return false; r = sd_bus_query_sender_creds(m, SD_BUS_CREDS_EUID, &creds); if (r < 0) @@ -54,7 +55,7 @@ static int bus_message_append_strv_key_value(sd_bus_message *m, const char **l) return r; } -static int bus_message_new_polkit_auth_call( +static int bus_message_new_polkit_auth_call_for_bus( sd_bus_message *m, const char *action, const char **details, @@ -115,7 +116,7 @@ int bus_test_polkit( /* Tests non-interactively! */ - r = check_good_user(call, good_user); + r = bus_message_check_good_user(call, good_user); if (r != 0) return r; @@ -129,7 +130,7 @@ int bus_test_polkit( _cleanup_(sd_bus_message_unrefp) sd_bus_message *request = NULL, *reply = NULL; int authorized = false, challenge = false; - r = bus_message_new_polkit_auth_call(call, action, details, /* interactive = */ false, &request); + r = bus_message_new_polkit_auth_call_for_bus(call, action, details, /* interactive = */ false, &request); if (r < 0) return r; @@ -190,8 +191,10 @@ typedef struct AsyncPolkitQuery { AsyncPolkitQueryAction *action; + sd_bus *bus; sd_bus_message *request; sd_bus_slot *slot; + Varlink *link; Hashmap *registry; sd_event_source *defer_event_source; @@ -213,6 +216,9 @@ static AsyncPolkitQuery *async_polkit_query_free(AsyncPolkitQuery *q) { sd_bus_message_unref(q->request); + sd_bus_unref(q->bus); + varlink_unref(q->link); + async_polkit_query_action_free(q->action); sd_event_source_disable_unref(q->defer_event_source); @@ -316,7 +322,7 @@ static int async_polkit_process_reply(sd_bus_message *reply, AsyncPolkitQuery *q if (!q->defer_event_source) { r = sd_event_add_defer( - sd_bus_get_event(sd_bus_message_get_bus(reply)), + sd_bus_get_event(q->bus), &q->defer_event_source, async_polkit_defer, q); @@ -332,13 +338,21 @@ static int async_polkit_process_reply(sd_bus_message *reply, AsyncPolkitQuery *q if (r < 0) return r; - r = sd_bus_message_rewind(q->request, true); - if (r < 0) - return r; + if (q->request) { + r = sd_bus_message_rewind(q->request, true); + if (r < 0) + return r; - r = sd_bus_enqueue_for_read(sd_bus_message_get_bus(q->request), q->request); - if (r < 0) - return r; + r = sd_bus_enqueue_for_read(q->bus, q->request); + if (r < 0) + return r; + } + + if (q->link) { + r = varlink_dispatch_again(q->link); + if (r < 0) + return r; + } return 1; } @@ -352,7 +366,10 @@ static int async_polkit_callback(sd_bus_message *reply, void *userdata, sd_bus_e r = async_polkit_process_reply(reply, q); if (r < 0) { log_debug_errno(r, "Processing asynchronous PolicyKit reply failed, ignoring: %m"); - (void) sd_bus_reply_method_errno(q->request, r, NULL); + if (q->request) + (void) sd_bus_reply_method_errno(q->request, r, NULL); + if (q->link) + varlink_error_errno(q->link, r); async_polkit_query_unref(q); } return r; @@ -366,11 +383,10 @@ static int async_polkit_query_check_action( assert(q); assert(action); - assert(ret_error); LIST_FOREACH(authorized, a, q->authorized_actions) if (streq(a->action, action) && strv_equal(a->details, (char**) details)) - return 1; + return 1; /* Allow! */ if (q->error_action && streq(q->error_action->action, action)) return sd_bus_error_copy(ret_error, &q->error); @@ -480,7 +496,7 @@ int bus_verify_polkit_async_full( assert(registry); assert(ret_error); - r = check_good_user(call, good_user); + r = bus_message_check_good_user(call, good_user); if (r != 0) return r; @@ -512,11 +528,7 @@ int bus_verify_polkit_async_full( if (c > 0) interactive = true; - r = hashmap_ensure_allocated(registry, NULL); - if (r < 0) - return r; - - r = bus_message_new_polkit_auth_call(call, action, details, interactive, &pk); + r = bus_message_new_polkit_auth_call_for_bus(call, action, details, interactive, &pk); if (r < 0) return r; @@ -528,6 +540,7 @@ int bus_verify_polkit_async_full( *q = (AsyncPolkitQuery) { .n_ref = 1, .request = sd_bus_message_ref(call), + .bus = sd_bus_ref(sd_bus_message_get_bus(call)), }; } @@ -544,7 +557,7 @@ int bus_verify_polkit_async_full( return -ENOMEM; if (!q->registry) { - r = hashmap_put(*registry, call, q); + r = hashmap_ensure_put(registry, /* hash_ops= */ NULL, call, q); if (r < 0) return r; @@ -571,3 +584,232 @@ Hashmap *bus_verify_polkit_async_registry_free(Hashmap *registry) { return hashmap_free(registry); #endif } + +static int varlink_check_good_user(Varlink *link, uid_t good_user) { + int r; + + assert(link); + + if (good_user == UID_INVALID) + return false; + + uid_t peer_uid; + r = varlink_get_peer_uid(link, &peer_uid); + if (r < 0) + return r; + + return good_user == peer_uid; +} + +static int varlink_check_peer_privilege(Varlink *link) { + int r; + + assert(link); + + uid_t peer_uid; + r = varlink_get_peer_uid(link, &peer_uid); + if (r < 0) + return r; + + uid_t our_uid = getuid(); + return peer_uid == our_uid || + (our_uid != 0 && peer_uid == 0); +} + +#if ENABLE_POLKIT +static int bus_message_new_polkit_auth_call_for_varlink( + sd_bus *bus, + Varlink *link, + const char *action, + const char **details, + bool interactive, + sd_bus_message **ret) { + + _cleanup_(sd_bus_message_unrefp) sd_bus_message *c = NULL; + _cleanup_(pidref_done) PidRef pidref = PIDREF_NULL; + int r; + + assert(bus); + assert(link); + assert(action); + assert(ret); + + r = varlink_get_peer_pidref(link, &pidref); + if (r < 0) + return r; + if (r == 0) /* if we couldn't get a pidfd this returns == 0 */ + return log_debug_errno(SYNTHETIC_ERRNO(EPERM), "Failed to get peer pidfd, cannot securely authenticate."); + + uid_t uid; + r = varlink_get_peer_uid(link, &uid); + if (r < 0) + return r; + + r = sd_bus_message_new_method_call( + bus, + &c, + "org.freedesktop.PolicyKit1", + "/org/freedesktop/PolicyKit1/Authority", + "org.freedesktop.PolicyKit1.Authority", + "CheckAuthorization"); + if (r < 0) + return r; + + r = sd_bus_message_append( + c, + "(sa{sv})s", + "unix-process", 2, + "pidfd", "h", (uint32_t) pidref.fd, + "uid", "i", (int32_t) uid, + action); + if (r < 0) + return r; + + r = bus_message_append_strv_key_value(c, details); + if (r < 0) + return r; + + r = sd_bus_message_append(c, "us", interactive, NULL); + if (r < 0) + return r; + + *ret = TAKE_PTR(c); + return 0; +} + +static bool varlink_allow_interactive_authentication(Varlink *link) { + _cleanup_(json_variant_unrefp) JsonVariant *v = NULL; + int r; + + assert(link); + + /* We look for the allowInteractiveAuthentication field in the message currently being dispatched, + * always under the same name. */ + + r = varlink_get_current_parameters(link, &v); + if (r < 0) + return r; + + JsonVariant *b; + b = json_variant_by_key(v, "allowInteractiveAuthentication"); + if (b) { + if (json_variant_is_boolean(b)) + return json_variant_boolean(b); + + log_debug("Incoming 'allowInteractiveAuthentication' field is not a boolean, ignoring."); + } + + return false; +} +#endif + +int varlink_verify_polkit_async( + Varlink *link, + sd_bus *bus, + const char *action, + const char **details, + uid_t good_user, + Hashmap **registry) { + + int r; + + assert(link); + assert(registry); + + /* 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. */ + + r = varlink_check_good_user(link, good_user); + if (r != 0) + return r; + + r = varlink_check_peer_privilege(link); + if (r != 0) + return r; + +#if ENABLE_POLKIT + _cleanup_(async_polkit_query_unrefp) AsyncPolkitQuery *q = NULL; + + q = async_polkit_query_ref(hashmap_get(*registry, link)); + /* 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) { + _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; + r = async_polkit_query_check_action(q, action, details, &error); + if (r < 0) { + /* Reply with a nice error */ + if (sd_bus_error_has_name(&error, SD_BUS_ERROR_INTERACTIVE_AUTHORIZATION_REQUIRED)) + return varlink_error(link, VARLINK_ERROR_INTERACTIVE_AUTHENTICATION_REQUIRED, NULL); + + if (ERRNO_IS_NEG_PRIVILEGE(r)) + return varlink_error(link, VARLINK_ERROR_PERMISSION_DENIED, NULL); + + return r; + } + if (r > 0) + return r; + } + + _cleanup_(sd_bus_unrefp) sd_bus *mybus = NULL; + if (!bus) { + r = sd_bus_open_system(&mybus); + if (r < 0) + return r; + + r = sd_bus_attach_event(mybus, varlink_get_event(link), 0); + if (r < 0) + return r; + + bus = mybus; + } + + bool interactive = varlink_allow_interactive_authentication(link); + + _cleanup_(sd_bus_message_unrefp) sd_bus_message *pk = NULL; + r = bus_message_new_polkit_auth_call_for_varlink(bus, link, action, details, interactive, &pk); + if (r < 0) + return r; + + if (!q) { + q = new(AsyncPolkitQuery, 1); + if (!q) + return -ENOMEM; + + *q = (AsyncPolkitQuery) { + .n_ref = 1, + .link = varlink_ref(link), + .bus = sd_bus_ref(bus), + }; + } + + assert(!q->action); + q->action = new(AsyncPolkitQueryAction, 1); + if (!q->action) + return -ENOMEM; + + *q->action = (AsyncPolkitQueryAction) { + .action = strdup(action), + .details = strv_copy((char**) details), + }; + if (!q->action->action || !q->action->details) + return -ENOMEM; + + if (!q->registry) { + r = hashmap_ensure_put(registry, /* hash_ops= */ NULL, link, q); + if (r < 0) + return r; + + q->registry = *registry; + } + + r = sd_bus_call_async(bus, &q->slot, pk, async_polkit_callback, q, 0); + if (r < 0) + return r; + + TAKE_PTR(q); + + return 0; +#endif + + return -EACCES; +} diff --git a/src/shared/bus-polkit.h b/src/shared/bus-polkit.h index d82ac4679c..ac2a90c3c7 100644 --- a/src/shared/bus-polkit.h +++ b/src/shared/bus-polkit.h @@ -5,6 +5,7 @@ #include "hashmap.h" #include "user-util.h" +#include "varlink.h" int bus_test_polkit(sd_bus_message *call, const char *action, const char **details, uid_t good_user, bool *_challenge, sd_bus_error *e); @@ -14,3 +15,13 @@ static inline int bus_verify_polkit_async(sd_bus_message *call, const char *acti } Hashmap *bus_verify_polkit_async_registry_free(Hashmap *registry); + +int varlink_verify_polkit_async(Varlink *link, sd_bus *bus, const char *action, const char **details, uid_t good_user, Hashmap **registry); + +/* A JsonDispatch initializer that makes sure the allowInteractiveAuthentication boolean field we want for + * polkit support in Varlink calls is ignored while regular dispatching (and does not result in errors + * regarding unexpected fields) */ +#define VARLINK_DISPATCH_POLKIT_FIELD { \ + .name = "allowInteractiveAuthentication", \ + .type = JSON_VARIANT_BOOLEAN, \ + } diff --git a/src/shared/varlink.h b/src/shared/varlink.h index bca4fab05d..a971762a51 100644 --- a/src/shared/varlink.h +++ b/src/shared/varlink.h @@ -222,6 +222,9 @@ DEFINE_TRIVIAL_CLEANUP_FUNC(VarlinkServer *, varlink_server_unref); /* This one we invented, and use for generically propagating system errors (errno) to clients */ #define VARLINK_ERROR_SYSTEM "io.systemd.System" +/* This one we invented and is a weaker version of "org.varlink.service.PermissionDenied", and indicates that if user would allow interactive auth, we might allow access */ +#define VARLINK_ERROR_INTERACTIVE_AUTHENTICATION_REQUIRED "io.systemd.InteractiveAuthenticationRequired" + /* These are errors defined in the Varlink spec */ #define VARLINK_ERROR_INTERFACE_NOT_FOUND "org.varlink.service.InterfaceNotFound" #define VARLINK_ERROR_METHOD_NOT_FOUND "org.varlink.service.MethodNotFound" From 2a1ffd3e3ada02f9b38adf0659423087074e62eb Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 24 Nov 2023 10:56:28 +0100 Subject: [PATCH 08/11] bus-polkit: port polkit_registry to use value destructors in hash_ops --- src/core/dbus.c | 2 +- src/home/homed-manager.c | 2 +- src/hostname/hostnamed.c | 2 +- src/import/importd.c | 2 +- src/locale/localed-util.c | 2 +- src/login/logind.c | 2 +- src/machine/machined.c | 2 +- src/network/networkd-manager.c | 3 +-- src/oom/oomd-manager.c | 2 +- src/portable/portabled.c | 2 +- src/resolve/resolved-manager.c | 2 +- src/shared/bus-polkit.c | 21 ++++++++++----------- src/shared/bus-polkit.h | 2 -- src/timedate/timedated.c | 2 +- src/timesync/timesyncd-manager.c | 2 +- 15 files changed, 23 insertions(+), 27 deletions(-) diff --git a/src/core/dbus.c b/src/core/dbus.c index f7d4a97096..e24c5bbc53 100644 --- a/src/core/dbus.c +++ b/src/core/dbus.c @@ -1073,7 +1073,7 @@ void bus_done(Manager *m) { assert(!m->subscribed); m->deserialized_subscribed = strv_free(m->deserialized_subscribed); - bus_verify_polkit_async_registry_free(m->polkit_registry); + m->polkit_registry = hashmap_free(m->polkit_registry); } int bus_fdset_add_all(Manager *m, FDSet *fds) { diff --git a/src/home/homed-manager.c b/src/home/homed-manager.c index c4525310fc..94b2ea5181 100644 --- a/src/home/homed-manager.c +++ b/src/home/homed-manager.c @@ -268,7 +268,7 @@ Manager* manager_free(Manager *m) { (void) home_wait_for_worker(h); m->bus = sd_bus_flush_close_unref(m->bus); - m->polkit_registry = bus_verify_polkit_async_registry_free(m->polkit_registry); + m->polkit_registry = hashmap_free(m->polkit_registry); m->device_monitor = sd_device_monitor_unref(m->device_monitor); diff --git a/src/hostname/hostnamed.c b/src/hostname/hostnamed.c index 893eb4cc0f..f0e643822a 100644 --- a/src/hostname/hostnamed.c +++ b/src/hostname/hostnamed.c @@ -91,7 +91,7 @@ static void context_destroy(Context *c) { assert(c); context_reset(c, UINT64_MAX); - bus_verify_polkit_async_registry_free(c->polkit_registry); + hashmap_free(c->polkit_registry); } static void context_read_etc_hostname(Context *c) { diff --git a/src/import/importd.c b/src/import/importd.c index e9bbbb628d..8bc8a32866 100644 --- a/src/import/importd.c +++ b/src/import/importd.c @@ -527,7 +527,7 @@ static Manager *manager_unref(Manager *m) { hashmap_free(m->transfers); - bus_verify_polkit_async_registry_free(m->polkit_registry); + hashmap_free(m->polkit_registry); m->bus = sd_bus_flush_close_unref(m->bus); sd_event_unref(m->event); diff --git a/src/locale/localed-util.c b/src/locale/localed-util.c index e4e57a0f4a..0489df573b 100644 --- a/src/locale/localed-util.c +++ b/src/locale/localed-util.c @@ -304,7 +304,7 @@ void context_clear(Context *c) { c->x11_cache = sd_bus_message_unref(c->x11_cache); c->vc_cache = sd_bus_message_unref(c->vc_cache); - c->polkit_registry = bus_verify_polkit_async_registry_free(c->polkit_registry); + c->polkit_registry = hashmap_free(c->polkit_registry); }; X11Context *context_get_x11_context(Context *c) { diff --git a/src/login/logind.c b/src/login/logind.c index 965e2a4aef..e96ddf5ae4 100644 --- a/src/login/logind.c +++ b/src/login/logind.c @@ -154,7 +154,7 @@ static Manager* manager_free(Manager *m) { if (m->unlink_nologin) (void) unlink_or_warn("/run/nologin"); - bus_verify_polkit_async_registry_free(m->polkit_registry); + hashmap_free(m->polkit_registry); sd_bus_flush_close_unref(m->bus); sd_event_unref(m->event); diff --git a/src/machine/machined.c b/src/machine/machined.c index 58a407d451..4e830a4bc2 100644 --- a/src/machine/machined.c +++ b/src/machine/machined.c @@ -96,7 +96,7 @@ static Manager* manager_unref(Manager *m) { sd_event_source_unref(m->nscd_cache_flush_event); #endif - bus_verify_polkit_async_registry_free(m->polkit_registry); + hashmap_free(m->polkit_registry); manager_varlink_done(m); diff --git a/src/network/networkd-manager.c b/src/network/networkd-manager.c index e81905ad98..8933fc4977 100644 --- a/src/network/networkd-manager.c +++ b/src/network/networkd-manager.c @@ -638,8 +638,7 @@ Manager* manager_free(Manager *m) { sd_device_monitor_unref(m->device_monitor); manager_varlink_done(m); - - bus_verify_polkit_async_registry_free(m->polkit_registry); + hashmap_free(m->polkit_registry); sd_bus_flush_close_unref(m->bus); free(m->dynamic_timezone); diff --git a/src/oom/oomd-manager.c b/src/oom/oomd-manager.c index 6081254b3d..23c3ae64ab 100644 --- a/src/oom/oomd-manager.c +++ b/src/oom/oomd-manager.c @@ -642,7 +642,7 @@ Manager* manager_free(Manager *m) { sd_event_source_unref(m->mem_pressure_context_event_source); sd_event_unref(m->event); - bus_verify_polkit_async_registry_free(m->polkit_registry); + hashmap_free(m->polkit_registry); sd_bus_flush_close_unref(m->bus); hashmap_free(m->monitored_swap_cgroup_contexts); diff --git a/src/portable/portabled.c b/src/portable/portabled.c index 136c5fa191..7c2ddd90ef 100644 --- a/src/portable/portabled.c +++ b/src/portable/portabled.c @@ -65,7 +65,7 @@ static Manager* manager_unref(Manager *m) { sd_event_source_unref(m->image_cache_defer_event); - bus_verify_polkit_async_registry_free(m->polkit_registry); + hashmap_free(m->polkit_registry); sd_bus_flush_close_unref(m->bus); sd_event_unref(m->event); diff --git a/src/resolve/resolved-manager.c b/src/resolve/resolved-manager.c index 0295662b5b..a0251b4b97 100644 --- a/src/resolve/resolved-manager.c +++ b/src/resolve/resolved-manager.c @@ -731,7 +731,7 @@ Manager *manager_free(Manager *m) { ordered_set_free(m->dns_extra_stub_listeners); - bus_verify_polkit_async_registry_free(m->polkit_registry); + hashmap_free(m->polkit_registry); sd_bus_flush_close_unref(m->bus); diff --git a/src/shared/bus-polkit.c b/src/shared/bus-polkit.c index 928b2c3d5b..fe9363aad4 100644 --- a/src/shared/bus-polkit.c +++ b/src/shared/bus-polkit.c @@ -236,6 +236,14 @@ static AsyncPolkitQuery *async_polkit_query_free(AsyncPolkitQuery *q) { DEFINE_PRIVATE_TRIVIAL_REF_UNREF_FUNC(AsyncPolkitQuery, async_polkit_query, async_polkit_query_free); DEFINE_TRIVIAL_CLEANUP_FUNC(AsyncPolkitQuery*, async_polkit_query_unref); +DEFINE_HASH_OPS_WITH_VALUE_DESTRUCTOR( + async_polkit_query_hash_ops, + void, + trivial_hash_func, + trivial_compare_func, + AsyncPolkitQuery, + async_polkit_query_unref); + static int async_polkit_defer(sd_event_source *s, void *userdata) { AsyncPolkitQuery *q = ASSERT_PTR(userdata); @@ -557,7 +565,7 @@ int bus_verify_polkit_async_full( return -ENOMEM; if (!q->registry) { - r = hashmap_ensure_put(registry, /* hash_ops= */ NULL, call, q); + r = hashmap_ensure_put(registry, &async_polkit_query_hash_ops, call, q); if (r < 0) return r; @@ -576,15 +584,6 @@ int bus_verify_polkit_async_full( return -EACCES; } -Hashmap *bus_verify_polkit_async_registry_free(Hashmap *registry) { -#if ENABLE_POLKIT - return hashmap_free_with_destructor(registry, async_polkit_query_unref); -#else - assert(hashmap_isempty(registry)); - return hashmap_free(registry); -#endif -} - static int varlink_check_good_user(Varlink *link, uid_t good_user) { int r; @@ -795,7 +794,7 @@ int varlink_verify_polkit_async( return -ENOMEM; if (!q->registry) { - r = hashmap_ensure_put(registry, /* hash_ops= */ NULL, link, q); + r = hashmap_ensure_put(registry, &async_polkit_query_hash_ops, link, q); if (r < 0) return r; diff --git a/src/shared/bus-polkit.h b/src/shared/bus-polkit.h index ac2a90c3c7..0fe3a4ca0e 100644 --- a/src/shared/bus-polkit.h +++ b/src/shared/bus-polkit.h @@ -14,8 +14,6 @@ static inline int bus_verify_polkit_async(sd_bus_message *call, const char *acti return bus_verify_polkit_async_full(call, action, details, false, UID_INVALID, registry, ret_error); } -Hashmap *bus_verify_polkit_async_registry_free(Hashmap *registry); - int varlink_verify_polkit_async(Varlink *link, sd_bus *bus, const char *action, const char **details, uid_t good_user, Hashmap **registry); /* A JsonDispatch initializer that makes sure the allowInteractiveAuthentication boolean field we want for diff --git a/src/timedate/timedated.c b/src/timedate/timedated.c index 53c2a6fb71..b4a58fc692 100644 --- a/src/timedate/timedated.c +++ b/src/timedate/timedated.c @@ -119,7 +119,7 @@ static void context_clear(Context *c) { assert(c); free(c->zone); - bus_verify_polkit_async_registry_free(c->polkit_registry); + hashmap_free(c->polkit_registry); sd_bus_message_unref(c->cache); sd_bus_slot_unref(c->slot_job_removed); diff --git a/src/timesync/timesyncd-manager.c b/src/timesync/timesyncd-manager.c index 1317bc0f76..6ed15aa402 100644 --- a/src/timesync/timesyncd-manager.c +++ b/src/timesync/timesyncd-manager.c @@ -962,7 +962,7 @@ Manager* manager_free(Manager *m) { sd_bus_flush_close_unref(m->bus); - bus_verify_polkit_async_registry_free(m->polkit_registry); + hashmap_free(m->polkit_registry); return mfree(m); } From caef0bc3dcd3b7d6883043529baac02cdb6b898c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 23 Nov 2023 22:22:27 +0100 Subject: [PATCH 09/11] creds: open up access to clients via Polkit Use auth_admin_keep, so that users don't have to re-auth interactively again and again when encrypting/decrypting batches of credentials. --- src/creds/creds.c | 38 +++++++++++++++++++---- src/creds/io.systemd.credentials.policy | 40 +++++++++++++++++++++++++ src/creds/meson.build | 3 ++ units/systemd-creds.socket | 2 +- 4 files changed, 76 insertions(+), 7 deletions(-) create mode 100644 src/creds/io.systemd.credentials.policy diff --git a/src/creds/creds.c b/src/creds/creds.c index ed39ffe51e..5586baff9a 100644 --- a/src/creds/creds.c +++ b/src/creds/creds.c @@ -4,6 +4,7 @@ #include #include "build.h" +#include "bus-polkit.h" #include "creds-util.h" #include "dirent-util.h" #include "escape.h" @@ -983,6 +984,7 @@ static int vl_method_encrypt(Varlink *link, JsonVariant *parameters, VarlinkMeth { "data", JSON_VARIANT_STRING, json_dispatch_unbase64_iovec, offsetof(MethodEncryptParameters, data), 0 }, { "timestamp", _JSON_VARIANT_TYPE_INVALID, json_dispatch_uint64, offsetof(MethodEncryptParameters, timestamp), 0 }, { "notAfter", _JSON_VARIANT_TYPE_INVALID, json_dispatch_uint64, offsetof(MethodEncryptParameters, not_after), 0 }, + VARLINK_DISPATCH_POLKIT_FIELD, {} }; _cleanup_(method_encrypt_parameters_done) MethodEncryptParameters p = { @@ -990,6 +992,7 @@ static int vl_method_encrypt(Varlink *link, JsonVariant *parameters, VarlinkMeth .not_after = UINT64_MAX, }; _cleanup_(iovec_done) struct iovec output = {}; + Hashmap **polkit_registry = ASSERT_PTR(userdata); int r; assert(link); @@ -1010,6 +1013,16 @@ static int vl_method_encrypt(Varlink *link, JsonVariant *parameters, VarlinkMeth if (p.not_after != UINT64_MAX && p.not_after < p.timestamp) return varlink_error_invalid_parameter_name(link, "notAfter"); + r = varlink_verify_polkit_async( + link, + /* bus= */ NULL, + "io.systemd.credentials.encrypt", + /* details= */ NULL, + /* good_user= */ UID_INVALID, + polkit_registry); + if (r <= 0) + return r; + r = encrypt_credential_and_warn( arg_with_key, p.name, @@ -1051,15 +1064,17 @@ static void method_decrypt_parameters_done(MethodDecryptParameters *p) { static int vl_method_decrypt(Varlink *link, JsonVariant *parameters, VarlinkMethodFlags flags, void *userdata) { static const JsonDispatch dispatch_table[] = { - { "name", JSON_VARIANT_STRING, json_dispatch_const_string, offsetof(MethodDecryptParameters, name), 0 }, - { "blob", JSON_VARIANT_STRING, json_dispatch_unbase64_iovec, offsetof(MethodDecryptParameters, blob), 0 }, - { "timestamp", _JSON_VARIANT_TYPE_INVALID, json_dispatch_uint64, offsetof(MethodDecryptParameters, timestamp), 0 }, + { "name", JSON_VARIANT_STRING, json_dispatch_const_string, offsetof(MethodDecryptParameters, name), 0 }, + { "blob", JSON_VARIANT_STRING, json_dispatch_unbase64_iovec, offsetof(MethodDecryptParameters, blob), JSON_MANDATORY }, + { "timestamp", _JSON_VARIANT_TYPE_INVALID, json_dispatch_uint64, offsetof(MethodDecryptParameters, timestamp), 0 }, + VARLINK_DISPATCH_POLKIT_FIELD, {} }; _cleanup_(method_decrypt_parameters_done) MethodDecryptParameters p = { .timestamp = UINT64_MAX, }; _cleanup_(iovec_done_erase) struct iovec output = {}; + Hashmap **polkit_registry = ASSERT_PTR(userdata); int r; assert(link); @@ -1073,11 +1088,19 @@ static int vl_method_decrypt(Varlink *link, JsonVariant *parameters, VarlinkMeth if (p.name && !credential_name_valid(p.name)) return varlink_error_invalid_parameter_name(link, "name"); - if (!p.blob.iov_base) - return varlink_error_invalid_parameter_name(link, "blob"); if (p.timestamp == UINT64_MAX) p.timestamp = now(CLOCK_REALTIME); + r = varlink_verify_polkit_async( + link, + /* bus= */ NULL, + "io.systemd.credentials.decrypt", + /* details= */ NULL, + /* good_user= */ UID_INVALID, + polkit_registry); + if (r <= 0) + return r; + r = decrypt_credential_and_warn( p.name, p.timestamp, @@ -1116,10 +1139,11 @@ static int run(int argc, char *argv[]) { if (arg_varlink) { _cleanup_(varlink_server_unrefp) VarlinkServer *varlink_server = NULL; + _cleanup_(hashmap_freep) Hashmap *polkit_registry = NULL; /* Invocation as Varlink service */ - r = varlink_server_new(&varlink_server, VARLINK_SERVER_ROOT_ONLY|VARLINK_SERVER_INHERIT_USERDATA); + r = varlink_server_new(&varlink_server, VARLINK_SERVER_ACCOUNT_UID|VARLINK_SERVER_INHERIT_USERDATA); if (r < 0) return log_error_errno(r, "Failed to allocate Varlink server: %m"); @@ -1134,6 +1158,8 @@ static int run(int argc, char *argv[]) { if (r < 0) return log_error_errno(r, "Failed to bind Varlink methods: %m"); + varlink_server_set_userdata(varlink_server, &polkit_registry); + r = varlink_server_loop_auto(varlink_server); if (r < 0) return log_error_errno(r, "Failed to run Varlink event loop: %m"); diff --git a/src/creds/io.systemd.credentials.policy b/src/creds/io.systemd.credentials.policy new file mode 100644 index 0000000000..f94571b215 --- /dev/null +++ b/src/creds/io.systemd.credentials.policy @@ -0,0 +1,40 @@ + + + + + + + + The systemd Project + https://systemd.io + + + Allow encryption and signing of system credentials. + Authentication is required for an application to encrypt and sign a system credential. + + auth_admin_keep + auth_admin_keep + auth_admin_keep + + + + + Allow decryption of system credentials. + Authentication is required for an application to decrypto a system credential. + + auth_admin_keep + auth_admin_keep + auth_admin_keep + + + diff --git a/src/creds/meson.build b/src/creds/meson.build index 85572568f6..24833110d5 100644 --- a/src/creds/meson.build +++ b/src/creds/meson.build @@ -23,3 +23,6 @@ if install_sysconfdir install_emptydir(sysconfdir / 'credstore.encrypted', install_mode : 'rwx------') endif + +install_data('io.systemd.credentials.policy', + install_dir : polkitpolicydir) diff --git a/units/systemd-creds.socket b/units/systemd-creds.socket index 794fac19a8..ac72ccc82f 100644 --- a/units/systemd-creds.socket +++ b/units/systemd-creds.socket @@ -16,7 +16,7 @@ Before=sockets.target [Socket] ListenStream=/run/systemd/io.systemd.Credentials FileDescriptorName=varlink -SocketMode=0600 +SocketMode=0666 Accept=yes [Install] From fa9a6db478e3f0f2753e4633af6d0d4881707c2b Mon Sep 17 00:00:00 2001 From: Luca Boccassi Date: Fri, 22 Dec 2023 23:23:20 +0100 Subject: [PATCH 10/11] json: add JSON_FORMAT_REFUSE_SENSITIVE to json_variant_format() Returns -EPERM if any node in the variant is marked as sensitive, useful to avoid leaking data to log messages and so on --- src/shared/json.c | 26 +++++++++++ src/shared/json.h | 23 +++++----- src/test/test-json.c | 105 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 143 insertions(+), 11 deletions(-) diff --git a/src/shared/json.c b/src/shared/json.c index af2fc9aa70..15675a7046 100644 --- a/src/shared/json.c +++ b/src/shared/json.c @@ -1771,6 +1771,28 @@ static int json_format(FILE *f, JsonVariant *v, JsonFormatFlags flags, const cha return 0; } +static bool json_variant_is_sensitive_recursive(JsonVariant *v) { + if (!v) + return false; + if (json_variant_is_sensitive(v)) + return true; + if (v == JSON_VARIANT_MAGIC_EMPTY_ARRAY || + v == JSON_VARIANT_MAGIC_EMPTY_OBJECT) + return false; + if (!json_variant_is_regular(v)) + return false; + if (!IN_SET(v->type, JSON_VARIANT_ARRAY, JSON_VARIANT_OBJECT)) + return false; + if (v->is_reference) + return json_variant_is_sensitive_recursive(v->reference); + + for (size_t i = 0; i < json_variant_elements(v); i++) + if (json_variant_is_sensitive_recursive(json_variant_by_index(v, i))) + return true; + + return false; +} + int json_variant_format(JsonVariant *v, JsonFormatFlags flags, char **ret) { _cleanup_(memstream_done) MemStream m = {}; size_t sz; @@ -1786,6 +1808,10 @@ int json_variant_format(JsonVariant *v, JsonFormatFlags flags, char **ret) { if (flags & JSON_FORMAT_OFF) return -ENOEXEC; + if ((flags & JSON_FORMAT_REFUSE_SENSITIVE)) + if (json_variant_is_sensitive_recursive(v)) + return -EPERM; + f = memstream_init(&m); if (!f) return -ENOMEM; diff --git a/src/shared/json.h b/src/shared/json.h index c40c23487a..975cf562a9 100644 --- a/src/shared/json.h +++ b/src/shared/json.h @@ -185,17 +185,18 @@ struct json_variant_foreach_state { int json_variant_get_source(JsonVariant *v, const char **ret_source, unsigned *ret_line, unsigned *ret_column); typedef enum JsonFormatFlags { - JSON_FORMAT_NEWLINE = 1 << 0, /* suffix with newline */ - JSON_FORMAT_PRETTY = 1 << 1, /* add internal whitespace to appeal to human readers */ - JSON_FORMAT_PRETTY_AUTO = 1 << 2, /* same, but only if connected to a tty (and JSON_FORMAT_NEWLINE otherwise) */ - JSON_FORMAT_COLOR = 1 << 3, /* insert ANSI color sequences */ - JSON_FORMAT_COLOR_AUTO = 1 << 4, /* insert ANSI color sequences if colors_enabled() says so */ - JSON_FORMAT_SOURCE = 1 << 5, /* prefix with source filename/line/column */ - JSON_FORMAT_SSE = 1 << 6, /* prefix/suffix with W3C server-sent events */ - JSON_FORMAT_SEQ = 1 << 7, /* prefix/suffix with RFC 7464 application/json-seq */ - JSON_FORMAT_FLUSH = 1 << 8, /* call fflush() after dumping JSON */ - JSON_FORMAT_EMPTY_ARRAY = 1 << 9, /* output "[]" for empty input */ - JSON_FORMAT_OFF = 1 << 10, /* make json_variant_format() fail with -ENOEXEC */ + JSON_FORMAT_NEWLINE = 1 << 0, /* suffix with newline */ + JSON_FORMAT_PRETTY = 1 << 1, /* add internal whitespace to appeal to human readers */ + JSON_FORMAT_PRETTY_AUTO = 1 << 2, /* same, but only if connected to a tty (and JSON_FORMAT_NEWLINE otherwise) */ + JSON_FORMAT_COLOR = 1 << 3, /* insert ANSI color sequences */ + JSON_FORMAT_COLOR_AUTO = 1 << 4, /* insert ANSI color sequences if colors_enabled() says so */ + JSON_FORMAT_SOURCE = 1 << 5, /* prefix with source filename/line/column */ + JSON_FORMAT_SSE = 1 << 6, /* prefix/suffix with W3C server-sent events */ + JSON_FORMAT_SEQ = 1 << 7, /* prefix/suffix with RFC 7464 application/json-seq */ + JSON_FORMAT_FLUSH = 1 << 8, /* call fflush() after dumping JSON */ + JSON_FORMAT_EMPTY_ARRAY = 1 << 9, /* output "[]" for empty input */ + JSON_FORMAT_OFF = 1 << 10, /* make json_variant_format() fail with -ENOEXEC */ + JSON_FORMAT_REFUSE_SENSITIVE = 1 << 11, /* return EPERM if any node in the tree is marked as senstitive */ } JsonFormatFlags; int json_variant_format(JsonVariant *v, JsonFormatFlags flags, char **ret); diff --git a/src/test/test-json.c b/src/test/test-json.c index c120a702c6..4ceb084c0c 100644 --- a/src/test/test-json.c +++ b/src/test/test-json.c @@ -104,6 +104,17 @@ static void test_variant_one(const char *data, Test test) { assert_se(json_variant_has_type(w, json_variant_type(v))); assert_se(json_variant_equal(v, w)); + s = mfree(s); + r = json_variant_format(w, JSON_FORMAT_REFUSE_SENSITIVE, &s); + assert_se(r == -EPERM); + assert_se(!s); + + s = mfree(s); + r = json_variant_format(w, JSON_FORMAT_PRETTY, &s); + assert_se(r >= 0); + assert_se(s); + assert_se((size_t) r == strlen(s)); + s = mfree(s); w = json_variant_unref(w); @@ -813,4 +824,98 @@ TEST(json_dispatch) { assert_se(foobar.l == INT16_MIN); } +TEST(json_sensitive) { + _cleanup_(json_variant_unrefp) JsonVariant *a = NULL, *b = NULL, *v = NULL; + _cleanup_free_ char *s = NULL; + int r; + + assert_se(json_build(&a, JSON_BUILD_STRV(STRV_MAKE("foo", "bar", "baz", "bar", "baz", "foo", "qux", "baz"))) >= 0); + assert_se(json_build(&b, JSON_BUILD_STRV(STRV_MAKE("foo", "bar", "baz", "qux"))) >= 0); + + json_variant_sensitive(a); + + assert_se(json_variant_format(a, JSON_FORMAT_REFUSE_SENSITIVE, &s) == -EPERM); + assert_se(!s); + + r = json_variant_format(b, JSON_FORMAT_REFUSE_SENSITIVE, &s); + assert_se(r >= 0); + assert_se(s); + assert_se((size_t) r == strlen(s)); + s = mfree(s); + + assert_se(json_build(&v, JSON_BUILD_OBJECT( + JSON_BUILD_PAIR("c", JSON_BUILD_INTEGER(INT64_MIN)), + JSON_BUILD_PAIR("d", JSON_BUILD_STRING("-9223372036854775808")), + JSON_BUILD_PAIR("e", JSON_BUILD_EMPTY_OBJECT))) >= 0); + json_variant_dump(v, JSON_FORMAT_COLOR|JSON_FORMAT_PRETTY, NULL, NULL); + + r = json_variant_format(v, JSON_FORMAT_REFUSE_SENSITIVE, &s); + assert_se(r >= 0); + assert_se(s); + assert_se((size_t) r == strlen(s)); + s = mfree(s); + v = json_variant_unref(v); + + assert_se(json_build(&v, JSON_BUILD_OBJECT( + JSON_BUILD_PAIR_VARIANT("b", b), + JSON_BUILD_PAIR("c", JSON_BUILD_INTEGER(INT64_MIN)), + JSON_BUILD_PAIR("d", JSON_BUILD_STRING("-9223372036854775808")), + JSON_BUILD_PAIR("e", JSON_BUILD_EMPTY_OBJECT))) >= 0); + json_variant_dump(v, JSON_FORMAT_COLOR|JSON_FORMAT_PRETTY, NULL, NULL); + + r = json_variant_format(v, JSON_FORMAT_REFUSE_SENSITIVE, &s); + assert_se(r >= 0); + assert_se(s); + assert_se((size_t) r == strlen(s)); + s = mfree(s); + v = json_variant_unref(v); + + assert_se(json_build(&v, JSON_BUILD_OBJECT( + JSON_BUILD_PAIR_VARIANT("b", b), + JSON_BUILD_PAIR_VARIANT("a", a), + JSON_BUILD_PAIR("c", JSON_BUILD_INTEGER(INT64_MIN)), + JSON_BUILD_PAIR("d", JSON_BUILD_STRING("-9223372036854775808")), + JSON_BUILD_PAIR("e", JSON_BUILD_EMPTY_OBJECT))) >= 0); + json_variant_dump(v, JSON_FORMAT_COLOR|JSON_FORMAT_PRETTY, NULL, NULL); + + assert_se(json_variant_format(v, JSON_FORMAT_REFUSE_SENSITIVE, &s) == -EPERM); + assert_se(!s); + v = json_variant_unref(v); + + assert_se(json_build(&v, JSON_BUILD_OBJECT( + JSON_BUILD_PAIR_VARIANT("b", b), + JSON_BUILD_PAIR("c", JSON_BUILD_INTEGER(INT64_MIN)), + JSON_BUILD_PAIR_VARIANT("a", a), + JSON_BUILD_PAIR("d", JSON_BUILD_STRING("-9223372036854775808")), + JSON_BUILD_PAIR("e", JSON_BUILD_EMPTY_OBJECT))) >= 0); + json_variant_dump(v, JSON_FORMAT_COLOR|JSON_FORMAT_PRETTY, NULL, NULL); + + assert_se(json_variant_format(v, JSON_FORMAT_REFUSE_SENSITIVE, &s) == -EPERM); + assert_se(!s); + v = json_variant_unref(v); + + assert_se(json_build(&v, JSON_BUILD_OBJECT( + JSON_BUILD_PAIR_VARIANT("b", b), + JSON_BUILD_PAIR("c", JSON_BUILD_INTEGER(INT64_MIN)), + JSON_BUILD_PAIR("d", JSON_BUILD_STRING("-9223372036854775808")), + JSON_BUILD_PAIR_VARIANT("a", a), + JSON_BUILD_PAIR("e", JSON_BUILD_EMPTY_OBJECT))) >= 0); + json_variant_dump(v, JSON_FORMAT_COLOR|JSON_FORMAT_PRETTY, NULL, NULL); + + assert_se(json_variant_format(v, JSON_FORMAT_REFUSE_SENSITIVE, &s) == -EPERM); + assert_se(!s); + v = json_variant_unref(v); + + assert_se(json_build(&v, JSON_BUILD_OBJECT( + JSON_BUILD_PAIR_VARIANT("b", b), + JSON_BUILD_PAIR("c", JSON_BUILD_INTEGER(INT64_MIN)), + JSON_BUILD_PAIR("d", JSON_BUILD_STRING("-9223372036854775808")), + JSON_BUILD_PAIR("e", JSON_BUILD_EMPTY_OBJECT), + JSON_BUILD_PAIR_VARIANT("a", a))) >= 0); + json_variant_dump(v, JSON_FORMAT_COLOR|JSON_FORMAT_PRETTY, NULL, NULL); + + assert_se(json_variant_format(v, JSON_FORMAT_REFUSE_SENSITIVE, &s) == -EPERM); + assert_se(!s); +} + DEFINE_TEST_MAIN(LOG_DEBUG); From 2e3414660cb0c6a024661638d0b237d88b5a7cbc Mon Sep 17 00:00:00 2001 From: Luca Boccassi Date: Fri, 22 Dec 2023 01:57:39 +0100 Subject: [PATCH 11/11] varlink: avoid logging content of message if it contains sensitive data This is important now that creds are sent via varlink systemd-creds[463]: varlink-3: Sending message: {"parameters":{"data":"Zm9vYmFyCg=="}} systemd-creds[462]: varlink-3: New incoming message: {"method":"io.systemd.Credentials.Encrypt","parameters":{"data":"Zm9vYmFyCg=="}} --- src/shared/varlink.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/shared/varlink.c b/src/shared/varlink.c index b6dc0d8590..2b40c7f3bd 100644 --- a/src/shared/varlink.c +++ b/src/shared/varlink.c @@ -961,10 +961,6 @@ static int varlink_parse_message(Varlink *v) { sz = e - begin + 1; - varlink_log(v, "New incoming message: %s", begin); /* FIXME: should we output the whole message here before validation? - * This may produce a non-printable journal entry if the message - * is invalid. We may also expose privileged information. */ - r = json_parse(begin, 0, &v->current, NULL, NULL); if (r < 0) { /* If we encounter a parse failure flush all data. We cannot possibly recover from this, @@ -1768,12 +1764,17 @@ Varlink* varlink_flush_close_unref(Varlink *v) { static int varlink_format_json(Varlink *v, JsonVariant *m) { _cleanup_(erase_and_freep) char *text = NULL; + bool sensitive = false; int r; assert(v); assert(m); - r = json_variant_format(m, 0, &text); + r = json_variant_format(m, JSON_FORMAT_REFUSE_SENSITIVE, &text); + if (r == -EPERM) { + sensitive = true; + r = json_variant_format(m, /* flags= */ 0, &text); + } if (r < 0) return r; assert(text[r] == '\0'); @@ -1781,7 +1782,7 @@ static int varlink_format_json(Varlink *v, JsonVariant *m) { if (v->output_buffer_size + r + 1 > VARLINK_BUFFER_MAX) return -ENOBUFS; - varlink_log(v, "Sending message: %s", text); + varlink_log(v, "Sending message: %s", sensitive ? "" : text); if (v->output_buffer_size == 0) { @@ -1812,7 +1813,7 @@ static int varlink_format_json(Varlink *v, JsonVariant *m) { v->output_buffer_index = 0; } - if (json_variant_is_sensitive(m)) + if (sensitive) v->output_buffer_sensitive = true; /* Propagate sensitive flag */ else text = mfree(text); /* No point in the erase_and_free() destructor declared above */