Re: [patch] Fix for BZ #18043 buffer-overflow (read past the end) in wordexp/parse_dollars/parse_param

["Carlos O'Donell" <carlos at redhat dot com>]

On 03/05/2015 11:33 PM, Paul Pluzhnikov wrote:
> On Thu, Mar 5, 2015 at 11:48 AM, Carlos O'Donell <carlos@redhat.com> wrote:
> 
>> > OK to commit as long as you verified that test case fails before
>> > and passes afterwards on at least x86_64.
> It doesn't. To make it fail I would have to mmap two pages, mprotect
> the second, and place the string at the end of the first page... which
> is quite a bit of code.
> 
> Must I do that? I guess if we hope to catch any regression here, I must...

Yes, we must. It's painful, but it's one of the only ways we accelerate
the development pace of the project and keep the quality high. We have to
trust that our testing is getting better and covering more of the cases
we catch. Eventually maybe we'll generalize your test and run all of the
cases like this with the string ending on a page boundary.

> Attached patch is verified to fail posix/wordexp-test with expected
> SIGSEGV due to overflow before the fix, and pass after.
 
Excellent.
 
> 2015-03-05  Paul Pluzhnikov  <ppluzhnikov@google.com>
> 
>         [BZ #18043]
>         * posix/wordexp.c (parse_param): Fix buffer overflow.
>         * posix/wordexp-test.c (do_bz18043): Add test case.

Perfect. Please check it in.

> -- Paul Pluzhnikov
> 
> 
> bz18043.patch2.txt
> 
> 
> diff --git a/posix/wordexp-test.c b/posix/wordexp-test.c
> index 8a312e0..c928438 100644
> --- a/posix/wordexp-test.c
> +++ b/posix/wordexp-test.c
> @@ -17,6 +17,7 @@
>  
>  #include <sys/stat.h>
>  #include <sys/types.h>
> +#include <sys/mman.h>
>  #include <fcntl.h>
>  #include <unistd.h>
>  #include <pwd.h>
> @@ -249,6 +250,31 @@ command_line_test (const char *words)
>      printf ("we_wordv[%d] = \"%s\"\n", i, we.we_wordv[i]);
>  }
>  
> +static int
> +do_bz18043 (void)
> +{
> +  const int pagesize = getpagesize ();
> +  char *start = mmap (0, 2 * pagesize, PROT_READ|PROT_WRITE,
> +		      MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
> +
> +  if (start == MAP_FAILED)
> +    return 1;
> +
> +  if (mprotect (start + pagesize, pagesize, PROT_NONE))
> +    return 2;
> +
> +  const char word[] = "${";
> +  char *word_start = start + pagesize - sizeof (word);
> +  memcpy (word_start, word, sizeof (word));
> +
> +  wordexp_t w;
> +  wordexp (word_start, &w, 0);
> +
> +  munmap (start, 2 * pagesize);
> +
> +  return 0;
> +}
> +
>  int
>  main (int argc, char *argv[])
>  {
> @@ -370,6 +396,9 @@ main (int argc, char *argv[])
>  
>    printf ("tests failed: %d\n", fail);
>  
> +  if (do_bz18043 ())
> +    ++fail;
> +
>    return fail != 0;
>  }
>  
> diff --git a/posix/wordexp.c b/posix/wordexp.c
> index e3d8d6b..1c14401 100644
> --- a/posix/wordexp.c
> +++ b/posix/wordexp.c
> @@ -1299,7 +1299,7 @@ parse_param (char **word, size_t *word_length, size_t *max_length,
>  	}
>        while (isdigit(words[++*offset]));
>      }
> -  else if (strchr ("*@$", words[*offset]) != NULL)
> +  else if (words[*offset] != '\0' && strchr ("*@$", words[*offset]) != NULL)
>      {
>        /* Special parameter. */
>        special = 1;