Re: paste -d\\ crash bug

[Jim Meyering]

Cristian Cadar <address@hidden> wrote:
>   We found a crash bug in paste, due to an unbounded buffer overflow.
> The bug is similar to the ptx bug that we reported earlier, and is due
> to a lone backslash following the -d flag.
>   Here is an input that crashes libc on my machine:
>
> $ paste -d\\ aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
> *** glibc detected *** paste: free(): invalid next size (normal):
> 0x09035888 ***
>
>   The problem seems to be in collapse_escapes() which when given a lone
> backslash, incorrectly advances 'strptr' past the end of the string, and
> continues copying from there, overflowing the 'delims' buffer.
>
>   As usual, we appreciate your confirmation of the bug.

Hi guys,

Thank you, again!

That bug has been there from the beginning (November 1992, when I first
imported the GNU textutils into a CVS repository).

Here's the patch I expect to push:

        paste -d\\: avoid heap overrun for backslash at end of delim list
        * src/paste.c: Include "quotearg.h".
        (collapse_escapes): Handle backslash-escaped backslash explicitly.
        Handle unescaped backslash at end of string by returning nonzero,
        rather than by overrunning memory.
        (main): Diagnose an invalid delimiter list -- carefully.
        Reported by Cristian Cadar, Daniel Dunbar and Dawson Engler.
        * tests/misc/paste-no-nl (delim-bs): Add a test to demonstrate the
        heap smashing capability.
        (delim-bs2): Prior to coreutils-5.1.2, this bug was a little harder
        to demonstrate: it would corrupt a first-argument containing e.g., \b
        * NEWS: Mention the bug fix.

Signed-off-by: Jim Meyering <address@hidden>
---
 NEWS                   |    3 +++
 src/paste.c            |   34 ++++++++++++++++++++++++++++++----
 tests/misc/paste-no-nl |   27 ++++++++++++++++++++++-----
 3 files changed, 55 insertions(+), 9 deletions(-)

diff --git a/NEWS b/NEWS
index 5250ed8..4ac4413 100644
--- a/NEWS
+++ b/NEWS
@@ -18,6 +18,9 @@ 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]

+  "paste -d'\' file" no longer overruns memory (heap since coreutils-5.1.2,
+  stack before then) [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/paste.c b/src/paste.c
index 3cb84cf..20d6953 100644
--- a/src/paste.c
+++ b/src/paste.c
@@ -1,5 +1,5 @@
 /* paste - merge lines of files
-   Copyright (C) 1997-2005 Free Software Foundation, Inc.
+   Copyright (C) 1997-2005, 2008 Free Software Foundation, Inc.
    Copyright (C) 1984 David M. Ihnat

    This program is free software: you can redistribute it and/or modify
@@ -42,6 +42,7 @@
 #include <sys/types.h>
 #include "system.h"
 #include "error.h"
+#include "quotearg.h"

 /* The official name of this program (e.g., no `g' prefix).  */
 #define PROGRAM_NAME "paste"
@@ -79,12 +80,17 @@ static struct option const longopts[] =
 /* Set globals delims and delim_end.  Copy STRPTR to DELIMS, converting
    backslash representations of special characters in STRPTR to their actual
    values. The set of possible backslash characters has been expanded beyond
-   that recognized by the Unix version.  */
+   that recognized by the Unix version.
+   Return 0 upon success.
+   If the string ends in an odd number of backslashes, ignore the
+   final backslash and return nonzero.  */

-static void
+static int
 collapse_escapes (char const *strptr)
 {
   char *strout = xstrdup (strptr);
+  bool backslash_at_end = false;
+
   delims = strout;

   while (*strptr)
@@ -123,6 +129,14 @@ collapse_escapes (char const *strptr)
              *strout++ = '\v';
              break;

+           case '\\':
+             *strout++ = '\\';
+             break;
+
+           case '\0':
+             backslash_at_end = true;
+             goto done;
+
            default:
              *strout++ = *strptr;
              break;
@@ -130,7 +144,11 @@ collapse_escapes (char const *strptr)
          strptr++;
        }
     }
