From 3e4deb033794c40790053ac45a582109bc8f96e4 Mon Sep 17 00:00:00 2001 From: Giovanni Campagna Date: Thu, 27 Dec 2012 02:33:52 +0100 Subject: [PATCH] Use GcrSecretExchange to pass the password from the client to the daemon GcrSecretExchange, from gcr, provides a way for two programs to communicate secret data without the risk of eavesdropping. We use it to pass the password, which would otherwise travel in plain-text, from the UI to the daemon. The two step protocol is implemented by two methods, BeginSetPassword and ContinueSetPassword. Due to the nature of the PIN implementation, and given that password and hint were tied before, ContinueSetPassword is actually a setter for all those three at once. For backwards compatibility, SetPassword is retained and continues to take the plain-text password, but it is deprecated. SetPIN is removed because it never appeared in a stable release. https://bugs.freedesktop.org/show_bug.cgi?id=51833 --- configure.ac | 5 +- data/org.freedesktop.Accounts.User.xml | 53 +++++++--- src/Makefile.am | 2 + src/daemon.c | 4 +- src/daemon.h | 1 + src/libaccountsservice/Makefile.am | 2 +- src/libaccountsservice/act-user.c | 71 +++++++++---- src/libaccountsservice/act-user.h | 13 ++- src/user.c | 177 ++++++++++++++++++++++++++------- 9 files changed, 250 insertions(+), 78 deletions(-) diff --git a/configure.ac b/configure.ac index 59a035d..142b09c 100644 --- a/configure.ac +++ b/configure.ac @@ -28,13 +28,14 @@ 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) +PKG_CHECK_MODULES(GCR, gcr-base-3) AM_MAINTAINER_MODE([enable]) # client library dependencies -LIBACCOUNTSSERVICE_LIBS="$GIO_LIBS" +LIBACCOUNTSSERVICE_LIBS="$GIO_LIBS $GCR_LIBS" AC_SUBST(LIBACCOUNTSSERVICE_LIBS) -LIBACCOUNTSSERVICE_CFLAGS="$GIO_CFLAGS" +LIBACCOUNTSSERVICE_CFLAGS="$GIO_CFLAGS $GCR_CFLAGS" AC_SUBST(LIBACCOUNTSSERVICE_CFLAGS) GOBJECT_INTROSPECTION_CHECK([0.9.12]) diff --git a/data/org.freedesktop.Accounts.User.xml b/data/org.freedesktop.Accounts.User.xml index f2f8909..8880052 100644 --- a/data/org.freedesktop.Accounts.User.xml +++ b/data/org.freedesktop.Accounts.User.xml @@ -460,7 +460,8 @@ - The crypted password. + The plain-text password. + This method is deprecated and unsafe. Use BeginSetPassword instead. @@ -497,36 +498,57 @@ - + - + - The password. + Unspecified opaque data that should be passed to gcr_secret_exchange_receive() - + + + + Begins a #GcrSecretExchange to set the new password for this user. + The expected next step is a call to ContinueSetPassword with the encrypted + passwords. + + + Note that setting a password has the side-effect of + unlocking the account. + + + + The caller does not need PolicyKit authorization, but the following ContinueSetPassword + might fail if the caller is not authorized. + + + + + + + + - The password hint. + The encrypted passwords. Each password is a DBus struct holding + - an unsigned integer specifying the kind of data: + - 0 if the password is a regular password + - 1 if the password is a really a hint (not a secret) + - 2 if the password is a PIN + - the result of calling gcr_secret_exchange_send(). The calls must + have been in the same order as they appear in the array. - - - - The PIN - - - - Sets a new password for this user. + Sets a new password, hint and PIN for this user. - Note that setting a PIN has the side-effect of + Note that setting a password has the side-effect of unlocking the account. @@ -542,6 +564,7 @@ if the caller lacks the appropriate PolicyKit authorization if the operation failed + if the caller did not call BeginSetPassword beforehand diff --git a/src/Makefile.am b/src/Makefile.am index 3ba60ab..023f932 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -18,6 +18,7 @@ INCLUDES = \ $(GIO_CFLAGS) \ $(GLIB_CFLAGS) \ $(POLKIT_CFLAGS) \ + $(GCR_CFLAGS) \ $(WARN_CFLAGS) noinst_LTLIBRARIES = libaccounts-generated.la @@ -51,6 +52,7 @@ accounts_daemon_SOURCES = \ accounts_daemon_LDADD = \ libaccounts-generated.la \ + $(GCR_LIBS) \ $(POLKIT_LIBS) accounts_daemon_pam_password_helper_SOURCES = \ diff --git a/src/daemon.c b/src/daemon.c index f2ee7ac..4666ea8 100644 --- a/src/daemon.c +++ b/src/daemon.c @@ -118,7 +118,8 @@ static const GDBusErrorEntry accounts_error_entries[] = { ERROR_USER_EXISTS, "org.freedesktop.Accounts.Error.UserExists" }, { ERROR_USER_DOES_NOT_EXIST, "org.freedesktop.Accounts.Error.UserDoesNotExist" }, { ERROR_PERMISSION_DENIED, "org.freedesktop.Accounts.Error.PermissionDenied" }, - { ERROR_NOT_SUPPORTED, "org.freedesktop.Accounts.Error.NotSupported" } + { ERROR_NOT_SUPPORTED, "org.freedesktop.Accounts.Error.NotSupported" }, + { ERROR_INVALID, "org.freedesktop.Accounts.Error.InvalidArguments" }, }; GQuark @@ -149,6 +150,7 @@ error_get_type (void) ENUM_ENTRY (ERROR_USER_DOES_NOT_EXIST, "UserDoesntExist"), ENUM_ENTRY (ERROR_PERMISSION_DENIED, "PermissionDenied"), ENUM_ENTRY (ERROR_NOT_SUPPORTED, "NotSupported"), + ENUM_ENTRY (ERROR_INVALID, "InvalidArguments"), { 0, 0, 0 } }; g_assert (NUM_ERRORS == G_N_ELEMENTS (values) - 1); diff --git a/src/daemon.h b/src/daemon.h index 996b452..5ef85eb 100644 --- a/src/daemon.h +++ b/src/daemon.h @@ -57,6 +57,7 @@ typedef enum { ERROR_USER_DOES_NOT_EXIST, ERROR_PERMISSION_DENIED, ERROR_NOT_SUPPORTED, + ERROR_INVALID, NUM_ERRORS } Error; diff --git a/src/libaccountsservice/Makefile.am b/src/libaccountsservice/Makefile.am index 5bff59f..f37f16a 100644 --- a/src/libaccountsservice/Makefile.am +++ b/src/libaccountsservice/Makefile.am @@ -120,7 +120,7 @@ AccountsService_1_0_gir_SCANNERFLAGS = \ $(libaccountsservice_la_CFLAGS) \ $(END_OF_LIST) -AccountsService_1_0_gir_INCLUDES = GObject-2.0 +AccountsService_1_0_gir_INCLUDES = GObject-2.0 Gio-2.0 AccountsService_1_0_gir_LIBS = libaccountsservice.la AccountsService_1_0_gir_FILES = \ $(libaccountsservice_la_sources) \ diff --git a/src/libaccountsservice/act-user.c b/src/libaccountsservice/act-user.c index 2cc3cb2..689f51b 100644 --- a/src/libaccountsservice/act-user.c +++ b/src/libaccountsservice/act-user.c @@ -30,6 +30,9 @@ #include #include +#define GCR_API_SUBJECT_TO_CHANGE +#include + #include "act-user-private.h" #include "accounts-user-generated.h" @@ -1669,40 +1672,68 @@ act_user_set_password (ActUser *user, } /** - * act_user_set_pin: + * act_user_set_multiple_passwords: * @user: the user object to alter. - * @password: the current user password - * @hint: a hint to help user recall password - * @pin: the new PIN + * @password_map: (element-type guint utf8): a #GHashTable mapping + * #ActUserPasswordType to a plain-text password * - * Changes the password of @user to @password, and sets - * the PIN for local logins to @pin. - * @hint is displayed to the user if they forget the password. + * Changes password, password hint and PIN of @user, using the + * values taken from @password_map. * * Note this function is synchronous and ignores errors. **/ +/* Note on the API: what we really want here is a list of (int-string) + values, but short of passing #GVariants (which is ugly), we can't + do that in a introspectable way without a boxed struct +*/ void -act_user_set_pin (ActUser *user, - const gchar *password, - const gchar *hint, - const gchar *pin) +act_user_set_multiple_passwords (ActUser *user, + GHashTable *password_map) { GError *error = NULL; - gchar *crypted; + GcrSecretExchange *exchange; + GHashTableIter iter; + GVariantBuilder builder; + char *exchange_begin; + gpointer key, value; g_return_if_fail (ACT_IS_USER (user)); - g_return_if_fail (password != NULL); + g_return_if_fail (password_map != NULL); g_return_if_fail (ACCOUNTS_IS_USER (user->accounts_proxy)); - if (!accounts_user_call_set_pin_sync (user->accounts_proxy, - password, - hint, - pin, - NULL, - &error)) { - g_warning ("SetPassword call failed: %s", error->message); + if (accounts_user_call_begin_set_password_sync (user->accounts_proxy, + &exchange_begin, + NULL, &error)) { + g_warning ("BeginSetPassword call failed: %s", error->message); g_error_free (error); + return; } + + exchange = gcr_secret_exchange_new (GCR_SECRET_EXCHANGE_PROTOCOL_1); + gcr_secret_exchange_receive (exchange, exchange_begin); + g_free (exchange_begin); + + g_variant_builder_init (&builder, G_VARIANT_TYPE ("a(us)")); + g_hash_table_iter_init (&iter, password_map); + while (g_hash_table_iter_next (&iter, &key, &value)) { + char *next_str; + + next_str = gcr_secret_exchange_send (exchange, value, -1); + g_variant_builder_add (&builder, "(us)", + GPOINTER_TO_INT (key), + next_str); + g_free (next_str); + } + + if (!accounts_user_call_continue_set_password_sync (user->accounts_proxy, + g_variant_builder_end (&builder), + NULL, + &error)) { + g_warning ("ContinueSetPassword call failed: %s", error->message); + g_error_free (error); + } + + g_object_unref (exchange); } /** diff --git a/src/libaccountsservice/act-user.h b/src/libaccountsservice/act-user.h index 4bfdff2..dd04d8e 100644 --- a/src/libaccountsservice/act-user.h +++ b/src/libaccountsservice/act-user.h @@ -46,6 +46,12 @@ typedef enum { ACT_USER_PASSWORD_MODE_NONE, } ActUserPasswordMode; +typedef enum { + ACT_USER_PASSWORD_REGULAR, + ACT_USER_PASSWORD_HINT, + ACT_USER_PASSWORD_PIN +} ActUserPasswordType; + typedef struct _ActUser ActUser; typedef struct _ActUserClass ActUserClass; @@ -100,10 +106,9 @@ void act_user_set_account_type (ActUser *user, void act_user_set_password (ActUser *user, const gchar *password, const gchar *hint); -void act_user_set_pin (ActUser *user, - const gchar *password, - const gchar *hint, - const gchar *pin); +void act_user_set_multiple_passwords (ActUser *user, + GHashTable *password_map); + void act_user_set_password_mode (ActUser *user, ActUserPasswordMode password_mode); void act_user_set_locked (ActUser *user, diff --git a/src/user.c b/src/user.c index 845f772..7ad199a 100644 --- a/src/user.c +++ b/src/user.c @@ -41,6 +41,9 @@ #include #include +#define GCR_API_SUBJECT_TO_CHANGE +#include + #include "daemon.h" #include "user.h" #include "accounts-user-generated.h" @@ -78,6 +81,8 @@ struct User { Daemon *daemon; + GHashTable *secret_exchanges; + uid_t uid; gid_t gid; gchar *user_name; @@ -1612,34 +1617,78 @@ user_set_password (AccountsUser *auser, } static void -user_change_pin_authorized_cb (Daemon *daemon, - User *user, - GDBusMethodInvocation *context, - gpointer data) +user_continue_change_password_authorized_cb (Daemon *daemon, + User *user, + GDBusMethodInvocation *context, + gpointer data) { - gchar **strings = data; + GVariant *passwords = data; + GVariantIter iter; + GcrSecretExchange *exchange; GError *error; - gchar *stdin; const gchar *argv[6]; + unsigned int type; + const char *encrypted_password; + char *password_hint, *password, *pin; + char *stdin; - sys_log (context, - "set password and hint of user '%s' (%d)", + sys_log (context, "change password of user '%s' (%d)", user->user_name, user->uid); + exchange = g_hash_table_lookup (user->secret_exchanges, + g_dbus_method_invocation_get_sender (context)); + g_object_ref (exchange); + g_hash_table_remove (user->secret_exchanges, g_dbus_method_invocation_get_sender (context)); + + if (exchange == NULL) { + /* The caller never called BeginSetPassword */ + throw_error (context, ERROR_INVALID, "secret exchange not initialized"); + return; + } + g_object_freeze_notify (G_OBJECT (user)); + password_hint = NULL; + password = NULL; + pin = NULL; + g_variant_iter_init (&iter, passwords); + while (g_variant_iter_next (&iter, "(u&s)", &type, &encrypted_password)) { + if (!gcr_secret_exchange_receive (exchange, encrypted_password)) { + throw_error (context, ERROR_INVALID, "could not decrypt password"); + goto out; + } + + switch (type) { + case 0: + g_free (password); + password = g_strdup (gcr_secret_exchange_get_secret (exchange, NULL)); + break; + case 1: + g_free (password_hint); + password_hint = g_strdup (gcr_secret_exchange_get_secret (exchange, NULL)); + break; + case 2: + g_free (pin); + pin = g_strdup (gcr_secret_exchange_get_secret (exchange, NULL)); + break; + + default: + throw_error (context, ERROR_INVALID, "invalid password type"); + goto out; + } + } + argv[0] = LIBEXECDIR "/accounts-daemon-pam-password-helper"; argv[1] = user->user_name; argv[2] = NULL; - stdin = build_pam_helper_stdin (strings[0], strings[1]); + stdin = build_pam_helper_stdin (password, pin);; error = NULL; + 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; + goto out; } g_free (stdin); @@ -1654,48 +1703,99 @@ user_change_pin_authorized_cb (Daemon *daemon, g_object_notify (G_OBJECT (user), "locked"); } - if (g_strcmp0 (user->password_hint, strings[1]) != 0) { + if (password_hint != NULL && + g_strcmp0 (user->password_hint, password_hint) != 0) { g_free (user->password_hint); - user->password_hint = g_strdup (strings[1]); + /* An empty password hint means no password hint */ + if (*password_hint) + user->password_hint = g_strdup (password_hint); + else + user->password_hint = NULL; g_object_notify (G_OBJECT (user), "password-hint"); } save_extra_data (user); - g_object_thaw_notify (G_OBJECT (user)); - accounts_user_emit_changed (ACCOUNTS_USER (user)); - accounts_user_complete_set_pin (ACCOUNTS_USER (user), context); + accounts_user_complete_continue_set_password (ACCOUNTS_USER (user), context); + + out: + g_free (password_hint); + g_free (password); + g_free (pin); + g_object_thaw_notify (G_OBJECT (user)); + g_object_unref (exchange); } static gboolean -user_set_pin (AccountsUser *auser, - GDBusMethodInvocation *context, - const gchar *password, - const gchar *hint, - const gchar *pin) +user_continue_set_password (AccountsUser *auser, + GDBusMethodInvocation *context, + GVariant *passwords) { User *user = (User*)auser; - gchar **data; - - data = g_new (gchar *, 3); - data[0] = g_strdup (password); - data[1] = g_strdup (hint); - data[2] = g_strdup (pin); - data[3] = NULL; daemon_local_check_auth (user->daemon, user, "org.freedesktop.accounts.user-administration", TRUE, - user_change_pin_authorized_cb, + user_continue_change_password_authorized_cb, context, - data, - (GDestroyNotify)free_passwords); + g_variant_ref (passwords), + (GDestroyNotify)g_variant_unref); - memset ((char*)password, 0, strlen (password)); - memset ((char*)pin, 0, strlen (pin)); + return TRUE; +} + +static void +on_name_vanished (GDBusConnection *connection, + const gchar *name, + gpointer user_data) +{ + User *user = USER (user_data); + + g_hash_table_remove (user->secret_exchanges, name); +} + +static void +on_exchange_unreffed (gpointer user_data, + GObject *object) +{ + g_bus_unwatch_name (GPOINTER_TO_INT (user_data)); +} + +static gboolean +user_begin_set_password (AccountsUser *auser, + GDBusMethodInvocation *context) +{ + User *user = (User*)auser; + const char *who; + GcrSecretExchange *exchange; + char *begin_str; + int name_id; + + who = g_dbus_method_invocation_get_sender (context); + exchange = g_hash_table_lookup (user->secret_exchanges, who); + + if (exchange != NULL) { + throw_error (context, ERROR_INVALID, "secret exchange already in progress"); + return TRUE; + } + + exchange = gcr_secret_exchange_new (GCR_SECRET_EXCHANGE_PROTOCOL_1); + g_hash_table_insert (user->secret_exchanges, g_strdup (who), exchange); + + name_id = g_bus_watch_name (G_BUS_TYPE_SYSTEM, who, + G_BUS_NAME_WATCHER_FLAGS_NONE, + NULL, /* name appeared */ + on_name_vanished, + auser, /* weak ref */ + NULL); + g_object_weak_ref (G_OBJECT (exchange), on_exchange_unreffed, GINT_TO_POINTER (name_id)); + + begin_str = gcr_secret_exchange_begin (exchange); + accounts_user_complete_begin_set_password (auser, context, begin_str); + g_free (begin_str); return TRUE; } @@ -1864,6 +1964,8 @@ user_finalize (GObject *object) user = USER (object); + g_hash_table_unref (user->secret_exchanges); + g_free (user->object_path); g_free (user->user_name); g_free (user->real_name); @@ -2031,7 +2133,8 @@ user_accounts_user_iface_init (AccountsUserIface *iface) iface->handle_set_location = user_set_location; iface->handle_set_locked = user_set_locked; iface->handle_set_password = user_set_password; - iface->handle_set_pin = user_set_pin; + iface->handle_continue_set_password = user_continue_set_password; + iface->handle_begin_set_password = user_begin_set_password; iface->handle_set_password_mode = user_set_password_mode; iface->handle_set_real_name = user_set_real_name; iface->handle_set_shell = user_set_shell; @@ -2063,6 +2166,10 @@ user_init (User *user) { user->system_bus_connection = NULL; user->object_path = NULL; + + user->secret_exchanges = g_hash_table_new_full (g_str_hash, g_str_equal, + g_free, g_object_unref); + user->user_name = NULL; user->real_name = NULL; user->account_type = ACCOUNT_TYPE_STANDARD; -- 1.8.0.2