From ca02f7b72a4e1b617d9cdc473800cff7ad4bfae1 Mon Sep 17 00:00:00 2001 From: Giovanni Campagna Date: Thu, 21 Jun 2012 22:34:10 +0200 Subject: [PATCH 1/7] Use PAM to change passwords Modifying /etc/shadow directly can be wrong in many configurations, and will fail to update other tokens (such as keyring passwords). Instead, use a small helper to interact with PAM (as PAM functions are not safe to use in process). Communication with the helper happens through a pipe, which also solves the problem of exposing the crypted password to all users via /proc. As the PAM stack requires an uncrypted password, the API of SetPassword was changed, and now the password travels in plaintext across the system bus. A later patch will solve this security issue using reversible encryption. https://bugs.freedesktop.org/show_bug.cgi?id=51833 --- configure.ac | 1 + data/Makefile.am | 3 + data/accountsservice | 4 + src/Makefile.am | 12 ++- src/libaccountsservice/act-user.c | 41 +--------- src/pam-password-helper.c | 152 ++++++++++++++++++++++++++++++++++++++ src/user.c | 29 ++++++-- src/util.c | 103 ++++++++++++++++++++++++++ src/util.h | 5 ++ 9 files changed, 302 insertions(+), 48 deletions(-) create mode 100644 data/accountsservice create mode 100644 src/pam-password-helper.c diff --git a/configure.ac b/configure.ac index 8360763..6a76a66 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/data/Makefile.am b/data/Makefile.am index 521c6c2..ebd1a69 100644 --- a/data/Makefile.am +++ b/data/Makefile.am @@ -18,6 +18,9 @@ policydir = $(datadir)/polkit-1/actions policy_in_files = org.freedesktop.accounts.policy.in policy_DATA = $(policy_in_files:.policy.in=.policy) +pamdir = $(sysconfdir)/pam.d +dist_pam_DATA = accountsservice + @INTLTOOL_POLICY_RULE@ if HAVE_SYSTEMD diff --git a/data/accountsservice b/data/accountsservice new file mode 100644 index 0000000..7dd7fb3 --- /dev/null +++ b/data/accountsservice @@ -0,0 +1,4 @@ +#%PAM-1.0 +password include password-auth + + diff --git a/src/Makefile.am b/src/Makefile.am index a053976..24bbd19 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -6,8 +6,12 @@ INCLUDES = \ -DDATADIR=\""$(datadir)"\" \ -DICONDIR=\"$(localstatedir)/lib/AccountsService/icons\" \ -DUSERDIR=\"$(localstatedir)/lib/AccountsService/users\" \ + -DLIBEXECDIR=\"$(libexecdir)\" \ -I$(srcdir) \ -I$(builddir) \ + $(DBUS_GLIB_CFLAGS) \ + $(GIO_CFLAGS) \ + $(GLIB_CFLAGS) \ $(POLKIT_CFLAGS) \ $(WARN_CFLAGS) @@ -27,7 +31,7 @@ accounts-generated.c accounts-generated.h: $(top_srcdir)/data/org.freedesktop.Ac accounts-user-generated.c accounts-user-generated.h: $(top_srcdir)/data/org.freedesktop.Accounts.User.xml Makefile gdbus-codegen --generate-c-code accounts-user-generated --c-namespace Accounts --interface-prefix=org.freedesktop.Accounts. $(top_srcdir)/data/org.freedesktop.Accounts.User.xml -libexec_PROGRAMS = accounts-daemon +libexec_PROGRAMS = accounts-daemon accounts-daemon-pam-password-helper accounts_daemon_SOURCES = \ $(enums_h_sources) \ @@ -44,6 +48,12 @@ accounts_daemon_LDADD = \ libaccounts-generated.la \ $(POLKIT_LIBS) +accounts_daemon_pam_password_helper_SOURCES = \ + pam-password-helper.c +accounts_daemon_pam_password_helper_LDADD = \ + $(GLIB_LIBS) \ + -lpam + CLEANFILES = $(BUILT_SOURCES) install-data-hook: diff --git a/src/libaccountsservice/act-user.c b/src/libaccountsservice/act-user.c index 80db669..e9373b1 100644 --- a/src/libaccountsservice/act-user.c +++ b/src/libaccountsservice/act-user.c @@ -1635,42 +1635,6 @@ act_user_set_account_type (ActUser *user, } } -static gchar -salt_char (GRand *rand) -{ - gchar salt[] = "ABCDEFGHIJKLMNOPQRSTUVXYZ" - "abcdefghijklmnopqrstuvxyz" - "./0123456789"; - - return salt[g_rand_int_range (rand, 0, G_N_ELEMENTS (salt))]; -} - -static gchar * -make_crypted (const gchar *plain) -{ - GString *salt; - gchar *result; - GRand *rand; - gint i; - - rand = g_rand_new (); - salt = g_string_sized_new (21); - - /* SHA 256 */ - g_string_append (salt, "$6$"); - for (i = 0; i < 16; i++) { - g_string_append_c (salt, salt_char (rand)); - } - g_string_append_c (salt, '$'); - - result = g_strdup (crypt (plain, salt->str)); - - g_string_free (salt, TRUE); - g_rand_free (rand); - - return result; -} - /** * act_user_set_password: * @user: the user object to alter. @@ -1694,17 +1658,14 @@ act_user_set_password (ActUser *user, g_return_if_fail (password != NULL); g_return_if_fail (ACCOUNTS_IS_USER (user->accounts_proxy)); - crypted = make_crypted (password); if (!accounts_user_call_set_password_sync (user->accounts_proxy, - crypted, + password, hint, NULL, &error)) { g_warning ("SetPassword call failed: %s", error->message); g_error_free (error); } - memset (crypted, 0, strlen (crypted)); - g_free (crypted); } /** diff --git a/src/pam-password-helper.c b/src/pam-password-helper.c new file mode 100644 index 0000000..000a619 --- /dev/null +++ b/src/pam-password-helper.c @@ -0,0 +1,152 @@ +/* -*- Mode: C; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 8 -*- + * + * Copyright (C) 2012 Giovanni Campagna + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA + * + */ + +#include "config.h" + +#include +#include +#include +#include +#include +#include +#include + +static char *password; + +static int +answer_pam_message (int num_msg, + const struct pam_message **msg, + struct pam_response **resp, + void *appdata_ptr) +{ + 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. */ + /* Despite what's documented, msg is not a pointer + to an array of pointers. It's a pointer to a pointer + to an array of structures. Ugh! + */ + for (i = 0; i < num_msg; i++) { + if ((*msg)[i].msg_style == PAM_PROMPT_ECHO_OFF) { + resps[i].resp = strdup(password); + } else { + resps[i].resp = NULL; + } + + resps[i].resp_retcode = 0; + + } + + *resp = resps; + + return PAM_SUCCESS; +} + +static char * +read_word (GIOChannel *from, + gboolean can_fail) +{ + char *str, *decoded; + gsize term_pos; + GIOStatus ok; + GError *error; + + error = NULL; + 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) +{ + const char *username; + pam_handle_t *pamh; + struct pam_conv conv = { answer_pam_message, NULL }; + int res; + GIOChannel *stdin; + + if (argc != 2) { + g_printerr ("Wrong number of arguments passed, 1 expected\n"); + return 1; + } + + username = argv[1]; + + stdin = g_io_channel_unix_new (STDIN_FILENO); + password = read_word (stdin, FALSE); + 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 */ + g_printerr ("Pam handle creation failed (not enough memory?)\n"); + return 2; + } + + res = pam_chauthtok (pamh, PAM_SILENT); + if (res != PAM_SUCCESS) { + g_printerr ("Password change failed: %s\n", pam_strerror (pamh, res)); + } + pam_end (pamh, res); + + g_free (password); + + return res == PAM_SUCCESS ? 0 : 2; +} diff --git a/src/user.c b/src/user.c index 6971ea3..8eb198d 100644 --- a/src/user.c +++ b/src/user.c @@ -1497,6 +1497,18 @@ user_set_password_mode (AccountsUser *auser, return TRUE; } +static char * +build_pam_helper_stdin (const char *password) +{ + char *encoded_password, *result; + + encoded_password = g_uri_escape_string (password, NULL, FALSE); + 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, @@ -1507,6 +1519,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)", @@ -1514,20 +1527,22 @@ user_change_password_authorized_cb (Daemon *daemon, g_object_freeze_notify (G_OBJECT (user)); - argv[0] = "/usr/sbin/usermod"; - argv[1] = "-p"; - argv[2] = strings[0]; - argv[3] = "--"; - argv[4] = user->user_name; - argv[5] = NULL; + argv[0] = LIBEXECDIR "/accounts-daemon-pam-password-helper"; + argv[1] = user->user_name; + argv[2] = NULL; + stdin = build_pam_helper_stdin (strings[0]); 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..1bb0129 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_IO_STATUS_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.1.2