From: Pádraig Brady Date: Tue, 24 Jun 2025 00:17:12 +0000 (+0100) Subject: od: fix various off-by-one issues with --strings with -N X-Git-Tag: v9.8~289 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=44809c3379d64b9e580042124ccea6665d746d9a;p=thirdparty%2Fcoreutils.git od: fix various off-by-one issues with --strings with -N * src/od.c (dump_strings): There are three related issues here due to not accounting for the terminating NUL char appropriately. 1. Ensure BUF always has enough space for the terminating NUL. This avoids CWE-122: Heap-based Buffer Overflow, where we wrote a single NUL byte directly after the allocated buffer. I.e., there should be no buffer overflow with: printf '%100s' | od -N100 -S1 2. Ensure we support -S == -N (END_OFFSET - STRING_MIN == ADDRESS): I.e., there should be output with: printf '%100s' | od -N10 -S10 3. Ensure we always output a valid address by ensuring the ADDRESS and I variables are kept in sync. I.e., this should output address 0000000 not 1777777777777777777777: printf '%100s' | od -N10 -S1 As well as fixing these we simplify by using a single loop to read the data, rather than two. * doc/coreutils.texi (od invocation): Clarify that -N implicitly NUL terminates strings. * tests/od/od-N.sh: Add test cases. * NEWS: Mention the bug fixes. Fixes https://bugs.gnu.org/78880 --- diff --git a/NEWS b/NEWS index 8b3d9d3cc2..e4d9c9b608 100644 --- a/NEWS +++ b/NEWS @@ -12,6 +12,10 @@ GNU coreutils NEWS -*- outline -*- copying to non-NFS files from NFSv4 files with trivial ACLs. [bug introduced in coreutils-9.6] + od --strings with -N now works correctly. Previously od might + write a NUL byte after a heap buffer, or output invalid addresses. + [These bugs were present in "the beginning".] + sort with key character offsets of SIZE_MAX, could induce a read of 1 byte before an allocated heap buffer. For example: 'sort +0.18446744073709551615R input' on 64 bit systems. diff --git a/doc/coreutils.texi b/doc/coreutils.texi index a1d45fb301..865ceff050 100644 --- a/doc/coreutils.texi +++ b/doc/coreutils.texi @@ -2075,6 +2075,8 @@ least @var{bytes} consecutive printable characters, followed by a zero byte (ASCII NUL). Prefixes and suffixes on @var{bytes} are interpreted as for the @option{-j} option. +If combined with the @option{-N} option, truncated strings +are considered ASCII NUL terminated. If @var{bytes} is omitted with @option{--strings}, the default is 3. diff --git a/src/od.c b/src/od.c index 88d467c73c..32e8498960 100644 --- a/src/od.c +++ b/src/od.c @@ -1505,70 +1505,58 @@ dump (void) } /* STRINGS mode. Find each "string constant" in the input. - A string constant is a run of at least 'string_min' ASCII - graphic (or formatting) characters terminated by a null. + A string constant is a run of at least STRING_MIN + printable characters terminated by a NUL or END_OFFSET. Based on a function written by Richard Stallman for a traditional version of od. Return true if successful. */ static bool dump_strings (void) { - idx_t bufsize = MAX (100, string_min); + idx_t bufsize = MAX (100, string_min + 1); char *buf = xmalloc (bufsize); uintmax_t address = n_bytes_to_skip; bool ok = true; while (true) { - idx_t i; - int c; - - /* See if the next 'string_min' chars are all printing chars. */ - tryline: + idx_t i = 0; + int c = 1; /* Init to 1 so can distinguish if NUL read. */ if (limit_bytes_to_format - && (end_offset < string_min || end_offset - string_min <= address)) + && (end_offset < string_min || end_offset - string_min < address)) break; - for (i = 0; i < string_min; i++) - { - ok &= read_char (&c); - address++; - if (c < 0) - { - free (buf); - return ok; - } - if (! isprint (c)) - /* Found a non-printing. Try again starting with next char. */ - goto tryline; - buf[i] = c; - } - - /* We found a run of 'string_min' printable characters. - Now see if it is terminated with a null byte. */ + /* Store consecutive printable characters to BUF. */ while (!limit_bytes_to_format || address < end_offset) { - if (i == bufsize) + if (i == bufsize - 1) buf = xpalloc (buf, &bufsize, 1, -1, sizeof *buf); ok &= read_char (&c); - address++; if (c < 0) { free (buf); return ok; } + address++; + buf[i++] = c; if (c == '\0') - break; /* It is; print this string. */ + break; /* Print this string. */ if (! isprint (c)) - goto tryline; /* It isn't; give up on this string. */ - buf[i++] = c; /* String continues; store it all. */ + { + c = -1; /* Give up on this string. */ + break; + } } - /* If we get here, the string is all printable and null-terminated, - so print it. It is all in 'buf' and 'i' is its length. */ + if (c == -1 || i - !c < string_min) + continue; + buf[i] = 0; - format_address (address - i - 1, ' '); + + /* If we get here, the string is all printable, so print it. */ + + format_address (address - i, ' '); for (i = 0; (c = buf[i]); i++) { diff --git a/tests/od/od-N.sh b/tests/od/od-N.sh index b057c9b83b..7c97eeee2a 100755 --- a/tests/od/od-N.sh +++ b/tests/od/od-N.sh @@ -20,8 +20,6 @@ print_ver_ od echo abcdefg > in || framework_failure_ - - (od -An -N3 -c; od -An -N3 -c) < in > out cat < exp || framework_failure_ a b c @@ -29,4 +27,31 @@ cat < exp || framework_failure_ EOF compare exp out || fail=1 +# coreutils <= 9.7 would buffer overflow with +# a single NUL byte after the heap buffer +printf '%100s' | od -N100 -S1 > out || fail=1 +printf '%07o %100s\n' 0 '' > exp || framework_failure_ +compare exp out || fail=1 + +# coreutils <= 9.7 would output nothing +printf '%100s' | od -N10 -S10 > out || fail=1 +printf '%07o %10s\n' 0 '' > exp || framework_failure_ +compare exp out || fail=1 + +# coreutils <= 9.7 would output an invalid address +printf '%100s' | od -N10 -S1 > out || fail=1 +printf '%07o %10s\n' 0 '' > exp || framework_failure_ +compare exp out || fail=1 + +# Ensure -S limits appropriately +printf '%10s\000' | od -N11 -S11 > out || fail=1 +compare /dev/null out || fail=1 +printf '%10s\000' | od -S11 > out || fail=1 +compare /dev/null out || fail=1 +printf '%10s' | od -S10 > out || fail=1 # Ignore unterminated at EOF? +compare /dev/null out || fail=1 +printf '\001%10s\000%10s\000' | od -S10 > out || fail=1 +printf '%07o %10s\n' 1 '' 12 '' > exp || framework_failure_ +compare exp out || fail=1 + Exit $fail