From dbe7fff4765bad673d214c7e3db7bc088e163bea Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 7 May 2020 10:38:38 +0200 Subject: [PATCH 1/2] pam_systemd/pam_systemd_home: rework how we cache user records Since acquiring user records involves plenty of IPC we try to cache user records in the PAM context between our various hooks. Previously we'd just cache whatever we acquired, and use it from the on, forever until the context is destroyed. This is problematic however, since some programs (notably sudo) use the same PAM context for multiple different operations. Specifically, sudo first authenticates the originating user before creating a session for the destination user, all with the same PAM context. Thankfully, there was a safety check for this case in place that re-validated that the cached user record actually matched our current idea of the user to operate on, but this just meant the hook would fail entirely. Let's rework this: let's key the cache by the user name, so that we do not confused by the changing of the user name during the context's lifecycle and always, strictly use the cached user record of the user we operate on. Essentially this just means we now include the user name in the PAM data field. Secondly, this gets rid of the extra PAM data field that indicates whether a user record is from homed or something else. To simplify things we instead just cache the user record twice: once for consumption by pam_systemd_home (which only wants homed records) and once shared by pam_systemd and pam_systemd_home (and whoever else wants it). The cache entries simply have different field names. --- src/home/pam_systemd_home.c | 115 +++++++++++++++++++++--------------- src/login/pam_systemd.c | 67 +++++++++++---------- 2 files changed, 105 insertions(+), 77 deletions(-) diff --git a/src/home/pam_systemd_home.c b/src/home/pam_systemd_home.c index 7662fa69eb..7a1d64854a 100644 --- a/src/home/pam_systemd_home.c +++ b/src/home/pam_systemd_home.c @@ -18,11 +18,6 @@ #include "user-record.h" #include "user-util.h" -/* Used for the "systemd-user-record-is-homed" PAM data field, to indicate whether we know whether this user - * record is managed by homed or by something else. */ -#define USER_RECORD_IS_HOMED INT_TO_PTR(1) -#define USER_RECORD_IS_OTHER INT_TO_PTR(2) - static int parse_argv( pam_handle_t *handle, int argc, const char **argv, @@ -74,7 +69,7 @@ static int acquire_user_record( _cleanup_(user_record_unrefp) UserRecord *ur = NULL; _cleanup_(sd_bus_unrefp) sd_bus *bus = NULL; const char *username = NULL, *json = NULL; - const void *b = NULL; + _cleanup_free_ char *homed_field = NULL; int r; assert(handle); @@ -95,31 +90,30 @@ static int acquire_user_record( if (STR_IN_SET(username, "root", NOBODY_USER_NAME) || !valid_user_group_name(username, 0)) return PAM_USER_UNKNOWN; - /* Let's check if a previous run determined that this user is not managed by homed. If so, let's exit early */ - r = pam_get_data(handle, "systemd-user-record-is-homed", &b); - if (!IN_SET(r, PAM_SUCCESS, PAM_NO_MODULE_DATA)) { - /* Failure */ - pam_syslog(handle, LOG_ERR, "Failed to get PAM user-record-is-homed flag: %s", pam_strerror(handle, r)); - return r; - } else if (b == NULL) - /* Nothing cached yet, need to acquire fresh */ - json = NULL; - else if (b != USER_RECORD_IS_HOMED) - /* Definitely not a homed record */ - return PAM_USER_UNKNOWN; - else { - /* It's a homed record, let's use the cache, so that we can share it between the session and - * the authentication hooks */ - r = pam_get_data(handle, "systemd-user-record", (const void**) &json); - if (!IN_SET(r, PAM_SUCCESS, PAM_NO_MODULE_DATA)) { - pam_syslog(handle, LOG_ERR, "Failed to get PAM user record data: %s", pam_strerror(handle, r)); - return r; - } - } + /* We cache the user record in the PAM context. We use a field name that includes the username, since + * clients might change the user name associated with a PAM context underneath us. Notably, 'sudo' + * creates a single PAM context and first authenticates it with the user set to the originating user, + * then updates the user for the destination user and issues the session stack with the same PAM + * context. We thus must be prepared that the user record changes between calls and we keep any + * caching separate. */ + homed_field = strjoin("systemd-home-user-record-", username); + if (!homed_field) + return pam_log_oom(handle); - if (!json) { + /* Let's use the cache, so that we can share it between the session and the authentication hooks */ + r = pam_get_data(handle, homed_field, (const void**) &json); + if (!IN_SET(r, PAM_SUCCESS, PAM_NO_MODULE_DATA)) { + pam_syslog(handle, LOG_ERR, "Failed to get PAM user record data: %s", pam_strerror(handle, r)); + return r; + } + if (r == PAM_SUCCESS && json) { + /* We determined earlier that this is not a homed user? Then exit early. (We use -1 as + * negative cache indicator) */ + if (json == (void*) -1) + return PAM_USER_UNKNOWN; + } else { _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; - _cleanup_free_ char *json_copy = NULL; + _cleanup_free_ char *generic_field = NULL, *json_copy = NULL; r = pam_acquire_bus_connection(handle, &bus); if (r != PAM_SUCCESS) @@ -146,23 +140,38 @@ static int acquire_user_record( if (r < 0) return pam_bus_log_parse_error(handle, r); + /* First copy: for the homed-specific data field, i.e. where we know the user record is from + * homed */ json_copy = strdup(json); if (!json_copy) return pam_log_oom(handle); - r = pam_set_data(handle, "systemd-user-record", json_copy, pam_cleanup_free); + r = pam_set_data(handle, homed_field, json_copy, pam_cleanup_free); if (r != PAM_SUCCESS) { - pam_syslog(handle, LOG_ERR, "Failed to set PAM user record data: %s", pam_strerror(handle, r)); + pam_syslog(handle, LOG_ERR, "Failed to set PAM user record data '%s': %s", + homed_field, pam_strerror(handle, r)); + return r; + } + + /* Take a second copy: for the generic data field, the one which we share with + * pam_systemd. While we insist on only reusing homed records, pam_systemd is fine with homed + * and non-homed user records. */ + json_copy = strdup(json); + if (!json_copy) + return pam_log_oom(handle); + + generic_field = strjoin("systemd-user-record-", username); + if (!generic_field) + return pam_log_oom(handle); + + r = pam_set_data(handle, generic_field, json_copy, pam_cleanup_free); + if (r != PAM_SUCCESS) { + pam_syslog(handle, LOG_ERR, "Failed to set PAM user record data '%s': %s", + homed_field, pam_strerror(handle, r)); return r; } TAKE_PTR(json_copy); - - r = pam_set_data(handle, "systemd-user-record-is-homed", USER_RECORD_IS_HOMED, NULL); - if (r != PAM_SUCCESS) { - pam_syslog(handle, LOG_ERR, "Failed to set PAM user record is homed flag: %s", pam_strerror(handle, r)); - return r; - } } r = json_parse(json, JSON_PARSE_SENSITIVE, &v, NULL, NULL); @@ -181,6 +190,7 @@ static int acquire_user_record( return PAM_SERVICE_ERR; } + /* Safety check if cached record actually matches what we are looking for */ if (!streq_ptr(username, ur->user_name)) { pam_syslog(handle, LOG_ERR, "Acquired user record does not match user name."); return PAM_SERVICE_ERR; @@ -193,23 +203,36 @@ static int acquire_user_record( user_unknown: /* Cache this, so that we don't check again */ - r = pam_set_data(handle, "systemd-user-record-is-homed", USER_RECORD_IS_OTHER, NULL); + r = pam_set_data(handle, homed_field, (void*) -1, NULL); if (r != PAM_SUCCESS) - pam_syslog(handle, LOG_ERR, "Failed to set PAM user-record-is-homed flag, ignoring: %s", pam_strerror(handle, r)); + pam_syslog(handle, LOG_ERR, "Failed to set PAM user record data '%s' to invalid, ignoring: %s", + homed_field, pam_strerror(handle, r)); return PAM_USER_UNKNOWN; } -static int release_user_record(pam_handle_t *handle) { +static int release_user_record(pam_handle_t *handle, const char *username) { + _cleanup_free_ char *homed_field = NULL, *generic_field = NULL; int r, k; - r = pam_set_data(handle, "systemd-user-record", NULL, NULL); - if (r != PAM_SUCCESS) - pam_syslog(handle, LOG_ERR, "Failed to release PAM user record data: %s", pam_strerror(handle, r)); + assert(handle); + assert(username); - k = pam_set_data(handle, "systemd-user-record-is-homed", NULL, NULL); + homed_field = strjoin("systemd-home-user-record-", username); + if (!homed_field) + return pam_log_oom(handle); + + r = pam_set_data(handle, homed_field, NULL, NULL); + if (r != PAM_SUCCESS) + pam_syslog(handle, LOG_ERR, "Failed to release PAM user record data '%s': %s", homed_field, pam_strerror(handle, r)); + + generic_field = strjoin("systemd-user-record-", username); + if (!generic_field) + return pam_log_oom(handle); + + k = pam_set_data(handle, generic_field, NULL, NULL); if (k != PAM_SUCCESS) - pam_syslog(handle, LOG_ERR, "Failed to release PAM user-record-is-homed flag: %s", pam_strerror(handle, k)); + pam_syslog(handle, LOG_ERR, "Failed to release PAM user record data '%s': %s", generic_field, pam_strerror(handle, k)); return IN_SET(r, PAM_SUCCESS, PAM_NO_MODULE_DATA) ? k : r; } @@ -545,7 +568,7 @@ static int acquire_home( /* We likely just activated the home directory, let's flush out the user record, since a * newer embedded user record might have been acquired from the activation. */ - r = release_user_record(handle); + r = release_user_record(handle, ur->user_name); if (!IN_SET(r, PAM_SUCCESS, PAM_NO_MODULE_DATA)) return r; } diff --git a/src/login/pam_systemd.c b/src/login/pam_systemd.c index b6f5562707..96ce4742f0 100644 --- a/src/login/pam_systemd.c +++ b/src/login/pam_systemd.c @@ -99,6 +99,7 @@ static int acquire_user_record( _cleanup_(user_record_unrefp) UserRecord *ur = NULL; const char *username = NULL, *json = NULL; + _cleanup_free_ char *field = NULL; int r; assert(handle); @@ -114,39 +115,18 @@ static int acquire_user_record( return PAM_SERVICE_ERR; } - /* If pam_systemd_homed (or some other module) already acqired the user record we can reuse it + /* If pam_systemd_homed (or some other module) already acquired the user record we can reuse it * here. */ - r = pam_get_data(handle, "systemd-user-record", (const void**) &json); - if (r != PAM_SUCCESS || !json) { - _cleanup_free_ char *formatted = NULL; + field = strjoin("systemd-user-record-", username); + if (!field) + return pam_log_oom(handle); - if (!IN_SET(r, PAM_SUCCESS, PAM_NO_MODULE_DATA)) { - pam_syslog(handle, LOG_ERR, "Failed to get PAM user record data: %s", pam_strerror(handle, r)); - return r; - } - - /* Request the record ourselves */ - r = userdb_by_name(username, 0, &ur); - if (r < 0) { - pam_syslog(handle, LOG_ERR, "Failed to get user record: %s", strerror_safe(r)); - return PAM_USER_UNKNOWN; - } - - r = json_variant_format(ur->json, 0, &formatted); - if (r < 0) { - pam_syslog(handle, LOG_ERR, "Failed to format user JSON: %s", strerror_safe(r)); - return PAM_SERVICE_ERR; - } - - /* And cache it for everyone else */ - r = pam_set_data(handle, "systemd-user-record", formatted, pam_cleanup_free); - if (r < 0) { - pam_syslog(handle, LOG_ERR, "Failed to set PAM user record data: %s", pam_strerror(handle, r)); - return r; - } - - TAKE_PTR(formatted); - } else { + r = pam_get_data(handle, field, (const void**) &json); + if (!IN_SET(r, PAM_SUCCESS, PAM_NO_MODULE_DATA)) { + pam_syslog(handle, LOG_ERR, "Failed to get PAM user record data: %s", pam_strerror(handle, r)); + return r; + } + if (r == PAM_SUCCESS && json) { _cleanup_(json_variant_unrefp) JsonVariant *v = NULL; /* Parse cached record */ @@ -171,6 +151,31 @@ static int acquire_user_record( pam_syslog(handle, LOG_ERR, "Acquired user record does not match user name."); return PAM_SERVICE_ERR; } + } else { + _cleanup_free_ char *formatted = NULL; + + /* Request the record ourselves */ + r = userdb_by_name(username, 0, &ur); + if (r < 0) { + pam_syslog(handle, LOG_ERR, "Failed to get user record: %s", strerror_safe(r)); + return PAM_USER_UNKNOWN; + } + + r = json_variant_format(ur->json, 0, &formatted); + if (r < 0) { + pam_syslog(handle, LOG_ERR, "Failed to format user JSON: %s", strerror_safe(r)); + return PAM_SERVICE_ERR; + } + + /* And cache it for everyone else */ + r = pam_set_data(handle, field, formatted, pam_cleanup_free); + if (r < 0) { + pam_syslog(handle, LOG_ERR, "Failed to set PAM user record data '%s': %s", + field, pam_strerror(handle, r)); + return r; + } + + TAKE_PTR(formatted); } if (!uid_is_valid(ur->uid)) { From 6c8428bb8bb7cbe18efa839093c842588cae5396 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 7 May 2020 11:01:42 +0200 Subject: [PATCH 2/2] pam_systemd_home: also store acquirement fd per user We might pin a home through authentication and a different one through a session, all from the same PAM context, like sudo does. Hence also store the referencing fd keyed by the user name. --- src/home/pam_systemd_home.c | 95 ++++++++++++++++++++++++++----------- 1 file changed, 68 insertions(+), 27 deletions(-) diff --git a/src/home/pam_systemd_home.c b/src/home/pam_systemd_home.c index 7a1d64854a..72ce062f54 100644 --- a/src/home/pam_systemd_home.c +++ b/src/home/pam_systemd_home.c @@ -62,27 +62,30 @@ static int parse_argv( static int acquire_user_record( pam_handle_t *handle, + const char *username, UserRecord **ret_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; - const char *username = NULL, *json = NULL; _cleanup_free_ char *homed_field = NULL; + const char *json = NULL; int r; assert(handle); - r = pam_get_user(handle, &username, NULL); - if (r != PAM_SUCCESS) { - pam_syslog(handle, LOG_ERR, "Failed to get user name: %s", pam_strerror(handle, r)); - return r; - } + if (!username) { + r = pam_get_user(handle, &username, NULL); + if (r != PAM_SUCCESS) { + pam_syslog(handle, LOG_ERR, "Failed to get user name: %s", pam_strerror(handle, r)); + return r; + } - if (isempty(username)) { - pam_syslog(handle, LOG_ERR, "User name not set."); - return PAM_SERVICE_ERR; + if (isempty(username)) { + pam_syslog(handle, LOG_ERR, "User name not set."); + return PAM_SERVICE_ERR; + } } /* Let's bypass all IPC complexity for the two user names we know for sure we don't manage, and for @@ -418,7 +421,9 @@ static int acquire_home( bool do_auth = please_authenticate, home_not_active = false, home_locked = false; _cleanup_(sd_bus_unrefp) sd_bus *bus = NULL; _cleanup_close_ int acquired_fd = -1; + _cleanup_free_ char *fd_field = NULL; const void *home_fd_ptr = NULL; + const char *username = NULL; unsigned n_attempts = 0; int r; @@ -432,8 +437,27 @@ static int acquire_home( * authenticates, while the other PAM hooks unset it so that they can a ref of their own without * authentication if possible, but with authentication if necessary. */ + r = pam_get_user(handle, &username, NULL); + if (r != PAM_SUCCESS) { + pam_syslog(handle, LOG_ERR, "Failed to get user name: %s", pam_strerror(handle, r)); + return r; + } + + if (isempty(username)) { + pam_syslog(handle, LOG_ERR, "User name not set."); + return PAM_SERVICE_ERR; + } + /* If we already have acquired the fd, let's shortcut this */ - r = pam_get_data(handle, "systemd-home-fd", &home_fd_ptr); + fd_field = strjoin("systemd-home-fd-", username); + if (!fd_field) + return pam_log_oom(handle); + + r = pam_get_data(handle, fd_field, &home_fd_ptr); + if (!IN_SET(r, PAM_SUCCESS, PAM_NO_MODULE_DATA)) { + pam_syslog(handle, LOG_ERR, "Failed to retrieve PAM home reference fd: %s", pam_strerror(handle, r)); + return r; + } if (r == PAM_SUCCESS && PTR_TO_FD(home_fd_ptr) >= 0) return PAM_SUCCESS; @@ -441,7 +465,7 @@ static int acquire_home( if (r != PAM_SUCCESS) return r; - r = acquire_user_record(handle, &ur); + r = acquire_user_record(handle, username, &ur); if (r != PAM_SUCCESS) return r; @@ -557,7 +581,7 @@ static int acquire_home( do_auth = true; } - r = pam_set_data(handle, "systemd-home-fd", FD_TO_PTR(acquired_fd), cleanup_home_fd); + r = pam_set_data(handle, fd_field, FD_TO_PTR(acquired_fd), cleanup_home_fd); if (r < 0) { pam_syslog(handle, LOG_ERR, "Failed to set PAM bus data: %s", pam_strerror(handle, r)); return r; @@ -578,15 +602,27 @@ static int acquire_home( return PAM_SUCCESS; } -static int release_home_fd(pam_handle_t *handle) { +static int release_home_fd(pam_handle_t *handle, const char *username) { + _cleanup_free_ char *fd_field = NULL; const void *home_fd_ptr = NULL; int r; - r = pam_get_data(handle, "systemd-home-fd", &home_fd_ptr); - if (r == PAM_NO_MODULE_DATA || PTR_TO_FD(home_fd_ptr) < 0) - return PAM_NO_MODULE_DATA; + assert(handle); + assert(username); - r = pam_set_data(handle, "systemd-home-fd", NULL, NULL); + fd_field = strjoin("systemd-home-fd-", username); + if (!fd_field) + return pam_log_oom(handle); + + r = pam_get_data(handle, fd_field, &home_fd_ptr); + if (r == PAM_NO_MODULE_DATA || (r == PAM_SUCCESS && PTR_TO_FD(home_fd_ptr) < 0)) + return PAM_NO_MODULE_DATA; + if (r != PAM_SUCCESS) { + pam_syslog(handle, LOG_ERR, "Failed to retrieve PAM home reference fd: %s", pam_strerror(handle, r)); + return r; + } + + r = pam_set_data(handle, fd_field, NULL, NULL); if (r != PAM_SUCCESS) pam_syslog(handle, LOG_ERR, "Failed to release PAM home reference fd: %s", pam_strerror(handle, r)); @@ -673,20 +709,25 @@ _public_ PAM_EXTERN int pam_sm_close_session( if (debug) pam_syslog(handle, LOG_DEBUG, "pam-systemd-homed session end"); - /* Let's explicitly drop the reference to the homed session, so that the subsequent ReleaseHome() - * call will be able to do its thing. */ - r = release_home_fd(handle); - if (r == PAM_NO_MODULE_DATA) /* Nothing to do, we never acquired an fd */ - return PAM_SUCCESS; - if (r != PAM_SUCCESS) - return r; - r = pam_get_user(handle, &username, NULL); if (r != PAM_SUCCESS) { pam_syslog(handle, LOG_ERR, "Failed to get user name: %s", pam_strerror(handle, r)); return r; } + if (isempty(username)) { + pam_syslog(handle, LOG_ERR, "User name not set."); + return PAM_SERVICE_ERR; + } + + /* Let's explicitly drop the reference to the homed session, so that the subsequent ReleaseHome() + * call will be able to do its thing. */ + r = release_home_fd(handle, username); + if (r == PAM_NO_MODULE_DATA) /* Nothing to do, we never acquired an fd */ + return PAM_SUCCESS; + if (r != PAM_SUCCESS) + return r; + r = pam_acquire_bus_connection(handle, &bus); if (r != PAM_SUCCESS) return r; @@ -738,7 +779,7 @@ _public_ PAM_EXTERN int pam_sm_acct_mgmt( if (r != PAM_SUCCESS) return r; - r = acquire_user_record(handle, &ur); + r = acquire_user_record(handle, NULL, &ur); if (r != PAM_SUCCESS) return r; @@ -846,7 +887,7 @@ _public_ PAM_EXTERN int pam_sm_chauthtok( if (r != PAM_SUCCESS) return r; - r = acquire_user_record(handle, &ur); + r = acquire_user_record(handle, NULL, &ur); if (r != PAM_SUCCESS) return r;