Brad Bishop | cdf4859 | 2019-02-04 08:30:59 -0500 | [diff] [blame] | 1 | From 6cc6aafee135ba44ea748250d7d29b562ca190e3 Mon Sep 17 00:00:00 2001 |
| 2 | From: Colin Walters <walters@verbum.org> |
| 3 | Date: Fri, 4 Jan 2019 14:24:48 -0500 |
| 4 | Subject: [PATCH] backend: Compare PolkitUnixProcess uids for temporary |
| 5 | authorizations |
| 6 | |
| 7 | It turns out that the combination of `(pid, start time)` is not |
| 8 | enough to be unique. For temporary authorizations, we can avoid |
| 9 | separate users racing on pid reuse by simply comparing the uid. |
| 10 | |
| 11 | https://bugs.chromium.org/p/project-zero/issues/detail?id=1692 |
| 12 | |
| 13 | And the above original email report is included in full in a new comment. |
| 14 | |
| 15 | Reported-by: Jann Horn <jannh@google.com> |
| 16 | |
| 17 | Closes: https://gitlab.freedesktop.org/polkit/polkit/issues/75 |
| 18 | |
| 19 | CVE: CVE-2019-6133 |
| 20 | Upstream-Status: Backport [https://gitlab.freedesktop.org/polkit/polkit.git] |
| 21 | |
| 22 | Signed-off-by: Ovidiu Panait <ovidiu.panait@windriver.com> |
| 23 | --- |
| 24 | src/polkit/polkitsubject.c | 2 + |
| 25 | src/polkit/polkitunixprocess.c | 71 ++++++++++++++++++- |
| 26 | .../polkitbackendinteractiveauthority.c | 39 +++++++++- |
| 27 | 3 files changed, 110 insertions(+), 2 deletions(-) |
| 28 | |
| 29 | diff --git a/src/polkit/polkitsubject.c b/src/polkit/polkitsubject.c |
| 30 | index d4c1182..ccabd0a 100644 |
| 31 | --- a/src/polkit/polkitsubject.c |
| 32 | +++ b/src/polkit/polkitsubject.c |
| 33 | @@ -99,6 +99,8 @@ polkit_subject_hash (PolkitSubject *subject) |
| 34 | * @b: A #PolkitSubject. |
| 35 | * |
| 36 | * Checks if @a and @b are equal, ie. represent the same subject. |
| 37 | + * However, avoid calling polkit_subject_equal() to compare two processes; |
| 38 | + * for more information see the `PolkitUnixProcess` documentation. |
| 39 | * |
| 40 | * This function can be used in e.g. g_hash_table_new(). |
| 41 | * |
| 42 | diff --git a/src/polkit/polkitunixprocess.c b/src/polkit/polkitunixprocess.c |
| 43 | index b02b258..78d7251 100644 |
| 44 | --- a/src/polkit/polkitunixprocess.c |
| 45 | +++ b/src/polkit/polkitunixprocess.c |
| 46 | @@ -51,7 +51,10 @@ |
| 47 | * @title: PolkitUnixProcess |
| 48 | * @short_description: Unix processs |
| 49 | * |
| 50 | - * An object for representing a UNIX process. |
| 51 | + * An object for representing a UNIX process. NOTE: This object as |
| 52 | + * designed is now known broken; a mechanism to exploit a delay in |
| 53 | + * start time in the Linux kernel was identified. Avoid |
| 54 | + * calling polkit_subject_equal() to compare two processes. |
| 55 | * |
| 56 | * To uniquely identify processes, both the process id and the start |
| 57 | * time of the process (a monotonic increasing value representing the |
| 58 | @@ -66,6 +69,72 @@ |
| 59 | * polkit_unix_process_new_for_owner() with trusted data. |
| 60 | */ |
| 61 | |
| 62 | +/* See https://gitlab.freedesktop.org/polkit/polkit/issues/75 |
| 63 | + |
| 64 | + But quoting the original email in full here to ensure it's preserved: |
| 65 | + |
| 66 | + From: Jann Horn <jannh@google.com> |
| 67 | + Subject: [SECURITY] polkit: temporary auth hijacking via PID reuse and non-atomic fork |
| 68 | + Date: Wednesday, October 10, 2018 5:34 PM |
| 69 | + |
| 70 | +When a (non-root) user attempts to e.g. control systemd units in the system |
| 71 | +instance from an active session over DBus, the access is gated by a polkit |
| 72 | +policy that requires "auth_admin_keep" auth. This results in an auth prompt |
| 73 | +being shown to the user, asking the user to confirm the action by entering the |
| 74 | +password of an administrator account. |
| 75 | + |
| 76 | +After the action has been confirmed, the auth decision for "auth_admin_keep" is |
| 77 | +cached for up to five minutes. Subject to some restrictions, similar actions can |
| 78 | +then be performed in this timespan without requiring re-auth: |
| 79 | + |
| 80 | + - The PID of the DBus client requesting the new action must match the PID of |
| 81 | + the DBus client requesting the old action (based on SO_PEERCRED information |
| 82 | + forwarded by the DBus daemon). |
| 83 | + - The "start time" of the client's PID (as seen in /proc/$pid/stat, field 22) |
| 84 | + must not have changed. The granularity of this timestamp is in the |
| 85 | + millisecond range. |
| 86 | + - polkit polls every two seconds whether a process with the expected start time |
| 87 | + still exists. If not, the temporary auth entry is purged. |
| 88 | + |
| 89 | +Without the start time check, this would obviously be buggy because an attacker |
| 90 | +could simply wait for the legitimate client to disappear, then create a new |
| 91 | +client with the same PID. |
| 92 | + |
| 93 | +Unfortunately, the start time check is bypassable because fork() is not atomic. |
| 94 | +Looking at the source code of copy_process() in the kernel: |
| 95 | + |
| 96 | + p->start_time = ktime_get_ns(); |
| 97 | + p->real_start_time = ktime_get_boot_ns(); |
| 98 | + [...] |
| 99 | + retval = copy_thread_tls(clone_flags, stack_start, stack_size, p, tls); |
| 100 | + if (retval) |
| 101 | + goto bad_fork_cleanup_io; |
| 102 | + |
| 103 | + if (pid != &init_struct_pid) { |
| 104 | + pid = alloc_pid(p->nsproxy->pid_ns_for_children); |
| 105 | + if (IS_ERR(pid)) { |
| 106 | + retval = PTR_ERR(pid); |
| 107 | + goto bad_fork_cleanup_thread; |
| 108 | + } |
| 109 | + } |
| 110 | + |
| 111 | +The ktime_get_boot_ns() call is where the "start time" of the process is |
| 112 | +recorded. The alloc_pid() call is where a free PID is allocated. In between |
| 113 | +these, some time passes; and because the copy_thread_tls() call between them can |
| 114 | +access userspace memory when sys_clone() is invoked through the 32-bit syscall |
| 115 | +entry point, an attacker can even stall the kernel arbitrarily long at this |
| 116 | +point (by supplying a pointer into userspace memory that is associated with a |
| 117 | +userfaultfd or is backed by a custom FUSE filesystem). |
| 118 | + |
| 119 | +This means that an attacker can immediately call sys_clone() when the victim |
| 120 | +process is created, often resulting in a process that has the exact same start |
| 121 | +time reported in procfs; and then the attacker can delay the alloc_pid() call |
| 122 | +until after the victim process has died and the PID assignment has cycled |
| 123 | +around. This results in an attacker process that polkit can't distinguish from |
| 124 | +the victim process. |
| 125 | +*/ |
| 126 | + |
| 127 | + |
| 128 | /** |
| 129 | * PolkitUnixProcess: |
| 130 | * |
| 131 | diff --git a/src/polkitbackend/polkitbackendinteractiveauthority.c b/src/polkitbackend/polkitbackendinteractiveauthority.c |
| 132 | index a1630b9..80e8141 100644 |
| 133 | --- a/src/polkitbackend/polkitbackendinteractiveauthority.c |
| 134 | +++ b/src/polkitbackend/polkitbackendinteractiveauthority.c |
| 135 | @@ -3031,6 +3031,43 @@ temporary_authorization_store_free (TemporaryAuthorizationStore *store) |
| 136 | g_free (store); |
| 137 | } |
| 138 | |
| 139 | +/* See the comment at the top of polkitunixprocess.c */ |
| 140 | +static gboolean |
| 141 | +subject_equal_for_authz (PolkitSubject *a, |
| 142 | + PolkitSubject *b) |
| 143 | +{ |
| 144 | + if (!polkit_subject_equal (a, b)) |
| 145 | + return FALSE; |
| 146 | + |
| 147 | + /* Now special case unix processes, as we want to protect against |
| 148 | + * pid reuse by including the UID. |
| 149 | + */ |
| 150 | + if (POLKIT_IS_UNIX_PROCESS (a) && POLKIT_IS_UNIX_PROCESS (b)) { |
| 151 | + PolkitUnixProcess *ap = (PolkitUnixProcess*)a; |
| 152 | + int uid_a = polkit_unix_process_get_uid ((PolkitUnixProcess*)a); |
| 153 | + PolkitUnixProcess *bp = (PolkitUnixProcess*)b; |
| 154 | + int uid_b = polkit_unix_process_get_uid ((PolkitUnixProcess*)b); |
| 155 | + |
| 156 | + if (uid_a != -1 && uid_b != -1) |
| 157 | + { |
| 158 | + if (uid_a == uid_b) |
| 159 | + { |
| 160 | + return TRUE; |
| 161 | + } |
| 162 | + else |
| 163 | + { |
| 164 | + g_printerr ("denying slowfork; pid %d uid %d != %d!\n", |
| 165 | + polkit_unix_process_get_pid (ap), |
| 166 | + uid_a, uid_b); |
| 167 | + return FALSE; |
| 168 | + } |
| 169 | + } |
| 170 | + /* Fall through; one of the uids is unset so we can't reliably compare */ |
| 171 | + } |
| 172 | + |
| 173 | + return TRUE; |
| 174 | +} |
| 175 | + |
| 176 | static gboolean |
| 177 | temporary_authorization_store_has_authorization (TemporaryAuthorizationStore *store, |
| 178 | PolkitSubject *subject, |
| 179 | @@ -3073,7 +3110,7 @@ temporary_authorization_store_has_authorization (TemporaryAuthorizationStore *st |
| 180 | TemporaryAuthorization *authorization = l->data; |
| 181 | |
| 182 | if (strcmp (action_id, authorization->action_id) == 0 && |
| 183 | - polkit_subject_equal (subject_to_use, authorization->subject)) |
| 184 | + subject_equal_for_authz (subject_to_use, authorization->subject)) |
| 185 | { |
| 186 | ret = TRUE; |
| 187 | if (out_tmp_authz_id != NULL) |
| 188 | -- |
| 189 | 2.20.1 |
| 190 | |