|Summary:||dl: RELRO handling crashes when kernel enforces MPROTECT restrictions|
|Product:||glibc||Reporter:||Pierre Ynard <linkfanel>|
|Component:||dynamic-link||Assignee:||Ulrich Drepper <drepper.fsp>|
|Severity:||normal||CC:||atoth, ireopenit, kees, pageexec, thoger|
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 <email@example.com> Date: Fri Jun 28 21:43:42 2013 +0000 Test for mprotect failure in dl-load.c (bug 12492).