Attachment 72158 Details for Bug 51833 – pam-helper: use pipes instead of command line arguments to set the password

[patch] pam-helper: use pipes instead of command line arguments to set the password

0001-pam-helper-use-pipes-instead-of-command-line-argumen.patch (text/plain), 12.94 KB, created by Giovanni Campagna on 2012-12-27 01:44:59 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Giovanni Campagna

Created: 2012-12-27 01:44:59 UTC

Size: 12.94 KB

patch

obsolete

>From 7fd2a673887202ae87d9f9fc4455d3292fdb8df8 Mon Sep 17 00:00:00 2001
>From: Giovanni Campagna <gcampagna@src.gnome.org>
>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 <sys/types.h>
> #include <security/pam_appl.h>
> #include <security/pam_misc.h>
>+#include <glib.h>
> 
> 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
>

Actions: View | Splinter Review

Attachments on bug 51833: 63941 | 63942 | 63943 | 63944 | 72158 | 72159 | 72741 | 72743 | 72744 | 72745 | 74904 | 74912 | 74913 | 74914 | 74915 | 74916 | 74917 | 74919

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.