Sign Up
Log In
Log In
or
Sign Up
Places
All Projects
Status Monitor
Collapse sidebar
SUSE:SLE-15-SP1:GA
polkit.9830
0001-Fix-CVE-FIXME-Trusting-client-supplied-UID...
Overview
Repositories
Revisions
Requests
Users
Attributes
Meta
File 0001-Fix-CVE-FIXME-Trusting-client-supplied-UID.patch of Package polkit.9830
From b77e3f0c13ac008905ad2a867c63f766af43ea95 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= <mitr@redhat.com> Date: Mon, 25 Jun 2018 19:24:06 +0200 Subject: [PATCH] Fix CVE-FIXME: Trusting client-supplied UID MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As part of CVE-2013-4288, the D-Bus clients were allowed (and encouraged) to submit the UID of the subject of authorization checks to avoid races against UID changes (notably using executables set-UID to root). However, that also allowed any client to submit an arbitrary UID, and that could be used to bypass "can only ask about / affect the same UID" checks in CheckAuthorization / RegisterAuthenticationAgent / UnregisterAuthenticationAgent. This allowed an attacker: - With CheckAuthorization, to cause the registered authentication agent in victim's session to pop up a dialog, or to determine whether the victim currently has a temporary authorization to perform an operation. (In principle, the attacker can also determine whether JavaScript rules allow the victim process to perform an operatin; however, usually rules base their decisions on information deterined from the supplied UID, so the attacker usually won't learn anything new.) - With RegisterAuthenticationAgent, to prevent the victim's authentication agent to work (for a specific victim process), or to learn about which operations requiring authorization the victim is attempting. To fix this, expose internal _polkit_unix_process_get_owner() / obsolete polkit_unix_process_get_owner() as a private polkit_unix_process_get_racy_uid__() (being more explicit about the dangers on relying on it), and use it in polkit_backend_session_monitor_get_user_for_subject() to return a boolean indicating whether the subject UID may be caller-chosen. Then, in the permission checks that require the subject to be equal to the caller, fail on caller-chosen UIDs (and continue through the pre-existing code paths which allow root, or root-designated server processes, to ask about arbitrary subjects.) Signed-off-by: Miloslav Trmač <mitr@redhat.com> --- src/polkit/polkitprivate.h | 2 + src/polkit/polkitunixprocess.c | 61 ++++++++++++++++--- .../polkitbackendinteractiveauthority.c | 39 +++++++----- .../polkitbackendsessionmonitor-systemd.c | 38 ++++++++++-- .../polkitbackendsessionmonitor.c | 40 ++++++++++-- .../polkitbackendsessionmonitor.h | 1 + 6 files changed, 148 insertions(+), 33 deletions(-) diff --git a/src/polkit/polkitprivate.h b/src/polkit/polkitprivate.h index 9f07063..c80142d 100644 --- a/src/polkit/polkitprivate.h +++ b/src/polkit/polkitprivate.h @@ -44,6 +44,8 @@ GVariant *polkit_action_description_to_gvariant (PolkitActionDescription *action GVariant *polkit_subject_to_gvariant (PolkitSubject *subject); GVariant *polkit_identity_to_gvariant (PolkitIdentity *identity); +gint polkit_unix_process_get_racy_uid__ (PolkitUnixProcess *process, GError **error); + PolkitSubject *polkit_subject_new_for_gvariant (GVariant *variant, GError **error); PolkitIdentity *polkit_identity_new_for_gvariant (GVariant *variant, GError **error); diff --git a/src/polkit/polkitunixprocess.c b/src/polkit/polkitunixprocess.c index d4ebf50..972b777 100644 --- a/src/polkit/polkitunixprocess.c +++ b/src/polkit/polkitunixprocess.c @@ -56,6 +56,14 @@ * To uniquely identify processes, both the process id and the start * time of the process (a monotonic increasing value representing the * time since the kernel was started) is used. + * + * NOTE: This object stores, and provides access to, the real UID of the + * process. That value can change over time (with set*uid*(2) and exec*(2)). + * Checks whether an operation is allowed need to take care to use the UID + * value as of the time when the operation was made (or, following the open() + * privilege check model, when the connection making the operation possible + * was initiated). That is usually done by initializing this with + * polkit_unix_process_new_for_owner() with trusted data. */ /** @@ -90,9 +98,6 @@ static void subject_iface_init (PolkitSubjectIface *subject_iface); static guint64 get_start_time_for_pid (gint pid, GError **error); -static gint _polkit_unix_process_get_owner (PolkitUnixProcess *process, - GError **error); - #if defined(HAVE_FREEBSD) || defined(HAVE_NETBSD) || defined(HAVE_OPENBSD) static gboolean get_kinfo_proc (gint pid, #if defined(HAVE_NETBSD) @@ -182,7 +187,7 @@ polkit_unix_process_constructed (GObject *object) { GError *error; error = NULL; - process->uid = _polkit_unix_process_get_owner (process, &error); + process->uid = polkit_unix_process_get_racy_uid__ (process, &error); if (error != NULL) { process->uid = -1; @@ -271,6 +276,12 @@ polkit_unix_process_class_init (PolkitUnixProcessClass *klass) * Gets the user id for @process. Note that this is the real user-id, * not the effective user-id. * + * NOTE: The UID may change over time, so the returned value may not match the + * current state of the underlying process; or the UID may have been set by + * polkit_unix_process_new_for_owner() or polkit_unix_process_set_uid(), + * in which case it may not correspond to the actual UID of the referenced + * process at all (at any point in time). + * * Returns: The user id for @process or -1 if unknown. */ gint @@ -708,13 +719,20 @@ out: return start_time; } -static gint -_polkit_unix_process_get_owner (PolkitUnixProcess *process, - GError **error) +/* + * Private: Return the "current" UID. Note that this is inherently racy, + * and the value may already be obsolete by the time this function returns; + * this function only guarantees that the UID was valid at some point during + * its execution. + */ +gint +polkit_unix_process_get_racy_uid__ (PolkitUnixProcess *process, + GError **error) { gint result; gchar *contents; gchar **lines; + guint64 start_time; #if defined(HAVE_FREEBSD) || defined(HAVE_OPENBSD) struct kinfo_proc p; #elif defined(HAVE_NETBSD) @@ -722,6 +740,7 @@ _polkit_unix_process_get_owner (PolkitUnixProcess *process, #else gchar filename[64]; guint n; + GError *local_error; #endif g_return_val_if_fail (POLKIT_IS_UNIX_PROCESS (process), 0); @@ -745,8 +764,10 @@ _polkit_unix_process_get_owner (PolkitUnixProcess *process, #if defined(HAVE_FREEBSD) result = p.ki_uid; + start_time = (guint64) p.ki_start.tv_sec; #else result = p.p_uid; + start_time = (guint64) p.p_ustart_sec; #endif #else @@ -781,17 +802,37 @@ _polkit_unix_process_get_owner (PolkitUnixProcess *process, else { result = real_uid; - goto out; + goto found; } } - g_set_error (error, POLKIT_ERROR, POLKIT_ERROR_FAILED, "Didn't find any line starting with `Uid:' in file %s", filename); + goto out; + +found: + /* The UID and start time are, sadly, not available in a single file. So, + * read the UID first, and then the start time; if the start time is the same + * before and after reading the UID, it couldn't have changed. + */ + local_error = NULL; + start_time = get_start_time_for_pid (process->pid, &local_error); + if (local_error != NULL) + { + g_propagate_error (error, local_error); + goto out; + } #endif + if (process->start_time != start_time) + { + g_set_error (error, POLKIT_ERROR, POLKIT_ERROR_FAILED, + "process with PID %d has been replaced", process->pid); + goto out; + } + out: g_strfreev (lines); g_free (contents); @@ -810,5 +851,5 @@ gint polkit_unix_process_get_owner (PolkitUnixProcess *process, GError **error) { - return _polkit_unix_process_get_owner (process, error); + return polkit_unix_process_get_racy_uid__ (process, error); } diff --git a/src/polkitbackend/polkitbackendinteractiveauthority.c b/src/polkitbackend/polkitbackendinteractiveauthority.c index 1cd60d3..cb6fdab 100644 --- a/src/polkitbackend/polkitbackendinteractiveauthority.c +++ b/src/polkitbackend/polkitbackendinteractiveauthority.c @@ -575,7 +575,7 @@ log_result (PolkitBackendInteractiveAuthority *authority, if (polkit_authorization_result_get_is_authorized (result)) log_result_str = "ALLOWING"; - user_of_subject = polkit_backend_session_monitor_get_user_for_subject (priv->session_monitor, subject, NULL); + user_of_subject = polkit_backend_session_monitor_get_user_for_subject (priv->session_monitor, subject, NULL, NULL); subject_str = polkit_subject_to_string (subject); @@ -847,6 +847,7 @@ polkit_backend_interactive_authority_check_authorization (PolkitBackendAuthority gchar *subject_str; PolkitIdentity *user_of_caller; PolkitIdentity *user_of_subject; + gboolean user_of_subject_matches; gchar *user_of_caller_str; gchar *user_of_subject_str; PolkitAuthorizationResult *result; @@ -892,7 +893,7 @@ polkit_backend_interactive_authority_check_authorization (PolkitBackendAuthority action_id); user_of_caller = polkit_backend_session_monitor_get_user_for_subject (priv->session_monitor, - caller, + caller, NULL, &error); if (error != NULL) { @@ -907,7 +908,7 @@ polkit_backend_interactive_authority_check_authorization (PolkitBackendAuthority g_debug (" user of caller is %s", user_of_caller_str); user_of_subject = polkit_backend_session_monitor_get_user_for_subject (priv->session_monitor, - subject, + subject, &user_of_subject_matches, &error); if (error != NULL) { @@ -937,7 +938,10 @@ polkit_backend_interactive_authority_check_authorization (PolkitBackendAuthority * We only allow this if, and only if, * * - processes may check for another process owned by the *same* user but not - * if details are passed (otherwise you'd be able to spoof the dialog) + * if details are passed (otherwise you'd be able to spoof the dialog); + * the caller supplies the user_of_subject value, so we additionally + * require it to match at least at one point in time (via + * user_of_subject_matches). * * - processes running as uid 0 may check anything and pass any details * @@ -945,7 +949,9 @@ polkit_backend_interactive_authority_check_authorization (PolkitBackendAuthority * then any uid referenced by that annotation is also allowed to check * to check anything and pass any details */ - if (!polkit_identity_equal (user_of_caller, user_of_subject) || has_details) + if (!user_of_subject_matches + || !polkit_identity_equal (user_of_caller, user_of_subject) + || has_details) { if (!may_identity_check_authorization (interactive_authority, action_id, user_of_caller)) { @@ -1110,9 +1116,10 @@ check_authorization_sync (PolkitBackendAuthority *authority, goto out; } - /* every subject has a user */ + /* every subject has a user; this is supplied by the client, so we rely + * on the caller to validate its acceptability. */ user_of_subject = polkit_backend_session_monitor_get_user_for_subject (priv->session_monitor, - subject, + subject, NULL, error); if (user_of_subject == NULL) goto out; @@ -2480,6 +2487,7 @@ polkit_backend_interactive_authority_register_authentication_agent (PolkitBacken PolkitSubject *session_for_caller; PolkitIdentity *user_of_caller; PolkitIdentity *user_of_subject; + gboolean user_of_subject_matches; AuthenticationAgent *agent; gboolean ret; gchar *caller_cmdline; @@ -2532,7 +2540,7 @@ polkit_backend_interactive_authority_register_authentication_agent (PolkitBacken goto out; } - user_of_caller = polkit_backend_session_monitor_get_user_for_subject (priv->session_monitor, caller, NULL); + user_of_caller = polkit_backend_session_monitor_get_user_for_subject (priv->session_monitor, caller, NULL, NULL); if (user_of_caller == NULL) { g_set_error (error, @@ -2541,7 +2549,7 @@ polkit_backend_interactive_authority_register_authentication_agent (PolkitBacken "Cannot determine user of caller"); goto out; } - user_of_subject = polkit_backend_session_monitor_get_user_for_subject (priv->session_monitor, subject, NULL); + user_of_subject = polkit_backend_session_monitor_get_user_for_subject (priv->session_monitor, subject, &user_of_subject_matches, NULL); if (user_of_subject == NULL) { g_set_error (error, @@ -2550,7 +2558,8 @@ polkit_backend_interactive_authority_register_authentication_agent (PolkitBacken "Cannot determine user of subject"); goto out; } - if (!polkit_identity_equal (user_of_caller, user_of_subject)) + if (!user_of_subject_matches + || !polkit_identity_equal (user_of_caller, user_of_subject)) { if (identity_is_root_user (user_of_caller)) { @@ -2643,6 +2652,7 @@ polkit_backend_interactive_authority_unregister_authentication_agent (PolkitBack PolkitSubject *session_for_caller; PolkitIdentity *user_of_caller; PolkitIdentity *user_of_subject; + gboolean user_of_subject_matches; AuthenticationAgent *agent; gboolean ret; gchar *scope_str; @@ -2691,7 +2701,7 @@ polkit_backend_interactive_authority_unregister_authentication_agent (PolkitBack goto out; } - user_of_caller = polkit_backend_session_monitor_get_user_for_subject (priv->session_monitor, caller, NULL); + user_of_caller = polkit_backend_session_monitor_get_user_for_subject (priv->session_monitor, caller, NULL, NULL); if (user_of_caller == NULL) { g_set_error (error, @@ -2700,7 +2710,7 @@ polkit_backend_interactive_authority_unregister_authentication_agent (PolkitBack "Cannot determine user of caller"); goto out; } - user_of_subject = polkit_backend_session_monitor_get_user_for_subject (priv->session_monitor, subject, NULL); + user_of_subject = polkit_backend_session_monitor_get_user_for_subject (priv->session_monitor, subject, &user_of_subject_matches, NULL); if (user_of_subject == NULL) { g_set_error (error, @@ -2709,7 +2719,8 @@ polkit_backend_interactive_authority_unregister_authentication_agent (PolkitBack "Cannot determine user of subject"); goto out; } - if (!polkit_identity_equal (user_of_caller, user_of_subject)) + if (!user_of_subject_matches + || !polkit_identity_equal (user_of_caller, user_of_subject)) { if (identity_is_root_user (user_of_caller)) { @@ -2819,7 +2830,7 @@ polkit_backend_interactive_authority_authentication_agent_response (PolkitBacken identity_str); user_of_caller = polkit_backend_session_monitor_get_user_for_subject (priv->session_monitor, - caller, + caller, NULL, error); if (user_of_caller == NULL) goto out; diff --git a/src/polkitbackend/polkitbackendsessionmonitor-systemd.c b/src/polkitbackend/polkitbackendsessionmonitor-systemd.c index 2a6c739..b00cdbd 100644 --- a/src/polkitbackend/polkitbackendsessionmonitor-systemd.c +++ b/src/polkitbackend/polkitbackendsessionmonitor-systemd.c @@ -29,6 +29,7 @@ #include <stdlib.h> #include <polkit/polkit.h> +#include <polkit/polkitprivate.h> #include "polkitbackendsessionmonitor.h" /* <internal> @@ -246,26 +247,40 @@ polkit_backend_session_monitor_get_sessions (PolkitBackendSessionMonitor *monito * polkit_backend_session_monitor_get_user: * @monitor: A #PolkitBackendSessionMonitor. * @subject: A #PolkitSubject. + * @result_matches: If not %NULL, set to indicate whether the return value matches current (RACY) state. * @error: Return location for error. * * Gets the user corresponding to @subject or %NULL if no user exists. * + * NOTE: For a #PolkitUnixProcess, the UID is read from @subject (which may + * come from e.g. a D-Bus client), so it may not correspond to the actual UID + * of the referenced process (at any point in time). This is indicated by + * setting @result_matches to %FALSE; the caller may reject such subjects or + * require additional privileges. @result_matches == %TRUE only indicates that + * the UID matched the underlying process at ONE point in time, it may not match + * later. + * * Returns: %NULL if @error is set otherwise a #PolkitUnixUser that should be freed with g_object_unref(). */ PolkitIdentity * polkit_backend_session_monitor_get_user_for_subject (PolkitBackendSessionMonitor *monitor, PolkitSubject *subject, + gboolean *result_matches, GError **error) { PolkitIdentity *ret; - guint32 uid; + gboolean matches; ret = NULL; + matches = FALSE; if (POLKIT_IS_UNIX_PROCESS (subject)) { - uid = polkit_unix_process_get_uid (POLKIT_UNIX_PROCESS (subject)); - if ((gint) uid == -1) + gint subject_uid, current_uid; + GError *local_error; + + subject_uid = polkit_unix_process_get_uid (POLKIT_UNIX_PROCESS (subject)); + if (subject_uid == -1) { g_set_error (error, POLKIT_ERROR, @@ -273,14 +288,24 @@ polkit_backend_session_monitor_get_user_for_subject (PolkitBackendSessionMonitor "Unix process subject does not have uid set"); goto out; } - ret = polkit_unix_user_new (uid); + local_error = NULL; + current_uid = polkit_unix_process_get_racy_uid__ (POLKIT_UNIX_PROCESS (subject), &local_error); + if (local_error != NULL) + { + g_propagate_error (error, local_error); + goto out; + } + ret = polkit_unix_user_new (subject_uid); + matches = (subject_uid == current_uid); } else if (POLKIT_IS_SYSTEM_BUS_NAME (subject)) { ret = (PolkitIdentity*)polkit_system_bus_name_get_user_sync (POLKIT_SYSTEM_BUS_NAME (subject), NULL, error); + matches = TRUE; } else if (POLKIT_IS_UNIX_SESSION (subject)) { + uid_t uid; if (sd_session_get_uid (polkit_unix_session_get_session_id (POLKIT_UNIX_SESSION (subject)), &uid) < 0) { @@ -292,9 +317,14 @@ polkit_backend_session_monitor_get_user_for_subject (PolkitBackendSessionMonitor } ret = polkit_unix_user_new (uid); + matches = TRUE; } out: + if (result_matches != NULL) + { + *result_matches = matches; + } return ret; } diff --git a/src/polkitbackend/polkitbackendsessionmonitor.c b/src/polkitbackend/polkitbackendsessionmonitor.c index e1a9ab3..ed30755 100644 --- a/src/polkitbackend/polkitbackendsessionmonitor.c +++ b/src/polkitbackend/polkitbackendsessionmonitor.c @@ -27,6 +27,7 @@ #include <glib/gstdio.h> #include <polkit/polkit.h> +#include <polkit/polkitprivate.h> #include "polkitbackendsessionmonitor.h" #define CKDB_PATH "/var/run/ConsoleKit/database" @@ -273,28 +274,40 @@ polkit_backend_session_monitor_get_sessions (PolkitBackendSessionMonitor *monito * polkit_backend_session_monitor_get_user: * @monitor: A #PolkitBackendSessionMonitor. * @subject: A #PolkitSubject. + * @result_matches: If not %NULL, set to indicate whether the return value matches current (RACY) state. * @error: Return location for error. * * Gets the user corresponding to @subject or %NULL if no user exists. * + * NOTE: For a #PolkitUnixProcess, the UID is read from @subject (which may + * come from e.g. a D-Bus client), so it may not correspond to the actual UID + * of the referenced process (at any point in time). This is indicated by + * setting @result_matches to %FALSE; the caller may reject such subjects or + * require additional privileges. @result_matches == %TRUE only indicates that + * the UID matched the underlying process at ONE point in time, it may not match + * later. + * * Returns: %NULL if @error is set otherwise a #PolkitUnixUser that should be freed with g_object_unref(). */ PolkitIdentity * polkit_backend_session_monitor_get_user_for_subject (PolkitBackendSessionMonitor *monitor, PolkitSubject *subject, + gboolean *result_matches, GError **error) { PolkitIdentity *ret; + gboolean matches; GError *local_error; - gchar *group; - guint32 uid; ret = NULL; + matches = FALSE; if (POLKIT_IS_UNIX_PROCESS (subject)) { - uid = polkit_unix_process_get_uid (POLKIT_UNIX_PROCESS (subject)); - if ((gint) uid == -1) + gint subject_uid, current_uid; + + subject_uid = polkit_unix_process_get_uid (POLKIT_UNIX_PROCESS (subject)); + if (subject_uid == -1) { g_set_error (error, POLKIT_ERROR, @@ -302,14 +315,26 @@ polkit_backend_session_monitor_get_user_for_subject (PolkitBackendSessionMonitor "Unix process subject does not have uid set"); goto out; } - ret = polkit_unix_user_new (uid); + local_error = NULL; + current_uid = polkit_unix_process_get_racy_uid__ (POLKIT_UNIX_PROCESS (subject), &local_error); + if (local_error != NULL) + { + g_propagate_error (error, local_error); + goto out; + } + ret = polkit_unix_user_new (subject_uid); + matches = (subject_uid == current_uid); } else if (POLKIT_IS_SYSTEM_BUS_NAME (subject)) { ret = (PolkitIdentity*)polkit_system_bus_name_get_user_sync (POLKIT_SYSTEM_BUS_NAME (subject), NULL, error); + matches = TRUE; } else if (POLKIT_IS_UNIX_SESSION (subject)) { + gint uid; + gchar *group; + if (!ensure_database (monitor, error)) { g_prefix_error (error, "Error getting user for session: Error ensuring CK database at " CKDB_PATH ": "); @@ -328,9 +353,14 @@ polkit_backend_session_monitor_get_user_for_subject (PolkitBackendSessionMonitor g_free (group); ret = polkit_unix_user_new (uid); + matches = TRUE; } out: + if (result_matches != NULL) + { + *result_matches = matches; + } return ret; } diff --git a/src/polkitbackend/polkitbackendsessionmonitor.h b/src/polkitbackend/polkitbackendsessionmonitor.h index 8f8a2ca..3972326 100644 --- a/src/polkitbackend/polkitbackendsessionmonitor.h +++ b/src/polkitbackend/polkitbackendsessionmonitor.h @@ -47,6 +47,7 @@ GList *polkit_backend_session_monitor_get_sessions (Polkit PolkitIdentity *polkit_backend_session_monitor_get_user_for_subject (PolkitBackendSessionMonitor *monitor, PolkitSubject *subject, + gboolean *result_matches, GError **error); PolkitSubject *polkit_backend_session_monitor_get_session_for_subject (PolkitBackendSessionMonitor *monitor, -- 2.17.1
Locations
Projects
Search
Status Monitor
Help
OpenBuildService.org
Documentation
API Documentation
Code of Conduct
Contact
Support
@OBShq
Terms
openSUSE Build Service is sponsored by
The Open Build Service is an
openSUSE project
.
Sign Up
Log In
Places
Places
All Projects
Status Monitor
