Subject: Re: [bug-diffutils] bug#35256: Bug report for -W argument (maximum width) - minor and not dangerous

From: Paul Eggert <[email-masked]>
Date: Tue, 27 Aug 2019 16:23:08 -0700

[email-masked] wrote:

> I know diff is used by A LOT of other programs, some of which are
> web-accessible

I'm afraid that ship sailed a while ago: if you let a remote attacker specify an 
arbitrary option to GNU diff there is lots of other trouble you can get into. 
For example, the -I option lets the attacker specify a regular expression that 
can cause diff to undergo exponential complexity. The general wisdom nowadays is 
to not expose command-line operands to attackers.

As for putting in a limit, the GNU Coding Standards say to not impose arbitrary 
limits. In some cases there are good reasons to impose a limit anyway but this 
one doesn't seem to rise to that level.

You do raise a good point that 'diff' shouldn't treat negative inputs as if they 
were large positive inputs, so I installed the attached patch.

Thanks for reporting the problem; your bug report was a pleasure to read.
>From 8d26b1403e8607811ccebdfe2822f2dad42a36d3 Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email-masked]>
Date: Tue, 27 Aug 2019 16:14:15 -0700
Subject: [PATCH] =?UTF-8?q?diff:=20don=E2=80=99t=20mistreat=20-N=20in=20ar?=
 =?UTF-8?q?g=20as=20a=20large=20number?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Problem reported by alec (Bug#35256).
* NEWS: Mention the fix.
* bootstrap.conf (gnulib_modules): Use strtoimax and xstrtoimax,
not strtoumax and strtoumax.
* src/cmp.c (bytes): Now signed, with -1 representing no limit.
All uses changed.
* src/cmp.c (specify_ignore_initial, main):
* src/diff.c (main):
* src/ifdef.c (format_group):
* src/sdiff.c (interact):
Use strtoimax, not strtoumax.
---
 NEWS           |  4 ++++
 bootstrap.conf |  4 ++--
 src/cmp.c      | 27 ++++++++++++++-------------
 src/diff.c     | 14 +++++++-------
 src/ifdef.c    |  6 +++---
 src/sdiff.c    | 10 +++++-----
 6 files changed, 35 insertions(+), 30 deletions(-)

diff --git a/NEWS b/NEWS
index 5c1ae5f..3ecd111 100644
--- a/NEWS
+++ b/NEWS
@@ -19,6 +19,10 @@ GNU diffutils NEWS                                    -*- outline -*-
   that was intended for stdin, stdout, or stderr.
   [bug#33965 present since "the beginning"]
 
+  cmp, diff and sdiff no longer treat negative command-line
+  option-arguments as if they were large positive numbers.
+  [bug#35256 introduced in 2.8]
+
 
 * Noteworthy changes in release 3.7 (2018-12-31) [stable]
 
diff --git a/bootstrap.conf b/bootstrap.conf
index 7d5ea62..1a20900 100644
--- a/bootstrap.conf
+++ b/bootstrap.conf
@@ -70,7 +70,7 @@ stdint
 strcase
 strftime
 strptime
-strtoumax
+strtoimax
 sys_wait
 system-quote
 unistd
@@ -85,7 +85,7 @@ xalloc
 xfreopen
 xreadlink
 xstdopen
-xstrtoumax
+xstrtoimax
 xvasprintf
 '
 
diff --git a/src/cmp.c b/src/cmp.c
index ce2bdb5..16e8869 100644
--- a/src/cmp.c
+++ b/src/cmp.c
@@ -75,8 +75,8 @@ static size_t buf_size;
 /* Initial prefix to ignore for each file.  */
 static off_t ignore_initial[2];
 
-/* Number of bytes to compare.  */
-static uintmax_t bytes = UINTMAX_MAX;
+/* Number of bytes to compare, or -1 if there is no limit.  */
+static intmax_t bytes = -1;
 
 /* Output format.  */
 static enum comparison_type
@@ -129,12 +129,12 @@ static char const valid_suffixes[] = "kKMGTPEZY0";
 static void
 specify_ignore_initial (int f, char **argptr, char delimiter)
 {
-  uintmax_t val;
+  intmax_t val;
   char const *arg = *argptr;
-  strtol_error e = xstrtoumax (arg, argptr, 0, &val, valid_suffixes);
-  if (! (e == LONGINT_OK
-         || (e == LONGINT_INVALID_SUFFIX_CHAR && **argptr == delimiter))
-      || TYPE_MAXIMUM (off_t) < val)
+  strtol_error e = xstrtoimax (arg, argptr, 0, &val, valid_suffixes);
+  if (! ((e == LONGINT_OK
+          || (e == LONGINT_INVALID_SUFFIX_CHAR && **argptr == delimiter))
+         && 0 <= val && val <= TYPE_MAXIMUM (off_t)))
     try_help ("invalid --ignore-initial value '%s'", arg);
   if (ignore_initial[f] < val)
     ignore_initial[f] = val;
@@ -237,10 +237,11 @@ main (int argc, char **argv)
 
       case 'n':
         {
-          uintmax_t n;
-          if (xstrtoumax (optarg, 0, 0, &n, valid_suffixes) != LONGINT_OK)
+          intmax_t n;
+          if (xstrtoimax (optarg, 0, 0, &n, valid_suffixes) != LONGINT_OK
+              || n < 0)
             try_help ("invalid --bytes value '%s'", optarg);
-          if (n < bytes)
+          if (! (0 <= bytes && bytes < n))
             bytes = n;
         }
         break;
@@ -341,7 +342,7 @@ main (int argc, char **argv)
         s0 = 0;
       if (s1 < 0)
         s1 = 0;
-      if (s0 != s1 && MIN (s0, s1) < bytes)
+      if (s0 != s1 && (bytes < 0 || MIN (s0, s1) < bytes))
         exit (EXIT_FAILURE);
     }
 
@@ -379,7 +380,7 @@ cmp (void)
   bool at_line_start = true;
   off_t line_number = 1;	/* Line number (1...) of difference. */
   off_t byte_number = 1;	/* Byte number (1...) of difference. */
-  uintmax_t remaining = bytes;	/* Remaining number of bytes to compare.  */
+  intmax_t remaining = bytes;	/* Remaining bytes to compare, or -1.  */
   size_t read0, read1;		/* Number of bytes read from each file. */
   size_t first_diff;		/* Offset (0...) in buffers of 1st diff. */
   size_t smaller;		/* The lesser of 'read0' and 'read1'. */
@@ -433,7 +434,7 @@ cmp (void)
     {
       size_t bytes_to_read = buf_size;
 
-      if (remaining != UINTMAX_MAX)
+      if (0 <= remaining)
         {
           if (remaining < bytes_to_read)
             bytes_to_read = remaining;
diff --git a/src/diff.c b/src/diff.c
index e9c2b11..c545642 100644
--- a/src/diff.c
+++ b/src/diff.c
@@ -282,7 +282,7 @@ main (int argc, char **argv)
   bool show_c_function = false;
   char const *from_file = NULL;
   char const *to_file = NULL;
-  uintmax_t numval;
+  intmax_t numval;
   char *numend;
 
   /* Do our initializations.  */
@@ -350,8 +350,8 @@ main (int argc, char **argv)
           {
             if (optarg)
               {
-                numval = strtoumax (optarg, &numend, 10);
-                if (*numend)
+                numval = strtoimax (optarg, &numend, 10);
+                if (*numend || numval < 0)
                   try_help ("invalid context length '%s'", optarg);
                 if (CONTEXT_MAX < numval)
                   numval = CONTEXT_MAX;
@@ -525,7 +525,7 @@ main (int argc, char **argv)
           break;
 
         case 'W':
-          numval = strtoumax (optarg, &numend, 10);
+          numval = strtoimax (optarg, &numend, 10);
           if (! (0 < numval && numval <= SIZE_MAX) || *numend)
             try_help ("invalid width '%s'", optarg);
           if (width != numval)
@@ -554,8 +554,8 @@ main (int argc, char **argv)
           return EXIT_SUCCESS;
 
         case HORIZON_LINES_OPTION:
-          numval = strtoumax (optarg, &numend, 10);
-          if (*numend)
+          numval = strtoimax (optarg, &numend, 10);
+          if (*numend || numval < 0)
             try_help ("invalid horizon length '%s'", optarg);
           horizon_lines = MAX (horizon_lines, MIN (numval, LIN_MAX));
           break;
@@ -609,7 +609,7 @@ main (int argc, char **argv)
           break;
 
         case TABSIZE_OPTION:
-          numval = strtoumax (optarg, &numend, 10);
+          numval = strtoimax (optarg, &numend, 10);
           if (! (0 < numval && numval <= SIZE_MAX - GUTTER_WIDTH_MINIMUM)
               || *numend)
             try_help ("invalid tabsize '%s'", optarg);
diff --git a/src/ifdef.c b/src/ifdef.c
index 65f1745..43f1f86 100644
--- a/src/ifdef.c
+++ b/src/ifdef.c
@@ -135,7 +135,7 @@ format_group (register FILE *out, char const *format, char endchar,
             /* Print if-then-else format e.g. '%(n=1?thenpart:elsepart)'.  */
             {
               int i;
-              uintmax_t value[2];
+              intmax_t value[2];
               FILE *thenout, *elseout;
 
               for (i = 0; i < 2; i++)
@@ -144,7 +144,7 @@ format_group (register FILE *out, char const *format, char endchar,
                     {
                       char *fend;
                       errno = 0;
-                      value[i] = strtoumax (f, &fend, 10);
+                      value[i] = strtoimax (f, &fend, 10);
                       if (errno)
                         goto bad_format;
                       f = fend;
@@ -152,7 +152,7 @@ format_group (register FILE *out, char const *format, char endchar,
                   else
                     {
                       value[i] = groups_letter_value (groups, *f);
-                      if (value[i] == -1)
+                      if (value[i] < 0)
                         goto bad_format;
                       f++;
                     }
diff --git a/src/sdiff.c b/src/sdiff.c
index 2ef83da..a61f4e7 100644
--- a/src/sdiff.c
+++ b/src/sdiff.c
@@ -1098,15 +1098,15 @@ interact (struct line_filter *diff,
       else
         {
           char *numend;
-          uintmax_t val;
+          intmax_t val;
           lin llen, rlen, lenmax;
           errno = 0;
-          val = strtoumax (diff_help + 1, &numend, 10);
-          if (LIN_MAX < val || errno || *numend != ',')
+          val = strtoimax (diff_help + 1, &numend, 10);
+          if (! (0 <= val && val <= LIN_MAX) || errno || *numend != ',')
             fatal (diff_help);
           llen = val;
-          val = strtoumax (numend + 1, &numend, 10);
-          if (LIN_MAX < val || errno || *numend)
+          val = strtoimax (numend + 1, &numend, 10);
+          if (! (0 <= val && val <= LIN_MAX) || errno || *numend)
             fatal (diff_help);
           rlen = val;
 
-- 
2.17.1