Bug 12492

Summary: dl: RELRO handling crashes when kernel enforces MPROTECT restrictions
Product: glibc Reporter: Pierre Ynard <linkfanel>
Component: dynamic-linkAssignee: Ulrich Drepper <drepper.fsp>
Status: RESOLVED FIXED    
Severity: normal CC: atoth, ireopenit, kees, pageexec, thoger
Priority: P2 Flags: fweimer: security-
Version: 2.11   
Target Milestone: ---   
Host: Target:
Build: Last reconfirmed:
Attachments: Proposed fix

Description Pierre Ynard 2011-02-15 14:44:32 UTC 
Created attachment 5242 [details]
Proposed fix

See Debian bug http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=611195

When dlopen'ing a library that needs to make the stack executable, the RELRO section is made writable again to modify the __stack_prot variable. However, the return value of the mprotect() call is not checked; so if mprotect() fails, instead of gracefully handling the error, the dynamic loader tries to write to __stack_prot anyway, which results in a segmentation fault. And this mprotect() call *will* fail on PaX kernels that enforce restrictions on it.

The simple fix is to check the return value and simply fail to load the problematic library, instead of crashing the whole process. And it's just good programming practice.
Comment 1 Ulrich Drepper 2011-02-15 19:30:30 UTC 
Just use a supported kernel.
Comment 2 PaX Team 2011-02-15 20:03:46 UTC 
mprotect() can fail on supported kernels (what are they anyway?), so it is simply bad programming practice to not check the return value.
Comment 3 Ulrich Drepper 2011-02-17 05:45:00 UTC 
No, it cannot fail in these situations.
Comment 4 PaX Team 2011-02-17 10:02:59 UTC 
merging and splitting vma structures require memory allocation that can fail.
Comment 5 PaX Team 2011-02-17 10:25:43 UTC 
i almost forgot, you didn't answer my question: what is your definition of a supported kernel?
Comment 6 Ulrich Drepper 2011-02-18 16:40:41 UTC 
Stop reopening the bug.  If you have to redefine what your own OS does then maintain your own version of everything else as well.
Comment 7 ireopenit 2011-02-18 16:53:35 UTC 
Do you have any real reason not to apply the submitted patch? Childish spite doesn't count.
Comment 8 Kees Cook 2011-02-18 17:08:11 UTC 
The stock Linux kernel can fail the mprotect call. The LSM mediates this routine. Even SELinux has hooks to reject certain calls. It seems prudent to check the return code since LSMs are being improved/changed all the time.
Comment 9 PaX Team 2011-02-18 19:43:01 UTC 
since this bug affects any kernel, including those of your own employer, it will stay open until it gets fixed. it's also obvious that failing the second mprotect in the same block of code means you've left ld.so's RELRO region writable and that's a security bug itself so you'd better check its return value as well and ask for a CVE.
Comment 10 Pierre Ynard 2011-02-21 04:05:59 UTC 
First of all, I'm sorry for all of this that I started :( I'm sorry to
use up so much of your time, I imagine that dealing with bug reports and
trivial issues must be boring and sometimes frustrating. I'm just trying
to help by taking care some of the dull janitor job like checking return
values: a patch is all ready and it only needs someone to commit it.

glibc is a great piece of software and I'm sure people look at it as a
coding model, so it would be a shame to leave examples that could look
like bad practice, even if that part of the code is not so important in
itself.

I'd love to see this matter just resolved. If you don't mind. Please
please accept my humble apology along with this patch.
Comment 11 Pierre Ynard 2011-03-02 22:12:09 UTC 
The patch was merged into eglibc (http://www.eglibc.org/archives/commits/msg01710.html). I guess than rather than "just use a supported kernel", I'll have to just use a supported libc :(
Pax Team, any suggestion about what should be done if the second mprotect() fails?
Comment 12 PaX Team 2011-03-02 22:30:32 UTC 
probably the same as with the first mprotect failure.
Comment 13 Pierre Ynard 2011-03-03 02:09:51 UTC 
But it still leaves the RELRO region writable, and I doubt the calling application is going to do anything about it
Comment 14 PaX Team 2011-03-03 08:40:54 UTC 
by default the code after call_lose_errno won't return to the application ;).
Comment 15 Charlie Sheen 2011-03-15 15:54:27 UTC 
__stack_prot |= PROT_READ|PROT_WRITE|PROT_EXEC;

Whoever wrote this, you are rwx-winning. You win at reads, you win at writes and you win at execution. I couldn't have done it better even if I banged a seven gram text-relocating rock.
Comment 16 Joseph Myers 2013-06-28 21:44:14 UTC 
Fixed for 2.18 by:

commit 0432680e8c2ecd832038387f92b462dea75e94cc
Author: Pierre Ynard <linkfanel@yahoo.fr>
Date:   Fri Jun 28 21:43:42 2013 +0000

    Test for mprotect failure in dl-load.c (bug 12492).
Comment 17 Jackie Rosen 2014-02-16 19:30:56 UTC Comment hidden (spam)