From 1f9688633e6b9a09c2f4bcd621412c8fa2522ba4 Mon Sep 17 00:00:00 2001 From: Giovanni Campagna Date: Thu, 27 Dec 2012 02:33:52 +0100 Subject: [PATCH 2/7] 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 | 73 ++++++++++++- src/Makefile.am | 2 + src/daemon.c | 4 +- src/daemon.h | 1 + src/libaccountsservice/act-user.c | 68 ++++++++++++ src/libaccountsservice/act-user.h | 7 ++ src/user.c | 189 +++++++++++++++++++++++++++++++++ 8 files changed, 345 insertions(+), 4 deletions(-) diff --git a/configure.ac b/configure.ac index 6a76a66..1f18895 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 88198d8..0a1d318 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,6 +498,76 @@ + + + + + + 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 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) + - the result of calling gcr_secret_exchange_send(). The calls must + have been in the same order as they appear in the array. + + + + + + + Sets a new password and hint for this user. + + + Note that setting a password has the side-effect of + unlocking the account. + + + + The caller needs one of the following PolicyKit authorizations: + + + org.freedesktop.accounts.user-administration + To change the password of a user + + + + + 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 24bbd19..5acfd80 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -13,6 +13,7 @@ INCLUDES = \ $(GIO_CFLAGS) \ $(GLIB_CFLAGS) \ $(POLKIT_CFLAGS) \ + $(GCR_CFLAGS) \ $(WARN_CFLAGS) noinst_LTLIBRARIES = libaccounts-generated.la @@ -46,6 +47,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 c7457d2..e3187c4 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 2ef4d0b..57cc079 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/act-user.c b/src/libaccountsservice/act-user.c index e9373b1..6313d1e 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,6 +1672,71 @@ act_user_set_password (ActUser *user, } /** + * act_user_set_multiple_passwords: + * @user: the user object to alter. + * @password_map: (element-type guint utf8): a #GHashTable mapping + * #ActUserPasswordType to a plain-text password + * + * Changes password and password hint 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_multiple_passwords (ActUser *user, + GHashTable *password_map) +{ + GError *error = NULL; + 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_map != NULL); + g_return_if_fail (ACCOUNTS_IS_USER (user->accounts_proxy)); + + 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); +} + +/** * act_user_set_password_mode: * @user: the user object to alter. * @password_mode: a #ActUserPasswordMode diff --git a/src/libaccountsservice/act-user.h b/src/libaccountsservice/act-user.h index 31b2cc6..d393ff2 100644 --- a/src/libaccountsservice/act-user.h +++ b/src/libaccountsservice/act-user.h @@ -46,6 +46,11 @@ typedef enum { ACT_USER_PASSWORD_MODE_NONE, } ActUserPasswordMode; +typedef enum { + ACT_USER_PASSWORD_REGULAR, + ACT_USER_PASSWORD_HINT +} ActUserPasswordType; + typedef struct _ActUser ActUser; typedef struct _ActUserClass ActUserClass; @@ -100,6 +105,8 @@ 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_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 8eb198d..c3e9e3d 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; @@ -1604,6 +1609,182 @@ user_set_password (AccountsUser *auser, } static void +user_continue_change_password_authorized_cb (Daemon *daemon, + User *user, + GDBusMethodInvocation *context, + gpointer data) + +{ + GVariant *passwords = data; + GVariantIter iter; + GcrSecretExchange *exchange; + GError *error; + const gchar *argv[6]; + unsigned int type; + const char *encrypted_password; + char *password_hint, *password; + char *stdin; + + 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; + 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; + + 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 (password); + 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); + goto out; + } + + if (user->password_mode != PASSWORD_MODE_REGULAR) { + user->password_mode = PASSWORD_MODE_REGULAR; + g_object_notify (G_OBJECT (user), "password-mode"); + } + + if (user->locked) { + user->locked = FALSE; + g_object_notify (G_OBJECT (user), "locked"); + } + + if (password_hint != NULL && + g_strcmp0 (user->password_hint, password_hint) != 0) { + g_free (user->password_hint); + /* 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); + + accounts_user_emit_changed (ACCOUNTS_USER (user)); + + accounts_user_complete_continue_set_password (ACCOUNTS_USER (user), context); + + out: + g_free (password_hint); + g_free (password); + g_object_thaw_notify (G_OBJECT (user)); + g_object_unref (exchange); +} + +static gboolean +user_continue_set_password (AccountsUser *auser, + GDBusMethodInvocation *context, + GVariant *passwords) +{ + User *user = (User*)auser; + + daemon_local_check_auth (user->daemon, + user, + "org.freedesktop.accounts.user-administration", + TRUE, + user_continue_change_password_authorized_cb, + context, + g_variant_ref (passwords), + (GDestroyNotify)g_variant_unref); + + 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; +} + +static void user_change_automatic_login_authorized_cb (Daemon *daemon, User *user, GDBusMethodInvocation *context, @@ -1767,6 +1948,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); @@ -1934,6 +2117,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_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; @@ -1965,6 +2150,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.1.2