Merge pull request #27346 from poettering/pam-fixes

pam-systemd: be more careful with sharing bus connections

src/home/pam_systemd_home.c

@@ -91,7 +91,8 @@ static int parse_env(
 static int acquire_user_record(
                 pam_handle_t *handle,
                 const char *username,
-                UserRecord **ret_record) {
+                UserRecord **ret_record,
+                PamBusData **bus_data) {
 
         _cleanup_(sd_bus_message_unrefp) sd_bus_message *reply = NULL;
         _cleanup_(json_variant_unrefp) JsonVariant *v = NULL;
@@ -140,7 +141,7 @@ static int acquire_user_record(
                 _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL;
                 _cleanup_free_ char *generic_field = NULL, *json_copy = NULL;
 
-                r = pam_acquire_bus_connection(handle, "pam-systemd-home", &bus);
+                r = pam_acquire_bus_connection(handle, "pam-systemd-home", &bus, bus_data);
                 if (r != PAM_SUCCESS)
                         return r;
 
@@ -472,7 +473,8 @@ static int acquire_home(
                 pam_handle_t *handle,
                 bool please_authenticate,
                 bool please_suspend,
-                bool debug) {
+                bool debug,
+                PamBusData **bus_data) {
 
         _cleanup_(user_record_unrefp) UserRecord *ur = NULL, *secret = NULL;
         bool do_auth = please_authenticate, home_not_active = false, home_locked = false;
@@ -513,11 +515,11 @@ static int acquire_home(
         if (r == PAM_SUCCESS && PTR_TO_FD(home_fd_ptr) >= 0)
                 return PAM_SUCCESS;
 
-        r = pam_acquire_bus_connection(handle, "pam-systemd-home", &bus);
+        r = pam_acquire_bus_connection(handle, "pam-systemd-home", &bus, bus_data);
         if (r != PAM_SUCCESS)
                 return r;
 
-        r = acquire_user_record(handle, username, &ur);
+        r = acquire_user_record(handle, username, &ur, bus_data);
         if (r != PAM_SUCCESS)
                 return r;
 
@@ -698,7 +700,7 @@ _public_ PAM_EXTERN int pam_sm_authenticate(
         if (debug)
                 pam_syslog(handle, LOG_DEBUG, "pam-systemd-homed authenticating");
 
-        return acquire_home(handle, /* please_authenticate= */ true, suspend_please, debug);
+        return acquire_home(handle, /* please_authenticate= */ true, suspend_please, debug, NULL);
 }
 
 _public_ PAM_EXTERN int pam_sm_setcred(pam_handle_t *pamh, int flags, int argc, const char **argv) {
@@ -710,6 +712,10 @@ _public_ PAM_EXTERN int pam_sm_open_session(
                 int flags,
                 int argc, const char **argv) {
 
+        /* Let's release the D-Bus connection once this function exits, after all the session might live
+         * quite a long time, and we are not going to process the bus connection in that time, so let's
+         * better close before the daemon kicks us off because we are not processing anything. */
+        _cleanup_(pam_bus_data_disconnectp) PamBusData *d = NULL;
         bool debug = false, suspend_please = false;
         int r;
 
@@ -725,9 +731,9 @@ _public_ PAM_EXTERN int pam_sm_open_session(
         if (debug)
                 pam_syslog(handle, LOG_DEBUG, "pam-systemd-homed session start");
 
-        r = acquire_home(handle, /* please_authenticate = */ false, suspend_please, debug);
+        r = acquire_home(handle, /* please_authenticate = */ false, suspend_please, debug, &d);
         if (r == PAM_USER_UNKNOWN) /* Not managed by us? Don't complain. */
-                goto success; /* Need to free the bus resource, as acquire_home() takes a reference. */
+                return PAM_SUCCESS;
         if (r != PAM_SUCCESS)
                 return r;
 
@@ -741,11 +747,6 @@ _public_ PAM_EXTERN int pam_sm_open_session(
                 return pam_syslog_pam_error(handle, LOG_ERR, r,
                                             "Failed to set PAM environment variable $SYSTEMD_HOME_SUSPEND: @PAMERR@");
 
-success:
-        /* Let's release the D-Bus connection, after all the session might live quite a long time, and we are
-         * not going to process the bus connection in that time, so let's better close before the daemon
-         * kicks us off because we are not processing anything. */
-        (void) pam_release_bus_connection(handle, "pam-systemd-home");
         return PAM_SUCCESS;
 }
 
@@ -785,7 +786,7 @@ _public_ PAM_EXTERN int pam_sm_close_session(
         if (r != PAM_SUCCESS)
                 return r;
 
-        r = pam_acquire_bus_connection(handle, "pam-systemd-home", &bus);
+        r = pam_acquire_bus_connection(handle, "pam-systemd-home", &bus, NULL);
         if (r != PAM_SUCCESS)
                 return r;
 
@@ -832,11 +833,11 @@ _public_ PAM_EXTERN int pam_sm_acct_mgmt(
         if (debug)
                 pam_syslog(handle, LOG_DEBUG, "pam-systemd-homed account management");
 
-        r = acquire_home(handle, /* please_authenticate = */ false, please_suspend, debug);
+        r = acquire_home(handle, /* please_authenticate = */ false, please_suspend, debug, NULL);
         if (r != PAM_SUCCESS)
                 return r;
 
-        r = acquire_user_record(handle, NULL, &ur);
+        r = acquire_user_record(handle, NULL, &ur, NULL);
         if (r != PAM_SUCCESS)
                 return r;
 
@@ -944,11 +945,11 @@ _public_ PAM_EXTERN int pam_sm_chauthtok(
         if (debug)
                 pam_syslog(handle, LOG_DEBUG, "pam-systemd-homed account management");
 
-        r = pam_acquire_bus_connection(handle, "pam-systemd-home", &bus);
+        r = pam_acquire_bus_connection(handle, "pam-systemd-home", &bus, NULL);
         if (r != PAM_SUCCESS)
                 return r;
 
-        r = acquire_user_record(handle, NULL, &ur);
+        r = acquire_user_record(handle, NULL, &ur, NULL);
         if (r != PAM_SUCCESS)
                 return r;
 

src/login/pam_systemd.c

@@ -788,6 +788,10 @@ _public_ PAM_EXTERN int pam_sm_open_session(
                 int flags,
                 int argc, const char **argv) {
 
+        /* Let's release the D-Bus connection once this function exits, after all the session might live
+         * quite a long time, and we are not going to process the bus connection in that time, so let's
+         * better close before the daemon kicks us off because we are not processing anything. */
+        _cleanup_(pam_bus_data_disconnectp) PamBusData *d = NULL;
         _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL;
         _cleanup_(sd_bus_message_unrefp) sd_bus_message *m = NULL, *reply = NULL;
         const char
@@ -947,8 +951,7 @@ _public_ PAM_EXTERN int pam_sm_open_session(
                 return pam_syslog_pam_error(handle, LOG_ERR, r, "Failed to get PAM systemd.runtime_max_sec data: @PAMERR@");
 
         /* Talk to logind over the message bus */
-
-        r = pam_acquire_bus_connection(handle, "pam-systemd", &bus);
+        r = pam_acquire_bus_connection(handle, "pam-systemd", &bus, &d);
         if (r != PAM_SUCCESS)
                 return r;
 
@@ -1108,15 +1111,7 @@ success:
         if (default_capability_ambient_set == UINT64_MAX)
                 default_capability_ambient_set = pick_default_capability_ambient_set(ur, service, seat);
 
-        r = apply_user_record_settings(handle, ur, debug, default_capability_bounding_set, default_capability_ambient_set);
-        if (r != PAM_SUCCESS)
-                return r;
-
-        /* Let's release the D-Bus connection, after all the session might live quite a long time, and we are
-         * not going to use the bus connection in that time, so let's better close before the daemon kicks us
-         * off because we are not processing anything. */
-        (void) pam_release_bus_connection(handle, "pam-systemd");
-        return PAM_SUCCESS;
+        return apply_user_record_settings(handle, ur, debug, default_capability_bounding_set, default_capability_ambient_set);
 }
 
 _public_ PAM_EXTERN int pam_sm_close_session(
@@ -1159,7 +1154,7 @@ _public_ PAM_EXTERN int pam_sm_close_session(
                 /* Before we go and close the FIFO we need to tell logind that this is a clean session
                  * shutdown, so that it doesn't just go and slaughter us immediately after closing the fd */
 
-                r = pam_acquire_bus_connection(handle, "pam-systemd", &bus);
+                r = pam_acquire_bus_connection(handle, "pam-systemd", &bus, NULL);
                 if (r != PAM_SUCCESS)
                         return r;
 

src/shared/pam-util.c

@@ -6,8 +6,10 @@
 
 #include "alloc-util.h"
 #include "errno-util.h"
+#include "format-util.h"
 #include "macro.h"
 #include "pam-util.h"
+#include "process-util.h"
 #include "stdio-util.h"
 #include "string-util.h"
 
@@ -49,53 +51,131 @@ int pam_syslog_pam_error(pam_handle_t *handle, int level, int error, const char
         return error;
 }
 
-static void cleanup_system_bus(pam_handle_t *handle, void *data, int error_status) {
-        /* The PAM_DATA_SILENT flag is the way that pam_end() communicates to the module stack that this
-         * invocation of pam_end() is not the final one, but in the process that is going to directly exec
-         * the child. This means we are being called after a fork(), and we do not want to try and clean
-         * up the sd-bus object, as it would affect the parent too and we'll hit an assertion. */
-        if (error_status & PAM_DATA_SILENT)
-                return (void) pam_syslog_pam_error(
-                                handle,
-                                LOG_ERR,
-                                SYNTHETIC_ERRNO(EUCLEAN),
-                                "Attempted to close sd-bus after fork, this should not happen.");
+/* A small structure we store inside the PAM session object, that allows us to resue bus connections but pins
+ * it to the process thoroughly. */
+struct PamBusData {
+        sd_bus *bus;
+        pam_handle_t *pam_handle;
+        char *cache_id;
+};
 
-        sd_bus_flush_close_unref(data);
+static PamBusData *pam_bus_data_free(PamBusData *d) {
+        /* The actual destructor */
+        if (!d)
+                return NULL;
+
+        /* NB: PAM sessions usually involve forking off a child process, and thus the PAM context might be
+         * duplicated in the child. This destructor might be called twice: both in the parent and in the
+         * child. sd_bus_flush_close_unref() however is smart enough to be a NOP when invoked in any other
+         * process than the one it was invoked from, hence we don't need to add any extra protection here to
+         * ensure that destruction of the bus connection in the child affects the parent's connection
+         * somehow. */
+        sd_bus_flush_close_unref(d->bus);
+        free(d->cache_id);
+
+        /* Note: we don't destroy pam_handle here, because this object is pinned by the handle, and not vice versa! */
+
+        return mfree(d);
 }
 
-int pam_acquire_bus_connection(pam_handle_t *handle, const char *module_name, sd_bus **ret) {
-        _cleanup_(sd_bus_unrefp) sd_bus *bus = NULL;
+DEFINE_TRIVIAL_CLEANUP_FUNC(PamBusData*, pam_bus_data_free);
+
+static void pam_bus_data_destroy(pam_handle_t *handle, void *data, int error_status) {
+        /* Destructor when called from PAM. Note that error_status is supposed to tell us via PAM_DATA_SILENT
+         * whether we are called in a forked off child of the PAM session or in the original parent. We don't
+         * bother with that however, and instead rely on the PID checks that sd_bus_flush_close_unref() does
+         * internally anyway. That said, we still generate a warning message, since this really shouldn't
+         * happen. */
+
+        if (error_status & PAM_DATA_SILENT)
+                pam_syslog(handle, LOG_WARNING, "Attempted to close sd-bus after fork, this should not happen.");
+
+        pam_bus_data_free(data);
+}
+
+static char* pam_make_bus_cache_id(const char *module_name) {
+        char *id;
+
+        /* We want to cache bus connections between hooks. But we don't want to allow them to be reused in
+         * child processes (because sd-bus doesn't support that). We also don't want them to be reused
+         * between our own PAM modules, because they might be linked against different versions of our
+         * utility functions and share different state. Hence include both a module ID and a PID in the data
+         * field ID. */
+
+        if (asprintf(&id, "system-bus-%s-" PID_FMT, ASSERT_PTR(module_name), getpid_cached()) < 0)
+                return NULL;
+
+        return id;
+}
+
+void pam_bus_data_disconnectp(PamBusData **_d) {
+        PamBusData *d = *ASSERT_PTR(_d);
+        pam_handle_t *handle;
+        int r;
+
+        /* Disconnects the connection explicitly (for use via _cleanup_()) when called */
+
+        if (!d)
+                return;
+
+        handle = ASSERT_PTR(d->pam_handle); /* Keep a reference to the session even after 'd' might be invalidated */
+
+        r = pam_set_data(handle, ASSERT_PTR(d->cache_id), NULL, NULL);
+        if (r != PAM_SUCCESS)
+                pam_syslog_pam_error(handle, LOG_ERR, r, "Failed to release PAM user record data, ignoring: @PAMERR@");
+
+        /* Note, the pam_set_data() call will invalidate 'd', don't access here anymore */
+}
+
+int pam_acquire_bus_connection(
+                pam_handle_t *handle,
+                const char *module_name,
+                sd_bus **ret_bus,
+                PamBusData **ret_pam_bus_data) {
+
+        _cleanup_(pam_bus_data_freep) PamBusData *d = NULL;
         _cleanup_free_ char *cache_id = NULL;
         int r;
 
         assert(handle);
         assert(module_name);
-        assert(ret);
+        assert(ret_bus);
 
-        cache_id = strjoin("system-bus-", module_name);
+        cache_id = pam_make_bus_cache_id(module_name);
         if (!cache_id)
                 return pam_log_oom(handle);
 
         /* We cache the bus connection so that we can share it between the session and the authentication hooks */
-        r = pam_get_data(handle, cache_id, (const void**) &bus);
-        if (r == PAM_SUCCESS && bus) {
-                *ret = sd_bus_ref(TAKE_PTR(bus)); /* Increase the reference counter, so that the PAM data stays valid */
-                return PAM_SUCCESS;
-        }
+        r = pam_get_data(handle, cache_id, (const void**) &d);
+        if (r == PAM_SUCCESS && d)
+                goto success;
         if (!IN_SET(r, PAM_SUCCESS, PAM_NO_MODULE_DATA))
                 return pam_syslog_pam_error(handle, LOG_ERR, r, "Failed to get bus connection: @PAMERR@");
 
-        r = sd_bus_open_system(&bus);
+        d = new(PamBusData, 1);
+        if (!d)
+                return pam_log_oom(handle);
+
+        *d = (PamBusData) {
+                .cache_id = TAKE_PTR(cache_id),
+                .pam_handle = handle,
+        };
+
+        r = sd_bus_open_system(&d->bus);
         if (r < 0)
                 return pam_syslog_errno(handle, LOG_ERR, r, "Failed to connect to system bus: %m");
 
-        r = pam_set_data(handle, cache_id, bus, cleanup_system_bus);
+        r = pam_set_data(handle, d->cache_id, d, pam_bus_data_destroy);
         if (r != PAM_SUCCESS)
                 return pam_syslog_pam_error(handle, LOG_ERR, r, "Failed to set PAM bus data: @PAMERR@");
 
-        sd_bus_ref(bus);
-        *ret = TAKE_PTR(bus);
+success:
+        *ret_bus = sd_bus_ref(d->bus);
+
+        if (ret_pam_bus_data)
+                *ret_pam_bus_data = d;
+
+        TAKE_PTR(d); /* don't auto-destroy anymore, it's installed now */
 
         return PAM_SUCCESS;
 }
@@ -106,7 +186,7 @@ int pam_release_bus_connection(pam_handle_t *handle, const char *module_name) {
 
         assert(module_name);
 
-        cache_id = strjoin("system-bus-", module_name);
+        cache_id = pam_make_bus_cache_id(module_name);
         if (!cache_id)
                 return pam_log_oom(handle);
 

src/shared/pam-util.h

@@ -24,9 +24,12 @@ static inline int pam_bus_log_parse_error(pam_handle_t *handle, int r) {
         return pam_syslog_errno(handle, LOG_ERR, r, "Failed to parse bus message: %m");
 }
 
+typedef struct PamBusData PamBusData;
+void pam_bus_data_disconnectp(PamBusData **d);
+
 /* Use a different module name per different PAM module. They are all loaded in the same namespace, and this
  * helps avoid a clash in the internal data structures of sd-bus. It will be used as key for cache items. */
-int pam_acquire_bus_connection(pam_handle_t *handle, const char *module_name, sd_bus **ret);
+int pam_acquire_bus_connection(pam_handle_t *handle, const char *module_name, sd_bus **ret_bus, PamBusData **ret_bus_data);
 int pam_release_bus_connection(pam_handle_t *handle, const char *module_name);
 
 void pam_cleanup_free(pam_handle_t *handle, void *data, int error_status);