As part of the propolice patch for glibc, a function __guard_setup() is called before the main() function is called. This function has an unwanted side-effect. For example, see the attached test.c program. This program will give the following results when executed on a system with a glibc with propolice patch: bash-2.05b$ gcc ./test.c bash-2.05b$ ./a.out I'm reading something now Hello there! I'm reading something now read() is called before main()! This is an unwanted side-effect of the implementation of the __guard_setup() function. The __guard_setup() function opens '/dev/urandom' and reads some bytes from it to setup a random guard. This is done by calling the open(), read() and close() functions, resulting in the unwanted side-effect. The solution looks simple. Instead of calling open(), read() and close(), the __guard_setup() function should call __open(), __read() and __close(). The attached patch will apply this fix to ssp.c which is part of the propolice patch and contains the __guard_setup() function. Reproducible: Always Steps to Reproduce: 1. Compile and run test.c 2. 3. Actual Results: function read() is called twice Expected Results: called it once
Created attachment 40757 [details] Example program
Created attachment 40758 [details, diff] Proposed patch
I ran a quick check of this on x86, sparc64, and o32 mips. x86 and mips both produced the side-effect of read() getting called first, but sparc64 didn't (and I even compiled it with -fstack-protector). w/ sparc64, main() gets called, then read(). Rather inteeresting.
Could the difference between x86/mips and sparc be between the used glibc's (w/ ERANDOM or w/o)?
This is indeed a good catch. Lets test this patch on some arches before commiting. Peter does the same problem shows up on uClibc environments?
It reads twice on uclibc independently of ssp[-all] enabled or disabled (tested x86 gentoo and my env) ERANDOM is not "ported" to uclibc, so that does not influence the case.
My concern here is that some other non x86 arches (sparc/mips/few others maybe) do not accept __open(), __close(), __read() with the __ prefix for symbol resolution. I can't recall when it's hit me before but it seems as one or two of these arches wanted _ vs __ and another wanted no _ or __ We have to do some testing before we can make this final. As we don't want to break up anybody's system.
I have done testing w/ libssp (it is not in glibc), the behaviour is the same (less the non-ssp case, but this is normal). I have then rebuilt the libssp adding __, and it works correctly now
So x86/ppc are good? Ok that leaves sparc{32,64}/amd64/arm/mips/hppa that we need to get confirmed reports from.
good to go for amd64...
Peter S. Mazinger and myself have been in contact about how this bug also relates to uClibc. In one of our mails to each other we bring up that there are quite a few more libc and syscalls used within the ssp.c The complete list includes. socket(), sendto(), getpid(), kill(), sigdelset(), sigprocmask(), sigfillset(), sigaction(), strcpy(), strncat(), strlen(), strncpy(), gettimeofday(), Erik Scheffers would you mind doing a test run with all of the appropriate libc internal functions on a native glibc host. A Proposed patch#2 would be really appreciated.
Created attachment 41960 [details, diff] Proposed fix for ssp.c. I haven't tested it but i hope it will work without pain, but still needs testing. I don't have time by now to test it further, pleas take care of that asap.
Could we generalize it so that it will also work on uClibc like #ifdef __UCLIBC__ #define somefunc __libc_somefunc #else #define somefunc __somefunc #endif
That will be also a good alternative to fix them, than replacing directly the calls, and just redefining their names. I'm starting another proposed patch ;-) Thanks.
Created attachment 41964 [details, diff] Final patch for uclibc and glibc. Added the new patch with the additions that Peter proposed in his last comment.Seems much more smart than replacing each call and appending the __ prefix, also it makes the fix available for Uclibc. Cheers.
Created attachment 41970 [details, diff] adds open/close/read and corrects uClibc part
The attached patch fails for me. See bug #66553 for an alternative patch, one that patches each line rather than trying the #define trick (I guess that won't work with uclibc). I think care has to be taken with these names; depending on architecture some are already #defined. Some of the proposed targets don't exist - __strcpy, __strncat, __strlen and __strncpy don't seem to. I don't think these need to be #defined on glibc anyway, as there's some "libc_hidden_proto" thingy although I'm not sure what it does :) Similarly sigdelset and sigfillset, although the __ variants of these do exist. sigaction is both a function and a struct name so that #define doesn't work either - needs to be of the form: #define sigaction(a,b,c) __sigaction(a,b,c) Lastly, is there any reason not to fixup write (e.g. #define write __write)?
Created attachment 49704 [details] modified ssp.c, works with glibc ssp.c updated inline with my previous comment for review (glibc part - I don't use uclibc so have no comment on that). Plonked it in sys-libs/glibc/files/2.3.4/ssp.c and changed ebuild to fetch from files/2.3.4 instead of files/2.3.3.
solar: what're your comments on this modified ssp.c? Should I throw it into the next glibc?
I would not merge this code as is. dietlibc/uClibc are not longer needed so this update is quite a tad more complex than need be. Each of the other libcs have unique own versions of these functions.
For info, pappy reckoned that only the __guard_setup needed to change, rather than all references as we have done here. From the point of view of fixing the side-effects of the _guard_setup he's right, however I'm worried that leaving references to public weak symbols in __stack_smash_handler provides a way for attackers to get around ssp. Ideally there would be a way of forcing the linker to resolve the references from an object within the library at link time, rather than leaving them to be resolved until load time. However I don't see any way to tell the linker to fully resolve one object in a library. Symbol visibility in glibc seems to be in quite a mess, frankly. The only other alternative is to change the calls; perhaps the patch Malte provided on bug #66553 will do, although there are some holes in that too, I think. We could fix just the __guard_setup and worry about public weak symbols later, in order to get a resolution of these bugs now if the plan is to make a new glibc release soon. A last note; the ssp.c in dietlibc and uclibc also have the same problems, unless they've changed since I last synced.
Created attachment 51578 [details, diff] Change __guard_setup to use unpublished glibc interfaces so that well-behaved preloading doesn't break. I've discovered that changing open() to __open() etc doesn't solve the problem of overloading per se; i.e. it's possibleto LD_PRELOAD many of the __*() functions since they are also WEAK. My conclusion is that fixing the "use preload to alter the behaviour of ssp" issue (assuming it is a problem - no-one has yet said it isn't), is more involved than I thought, and is a topic for another day. So in the interest of resolving these bugs, the attached patch solves the startup side-effect problems and nothing else. I think this is a sensible approach.
I agree with comment #22 and support that patch.
alright, I've committed comment #22's changes that in the 1.3 patch tarball in glibc-20050125-r1.
Thank you Jeremy. Users from bug #66553 and bug #81364 please test and see if this solves your problems. changing resolution to TEST-REQUEST
*** Bug 81364 has been marked as a duplicate of this bug. ***
FWIW, and for anyone who like me didn't know, the loader ignores LD_LIBRARY_PATH and restricts LD_PRELOAD to suid libraries when an executable is suid. Thus on a well-configured system, an unpriviledged attacker can't use preloading to inject malicious code into a process built from an suid executable (ref. man ld.so). The upshot is that although the stack protector could be thwarted on normal binaries through use of preloading, that's of little use to an attacker as it doesn't give him anything he doesn't already have (i.e. unpriviledged access to the system).
