Re: pr buffer overflow

[Jim Meyering]

Cristian Cadar <address@hidden> wrote:
>   Hi Jim, we found a buffer overflow in pr, due to the invalid
> processing of backspaces and tabs.
>   Here is a simple input that our tool generated:

Thank you, yet again.
FYI, here's a one-liner that consistently segfaults for me:

  perl -e 'print "a","\b"x100,"\t"' | pr -e300 > /dev/null

Here's a fix:

        pr -e, with a mix of backspaces and TABs, could corrupt the heap
        * tests/pr/Test.pm: New tests for the above.
        * src/pr.c (char_to_clump): Ensure that "input_position" never
        goes below 0.
        Also, elide any backspace encountered when input_position is 0,
        to be compatible at least with /bin/pr from Solaris 10.
        This bug is present in the original version:
        b25038ce9a234ea0906ddcbd8a0012e917e6c661
        * NEWS [Bug fixes]: Mention this.
        Report and diagnosis by Cristian Cadar, Daniel Dunbar and Dawson Engler
        in http://thread.gmane.org/gmane.comp.gnu.coreutils.bugs/13272

---
 NEWS             |    3 +++
 src/pr.c         |   12 +++++++++++-
 tests/pr/Test.pm |   12 +++++++++++-
 3 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/NEWS b/NEWS
index 3cc7151..d2734aa 100644
--- a/NEWS
+++ b/NEWS
@@ -42,6 +42,9 @@ GNU coreutils NEWS                                    -*- 
outline -*-
   "paste -d'\' file" no longer overruns memory (heap since coreutils-5.1.2,
   stack before then) [bug present in the original version, in 1992]

+  "pr -e" with a mix of backspaces and TABs no longer corrupts the heap
+  [bug present in the original version, in 1992]
+
   "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),
diff --git a/src/pr.c b/src/pr.c
index 77d4c8a..14c9d22 100644
--- a/src/pr.c
+++ b/src/pr.c
@@ -2726,7 +2726,17 @@ char_to_clump (char c)
       *s = c;
     }

-  input_position += width;
+  /* Too many backspaces must put us in position 0 -- never negative.  */
+  if (width < 0 && input_position == 0)
+    {
+      chars = 0;
+      input_position = 0;
+    }
+  else if (width < 0 && input_position <= -width)
+    input_position = 0;
+  else
+    input_position += width;
+
   return chars;
 }

diff --git a/tests/pr/Test.pm b/tests/pr/Test.pm
index 0bbad45..5dea416 100644
--- a/tests/pr/Test.pm
+++ b/tests/pr/Test.pm
@@ -1,6 +1,6 @@
 # -*-perl-*-

-# Copyright (C) 1996, 1997, 1998, 1999, 2000, 2001, 2002, 2004, 2005, 2007
+# Copyright (C) 1996-2002, 2004, 2005, 2007-2008
 # Free Software Foundation, Inc.

 # This program is free software: you can redistribute it and/or modify
@@ -373,6 +373,16 @@ my @tv = (
 # Before coreutils-5.3.1, --pages=1:-1 would be treated like
 # --pages=1:18446744073709551615.
 ['neg-page', '--pages=1:-1', '', '', 1],
+
+# Up to coreutils-6.10, this would cause pr to decrement its
+# internal "input_position" below zero and sometimes segfault.
+['neg-inp-pos1', '-t -e', "\b\b\b\b\b\b\tx\n", "        x\n", 0],
+# NB: while there are 4 backspaces in the input, there are only 3 in the output
+['neg-inp-pos2', '-t -e', "abc\b\b\b\b\tx", "abc\b\b\b        x\n", 0],
+
+# This would clobber so much of the heap, it'd segfault or abort every time.
+['smash-heap', '-t -e300', "a".("\b"x50)."\t", "a\b".(" "x300)."\n", 0],
+['smash-heap8', '-t -e',   "a".("\b"x50)."\t", "a\b".(" "x  8)."\n", 0],
 );
 #']]);

--
1.5.5.68.gd193e