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

From: [email-masked] (GNU bug Tracking System)
Date: Tue, 27 Aug 2019 23:24:02 +0000

Your bug report

#35256: Bug report for -W argument (maximum width) - minor and not dangerous

which was filed against the diffutils package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
35256: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=35256
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: [bug-diffutils] bug#35256: Bug report for -W argument (maximum width) - minor and not dangerous
From: Paul Eggert <[email-masked]>
To: [email-masked]
Cc: [email-masked]
Date: Tue, 27 Aug 2019 16:23:08 -0700
List-Id: <debbugs-submit.debbugs.gnu.org>

[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

Subject: Bug report for -W argument (maximum width) - minor and not dangerous
From: [email-masked]
To: [email-masked]
Date: Sat, 13 Apr 2019 12:29:45 +0100
List-Id: <debbugs-submit.debbugs.gnu.org>

Hello there.

I was hoping to view a side-by-side diff of something and, perhaps
unfairly, was hoping for a setting where diff would choose a width
such that there were no truncations and I would use less with no
wrapping to inspect the results.

My first attempt was "-W 0" (a width of 0 has no "legit" meaning
afterall) - error, so I tried -1. This leads to a weird situation
where it seems to just output loads of tabs - while it'll probably
still terminate eventually the behaviour is unreasonable.

To try this yourself run something like:

diff -y ./maps ./task/4974/maps -W -1 

from /proc/XXXX where XXXX is some PID for a program with threads (eg
firefox) and the 4974 is any task that isn't XXXX

ADDENDUM: The -1 isn't important, 9999999999999 also illustrates the
problem - END ADDENDUM

Looking at the code (in the 3.7 tarball, src/diff.c modified on 18th
of December 2018) notice:

Line 284:
  uintmax_t numval;
Line 525:
    case 'W':
      numval = strtoumax (optarg, &numend, 10);
      if (! (0 < numval && numval <= SIZE_MAX) || *numend)
        try_help ("invalid width '%s'", optarg);
      if (width != numval)
        {
          if (width)
        fatal ("conflicting width options");
          width = numval;
        }
      break;

For convenience:
      uintmax_t strtoumax(const char *nptr, char **endptr, int
base);
and it may set errno, my man page doesn't say whether this -1
behaviour is "okay" however it probably is, unsigned afterall, this
means that numval is going to be a really really big value. 

ABUSE POTENTIAL:

Just basic DOS (denial-of-service) stuff, a CPU usage spike comes from
diff itself and it seems to output a lot of tabs (a good 275mib / sec
on my machine) and will probably do so for a good few years before
anything else comes out, a testament to the robustness of diff is that
it did this, and its memory usage didn't start ballooning.

I know diff is used by A LOT of other programs, some of which are
web-accessible (eg mediawiki uses diff - and will by default if it
finds it), many of my projects use it too. It is not a big stretch to
imagine someone has a web-service out there which allows side-by-side
format, and not much of a further leap to assume that someone might
have an input box for width which exposes -W, guarded only by a regex
of the form ^[1-9][0-9]* (which yes, wont allow -1 but will allow
9999999999999)

You could bring a server to its knees pretty quickly using just diff's
CPU usage and a few tabs using this - that's not even considering
whether or not the system hypothesised here doesn't have trouble with
memory from a convenient get_line() function first.

While not really diff's fault or problem, a potential solution
detailed below would fix it and not cause any problems for those with
legit (?) needs for really wide diffs 

SUGGESTIONS:
Humans are limiting here, improvements and the growth of computers
wont really affect the maximum width so putting a limit in place is
reasonable. I make no claim there is a "maximum useful width" so being
able to override will ensure my half-assed musings on such a limit
wont cause any problems in the future. 

I'd go with something like
#define REASONABLE_LIMIT 1000

Add a check that numval is <= get_reasonable_specified_width_limit()
after the existing checks, if not output an error in the form of:

"You probably don't want to do that, see [wherever], if you do specify
--we-have-evolved-cylindical-lenses-now or set the environmental
variable GNU_DIFF_REASONABLE_LIMIT to a new limit, using 0 for none"

Lastly, for what it's worth from a perfect stranger:

I'm very impressed that diff didn't start consuming huge amounts of
memory, and a little saddened that it is impressive! 

Thanks very much for diff and your work on it, you have no idea how
many things it underpins!