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

2 files changed, 72 insertions, 1 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:
  *