From 7fd2a673887202ae87d9f9fc4455d3292fdb8df8 Mon Sep 17 00:00:00 2001 From: Giovanni Campagna Date: Thu, 27 Dec 2012 00:25:33 +0100 Subject: [PATCH 1/2] pam-helper: use pipes instead of command line arguments to set the password Command line arguments are by default accessible to all users through /proc, which leaves a window for an attacker to retrieve a password while it is changed. Using a pipe avoids this problem. https://bugs.freedesktop.org/show_bug.cgi?id=51833 --- configure.ac | 1 + src/Makefile.am | 1 + src/pam-password-helper.c | 67 ++++++++++++++++++++++++++---- src/user.c | 43 +++++++++++++++---- src/util.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++ src/util.h | 5 +++ 6 files changed, 204 insertions(+), 16 deletions(-) diff --git a/configure.ac b/configure.ac index 5cf120a..59a035d 100644 --- a/configure.ac +++ b/configure.ac @@ -25,6 +25,7 @@ AC_SUBST(LT_CURRENT) AC_SUBST(LT_REVISION) AC_SUBST(LT_AGE) +PKG_CHECK_MODULES(GLIB, glib-2.0) PKG_CHECK_MODULES(GIO, gio-unix-2.0) PKG_CHECK_MODULES(POLKIT, gio-unix-2.0 polkit-gobject-1) diff --git a/src/Makefile.am b/src/Makefile.am index c510e13..3ba60ab 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -56,6 +56,7 @@ accounts_daemon_LDADD = \ accounts_daemon_pam_password_helper_SOURCES = \ pam-password-helper.c accounts_daemon_pam_password_helper_LDADD = \ + $(GLIB_LIBS) \ -lpam CLEANFILES = $(BUILT_SOURCES) diff --git a/src/pam-password-helper.c b/src/pam-password-helper.c index d569734..a544262 100644 --- a/src/pam-password-helper.c +++ b/src/pam-password-helper.c @@ -27,6 +27,7 @@ #include #include #include +#include const char *password; const char *pin; @@ -40,7 +41,10 @@ answer_pam_message (int num_msg, int i; struct pam_response *resps; + /* Must use malloc here, not g_malloc (and same below, must + use strdup, not g_strdup) */ resps = malloc((sizeof (struct pam_response)) * num_msg); + /* We run non-interactively, so just copy the password into each question, and hope that configuration is correct. */ @@ -57,6 +61,50 @@ answer_pam_message (int num_msg, return PAM_SUCCESS; } +static char * +read_word (GIOChannel *from, + gboolean can_fail) +{ + char *str, *decoded; + gsize term_pos; + GIOStatus ok; + GError *error; + + while ((ok = g_io_channel_read_line (from, + &str, + NULL, + &term_pos, + &error)) == G_IO_STATUS_AGAIN); + + if (ok == G_IO_STATUS_EOF && can_fail) + return NULL; + + if (ok != G_IO_STATUS_NORMAL) { + if (error) { + g_printerr ("Error reading from standard input: %s\n", error->message); + g_error_free (error); + } else { + g_printerr ("Generic error reading from standard input\n"); + } + + exit(1); + } + + str[term_pos] = 0; + + /* URI escaping is a simple form of encoding that avoids + ambiguity with \n while allowing arbitrary byte sequences */ + decoded = g_uri_unescape_string (str, ""); + + if (decoded == NULL) { + g_printerr ("Failed to decode password (probably contained a NUL).\n"); + exit(1); + } + + g_free (str); + return decoded; +} + int main (int argc, char **argv) @@ -65,30 +113,31 @@ main (int argc, pam_handle_t *pamh; struct pam_conv conv = { answer_pam_message, NULL }; int res; + GIOChannel *stdin; - if (argc < 3) { - fprintf (stderr, "Wrong number of arguments passed, at least 2 expected\n"); + if (argc != 2) { + g_printerr ("Wrong number of arguments passed, 1 expected\n"); return 1; } username = argv[1]; - password = argv[2]; - if (argc == 4) - pin = argv[3]; - else - pin = NULL; + + stdin = g_io_channel_unix_new (STDIN_FILENO); + password = read_word (stdin, FALSE); + pin = read_word (stdin, TRUE); + g_io_channel_unref (stdin); res = pam_start ("accountsservice", username, &conv, &pamh); if (res != PAM_SUCCESS) { /* pam_strerror can't be used without a pam handle */ - fprintf (stderr, "Pam handle creation failed (not enough memory?)\n"); + g_printerr ("Pam handle creation failed (not enough memory?)\n"); return 2; } res = pam_chauthtok (pamh, PAM_SILENT); if (res != PAM_SUCCESS) { - fprintf (stderr, "Password change failed: %s\n", pam_strerror (pamh, res)); + g_printerr ("Password change failed: %s\n", pam_strerror (pamh, res)); } pam_end (pamh, res); diff --git a/src/user.c b/src/user.c index 834a4e0..845f772 100644 --- a/src/user.c +++ b/src/user.c @@ -1496,6 +1496,27 @@ user_set_password_mode (AccountsUser *auser, return TRUE; } +static char * +build_pam_helper_stdin (const char *password, + const char *pin) +{ + char *encoded_pin, *encoded_password, *result; + + encoded_password = g_uri_escape_string (password, NULL, FALSE); + + if (pin != NULL) { + encoded_pin = g_uri_escape_string (pin, NULL, FALSE); + + result = g_strdup_printf ("%s\n%s\n", encoded_password, encoded_pin); + g_free (encoded_pin); + } else { + result = g_strdup_printf ("%s\n", encoded_password); + } + + g_free (encoded_password); + return result; +} + static void user_change_password_authorized_cb (Daemon *daemon, User *user, @@ -1506,6 +1527,7 @@ user_change_password_authorized_cb (Daemon *daemon, gchar **strings = data; GError *error; const gchar *argv[6]; + char *stdin; sys_log (context, "set password and hint of user '%s' (%d)", @@ -1515,16 +1537,20 @@ user_change_password_authorized_cb (Daemon *daemon, argv[0] = LIBEXECDIR "/accounts-daemon-pam-password-helper"; argv[1] = user->user_name; - argv[2] = strings[0]; - argv[3] = NULL; + argv[2] = NULL; + stdin = build_pam_helper_stdin (strings[0], NULL); error = NULL; - if (!spawn_with_login_uid (context, argv, &error)) { + + if (!spawn_with_login_uid_and_stdin (context, argv, stdin, &error)) { throw_error (context, ERROR_FAILED, "running '%s' failed: %s", argv[0], error->message); g_error_free (error); + g_free (stdin); return; } + g_free (stdin); + if (user->password_mode != PASSWORD_MODE_REGULAR) { user->password_mode = PASSWORD_MODE_REGULAR; g_object_notify (G_OBJECT (user), "password-mode"); @@ -1594,6 +1620,7 @@ user_change_pin_authorized_cb (Daemon *daemon, { gchar **strings = data; GError *error; + gchar *stdin; const gchar *argv[6]; sys_log (context, @@ -1604,17 +1631,19 @@ user_change_pin_authorized_cb (Daemon *daemon, argv[0] = LIBEXECDIR "/accounts-daemon-pam-password-helper"; argv[1] = user->user_name; - argv[2] = strings[0]; - argv[3] = strings[2]; - argv[4] = NULL; + argv[2] = NULL; + stdin = build_pam_helper_stdin (strings[0], strings[1]); error = NULL; - if (!spawn_with_login_uid (context, argv, &error)) { + if (!spawn_with_login_uid_and_stdin (context, argv, stdin, &error)) { throw_error (context, ERROR_FAILED, "running '%s' failed: %s", argv[0], error->message); g_error_free (error); + g_free (stdin); return; } + g_free (stdin); + if (user->password_mode != PASSWORD_MODE_REGULAR) { user->password_mode = PASSWORD_MODE_REGULAR; g_object_notify (G_OBJECT (user), "password-mode"); diff --git a/src/util.c b/src/util.c index 2e363a3..9898910 100644 --- a/src/util.c +++ b/src/util.c @@ -248,6 +248,109 @@ spawn_with_login_uid (GDBusMethodInvocation *context, return TRUE; } +static char * +read_all_from_fd (int fd, + GError **error) +{ + GIOChannel *channel; + char *str; + GIOStatus status; + + channel = g_io_channel_unix_new (fd); + while ((status = g_io_channel_read_to_end (channel, &str, NULL, error)) + == G_IO_STATUS_AGAIN); + + if (status != G_IO_STATUS_NORMAL) + str = NULL; + + g_io_channel_unref (channel); + return str; +} + +static GIOStatus +write_all_to_fd (int fd, + const char *str, + GError **error) +{ + GIOChannel *channel; + GIOStatus status; + gsize written; + + channel = g_io_channel_unix_new (fd); + written = 0; + while ((status = g_io_channel_write_chars (channel, str + written, -1, &written, error)) + == G_IO_STATUS_AGAIN); + + g_io_channel_unref (channel); + return status; +} + +gboolean +spawn_with_login_uid_and_stdin (GDBusMethodInvocation *context, + const gchar *argv[], + const gchar *stdin, + GError **error) +{ + GError *local_error; + gchar loginuid[20]; + gint std_in, std_err; + gint status; + char *std_err_str; + GPid pid; + + get_caller_loginuid (context, loginuid, 20); + + local_error = NULL; + + if (!g_spawn_async_with_pipes (NULL, (gchar**)argv, NULL, + G_SPAWN_DO_NOT_REAP_CHILD, + setup_loginuid, loginuid, + &pid, &std_in, NULL, &std_err, &local_error)) { + g_propagate_error (error, local_error); + return FALSE; + } + + if (write_all_to_fd (std_in, stdin, &local_error)) { + g_prefix_error (&local_error, "Error writing to child standard input: "); + g_propagate_error (error, local_error); + close (std_in); + close (std_err); + return FALSE; + } + + close (std_in); + /* We need to read from stderr before calling waitpid, or the child + could be blocked on a pipe buffer flush */ + std_err_str = read_all_from_fd (std_err, &local_error); + /* At this point, it is safe to close std_err: EOF is returned from a pipe + read only when the writing end is closed, so when we return from read_all_from_fd() + the child is already done, and there is no risk of SIGPIPE + */ + close (std_err); + + waitpid (pid, &status, 0); + + if (std_err_str == NULL) { + g_prefix_error (&local_error, "Error reading from child standard error: "); + g_propagate_error (error, local_error); + return FALSE; + } + + if (WEXITSTATUS (status) != 0) { + g_set_error (error, + G_SPAWN_ERROR, + G_SPAWN_ERROR_FAILED, + "%s returned an error (%d): %s", + argv[0], WEXITSTATUS(status), std_err_str); + g_free (std_err_str); + return FALSE; + } + + g_free (std_err_str); + + return TRUE; +} + gint get_user_groups (const gchar *user, gid_t group, diff --git a/src/util.h b/src/util.h index 41ba545..157a476 100644 --- a/src/util.h +++ b/src/util.h @@ -36,6 +36,11 @@ gboolean spawn_with_login_uid (GDBusMethodInvocation *context, const gchar *argv[], GError **error); +gboolean spawn_with_login_uid_and_stdin (GDBusMethodInvocation *context, + const gchar *argv[], + const gchar *stdin, + GError **error); + gint get_user_groups (const gchar *username, gid_t group, gid_t **groups); -- 1.8.0.2