From 9891f1d371a1d373b0fdf8a44389a4e23b396399 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 20 Feb 2024 12:27:42 +0900 Subject: [PATCH 1/3] pam-util: add one more debugging log about sd-bus connection Continuation of b5b2510800fa50187f5c7342cb9471fdb97bc4b0. --- src/shared/pam-util.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/shared/pam-util.c b/src/shared/pam-util.c index fabe21f0cc..c93ab9afeb 100644 --- a/src/shared/pam-util.c +++ b/src/shared/pam-util.c @@ -179,6 +179,8 @@ int pam_acquire_bus_connection( if (r != PAM_SUCCESS) return pam_syslog_pam_error(handle, LOG_ERR, r, "Failed to set PAM bus data: @PAMERR@"); + pam_syslog(handle, LOG_DEBUG, "New sd-bus connection (%s) opened.", d->cache_id); + success: *ret_bus = sd_bus_ref(d->bus); From 0324e3d045ad97d831810262a0511af94d4f79a4 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 21 Feb 2024 12:45:27 +0900 Subject: [PATCH 2/3] pam_systemd_home: open sd-bus session when necessary No functional change, just refactoring. --- src/home/pam_systemd_home.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/home/pam_systemd_home.c b/src/home/pam_systemd_home.c index 21caa5a058..4904516724 100644 --- a/src/home/pam_systemd_home.c +++ b/src/home/pam_systemd_home.c @@ -105,7 +105,6 @@ static int acquire_user_record( _cleanup_(sd_bus_message_unrefp) sd_bus_message *reply = NULL; _cleanup_(json_variant_unrefp) JsonVariant *v = NULL; _cleanup_(user_record_unrefp) UserRecord *ur = NULL; - _cleanup_(sd_bus_unrefp) sd_bus *bus = NULL; _cleanup_free_ char *homed_field = NULL; const char *json = NULL; int r; @@ -148,6 +147,7 @@ static int acquire_user_record( } else { _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; _cleanup_free_ char *generic_field = NULL, *json_copy = NULL; + _cleanup_(sd_bus_unrefp) sd_bus *bus = NULL; r = pam_acquire_bus_connection(handle, "pam-systemd-home", &bus, bus_data); if (r != PAM_SUCCESS) @@ -501,7 +501,6 @@ static int acquire_home( _cleanup_(user_record_unrefp) UserRecord *ur = NULL, *secret = NULL; bool do_auth = FLAGS_SET(flags, ACQUIRE_MUST_AUTHENTICATE), home_not_active = false, home_locked = false, unrestricted = false; - _cleanup_(sd_bus_unrefp) sd_bus *bus = NULL; _cleanup_close_ int acquired_fd = -EBADF; _cleanup_free_ char *fd_field = NULL; const void *home_fd_ptr = NULL; @@ -552,10 +551,6 @@ 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, bus_data); - if (r != PAM_SUCCESS) - return r; - r = acquire_user_record(handle, username, debug, &ur, bus_data); if (r != PAM_SUCCESS) return r; @@ -567,6 +562,11 @@ static int acquire_home( * that by collecting and passing multiple passwords in that case. Hence we treat bad passwords as a * request to collect one more password and pass the new all all previously used passwords again. */ + _cleanup_(sd_bus_unrefp) sd_bus *bus = NULL; + r = pam_acquire_bus_connection(handle, "pam-systemd-home", &bus, bus_data); + if (r != PAM_SUCCESS) + return r; + for (;;) { _cleanup_(sd_bus_message_unrefp) sd_bus_message *m = NULL, *reply = NULL; _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; @@ -852,7 +852,6 @@ _public_ PAM_EXTERN int pam_sm_close_session( _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; _cleanup_(sd_bus_message_unrefp) sd_bus_message *m = NULL; - _cleanup_(sd_bus_unrefp) sd_bus *bus = NULL; const char *username = NULL; bool debug = false; int r; @@ -880,6 +879,7 @@ _public_ PAM_EXTERN int pam_sm_close_session( if (r != PAM_SUCCESS) return r; + _cleanup_(sd_bus_unrefp) sd_bus *bus = NULL; r = pam_acquire_bus_connection(handle, "pam-systemd-home", &bus, NULL); if (r != PAM_SUCCESS) return r; @@ -1029,7 +1029,6 @@ _public_ PAM_EXTERN int pam_sm_chauthtok( _cleanup_(user_record_unrefp) UserRecord *ur = NULL, *old_secret = NULL, *new_secret = NULL; const char *old_password = NULL, *new_password = NULL; - _cleanup_(sd_bus_unrefp) sd_bus *bus = NULL; unsigned n_attempts = 0; bool debug = false; int r; @@ -1042,10 +1041,6 @@ _public_ PAM_EXTERN int pam_sm_chauthtok( pam_debug_syslog(handle, debug, "pam-systemd-homed account management"); - r = pam_acquire_bus_connection(handle, "pam-systemd-home", &bus, NULL); - if (r != PAM_SUCCESS) - return r; - r = acquire_user_record(handle, NULL, debug, &ur, NULL); if (r != PAM_SUCCESS) return r; @@ -1102,6 +1097,11 @@ _public_ PAM_EXTERN int pam_sm_chauthtok( if (r < 0) return pam_syslog_errno(handle, LOG_ERR, r, "Failed to store new password: %m"); + _cleanup_(sd_bus_unrefp) sd_bus *bus = NULL; + r = pam_acquire_bus_connection(handle, "pam-systemd-home", &bus, NULL); + if (r != PAM_SUCCESS) + return r; + for (;;) { _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; _cleanup_(sd_bus_message_unrefp) sd_bus_message *m = NULL; From a6a6197e9d8363e133d1d8deb9e2aa84c28f5029 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 21 Feb 2024 13:21:31 +0900 Subject: [PATCH 3/3] pam_systemd_home: always close already opened bus connection in open_session() Fixes #31375. --- src/home/pam_systemd_home.c | 6 ++++++ src/shared/pam-util.c | 26 ++++++++++++++++++++++++++ src/shared/pam-util.h | 1 + 3 files changed, 33 insertions(+) diff --git a/src/home/pam_systemd_home.c b/src/home/pam_systemd_home.c index 4904516724..077f9208cb 100644 --- a/src/home/pam_systemd_home.c +++ b/src/home/pam_systemd_home.c @@ -826,6 +826,12 @@ _public_ PAM_EXTERN int pam_sm_open_session( if (r != PAM_SUCCESS) return r; + /* Explicitly get saved PamBusData here. Otherwise, this function may succeed without setting 'd' + * even if there is an opened sd-bus connection, and it will be leaked. See issue #31375. */ + r = pam_get_bus_data(handle, "pam-systemd-home", &d); + if (r != PAM_SUCCESS) + return r; + r = acquire_home(handle, flags, debug, &d); if (r == PAM_USER_UNKNOWN) /* Not managed by us? Don't complain. */ return PAM_SUCCESS; diff --git a/src/shared/pam-util.c b/src/shared/pam-util.c index c93ab9afeb..e3d92038aa 100644 --- a/src/shared/pam-util.c +++ b/src/shared/pam-util.c @@ -209,6 +209,32 @@ int pam_release_bus_connection(pam_handle_t *handle, const char *module_name) { return PAM_SUCCESS; } +int pam_get_bus_data( + pam_handle_t *handle, + const char *module_name, + PamBusData **ret) { + + PamBusData *d = NULL; + _cleanup_free_ char *cache_id = NULL; + int r; + + assert(handle); + assert(module_name); + assert(ret); + + 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**) &d); + 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@"); + + *ret = d; + return PAM_SUCCESS; +} + void pam_cleanup_free(pam_handle_t *handle, void *data, int error_status) { /* A generic destructor for pam_set_data() that just frees the specified data */ free(data); diff --git a/src/shared/pam-util.h b/src/shared/pam-util.h index 3439d4246e..51bffc329e 100644 --- a/src/shared/pam-util.h +++ b/src/shared/pam-util.h @@ -37,6 +37,7 @@ void pam_bus_data_disconnectp(PamBusData **d); * 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_bus, PamBusData **ret_bus_data); int pam_release_bus_connection(pam_handle_t *handle, const char *module_name); +int pam_get_bus_data(pam_handle_t *handle, const char *module_name, PamBusData **ret); void pam_cleanup_free(pam_handle_t *handle, void *data, int error_status);