Skip to content
Snippets Groups Projects
Commit 5ed4ed8a authored by Jeremy Bícha's avatar Jeremy Bícha
Browse files

Compare PolkitUnixProcess uids for temporary authorizations

Fix temporary auth hijacking via PID reuse and non-atomic fork

CVE-2019-6133
Closes: #918985

Gbp-Dch: Full
parent e5f16ae0
No related branches found
No related tags found
No related merge requests found
From: Colin Walters <walters@verbum.org>
Date: Fri, 4 Jan 2019 14:24:48 -0500
Subject: 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
(cherry picked from commit 6cc6aafee135ba44ea748250d7d29b562ca190e3)
---
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 78ec745..fadcfe9 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 4a42567..53537fa 100644
--- a/src/polkit/polkitunixprocess.c
+++ b/src/polkit/polkitunixprocess.c
@@ -44,7 +44,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
@@ -59,6 +62,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 97a8d80..1e17dfd 100644
--- a/src/polkitbackend/polkitbackendinteractiveauthority.c
+++ b/src/polkitbackend/polkitbackendinteractiveauthority.c
@@ -2870,6 +2870,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,
@@ -2912,7 +2949,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)
......@@ -47,3 +47,4 @@
06_systemd-service.patch
10_build-against-libsystemd.patch
Move-D-Bus-policy-file-to-usr-share-dbus-1-system.d.patch
0.116/backend-Compare-PolkitUnixProcess-uids-for-temporary-auth.patch
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment