Sign Up
Log In
Log In
or
Sign Up
Places
All Projects
Status Monitor
Collapse sidebar
SUSE:SLE-12-SP1:GA
polkit
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
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(-) Index: polkit-0.113/src/polkit/polkitprivate.h =================================================================== --- polkit-0.113.orig/src/polkit/polkitprivate.h +++ polkit-0.113/src/polkit/polkitprivate.h @@ -44,6 +44,8 @@ GVariant *polkit_action_description_to_g 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); Index: polkit-0.113/src/polkit/polkitunixprocess.c =================================================================== --- polkit-0.113.orig/src/polkit/polkitunixprocess.c +++ polkit-0.113/src/polkit/polkitunixprocess.c @@ -49,6 +49,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. */ /** @@ -83,9 +91,6 @@ static void subject_iface_init (PolkitSu static guint64 get_start_time_for_pid (gint pid, GError **error); -static gint _polkit_unix_process_get_owner (PolkitUnixProcess *process, - GError **error); - #ifdef HAVE_FREEBSD static gboolean get_kinfo_proc (gint pid, struct kinfo_proc *p); #endif @@ -170,7 +175,7 @@ polkit_unix_process_constructed (GObject { 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; @@ -259,6 +264,12 @@ polkit_unix_process_class_init (PolkitUn * 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 @@ -655,9 +666,15 @@ 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; @@ -667,7 +684,9 @@ _polkit_unix_process_get_owner (PolkitUn #else gchar filename[64]; guint n; + GError *local_error; #endif + guint64 start_time; g_return_val_if_fail (POLKIT_IS_UNIX_PROCESS (process), 0); g_return_val_if_fail (error == NULL || *error == NULL, 0); @@ -689,6 +708,7 @@ _polkit_unix_process_get_owner (PolkitUn } result = p.ki_uid; + start_time = (guint64) p.ki_start.tv_sec; #else /* see 'man proc' for layout of the status file @@ -722,17 +742,37 @@ _polkit_unix_process_get_owner (PolkitUn 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); @@ -751,5 +791,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); } Index: polkit-0.113/src/polkitbackend/polkitbackendinteractiveauthority.c =================================================================== --- polkit-0.113.orig/src/polkitbackend/polkitbackendinteractiveauthority.c +++ polkit-0.113/src/polkitbackend/polkitbackendinteractiveauthority.c @@ -572,7 +572,7 @@ log_result (PolkitBackendInteractiveAuth 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); @@ -844,6 +844,7 @@ polkit_backend_interactive_authority_che 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; @@ -889,7 +890,7 @@ polkit_backend_interactive_authority_che action_id); user_of_caller = polkit_backend_session_monitor_get_user_for_subject (priv->session_monitor, - caller, + caller, NULL, &error); if (error != NULL) { @@ -904,7 +905,7 @@ polkit_backend_interactive_authority_che 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) { @@ -934,7 +935,10 @@ polkit_backend_interactive_authority_che * 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 * @@ -942,7 +946,9 @@ polkit_backend_interactive_authority_che * 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)) { @@ -1107,9 +1113,10 @@ check_authorization_sync (PolkitBackendA 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; @@ -2475,6 +2482,7 @@ polkit_backend_interactive_authority_reg PolkitSubject *session_for_caller; PolkitIdentity *user_of_caller; PolkitIdentity *user_of_subject; + gboolean user_of_subject_matches; AuthenticationAgent *agent; gboolean ret; gchar *caller_cmdline; @@ -2527,7 +2535,7 @@ polkit_backend_interactive_authority_reg 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, @@ -2536,7 +2544,7 @@ polkit_backend_interactive_authority_reg "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, @@ -2545,7 +2553,8 @@ polkit_backend_interactive_authority_reg "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)) { @@ -2638,6 +2647,7 @@ polkit_backend_interactive_authority_unr PolkitSubject *session_for_caller; PolkitIdentity *user_of_caller; PolkitIdentity *user_of_subject; + gboolean user_of_subject_matches; AuthenticationAgent *agent; gboolean ret; gchar *scope_str; @@ -2686,7 +2696,7 @@ polkit_backend_interactive_authority_unr 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, @@ -2695,7 +2705,7 @@ polkit_backend_interactive_authority_unr "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, @@ -2704,7 +2714,8 @@ polkit_backend_interactive_authority_unr "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)) { @@ -2814,7 +2825,7 @@ polkit_backend_interactive_authority_aut 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; Index: polkit-0.113/src/polkitbackend/polkitbackendsessionmonitor-systemd.c =================================================================== --- polkit-0.113.orig/src/polkitbackend/polkitbackendsessionmonitor-systemd.c +++ polkit-0.113/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_sessi * 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_ "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_ } ret = polkit_unix_user_new (uid); + matches = TRUE; } out: + if (result_matches != NULL) + { + *result_matches = matches; + } return ret; } Index: polkit-0.113/src/polkitbackend/polkitbackendsessionmonitor.c =================================================================== --- polkit-0.113.orig/src/polkitbackend/polkitbackendsessionmonitor.c +++ polkit-0.113/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_sessi * 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_ "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_ g_free (group); ret = polkit_unix_user_new (uid); + matches = TRUE; } out: + if (result_matches != NULL) + { + *result_matches = matches; + } return ret; } Index: polkit-0.113/src/polkitbackend/polkitbackendsessionmonitor.h =================================================================== --- polkit-0.113.orig/src/polkitbackend/polkitbackendsessionmonitor.h +++ polkit-0.113/src/polkitbackend/polkitbackendsessionmonitor.h @@ -47,6 +47,7 @@ GList *polkit_back 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,
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
