Bug 99626 - Multiple memory leaks in options parsing in pkcheck.c
Summary: Multiple memory leaks in options parsing in pkcheck.c
Status: RESOLVED MOVED
Alias: None
Product: PolicyKit
Classification: Unclassified
Component: libpolkit (show other bugs)
Version: unspecified
Hardware: Other All
: medium major
Assignee: David Zeuthen (not reading bugmail)
QA Contact: David Zeuthen (not reading bugmail)
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-02-01 13:49 UTC by Leonard den Ottolander
Modified: 2018-08-20 21:34 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
Disallow multiple specification of parameters to avoid memory leaking. (1.03 KB, patch)
2017-02-01 14:28 UTC, Leonard den Ottolander
Details | Splinter Review

Description Leonard den Ottolander 2017-02-01 13:49:07 UTC
https://googleprojectzero.blogspot.nl/2014/08/the-poisoned-nul-byte-2014-edition.html "Step 5: Aha! use a command-line argument spray to effect a heap spray and collide the heap into the stack" shows an example of "heap spraying" caused by a memory leak in the options parsing in pkexec.c. This leak has been fixed by disallowing multiple uses of the --user / -u option.

However, the same issue exists in pkcheck.c multiple times:

--action-id / -a can be specified multiple times, causing action_id to be repeatedly initialized with a g_strdup (argv[n]).

--system-bus-name has the same problem, only here initialization is done with polkit_system_bus_name_new (argv[n]).

These leaks can be solved in the same way as in pkexec.c, just disallow multiple specifications of those options.
Comment 1 Leonard den Ottolander 2017-02-01 14:28:18 UTC
Created attachment 129272 [details] [review]
Disallow multiple specification of parameters to avoid memory leaking.

FYI, patch vs. 0.112.
Comment 2 Miloslav Trmac 2017-02-01 21:06:37 UTC
Thanks for the patch.

Note that pkcheck is not set-uid, so there is no privilege escalation; this only allows the user to waste memory under their own account, which they are perfectly capable of doing, and allowed to do, in many other ways.
Comment 3 Leonard den Ottolander 2017-02-01 23:07:27 UTC
pkcheck might not be setuid but it's still a potent attack vector that should be and easily is eliminated. I mean downstream also.
Comment 4 Leonard den Ottolander 2017-02-01 23:14:14 UTC
Quite a way to defend a blatant memory leak by the way ;-D
Comment 5 Miloslav Trmac 2017-02-02 13:33:35 UTC
Oh I am not saying that this code is perfect, something like this patch, or at the very least freeing the old values, should indeed be applied upstream.

I _am_ saying that I can’t see why this should be a vulnerability, _and_ providing my rationale so that any mistake can be pointed out.

> pkcheck might not be setuid but it's still a potent attack vector

An attack on what? There is no privilege escalation, so there pretty much by definition is no attack.

Anyone able to run pkcheck with a hundred parameters is able to run something like
> while :; do a=a$a$a; done
in bash, or consume memory in many other ways in processes running under their UID. What makes pkcheck different? What is being attacked, and what is the attack mechanism?
Comment 6 Miloslav Trmac 2017-02-02 13:36:34 UTC
Also, this is not a long-term memory leak where a single process running over hours accumulates unfreed memory during perfectly normal operation, and then crashes because of that = fails to serve its primary purpose.

pkcheck allocates all that memory, performs a single quick operation, and immediately quits, freeing the memory again. And this can only happen when the user intentionally causes it to allocate memory, so arguably allocating memory is, ridiculous as it may sound, the primary purpose of the operation (unlike a memory leak in a long-running process, which is almost always wanted by noone).
Comment 7 Leonard den Ottolander 2017-02-02 13:59:02 UTC
These memory leaks allow "heap spraying". Heap spraying is an attack vector that gives an adversary a serious edge, because they can essentially initialize large parts of the heap. This makes it easy for a local attacker to leverage an attack.

The author clearly states that in his example exploit he gives himself a break, takes the path of least resistance if you will, by working on a 32 bit system instead of a 64 system (smaller address space), and choosing a more easily exploitable binary so he does not have to add a privilege escalation.

However, the heap spraying is in itself a very potent local attack vector (bad boy with a shell on your system can use the exploit to "work the heap" before launching his actual attack).

The fact that the binary in the example is setuid is orthogonal to the fact that heap spraying is a very serious attack vector.

So even though pkcheck is not setuid, these memory leaks in the option parsing give a local attacker a big tool to try to attempt a privilege escalation.


Memory leaks are always bad, but these are seriously bad because they are attacker controlled.

These are very serious bugs so if my arguments are not convincing please consult one of your security advisors.
Comment 8 Leonard den Ottolander 2017-02-02 13:59:29 UTC
Or for that matter, mail the author of that post. I believe he will agree with me.
Comment 9 Miloslav Trmac 2017-02-02 14:16:45 UTC
> The fact that the binary in the example is setuid is orthogonal to the fact that heap spraying is a very serious attack vector.

Again, what is being attacked?

(And heap spraying is not an attack vector; it is at best a technique to exploit _another_ vulnerability. Without that iconv off-by-one heap spraying pkexec would have been perfectly harmless.)

> So even though pkcheck is not setuid, these memory leaks in the option parsing give a local attacker a big tool to try to attempt a privilege escalation.

The point of the memory manipulation is that there is a process with _escalated privileges_ process which can be manipulated to corrupt its memory and then do what the attacker wants it to. That allows the attacker to perform an operation with those escalated privileges = escalate the attacker’s privileges.

When the target does not have escalated privileges (as is the case with pkcheck), manipulating the target’s memory does not give the user anything they don’t already have, in particular:

> bad boy with a shell on your system can use the exploit to "work the heap" before launching his actual attack

bady boy with a shell on my system can use vim+gcc, or perl, to craft a process with any memory layout they desire. pkcheck does not make _anything_ easier for them, it does not give them _any_ extra privileges; if anything, memory manipulation via pkcheck is way more difficult than any of the direct methods.

---

An attacker able to run an arbitrary command line is already capable to create a process with an arbitrary memory layout; there is no privilege escalation.

An attacker able to exhaust memory/swap in pkcheck is already capable to exhaust memory/swap with any other tool, including pure shell; there is no privilege escalation.

When there is no privilege escalation, there is no vulnerability.
Comment 10 GitLab Migration User 2018-08-20 21:34:01 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/polkit/polkit/issues/4.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.