+
+ done:;
+
   delim_end = strout;
+  return backslash_at_end ? 1 : 0;
 }

 /* Report a write error and exit.  */
@@ -481,7 +499,15 @@ main (int argc, char **argv)
   if (optind == argc)
     argv[argc++] = "-";

-  collapse_escapes (delim_arg);
+  if (collapse_escapes (delim_arg))
+    {
+      /* Don't use the default quoting style, because that would double the
+        number of displayed backslashes, making the diagnostic look bogus.  */
+      set_quoting_style (NULL, escape_quoting_style);
+      error (EXIT_FAILURE, 0,
+            _("delimiter list ends with an unescaped backslash: %s"),
+            quotearg_colon (delim_arg));
+    }

   if (!serial_merge)
     ok = paste_parallel (argc - optind, &argv[optind]);
diff --git a/tests/misc/paste-no-nl b/tests/misc/paste-no-nl
index ec23991..244bc2e 100755
--- a/tests/misc/paste-no-nl
+++ b/tests/misc/paste-no-nl
@@ -1,8 +1,8 @@
 #!/bin/sh
 # -*- perl -*-
-# Ensure that paste properly handles files lacking a final newline.
+# Test paste.

-# Copyright (C) 2003, 2005, 2007 Free Software Foundation, Inc.
+# Copyright (C) 2003, 2005, 2007-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
@@ -28,13 +28,15 @@ use strict;

 (my $program_name = $0) =~ s|.*/||;

-$ENV{PROG} = 'paste';
-
 # Turn off localization of executable's ouput.
 @ENV{qw(LANGUAGE LANG LC_ALL)} = ('C') x 3;

+my $prog = 'paste';
+my $msg = "$prog: delimiter string ends with an odd number of backslashes: ";
+
 my @Tests =
   (
+   # Ensure that paste properly handles files lacking a final newline.
    ['no-nl-1', {IN=>"a"},   {IN=>"b"},   {OUT=>"a\tb\n"}],
    ['no-nl-2', {IN=>"a\n"}, {IN=>"b"},   {OUT=>"a\tb\n"}],
    ['no-nl-3', {IN=>"a"},   {IN=>"b\n"}, {OUT=>"a\tb\n"}],
@@ -46,12 +48,27 @@ my @Tests =
    ['no-nla2', '-d" "', {IN=>"1\na\n"}, {IN=>"2\nb"},   {OUT=>"1 2\na b\n"}],
    ['no-nla3', '-d" "', {IN=>"1\na"},   {IN=>"2\nb\n"}, {OUT=>"1 2\na b\n"}],
    ['no-nla4', '-d" "', {IN=>"1\na\n"}, {IN=>"2\nb\n"}, {OUT=>"1 2\na b\n"}],
+
+   # Specifying a delimiter with a trailing backslash would overrun a
+   # malloc'd buffer.
+   ['delim-bs1', q!-d'\'!, {IN=>{'a'x50=>''}}, {EXIT => 1},
+    # We print a single backslash into the expected output, so need four
+    # (two, each escaped) here.
+    {ERR => $msg . q!\\\\! . "\n"} ],
+
+   # Prior to coreutils-5.1.2, this sort of abuse would make paste
+   # scribble on command-line arguments.  With paste from coreutils-5.1.0,
+   # this example would mangle the first file name argument, if it contains
+   # accepted backslash-escapes:
+   # $ paste -d\\ '123\b\b\b.....@' 2>&1 |cat -A
+   # paste: address@hidden@: No such file or directory$
+   ['delim-bs2', q!-d'\'!, {IN=>{'123\b\b\b.....@'=>''}}, {EXIT => 1},
+    {ERR => $msg . q!\\\\! . "\n"} ],
   );

 my $save_temps = $ENV{DEBUG};
 my $verbose = $ENV{VERBOSE};

-my $prog = $ENV{PROG} || die "$0: \$PROG not specified in environment\n";
 my $fail = run_tests ($program_name, $prog, address@hidden, $save_temps, 
$verbose);
 exit $fail;
 EOF
--
1.5.5.rc1.13.g79388