Subject: bug#47412: closed (Re: bug#47412: env: fragile argument parsing)

From: [email-masked] (GNU bug Tracking System)
Date: Fri, 26 Mar 2021 20:53:01 +0000

Your bug report

#47412: env: fragile argument parsing

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

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

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

Subject: Re: bug#47412: env: fragile argument parsing
From: Paul Eggert <[email-masked]>
To: Frank Busse <[email-masked]>
Cc: [email-masked]
Date: Fri, 26 Mar 2021 13:52:40 -0700
List-Id: <debbugs-submit.debbugs.gnu.org>

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

Subject: env: fragile argument parsing
From: Frank Busse <[email-masked]>
To: [email-masked]
Date: Fri, 26 Mar 2021 15:00:17 +0000
List-Id: <debbugs-submit.debbugs.gnu.org>

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
0x603000000028 at pc 0x562e1cc1078a bp 0x7ffd59964dd0 sp 0x7ffd59964dc0
WRITE of size 8 at 0x603000000028 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)

0x603000000028 is located 0 bytes to the right of 24-byte region
[0x603000000010,0x603000000028)
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
0x603000000030 at pc 0x55821372878a bp 0x7ffdd6e4bc40 sp 0x7ffdd6e4bc30
WRITE of size 8 at 0x603000000030 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)

0x603000000030 is located 0 bytes to the right of 32-byte region
[0x603000000010,0x603000000030) 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