diff --git a/src/home/pam_systemd_home.c b/src/home/pam_systemd_home.c index 6a3e656035..db5b1b8d4a 100644 --- a/src/home/pam_systemd_home.c +++ b/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; diff --git a/src/login/pam_systemd.c b/src/login/pam_systemd.c index 3a009d8e37..021b380507 100644 --- a/src/login/pam_systemd.c +++ b/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; diff --git a/src/shared/pam-util.c b/src/shared/pam-util.c index 4d864af717..82e897ee53 100644 --- a/src/shared/pam-util.c +++ b/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); diff --git a/src/shared/pam-util.h b/src/shared/pam-util.h index d9c906aaad..5afabf257b 100644 --- a/src/shared/pam-util.h +++ b/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);