diff mbox

[v2] Don't allow attackers to inject arbitrary data into stack through LD_DEBUG

Message ID 1439209865-17030-1-git-send-email-alexinbeijing@gmail.com
State New
Headers show

Commit Message

Alex Dowad Aug. 10, 2015, 12:31 p.m. UTC 
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. Simply avoid copying the string onto the
stack. Because the string may not be null-terminated, we need to provide a string
length to _dl_error_printf.
---

Thanks to Paul Eggert, Andreas Schwab, and Rich Felker for feedback on the v1.

 elf/dl-misc.c | 7 ++++---
 elf/rtld.c    | 3 +--
 2 files changed, 5 insertions(+), 5 deletions(-)

Comments

Andreas Schwab Aug. 10, 2015, 1:09 p.m. UTC | #1 
Alex Dowad <alexinbeijing@gmail.com> writes:

> diff --git a/elf/rtld.c b/elf/rtld.c
> index 6dcbabc..e10293a 100644
> --- a/elf/rtld.c
> +++ b/elf/rtld.c
> @@ -2405,9 +2405,8 @@ process_dl_debug (const char *dl_debug)
>  	    {
>  	      /* Display a warning and skip everything until next
>  		 separator.  */
> -	      char *copy = strndupa (dl_debug, len);
>  	      _dl_error_printf ("\
> -warning: debug option `%s' unknown; try LD_DEBUG=help\n", copy);
> +warning: debug option `%.*s' unknown; try LD_DEBUG=help\n", len, dl_debug);

len is size_t, but .* expects int.

Andreas.
Ondřej Bílka Aug. 10, 2015, 9:41 p.m. UTC | #2 
On Mon, Aug 10, 2015 at 03:09:20PM +0200, Andreas Schwab wrote:
> Alex Dowad <alexinbeijing@gmail.com> writes:
> 
> > diff --git a/elf/rtld.c b/elf/rtld.c
> > index 6dcbabc..e10293a 100644
> > --- a/elf/rtld.c
> > +++ b/elf/rtld.c
> > @@ -2405,9 +2405,8 @@ process_dl_debug (const char *dl_debug)
> >  	    {
> >  	      /* Display a warning and skip everything until next
> >  		 separator.  */
> > -	      char *copy = strndupa (dl_debug, len);
> >  	      _dl_error_printf ("\
> > -warning: debug option `%s' unknown; try LD_DEBUG=help\n", copy);
> > +warning: debug option `%.*s' unknown; try LD_DEBUG=help\n", len, dl_debug);
> 
> len is size_t, but .* expects int.
>
would cast suffice as we don't care about printing only part of 1gb+
strings instead entire string?

Otherwise patch looks sensible for me. I would be even stricter and
disallow LD_DEBUG for suid programs. Now it doesn't seem to write
something that could be clasiffied as data leak but its better to be
safe.
diff mbox

Patch

diff --git a/elf/dl-misc.c b/elf/dl-misc.c
index 8fd6710..81b332c 100644
--- a/elf/dl-misc.c
+++ b/elf/dl-misc.c
@@ -143,7 +143,7 @@  _dl_debug_vdprintf (int fd, int tag_p, const char *fmt, va_list arg)
 	      ++fmt;
 	    }
 
-	  /* See whether with comes from a parameter.  Note that no other
+	  /* See whether width comes from a parameter.  Note that no other
 	     way to specify the width is implemented.  */
 	  if (*fmt == '*')
 	    {
@@ -205,9 +205,10 @@  _dl_debug_vdprintf (int fd, int tag_p, const char *fmt, va_list arg)
 	    case 's':
 	      /* Get the string argument.  */
 	      iov[niov].iov_base = va_arg (arg, char *);
-	      iov[niov].iov_len = strlen (iov[niov].iov_base);
 	      if (prec != -1)
-		iov[niov].iov_len = MIN ((size_t) prec, iov[niov].iov_len);
+	        iov[niov].iov_len = strnlen (iov[niov].iov_base, (size_t) prec);
+	      else
+	        iov[niov].iov_len = strlen (iov[niov].iov_base);
 	      ++niov;
 	      break;
 
diff --git a/elf/rtld.c b/elf/rtld.c
index 6dcbabc..e10293a 100644
--- a/elf/rtld.c
+++ b/elf/rtld.c
@@ -2405,9 +2405,8 @@  process_dl_debug (const char *dl_debug)
 	    {
 	      /* Display a warning and skip everything until next
 		 separator.  */
-	      char *copy = strndupa (dl_debug, len);
 	      _dl_error_printf ("\
-warning: debug option `%s' unknown; try LD_DEBUG=help\n", copy);
+warning: debug option `%.*s' unknown; try LD_DEBUG=help\n", len, dl_debug);
 	    }
 
 	  dl_debug += len;