Commit c898fdf4 authored by Colin Walters's avatar Colin Walters

Merge branch 'uid-compare' into 'master'

backend: Compare PolkitUnixProcess uids for temporary authorizations

Closes #75

See merge request !19
parents 5230646d 6cc6aafe
......@@ -99,6 +99,8 @@ polkit_subject_hash (PolkitSubject *subject)
* @b: A #PolkitSubject.
*
* Checks if @a and @b are equal, ie. represent the same subject.
* However, avoid calling polkit_subject_equal() to compare two processes;
* for more information see the `PolkitUnixProcess` documentation.
*
* This function can be used in e.g. g_hash_table_new().
*
......
......@@ -51,7 +51,10 @@
* @title: PolkitUnixProcess
* @short_description: Unix processs
*
* An object for representing a UNIX process.
* An object for representing a UNIX process. NOTE: This object as
* designed is now known broken; a mechanism to exploit a delay in
* start time in the Linux kernel was identified. Avoid
* calling polkit_subject_equal() to compare two processes.
*
* To uniquely identify processes, both the process id and the start
* time of the process (a monotonic increasing value representing the
......@@ -66,6 +69,72 @@
* polkit_unix_process_new_for_owner() with trusted data.
*/
/* See https://gitlab.freedesktop.org/polkit/polkit/issues/75
But quoting the original email in full here to ensure it's preserved:
From: Jann Horn <jannh@google.com>
Subject: [SECURITY] polkit: temporary auth hijacking via PID reuse and non-atomic fork
Date: Wednesday, October 10, 2018 5:34 PM
When a (non-root) user attempts to e.g. control systemd units in the system
instance from an active session over DBus, the access is gated by a polkit
policy that requires "auth_admin_keep" auth. This results in an auth prompt
being shown to the user, asking the user to confirm the action by entering the
password of an administrator account.
After the action has been confirmed, the auth decision for "auth_admin_keep" is
cached for up to five minutes. Subject to some restrictions, similar actions can
then be performed in this timespan without requiring re-auth:
- The PID of the DBus client requesting the new action must match the PID of
the DBus client requesting the old action (based on SO_PEERCRED information
forwarded by the DBus daemon).
- The "start time" of the client's PID (as seen in /proc/$pid/stat, field 22)
must not have changed. The granularity of this timestamp is in the
millisecond range.
- polkit polls every two seconds whether a process with the expected start time
still exists. If not, the temporary auth entry is purged.
Without the start time check, this would obviously be buggy because an attacker
could simply wait for the legitimate client to disappear, then create a new
client with the same PID.
Unfortunately, the start time check is bypassable because fork() is not atomic.
Looking at the source code of copy_process() in the kernel:
p->start_time = ktime_get_ns();
p->real_start_time = ktime_get_boot_ns();
[...]
retval = copy_thread_tls(clone_flags, stack_start, stack_size, p, tls);
if (retval)
goto bad_fork_cleanup_io;
if (pid != &init_struct_pid) {
pid = alloc_pid(p->nsproxy->pid_ns_for_children);
if (IS_ERR(pid)) {
retval = PTR_ERR(pid);
goto bad_fork_cleanup_thread;
}
}
The ktime_get_boot_ns() call is where the "start time" of the process is
recorded. The alloc_pid() call is where a free PID is allocated. In between
these, some time passes; and because the copy_thread_tls() call between them can
access userspace memory when sys_clone() is invoked through the 32-bit syscall
entry point, an attacker can even stall the kernel arbitrarily long at this
point (by supplying a pointer into userspace memory that is associated with a
userfaultfd or is backed by a custom FUSE filesystem).
This means that an attacker can immediately call sys_clone() when the victim
process is created, often resulting in a process that has the exact same start
time reported in procfs; and then the attacker can delay the alloc_pid() call
until after the victim process has died and the PID assignment has cycled
around. This results in an attacker process that polkit can't distinguish from
the victim process.
*/
/**
* PolkitUnixProcess:
*
......
......@@ -3031,6 +3031,43 @@ temporary_authorization_store_free (TemporaryAuthorizationStore *store)
g_free (store);
}
/* See the comment at the top of polkitunixprocess.c */
static gboolean
subject_equal_for_authz (PolkitSubject *a,
PolkitSubject *b)
{
if (!polkit_subject_equal (a, b))
return FALSE;
/* Now special case unix processes, as we want to protect against
* pid reuse by including the UID.
*/
if (POLKIT_IS_UNIX_PROCESS (a) && POLKIT_IS_UNIX_PROCESS (b)) {
PolkitUnixProcess *ap = (PolkitUnixProcess*)a;
int uid_a = polkit_unix_process_get_uid ((PolkitUnixProcess*)a);
PolkitUnixProcess *bp = (PolkitUnixProcess*)b;
int uid_b = polkit_unix_process_get_uid ((PolkitUnixProcess*)b);
if (uid_a != -1 && uid_b != -1)
{
if (uid_a == uid_b)
{
return TRUE;
}
else
{
g_printerr ("denying slowfork; pid %d uid %d != %d!\n",
polkit_unix_process_get_pid (ap),
uid_a, uid_b);
return FALSE;
}
}
/* Fall through; one of the uids is unset so we can't reliably compare */
}
return TRUE;
}
static gboolean
temporary_authorization_store_has_authorization (TemporaryAuthorizationStore *store,
PolkitSubject *subject,
......@@ -3073,7 +3110,7 @@ temporary_authorization_store_has_authorization (TemporaryAuthorizationStore *st
TemporaryAuthorization *authorization = l->data;
if (strcmp (action_id, authorization->action_id) == 0 &&
polkit_subject_equal (subject_to_use, authorization->subject))
subject_equal_for_authz (subject_to_use, authorization->subject))
{
ret = TRUE;
if (out_tmp_authz_id != NULL)
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment