[email protected] 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 protected]> 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
