Subject: bug#11467: Parfait problems with GNU coreutils

From: Rich Burridge <[email-masked]>
Date: Mon, 14 May 2012 05:25:09 -0700

Hi,

I've been running parfait [1] on GNU coreutils  and it found the following
problems:

Error: Buffer overrun
    Buffer overflow (CWE 120): In array dereference of word_limit[-1] with index '-1'
       Array size is 1000 elements (of 28 bytes each), -1 is -1
         at line 590 of components/coreutils/coreutils-8.5/src/fmt.c in function 'get_paragraph'.
    Read outside array bounds (CWE 125): In array dereference of word_limit[-1] with index '-1'
       Array size is 1000 elements (of 28 bytes each), -1 is -1
         at line 590 of components/coreutils/coreutils-8.5/src/fmt.c in function 'get_paragraph'.
Error: Null pointer dereference (CWE 476)
    Read from null pointer 's'
         at line 3389 of components/coreutils/coreutils-8.5/src/sort.c in function 'main'.
           Function 'parse_field_count' may return constant 'NULL' at line 3130, called at line 3387.
           Null pointer introduced at line 3130 in function 'parse_field_count'.
Error: Null pointer dereference (CWE 476)
    Read from null pointer 'bitsp'
         at line 1546 of components/coreutils/coreutils-8.5/src/stty.c in function 'display_changed'.
           Function 'mode_type_flag' may return constant 'NULL' at line 1453, called at line 1544.
           Null pointer introduced at line 1453 in function 'mode_type_flag'.
Error: Null pointer dereference (CWE 476)
    Read from null pointer 'bitsp'
         at line 1623 of components/coreutils/coreutils-8.5/src/stty.c in function 'display_all'.
           Function 'mode_type_flag' may return constant 'NULL' at line 1453, called at line 1621.
           Null pointer introduced at line 1453 in function 'mode_type_flag'.
Error: Null pointer dereference (CWE 476)
    Read from null pointer 'bitsp'
         at line 1838 of components/coreutils/coreutils-8.5/src/stty.c in function 'sane_mode'.
           Function 'mode_type_flag' may return constant 'NULL' at line 1453, called at line 1837.
           Null pointer introduced at line 1453 in function 'mode_type_flag'.
         at line 1843 of components/coreutils/coreutils-8.5/src/stty.c in function 'sane_mode'.
           Function 'mode_type_flag' may return constant 'NULL' at line 1453, called at line 1842.
           Null pointer introduced at line 1453 in function 'mode_type_flag'.
Error: Null pointer dereference (CWE 476)
    Write to null pointer 'bitsp'
         at line 1838 of components/coreutils/coreutils-8.5/src/stty.c in function 'sane_mode'.
           Function 'mode_type_flag' may return constant 'NULL' at line 1453, called at line 1837.
           Null pointer introduced at line 1453 in function 'mode_type_flag'.
         at line 1843 of components/coreutils/coreutils-8.5/src/stty.c in function 'sane_mode'.
           Function 'mode_type_flag' may return constant 'NULL' at line 1453, called at line 1842.
           Null pointer introduced at line 1453 in function 'mode_type_flag'.

I've attached a patch that we are applying to the code to fix these problems.

Here's the evaluation of why the changes have been made:

There are three different types of Parfait errors here:

1/ In fmt.c, in the get_paragraph() routine, Parfait thinks that there
    is a potential problem of a negative index into the 'word' array.
    But that situation is impossible. Earlier in the routine, get_line()
    is called, and this has to have incremented word_limit. The solution
    was to add a Parfait style comment before the offending line of code
    to shut parfait up.

2/ In sort.c, there is an occurrence in the main() routine where 's' was
    being dereferenced, but it could have been NULL. A check was added
    to only do the dereferencing if that wasn't the case. It was noticed
    that there was already similar code in the same routine, so this seems
    a reasonable solution.

3/ In stty.c, there were two occurrences where bitsp was being dereferenced
    that could have been a NULL pointer. In each case, a check was added
    to only do the dereferencing if that wasn't the case. It was noticed
    that there was already similar code in the sane_mode() routine, so this
    seems a reasonable solution.

These changes make us "parfait clean".

I realize there is a later version of GNU coreutils, but this is the version that currently
ships with Solaris 11. If these problems still exist in the current version, (and assuming
you think these changes are good), hopefully you'll like to make similar changes
in the next version of GNU coreutils.

Thanks.

[1] http://labs.oracle.com/projects/parfait/

--- coreutils-8.5/src/fmt.c.orig	2012-04-30 10:37:50.082717390 -0700
+++ coreutils-8.5/src/fmt.c	2012-05-01 11:28:00.745415600 -0700
@@ -587,6 +587,7 @@
       while (same_para (c) && in_column == other_indent)
         c = get_line (f, c);
     }
+  /* Parfait_ALLOW buffer-overflow word_limit will be incremented by get_line */
   (word_limit - 1)->period = (word_limit - 1)->final = true;
   next_char = c;
   return true;
--- coreutils-8.5/src/sort.c.orig	2012-04-30 10:19:57.802726721 -0700
+++ coreutils-8.5/src/sort.c	2012-04-30 10:20:49.681778327 -0700
@@ -3386,7 +3386,7 @@
                           char const *optarg1 = argv[optind++];
                           s = parse_field_count (optarg1 + 1, &key->eword,
                                              N_("invalid number after `-'"));
-                          if (*s == '.')
+                          if (s && *s == '.')
                             s = parse_field_count (s + 1, &key->echar,
                                                N_("invalid number after `.'"));
                           if (!key->echar && key->eword)
--- coreutils-8.5/src/stty.c.orig	2012-04-30 10:29:20.174932138 -0700
+++ coreutils-8.5/src/stty.c	2012-04-30 11:20:00.683384538 -0700
@@ -1543,7 +1543,7 @@
 
       bitsp = mode_type_flag (mode_info[i].type, mode);
       mask = mode_info[i].mask ? mode_info[i].mask : mode_info[i].bits;
-      if ((*bitsp & mask) == mode_info[i].bits)
+      if (bitsp && ((*bitsp & mask) == mode_info[i].bits))
         {
           if (mode_info[i].flags & SANE_UNSET)
             {
@@ -1620,7 +1620,7 @@
 
       bitsp = mode_type_flag (mode_info[i].type, mode);
       mask = mode_info[i].mask ? mode_info[i].mask : mode_info[i].bits;
-      if ((*bitsp & mask) == mode_info[i].bits)
+      if (bitsp && ((*bitsp & mask) == mode_info[i].bits))
         wrapf ("%s", mode_info[i].name);
       else if (mode_info[i].flags & REV)
         wrapf ("-%s", mode_info[i].name);
@@ -1835,12 +1835,14 @@
       if (mode_info[i].flags & SANE_SET)
         {
           bitsp = mode_type_flag (mode_info[i].type, mode);
-          *bitsp = (*bitsp & ~mode_info[i].mask) | mode_info[i].bits;
+          if (bitsp)
+            *bitsp = (*bitsp & ~mode_info[i].mask) | mode_info[i].bits;
         }
       else if (mode_info[i].flags & SANE_UNSET)
         {
           bitsp = mode_type_flag (mode_info[i].type, mode);
-          *bitsp = *bitsp & ~mode_info[i].mask & ~mode_info[i].bits;
+          if (bitsp)
+            *bitsp = *bitsp & ~mode_info[i].mask & ~mode_info[i].bits;
         }
     }
 }