Re: ptx bug -- unbounded buffer overflow

[Cristian Cadar]

  Hello Jim,

  Thanks for the prompt answer.
  Could you please acknowledge Daniel Dunbar (address@hidden) and
Dawson Engler (address@hidden) as well?
  
  Our tool works by generating test cases that achieve very high
statement and branch coverage.  At this point, the tool is not mature
enough for distribution, but if you'd like to send you the test suites
that we generate after we finish our study, we can do that.

  We'll send you any more bug reports that our tool generates.  We plan
to focus on coreutils over the next two weeks, so this synchronizes well
with your release deadline.

  Best,
  Cristian

On Fri, 2008-03-21 at 11:11 +0100, Jim Meyering wrote:
> Cristian Cadar <address@hidden> wrote:
> >   Hello, I'm part of a research group at Stanford, working on automatic
> > bug-finding tools.  We are currently testing coreutils, and we found a
> > crash bug in ptx due to an unbounded buffer overflow.
> >
> >   Here is a trivial test case that triggers the bug in the current
> > version of coreutils (6.10):
> >
> > $ ptx -F\\
> >
> >   Another example, which overflows more bytes would be:
> > $ ptx -F\\ abcdef
> >
> > (the overflow increases w/ the length of the second argument).
> >
> >   The problem is in function copy_unescaped_string(const char *string),
> > which in the presence of backslashes can advance the pointer "string"
> > past the end of the buffer.  This in turn causes an unbounded overflow
> > of the buffer malloc-ed at the very beginning of the function, which in
> > turn can be used to corrupt the heap metadata and crash the program.
> 
> Thank you for finding/reporting that.  It is indeed a bug.
> I've included a patch and a test-suite addition below.
> 
> Are your tools freely available?
> 
> If you have any more reports like that, please send them in soon.
> I expect to make a new release in the next week or two.
> 
> Regarding attribution, for now, I've listed only your name
> in the commit log and THANKS file.
> Let me know if something else would be more appropriate.
> 
>       ptx: avoid heap overrun for backslash at end of optarg string
>       * src/ptx.c (copy_unescaped_string): Ignore a lone backslash
>       at end of string.  Reported by Cristian Cadar.
>       * tests/misc/Makefile.am (TESTS): Add ptx-overrun.
>       * tests/misc/ptx-overrun: New file.  Test for the above fix.
>       * NEWS: Mention the fix.
> 
> Signed-off-by: Jim Meyering <address@hidden>
> ---
>  NEWS                   |    5 +++++
>  THANKS                 |    1 +
>  src/ptx.c              |    4 ++++
>  tests/misc/Makefile.am |    3 ++-
>  tests/misc/ptx-overrun |   40 ++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 52 insertions(+), 1 deletions(-)
>  create mode 100755 tests/misc/ptx-overrun
> 
> diff --git a/NEWS b/NEWS
> index 93f1331..3be7db8 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -16,6 +16,11 @@ GNU coreutils NEWS                                    -*- 
> outline -*-
>    when the destination had two or more hard links.  It no longer does that.
>    [bug introduced in coreutils-5.3.0]
> 
> +  "ptx -F'\' long-file-name" would overrun a malloc'd buffer and corrupt
> +  the heap.  That was triggered by a lone backslash (or odd number of them)
> +  at the end of the option argument to --flag-truncation=STRING (-F),
> +  --word-regexp=REGEXP (-W), or --sentence-regexp=REGEXP (-S).
> +
>    "rmdir --ignore-fail-on-non-empty" detects and ignores the failure
>    in more cases when a directory is empty.
> 
> diff --git a/THANKS b/THANKS
> index 186bf5f..b5dc348 100644
> --- a/THANKS
> +++ b/THANKS
> @@ -105,6 +105,7 @@ Colin Plumb                         address@hidden
>  Colin Watson                        address@hidden
>  Collin Rogowski                     address@hidden
>  Cray-Cyber Project                  http://www.cray-cyber.org
> +Cristian Cadar                      address@hidden
>  Cyril Bouthors                      address@hidden
>  Dale Scheetz                        address@hidden
>  Dan Hagerty                         address@hidden
> diff --git a/src/ptx.c b/src/ptx.c
> index dafcbe2..8f7ae95 100644
> --- a/src/ptx.c
> +++ b/src/ptx.c
> @@ -388,6 +388,10 @@ copy_unescaped_string (const char *string)
>             string++;
>             break;
> 
> +         case '\0':          /* lone backslash at end of string */
> +           /* ignore it */
> +           break;
> +
>           default:
>             *cursor++ = '\\';
>             *cursor++ = *string++;
> diff --git a/tests/misc/Makefile.am b/tests/misc/Makefile.am
> index 2be132f..f3ed132 100644
> --- a/tests/misc/Makefile.am
> +++ b/tests/misc/Makefile.am
> @@ -1,6 +1,6 @@
>  # Make miscellaneous coreutils tests.                        -*-Makefile-*-
> 
> -# Copyright (C) 2001-2007 Free Software Foundation, Inc.
> +# Copyright (C) 2001-2008 Free Software Foundation, Inc.
> 
>  # This program is free software: you can redistribute it and/or modify
>  # it under the terms of the GNU General Public License as published by
> @@ -38,6 +38,7 @@ TESTS = \
>    ls-time \
>    ls-misc \
>    date \
> +  ptx-overrun \
>    xstrtol \
>    od \
>    mktemp \
> diff --git a/tests/misc/ptx-overrun b/tests/misc/ptx-overrun
> new file mode 100755
> index 0000000..beadf7f
> --- /dev/null
> +++ b/tests/misc/ptx-overrun
> @@ -0,0 +1,40 @@
> +#!/bin/sh
> +# Trigger a heap-clobbering bug in ptx from coreutils-6.10 and earlier.
> +
> +# Copyright (C) 2008 Free Software Foundation, Inc.
> +
> +# This program is free software: you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation, either version 3 of the License, or
> +# (at your option) any later version.
> +
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +if test "$VERBOSE" = yes; then
> +  set -x
> +  ptx --version
> +fi
> +
> +. $srcdir/../test-lib.sh
> +
> +# Using a long file name makes an abort more likely.
> +# Even with no file name, valgrind detects the buffer overrun.
> +f=01234567890123456789012345678901234567890123456789
> +touch $f empty || framework_failure
> +
> +fail=0
> +
> +# Specifying a regular expression ending in a lone backslash
> +# would cause ptx to write beyond the end of a malloc'd buffer.
> +ptx -F '\'      $f < /dev/null  > out || fail=1
> +ptx -S 'foo\'   $f < /dev/null >> out || fail=1
> +ptx -W 'bar\\\' $f < /dev/null >> out || fail=1
> +compare out empty || fail=1
> +
> +(exit $fail); exit $fail
> --
> 1.5.5.rc0.22.g467c