Bugzilla Bug 65892

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 an attachment (id=40757) [details]
Example program

Created an attachment (id=40758) [details]
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 an attachment (id=41960) [details]
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 an attachment (id=41964) [details]
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 an attachment (id=41970) [details]
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 an attachment (id=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 an attachment (id=51578) [details]
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).