Re: ptx bug -- unbounded buffer overflow

[Jim Meyering]

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