Bug 1418278 - Memory leaks in pkexec.c and pkcheck.c
Summary: Memory leaks in pkexec.c and pkcheck.c
Keywords:
Status: CLOSED WONTFIX
Alias: None
Product: Red Hat Enterprise Linux 6
Classification: Red Hat
Component: polkit
Version: 6.8
Hardware: Unspecified
OS: Unspecified
unspecified
high
Target Milestone: rc
: ---
Assignee: Miloslav Trmač
QA Contact: BaseOS QE Security Team
URL:
Whiteboard:
Depends On:
Blocks: 1418287
TreeView+ depends on / blocked
 
Reported: 2017-02-01 12:43 UTC by Leonard den Ottolander
Modified: 2017-02-02 14:49 UTC (History)
1 user (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
: 1418287 (view as bug list)
Environment:
Last Closed: 2017-02-01 21:12:33 UTC
Target Upstream Version:


Attachments (Terms of Use)
Free opt_user variable memory before duping. (637 bytes, patch)
2017-02-01 12:43 UTC, Leonard den Ottolander
no flags Details | Diff
Disallow multiple specification of parameters to avoid memory leaking. (1.51 KB, patch)
2017-02-01 14:23 UTC, Leonard den Ottolander
no flags Details | Diff

Description Leonard den Ottolander 2017-02-01 12:43:28 UTC
Created attachment 1246642 [details]
Free opt_user variable memory before duping.

In polkit-0.96-11.el6 pkexec.c is a memory leak that can be used to "spray the heap". Compare 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".

This exploitation path is easily fixed by freeing the initialized opt_user before reassigning it, along the lines of attached patch.

Comment 1 Leonard den Ottolander 2017-02-01 13:10:31 UTC
Upstream fixed this by not allowing multiple opt_user specifications at the same location where I inserted the counter test.

      else if (strcmp (argv[n], "--user") == 0 || strcmp (argv[n], "-u") == 0)
        {
          n++;
          if (n >= (guint) argc)
            {
              usage (argc, argv);
              goto out;
            }

          if (opt_user != NULL)
            {
              g_printerr ("--user specified twice\n");
              goto out;
            }
          opt_user = g_strdup (argv[n]);
        }

Comment 2 Leonard den Ottolander 2017-02-01 13:50:55 UTC
Similar issues with pkcheck.c, you might want to fix those too!

https://bugs.freedesktop.org/show_bug.cgi?id=99626

Comment 3 Leonard den Ottolander 2017-02-01 14:23:46 UTC
Created attachment 1246671 [details]
Disallow multiple specification of parameters to avoid memory leaking.

Updated patch to fix both pkcheck.c and pkexec.c.

Comment 4 Miloslav Trmač 2017-02-01 21:12:33 UTC
Thanks for your report.

There is nothing inherently exploitable about the attacker being able to allocate strings in memory _in pkexec_; the described vulnerability was in glibc, this was only a possible way to take advantage of it. That glibc vulnerability has been fixed in RHEL 6, see http://rhn.redhat.com/errata/RHSA-2014-1391.html .

Because these patches cause previously working commands to be rejected, they have a risk of breaking customers’ (admittedly misguided) scripts. On balance, I believe making the change is not worth this risk.

Comment 5 Leonard den Ottolander 2017-02-02 14:14:56 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.

The fact that the attack surface on pkcheck is small 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.

I dispute your WONTFIX.

Comment 6 Miloslav Trmač 2017-02-02 14:20:59 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.

No, it is essential; exploiting pkcheck does not give the attacker any privileges they don’t already have. pkcheck pretty much by definition cannot be vulnerable to the user running it because they run with exactly the same privilege [pkcheck _could_ be vulnerable to other users’ actions, in principle]. If there can’t be a vulnerability, there can’t be an attack vector either.

Comment 7 Leonard den Ottolander 2017-02-02 14:49:10 UTC
By the way, even though you might claim pkcheck might not be vulnerable,

pkexec *is* vulnerable as the article shows.

You should at least fix pkexec!


Note You need to log in before you can comment on or make changes to this bug.