CentOS - Feb 2017 - Serious attack vector on pkcheck ignored by Red Hat

On Thu, 2017-02-02 at 10:39 -0800, Gordon Messmer wrote:
> It took me a while to find the patch that you mentioned, which is 
> probably why your bugs are being disregarded.
It is beyond my control where patches are listed in the Red Hat bugzilla
pages. I don't think the Red Hat employee involved should have a hard
time finding it in my report.
> Open a new bug report and focus on this patch, exclusively:
>
https://cgit.freedesktop.org/polkit/commit/src/programs/pkexec.c?id=6c992bc8aefa195a41eaa41c07f46f17de18e25c
> 
> The upstream developer has disallowed multiple --user specifications in 
> order to close a memory leak.
Yes. This seems to be a better solution than the one I came up with
initially and which you mention below, because multiple invocations of
--user are meaningless in this context.
> That memory leak can be used to cause the 
> heap and the stack to run in to each other, and that flaw has previously 
> been combined with bugs in glibc to produce an exploit.  The glibc bug 
> is now fixed, but there is still a risk that collision could be 
> exploitable in combination with other, as yet undiscovered bugs.
Yes. I understand perfectly well how this works, which is why I am so
concerned. And that is exactly why I also want to fix those memory leaks
in pkcheck.c. There might not be a known exploit for pkcheck.c but the
vector ("heap spraying" because of a memory leak) is the same. That
"heap spraying" will make it easier for an attacker to leverage any
attack including a privilege escalation so it is worrisome even if the
binary itself is not setuid.
> If Red 
> Hat is concerned with changing the behavior of pkexec in scripts, then 
> they can still fix the memory leak without otherwise changing the 
> behavior of the program by adding:
> 
> if (opt_user != NULL)
>    {
>      g_free(opt_user);
>    }
That is the initial fix I proposed, but I changed it to use the upstream
fix of not allowing multiple invocations of --user. Multiple invocations
of --user are pointless in this context, so I believe the upstream fix
is just fine. And probably *more* acceptable for "midstream", i.e. Red
Hat. But either will do.

I applied a similar fix to pkcheck.c, because the memory leaks are
identical to the one in pkexec.c, so even though not quite as
exploitable that (very powerful) vector is in that binary too.

The argument that the binary must be setuid to make this worrisome is
bogus. The vector might enable a privilege escalation because (at least
on a 32-bit system) a shell user is able to initialize the entire heap
with data of his liking making it way easier to mount any attack.
> ..instead of the upstream solution of failing on multiple --user 
> specifications.  This will correct the leak and won't break any scripts
> that call --user multiple times.
Scripts shouldn't call --user multiple times. When using the fix with
g_free() only the last specification will be used.

I think proposing a fix different from the one applied upstream (as in
freedesktop.org) might also be considered "convoluting the issue".
> That's it.  Keep your bug report simple.  Focus on the program that 
> presents a security vulnerability due to being SUID root. Offer a 
> solution that doesn't break any existing user applications.  Since the 
> problem has been fixed upstream already, you don't need any bug reports
> with freedesktop.org, just with Red Hat for the polkit-112 package.
The fixes I provided for upstream are for pkcheck.c only (because
pkexec.c has been fixed there). Again, even though not directly
(knowingly) exploitable allowing a shell user to heap spray *any* binary
is bad. Just the fact that I have to argue that fact so extensively is
troubling. Any memory leak should be fixed. A memory leak that allows a
(local) user to entirely control the contents of the heap should be
fixed immediately.

I have no indications the fixes I supplied for pkcheck.c will not be
applied upstream.

Regards,
Leonard.

-- 
mount -t life -o ro /dev/dna /genetic/research

On 02/02/2017 11:46 AM, Leonard den Ottolander wrote:
>
>> That memory leak can be used to cause the
>> heap and the stack to run in to each other, and that flaw has
previously
>> been combined with bugs in glibc to produce an exploit.  The glibc bug
>> is now fixed, but there is still a risk that collision could be
>> exploitable in combination with other, as yet undiscovered bugs.
> Yes. I understand perfectly well how this works, which is why I am so
> concerned.
I apologize if my intent was unclear.  I was providing you with the text 
that you should use in your bug report.  I am not explaining the problem 
to you, I am showing you a clear way to explain the problem in the bug 
report.  You should use the appropriate parts of the text I provided, 
and basically nothing else.
>   And that is exactly why I also want to fix those memory leaks
> in pkcheck.c. There might not be a known exploit for pkcheck.c but the
> vector ("heap spraying" because of a memory leak) is the same.
No.  Stop.  Drop the issue of pkcheck.c entirely.  No one will listen to 
you as long as you continue to refer to that file. Completely stop 
talking about it.

pkcheck.c executes entirely in the security context of the user's own 
session.  It does not have any security rights that the user does not 
have.  The user may be able to cause it to crash or execute code, but it 
cannot execute any code that the user wouldn't have been able to call on 
their own, directly.  It is not a security flaw, because it *doesn't 
change the user's security context.*  Your bugs will continue to be 
closed with WONTFIX until you understand that.

I am honestly trying to help you.  I would like to see the issue with 
pkexec.c fixed.  But you have to listen to this point: causing a program 
to crash isn't a security flaw unless that program operates in a 
different security context than your own.  Crashing your own programs, 
within your own security context, isn't a security flaw.  It's just a 
bug.  Stop chasing the bug, and focus on the potential security flaw in 
psexec.c

On Thu, 2017-02-02 at 12:18 -0800, Gordon Messmer wrote:
> I apologize if my intent was unclear.  I was providing you with the text 
> that you should use in your bug report.  I am not explaining the problem 
> to you, I am showing you a clear way to explain the problem in the bug 
> report.  You should use the appropriate parts of the text I provided, 
> and basically nothing else.
Ok. No offense was taken :-) . But, seeing that the upstream and
midstream developer are the same I think he understands the issue quite
well himself. (That is, apart from the fact that the binary needs not to
be setuid for this to be worrisome ;) .
> No.  Stop.  Drop the issue of pkcheck.c entirely.  No one will listen to 
> you as long as you continue to refer to that file. Completely stop 
> talking about it.
Oops, too late. I did mention it as an FYI in the new reports. But the
subject is about pkexec.c only.

https://bugzilla.redhat.com/show_bug.cgi?id=1418824
https://bugzilla.redhat.com/show_bug.cgi?id=1418825
> pkcheck.c executes entirely in the security context of the user's own 
> session.  It does not have any security rights that the user does not 
> have.
I'll try this one more time. "Heap spraying" in itself is a very
powerful attack vector. It will simplify any attack on any binary that
is affected. Read the article and weep. (The ridiculous value of that
kernel parameter is making matters even worse, but I understand I'll
have to take up that issue elsewhere.) 

We do not need the privilege escalation in the binary. The vector will
make any attack way easier, including a potential privilege escalation.
So by continuing to have these memory leaks in the binary you are making
it easier for a malevolent local user to mount an attack that might
cause the "desired" privilege escalation.

But I agree that to get the more serious issue fixed I should stop
talking about pkcheck.c in those bug reports ;) .

Thanks for your input.

Regards,
Leonard.

-- 
mount -t life -o ro /dev/dna /genetic/research