bug#47412: env: fragile argument parsing
I also installed the attached two followup patches to document this and
issue a better warning in rare cases.
The -S code could use some more fixes in this area too - it can probably
still dump core on platforms like the Hurd that don't limit exec arg
size - but one thing at a time.
>From 6c4efdc0f51c8e253f16da2ec60cdf647bec3c06 Mon Sep 17 00:00:00 2001
From: Paul Eggert
Date: Fri, 26 Mar 2021 14:00:37 -0700
Subject: [PATCH] doc: document env fix
* NEWS, doc/coreutils.texi (env invocation): Document recent change.
---
NEWS | 3 +++
doc/coreutils.texi | 2 ++
2 files changed, 5 insertions(+)
diff --git a/NEWS b/NEWS
index 97cb4bd64..802f4b427 100644
--- a/NEWS
+++ b/NEWS
@@ -17,6 +17,9 @@ GNU coreutils NEWS-*- outline -*-
heavily changed during the run.
[bug introduced in coreutils-8.25]
+ env -S no longer crashes when given unusual whitespace characters
+ [bug introduced in coreutils-8.30]
+
expr no longer mishandles unmatched \(...\) in regular expressions.
[bug introduced in coreutils-6.0]
diff --git a/doc/coreutils.texi b/doc/coreutils.texi
index ac0b4467d..06ecdd74c 100644
--- a/doc/coreutils.texi
+++ b/doc/coreutils.texi
@@ -17592,6 +17592,8 @@ hello
Running @command{env -Sstring} splits the @var{string} into
arguments based on unquoted spaces or tab characters.
+(Newlines, carriage returns, vertical tabs and form feeds are treated
+like spaces and tabs.)
In the following contrived example the @command{awk} variable
@samp{OFS} will be @code{xyz} as these spaces are inside
--
2.30.2
>From 5f99c7533df49f25819d7bb850be5c6cb49aa13d Mon Sep 17 00:00:00 2001
From: Paul Eggert
Date: Fri, 26 Mar 2021 14:51:55 -0700
Subject: [PATCH] env: improve whitespace warning
* src/env.c (main): Issue -S warning for any whitespace,
not just space.
---
src/env.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/env.c b/src/env.c
index d07918fee..341777cb8 100644
--- a/src/env.c
+++ b/src/env.c
@@ -942,7 +942,7 @@ main (int argc, char **argv)
int exit_status = errno == ENOENT ? EXIT_ENOENT : EXIT_CANNOT_INVOKE;
error (0, errno, "%s", quote (argv[optind]));
- if (exit_status == EXIT_ENOENT && strchr (argv[optind], ' '))
+ if (exit_status == EXIT_ENOENT && strpbrk (argv[optind], C_ISSPACE_CHARS))
error (0, 0, _("use -[v]S to pass options in shebang lines"));
return exit_status;
--
2.30.2
bug#47412: env: fragile argument parsing
Thanks for the bug report. I installed the attached to fix it and am
closing the report.
>From 6dd466eda6fa3f1f7d2a9474ec926ccd2ede98e9 Mon Sep 17 00:00:00 2001
From: Paul Eggert
Date: Fri, 26 Mar 2021 13:49:49 -0700
Subject: [PATCH] env: fix address violation with '\v' in -S
Problem reported by Frank Busse (Bug#47412).
* src/env.c (C_ISSPACE_CHARS): New macro.
(shortopts, build_argv, main): Treate all C-locale space
characters like space and tab, for compatibility with FreeBSD.
(validate_split_str, build_argv, parse_split_string):
Use the C locale, not the current locale, to determine whether a
byte is a space character.
---
src/env.c | 23 ---
1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/src/env.c b/src/env.c
index ba9da1113..e13a312cd 100644
--- a/src/env.c
+++ b/src/env.c
@@ -73,7 +73,10 @@ static bool sig_mask_changed;
/* Whether to list non default handling. */
static bool report_signal_handling;
-static char const shortopts[] = "+C:iS:u:v0 \t";
+/* isspace characters in the C locale. */
+#define C_ISSPACE_CHARS " \t\n\v\f\r"
+
+static char const shortopts[] = "+C:iS:u:v0" C_ISSPACE_CHARS;
/* For long options that have no equivalent short option, use a
non-character as a pseudo short option, starting with CHAR_MAX + 1. */
@@ -277,7 +280,7 @@ validate_split_str (const char* str, size_t* /*out*/ bufsize,
size_t buflen;
int cnt = 1;
- assert (str && str[0] && !isspace (str[0])); /* LCOV_EXCL_LINE */
+ assert (str && str[0] && !c_isspace (str[0])); /* LCOV_EXCL_LINE */
dq = sq = sp = false;
buflen = strlen (str)+1;
@@ -286,7 +289,7 @@ validate_split_str (const char* str, size_t* /*out*/ bufsize,
{
const char next = *(str+1);
- if (isspace (*str) && !dq && !sq)
+ if (c_isspace (*str) && !dq && !sq)
{
sp = true;
}
@@ -392,7 +395,7 @@ build_argv (const char* str, int extra_argc)
} \
} while (0)
- assert (str && str[0] && !isspace (str[0])); /* LCOV_EXCL_LINE */
+ assert (str && str[0] && !c_isspace (str[0])); /* LCOV_EXCL_LINE */
validate_split_str (str, &buflen, &newargc);
@@ -433,13 +436,12 @@ build_argv (const char* str, int extra_argc)
++str;
continue;
-case ' ':
-case '\t':
- /* space/tab outside quotes starts a new argument. */
+case ' ': case '\t': case '\n': case '\v': case '\f': case '\r':
+ /* Start a new argument if outside quotes. */
if (sq || dq)
break;
sep = true;
- str += strspn (str, " \t"); /* skip whitespace. */
+ str += strspn (str, C_ISSPACE_CHARS);
continue;
case '#':
@@ -540,7 +542,7 @@ parse_split_string (const char* str, int /*out*/ *orig_optind,
char **newargv, **nextargv;
- while (isspace (*str))
+ while (c_isspace (*str))
str++;
if (*str == '\0')
return;
@@ -848,8 +850,7 @@ main (int argc, char **argv)
case 'S':
parse_split_string (optarg, &optind, &argc, &argv);
break;
-case ' ':
-case '\t':
+case ' ': case '\t': case '\n': case '\v': case '\f': case '\r':
/* These are undocumented options. Attempt to detect
incorrect shebang usage with extraneous space, e.g.:
#!/usr/bin/env -i command
--
2.30.2
bug#47412: env: fragile argument parsing
On 3/26/21 1:12 PM, Pádraig Brady wrote: I'll fix it up. I've got a fix. My goodness, that part of the code is messy.
bug#47412: env: fragile argument parsing
On 26/03/2021 15:00, Frank Busse wrote:
Hi,
env crashes for some nonsensical command line arguments (reported by
KLEE), e.g.:
---
python3 -c "import os; os.execl('./src/env', 'env', b'--s=\"\"\t\x0b')"
=
==140651==ERROR: AddressSanitizer: heap-buffer-overflow on address
0x60300028 at pc 0x562e1cc1078a bp 0x7ffd59964dd0 sp 0x7ffd59964dc0
WRITE of size 8 at 0x60300028 thread T0
#0 0x562e1cc10789 in build_argv src/env.c:511
#1 0x562e1cc10982 in parse_split_string src/env.c:548
#2 0x562e1cc127bc in main src/env.c:849
#3 0x7f1c167e3b24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24)
#4 0x562e1cc0e54d in _start (coreutils-8.32/src/env+0x654d)
0x60300028 is located 0 bytes to the right of 24-byte region
[0x60300010,0x60300028)
allocated by thread T0 here:
#0 0x7f1c16a3b459 in __interceptor_malloc
/build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:145
#1 0x562e1cc19463 in xmalloc lib/xmalloc.c:41
#2 0x562e1cc0ff54 in build_argv src/env.c:404
#3 0x562e1cc10982 in parse_split_string src/env.c:548
#4 0x562e1cc127bc in main src/env.c:849
#5 0x7f1c167e3b24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24)
Confirmed on an ASAN build of the latest source.
I'll fix it up.
thanks!
Pádraig
bug#47412: env: fragile argument parsing
Hi,
env crashes for some nonsensical command line arguments (reported by
KLEE), e.g.:
---
> python3 -c "import os; os.execl('./src/env', 'env', b'--s=\"\"\t\x0b')"
=
==140651==ERROR: AddressSanitizer: heap-buffer-overflow on address
0x60300028 at pc 0x562e1cc1078a bp 0x7ffd59964dd0 sp 0x7ffd59964dc0
WRITE of size 8 at 0x60300028 thread T0
#0 0x562e1cc10789 in build_argv src/env.c:511
#1 0x562e1cc10982 in parse_split_string src/env.c:548
#2 0x562e1cc127bc in main src/env.c:849
#3 0x7f1c167e3b24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24)
#4 0x562e1cc0e54d in _start (coreutils-8.32/src/env+0x654d)
0x60300028 is located 0 bytes to the right of 24-byte region
[0x60300010,0x60300028)
allocated by thread T0 here:
#0 0x7f1c16a3b459 in __interceptor_malloc
/build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:145
#1 0x562e1cc19463 in xmalloc lib/xmalloc.c:41
#2 0x562e1cc0ff54 in build_argv src/env.c:404
#3 0x562e1cc10982 in parse_split_string src/env.c:548
#4 0x562e1cc127bc in main src/env.c:849
#5 0x7f1c167e3b24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24)
SUMMARY: AddressSanitizer: heap-buffer-overflow src/env.c:511 in build_argv
---
or
---
> python3 -c "import os; os.execl('./src/env', 'env', b'--s=\xff
> \r\x0b\t\x0b-')"
=
==140886==ERROR: AddressSanitizer: heap-buffer-overflow on address
0x60300030 at pc 0x55821372878a bp 0x7ffdd6e4bc40 sp 0x7ffdd6e4bc30
WRITE of size 8 at 0x60300030 thread T0
#0 0x558213728789 in build_argv src/env.c:511
#1 0x558213728982 in parse_split_string src/env.c:548
#2 0x55821372a7bc in main src/env.c:849
#3 0x7f5b05ec5b24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24)
#4 0x55821372654d in _start (coreutils-8.32/src/env+0x654d)
0x60300030 is located 0 bytes to the right of 32-byte region
[0x60300010,0x60300030) allocated by thread T0 here:
#0 0x7f5b0611d459 in
__interceptor_malloc/build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:145
#1 0x558213731463 in xmalloc lib/xmalloc.c:41
#2 0x558213727f54 in build_argv src/env.c:404
#3 0x558213728982 in parse_split_string src/env.c:548
#4 0x55821372a7bc in main src/env.c:849
#5 0x7f5b05ec5b24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24)
SUMMARY: AddressSanitizer: heap-buffer-overflow src/env.c:511 in build_argv
---
Version: 8.32
Configure: CFLAGS="-ggdb -O0 -fsanitize=address" ./configure --without-selinux
--without-gmp --disable-acl --disable-largefile --disable-libsmack
--disable-xattr --disable-libcap --disable-nls
Kind regards,
Frank
