From 6cc6aafee135ba44ea748250d7d29b562ca190e3 Mon Sep 17 00:00:00 2001

From: Colin Walters <walters@verbum.org>

Date: Fri, 4 Jan 2019 14:24:48 -0500

Subject: [PATCH] backend: Compare PolkitUnixProcess uids for temporary

 authorizations

It turns out that the combination of `(pid, start time)` is not

enough to be unique.  For temporary authorizations, we can avoid

separate users racing on pid reuse by simply comparing the uid.

https://bugs.chromium.org/p/project-zero/issues/detail?id=1692

And the above original email report is included in full in a new comment.

Reported-by: Jann Horn <jannh@google.com>

Closes: https://gitlab.freedesktop.org/polkit/polkit/issues/75

---

 src/polkit/polkitsubject.c                    |  2 +

 src/polkit/polkitunixprocess.c                | 71 ++++++++++++++++++-

 .../polkitbackendinteractiveauthority.c       | 39 +++++++++-

 3 files changed, 110 insertions(+), 2 deletions(-)

diff --git a/src/polkit/polkitsubject.c b/src/polkit/polkitsubject.c

index d4c1182..ccabd0a 100644

--- a/src/polkit/polkitsubject.c

+++ b/src/polkit/polkitsubject.c

@@ -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().

  *

diff --git a/src/polkit/polkitunixprocess.c b/src/polkit/polkitunixprocess.c

index b02b258..78d7251 100644

--- a/src/polkit/polkitunixprocess.c

+++ b/src/polkit/polkitunixprocess.c

@@ -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:

  *

diff --git a/src/polkitbackend/polkitbackendinteractiveauthority.c b/src/polkitbackend/polkitbackendinteractiveauthority.c

index a1630b9..80e8141 100644

--- a/src/polkitbackend/polkitbackendinteractiveauthority.c

+++ b/src/polkitbackend/polkitbackendinteractiveauthority.c

@@ -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)

-- 

2.19.2