Re: ptx bug -- unbounded buffer overflow

[Dawson Engler]

Cristi,

were there any statements in ptx you were having a hard time hitting that it
might make sense to ask Jim about?

> 
> 
>   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
> 
> 
> 
> 
>