| Message ID | 1439153945-22973-1-git-send-email-alexinbeijing@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Sun, Aug 09, 2015 at 10:59:05PM +0200, Alex Dowad wrote: > C programs which use uninitialized stack variables can be exploited if an attacker > can control the contents of memory where the buggy function's stack frame lands. > If the buggy function is called very early in the program's execution, that memory > might still hold values written by ld.so, so manipulation of ld.so is one way to > carry out such an exploit. > > But how can an unprivileged user, running a set-UID program, cause ld.so to write > arbitrary data onto the stack? One way is to assign it to LD_DEBUG. When printing the > resulting warning message, ld.so uses alloca to create a buffer on the stack, and > copies the entire contents of LD_DEBUG into it (even if it is dozens of kilobytes long). > > Of course, people shouldn't write programs which use uninitialized variables, but it > is easy enough to plug this hole if they do -- zero out the buffer after printing the > warning message. Call it the "pants and suspenders" approach. > --- > > Dear GLibc devs, > > Does this look like something you might be interested in merging? I've never contributed > before, and haven't filled in any copyright attribution papers, but am happy to do so > if necessary. > > I discovered this issue while playing through a hacking "wargame" where players must > develop exploits against programs with deliberately planted security vulnerabilities. > It helped me "beat" the game by achieving a privilege escalation. > > Regards, > Alex Dowad > > elf/rtld.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/elf/rtld.c b/elf/rtld.c > index 6dcbabc..ee194a6 100644 > --- a/elf/rtld.c > +++ b/elf/rtld.c > @@ -2408,6 +2408,8 @@ process_dl_debug (const char *dl_debug) > char *copy = strndupa (dl_debug, len); > _dl_error_printf ("\ > warning: debug option `%s' unknown; try LD_DEBUG=help\n", copy); > + /* Don't let an attacker inject arbitrary data into the stack area */ > + __builtin_memset(copy, 0, len); > } > > dl_debug += len; This memset will be optimized out by any decent compiler. Some mechanism to prevent that is needed. BTW are you sure you haven't uncovered a much more serious bug? Unbounded alloca allows the clobbering of arbitrary memory. While it's not entirely unbounded, the environment/argv size limit was removed on modern Linux, so it's quite possible to have multi-MB or even GB strings there. It's possible/likely that we got lucky and get an unconditional crash at a point where there's only one thread and no signal handlers, but I still think this should be checked and the bogus alloca removed. Rich
Alex Dowad <alexinbeijing@gmail.com> writes: > diff --git a/elf/rtld.c b/elf/rtld.c > index 6dcbabc..ee194a6 100644 > --- a/elf/rtld.c > +++ b/elf/rtld.c > @@ -2408,6 +2408,8 @@ process_dl_debug (const char *dl_debug) > char *copy = strndupa (dl_debug, len); > _dl_error_printf ("\ > warning: debug option `%s' unknown; try LD_DEBUG=help\n", copy); Use %.*s instead. Andreas.
On 08/09/2015 06:43 PM, Rich Felker wrote: >> warning: debug option `%s' unknown; try LD_DEBUG=help\n", copy); >> + /* Don't let an attacker inject arbitrary data into the stack area */ >> + __builtin_memset(copy, 0, len); >> } > This memset will be optimized out by any decent compiler. Some > mechanism to prevent that is needed. Funny you should mention that ... (see the other message I just posted) > BTW are you sure you haven't uncovered a much more serious bug? > Unbounded alloca allows the clobbering of arbitrary memory. While it's > not entirely unbounded, the environment/argv size limit was removed on > modern Linux, so it's quite possible to have multi-MB or even GB > strings there. It's possible/likely that we got lucky and get an > unconditional crash at a point where there's only one thread and no > signal handlers, but I still think this should be checked and the > bogus alloca removed. Agree, but note that this might be happening so early that malloc isn't available (I haven't checked); is it really necessary to copy the string at all? zw
On Mon, Aug 10, 2015 at 12:43 AM, Rich Felker <dalias@libc.org> wrote: > On Sun, Aug 09, 2015 at 10:59:05PM +0200, Alex Dowad wrote: >> C programs which use uninitialized stack variables can be exploited if an attacker >> can control the contents of memory where the buggy function's stack frame lands. >> If the buggy function is called very early in the program's execution, that memory >> might still hold values written by ld.so, so manipulation of ld.so is one way to >> carry out such an exploit. >> >> But how can an unprivileged user, running a set-UID program, cause ld.so to write >> arbitrary data onto the stack? One way is to assign it to LD_DEBUG. When printing the >> resulting warning message, ld.so uses alloca to create a buffer on the stack, and >> copies the entire contents of LD_DEBUG into it (even if it is dozens of kilobytes long). >> >> Of course, people shouldn't write programs which use uninitialized variables, but it >> is easy enough to plug this hole if they do -- zero out the buffer after printing the >> warning message. Call it the "pants and suspenders" approach. >> --- >> >> Dear GLibc devs, >> >> Does this look like something you might be interested in merging? I've never contributed >> before, and haven't filled in any copyright attribution papers, but am happy to do so >> if necessary. >> >> I discovered this issue while playing through a hacking "wargame" where players must >> develop exploits against programs with deliberately planted security vulnerabilities. >> It helped me "beat" the game by achieving a privilege escalation. >> >> Regards, >> Alex Dowad >> >> elf/rtld.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/elf/rtld.c b/elf/rtld.c >> index 6dcbabc..ee194a6 100644 >> --- a/elf/rtld.c >> +++ b/elf/rtld.c >> @@ -2408,6 +2408,8 @@ process_dl_debug (const char *dl_debug) >> char *copy = strndupa (dl_debug, len); >> _dl_error_printf ("\ >> warning: debug option `%s' unknown; try LD_DEBUG=help\n", copy); >> + /* Don't let an attacker inject arbitrary data into the stack area */ >> + __builtin_memset(copy, 0, len); >> } >> >> dl_debug += len; > > This memset will be optimized out by any decent compiler. Some > mechanism to prevent that is needed. > > BTW are you sure you haven't uncovered a much more serious bug? > Unbounded alloca allows the clobbering of arbitrary memory. While it's > not entirely unbounded, the environment/argv size limit was removed on > modern Linux, so it's quite possible to have multi-MB or even GB > strings there. It's possible/likely that we got lucky and get an > unconditional crash at a point where there's only one thread and no > signal handlers, but I still think this should be checked and the > bogus alloca removed. Hmm. I can't see any reason why this code needs to copy the string at all. It seems like it should be able to just print it from where it originally resides in the env var array. Is there something I'm missing? (CC'ing U. Drepper here since he originally wrote this code...)
On Mon, Aug 10, 2015 at 1:01 AM, Andreas Schwab <schwab@linux-m68k.org> wrote: > Alex Dowad <alexinbeijing@gmail.com> writes: > >> diff --git a/elf/rtld.c b/elf/rtld.c >> index 6dcbabc..ee194a6 100644 >> --- a/elf/rtld.c >> +++ b/elf/rtld.c >> @@ -2408,6 +2408,8 @@ process_dl_debug (const char *dl_debug) >> char *copy = strndupa (dl_debug, len); >> _dl_error_printf ("\ >> warning: debug option `%s' unknown; try LD_DEBUG=help\n", copy); > > Use %.*s instead. Thanks for your reply. That would help to avoid potentially voluminous output to the console, but doesn't fix the (potential) security hole of copying an arbitrary, attacker-supplied string onto the stack. Do you think there is any reason to copy the string at all? It seems like it should be possible to just print it from whereever it originally happened to be in memory. Thanks, Alex
Alex <alexinbeijing@gmail.com> writes: > On Mon, Aug 10, 2015 at 1:01 AM, Andreas Schwab <schwab@linux-m68k.org> wrote: >> Alex Dowad <alexinbeijing@gmail.com> writes: >> >>> diff --git a/elf/rtld.c b/elf/rtld.c >>> index 6dcbabc..ee194a6 100644 >>> --- a/elf/rtld.c >>> +++ b/elf/rtld.c >>> @@ -2408,6 +2408,8 @@ process_dl_debug (const char *dl_debug) >>> char *copy = strndupa (dl_debug, len); >>> _dl_error_printf ("\ >>> warning: debug option `%s' unknown; try LD_DEBUG=help\n", copy); >> >> Use %.*s instead. > > Thanks for your reply. That would help to avoid potentially voluminous > output to the console, but doesn't fix the (potential) security hole > of copying an arbitrary, attacker-supplied string onto the stack. You don't need the copy any more. Andreas.
On Mon, Aug 10, 2015 at 10:49 AM, Andreas Schwab <schwab@suse.de> wrote: > Alex <alexinbeijing@gmail.com> writes: > >> On Mon, Aug 10, 2015 at 1:01 AM, Andreas Schwab <schwab@linux-m68k.org> wrote: >>> Alex Dowad <alexinbeijing@gmail.com> writes: >>> >>>> diff --git a/elf/rtld.c b/elf/rtld.c >>>> index 6dcbabc..ee194a6 100644 >>>> --- a/elf/rtld.c >>>> +++ b/elf/rtld.c >>>> @@ -2408,6 +2408,8 @@ process_dl_debug (const char *dl_debug) >>>> char *copy = strndupa (dl_debug, len); >>>> _dl_error_printf ("\ >>>> warning: debug option `%s' unknown; try LD_DEBUG=help\n", copy); >>> >>> Use %.*s instead. >> >> Thanks for your reply. That would help to avoid potentially voluminous >> output to the console, but doesn't fix the (potential) security hole >> of copying an arbitrary, attacker-supplied string onto the stack. > > You don't need the copy any more. Andreas, I'm a bit slow here so please help me out: why is the copy needed *even if* printf("%s", ...) is used? I've been trying to figure out why the original author used strndupa in the first place but haven't wrapped my mind around it yet.
Alex wrote: > I've been trying to figure > out why the original author used strndupa in the first place but > haven't wrapped my mind around it yet. Presumably the original author didn't know about "%.*s". The string is not null-terminated, so "%s" won't work, and I guess the original author used strndupa to create a null-terminated copy. Beware of int overflow when using "%.*s", by the way.
Alex <alexinbeijing@gmail.com> writes: > Andreas, I'm a bit slow here so please help me out: why is the copy > needed *even if* printf("%s", ...) is used? Not "even if", but "because". You need to cut off the string before the separator. Andreas.
Paul Eggert <eggert@cs.ucla.edu> writes:
> Presumably the original author didn't know about "%.*s".
It didn't exist back then.
Andreas.
On Mon, Aug 10, 2015 at 11:03 AM, Paul Eggert <eggert@cs.ucla.edu> wrote: > Alex wrote: >> >> I've been trying to figure >> out why the original author used strndupa in the first place but >> haven't wrapped my mind around it yet. > > Presumably the original author didn't know about "%.*s". The string is not > null-terminated, so "%s" won't work, and I guess the original author used > strndupa to create a null-terminated copy. > > Beware of int overflow when using "%.*s", by the way. Thanks to Paul Eggert and Andreas Schwab for your helpful feedback! It turns out that implementing the recommended fix will require a small tweak in _dl_debug_vdprintf. I will send a v2 patch after testing. AD
On Mon, Aug 10, 2015 at 11:15:36AM +0200, Andreas Schwab wrote: > Paul Eggert <eggert@cs.ucla.edu> writes: > > > Presumably the original author didn't know about "%.*s". > > It didn't exist back then. This is factually incorrect. C89 4.9.6.1 paragraph 3 specifies the * form for width and precision. printf has 'always' had this feature, or at least since long before there was a GNU dynamic linker. Rich
Rich Felker <dalias@libc.org> writes: > On Mon, Aug 10, 2015 at 11:15:36AM +0200, Andreas Schwab wrote: >> Paul Eggert <eggert@cs.ucla.edu> writes: >> >> > Presumably the original author didn't know about "%.*s". >> >> It didn't exist back then. > > This is factually incorrect. Read the history before making such a bold statement. > C89 _dl_error_printf isn't C89. Andreas.
On Thu, Aug 13, 2015 at 09:56:42AM +0200, Andreas Schwab wrote: > Rich Felker <dalias@libc.org> writes: > > > On Mon, Aug 10, 2015 at 11:15:36AM +0200, Andreas Schwab wrote: > >> Paul Eggert <eggert@cs.ucla.edu> writes: > >> > >> > Presumably the original author didn't know about "%.*s". > >> > >> It didn't exist back then. > > > > This is factually incorrect. > > Read the history before making such a bold statement. > > > C89 > > _dl_error_printf isn't C89. OK, I misunderstood the code. Apologies. Rich
diff --git a/elf/rtld.c b/elf/rtld.c index 6dcbabc..ee194a6 100644 --- a/elf/rtld.c +++ b/elf/rtld.c @@ -2408,6 +2408,8 @@ process_dl_debug (const char *dl_debug) char *copy = strndupa (dl_debug, len); _dl_error_printf ("\ warning: debug option `%s' unknown; try LD_DEBUG=help\n", copy); + /* Don't let an attacker inject arbitrary data into the stack area */ + __builtin_memset(copy, 0, len); } dl_debug += len;