[Bug libc/18096] New: null deref in wordexp/parse_dollars/parse_arith

[Bug libc/18096] New: null deref in wordexp/parse_dollars/parse_arith

[konstantin.s.serebryany at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=18096

            Bug ID: 18096
           Summary: null deref in wordexp/parse_dollars/parse_arith
           Product: glibc
           Version: unspecified
            Status: NEW
          Severity: normal
          Priority: P2
         Component: libc
          Assignee: unassigned at sourceware dot org
          Reporter: konstantin.s.serebryany at gmail dot com
                CC: drepper.fsp at gmail dot com

#include <wordexp.h>
#include <string.h>
int main() {
  char *p = strdup("$[]");
  wordexp_t w;
  wordexp(p, &w, 0);
}

gcc we5.c && ./a.out 

    #0 0x7fe40ab0d3ae in parse_arith
/build/buildd/eglibc-2.19/posix/wordexp.c:774
    #1 0x7fe40ab0b123 in parse_dollars
/build/buildd/eglibc-2.19/posix/wordexp.c:2096
    #2 0x7fe40ab0dfeb in wordexp /build/buildd/eglibc-2.19/posix/wordexp.c:2348

2.19 and fresh trunk are affected.
Same fuzzer, see https://sourceware.org/glibc/wiki/FuzzingLibc

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/18096] null deref in wordexp/parse_dollars/parse_arith

[konstantin.s.serebryany at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=18096

--- Comment #1 from Kostya Serebryany <konstantin.s.serebryany at gmail dot com> ---
Another similar looking, but on different line: 

#include <wordexp.h>
#include <string.h>
int main() {
  char *p = strdup("$(())");
  wordexp_t w;
  wordexp(p, &w, 0);
}


    #0 0x7fc1c04a26f6 in parse_arith
/build/buildd/eglibc-2.19/posix/wordexp.c:736
    #1 0x7fc1c04a0123 in parse_dollars
/build/buildd/eglibc-2.19/posix/wordexp.c:2096
    #2 0x7fc1c04a2feb in wordexp /build/buildd/eglibc-2.19/posix/wordexp.c:2348


The following stops the bug for me: 
--- a/posix/wordexp.c
+++ b/posix/wordexp.c
@@ -736,7 +736,7 @@ parse_arith (char **word, size_t *word_length, size_t
*max_length,
              ++(*offset);

              /* Go - evaluate. */
-             if (*expr && eval_expr (expr, &numresult) != 0)
+             if (expr && *expr && eval_expr (expr, &numresult) != 0)
                { 
                  free (expr);
                  return WRDE_SYNTAX;
@@ -774,7 +774,7 @@ parse_arith (char **word, size_t *word_length, size_t
*max_length,
              long int numresult = 0;

              /* Go - evaluate. */
-             if (*expr && eval_expr (expr, &numresult) != 0)
+             if (expr && *expr && eval_expr (expr, &numresult) != 0)
                { 
                  free (expr);
                  return WRDE_SYNTAX;

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/18096] null deref in wordexp/parse_dollars/parse_arith

[konstantin.s.serebryany at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=18096

--- Comment #2 from Kostya Serebryany <konstantin.s.serebryany at gmail dot com> ---
One more case: 

  char *p = strdup("[a:*${C?}");

==3359==    at 0x4F1EF33: parse_param (wordexp.c:1843)
==3359==    by 0x4F1EF33: parse_dollars (wordexp.c:2102)
==3359==    by 0x4F20598: parse_glob (wordexp.c:490)
==3359==    by 0x4F20598: wordexp (wordexp.c:2416)

This time, the bug is here: 
@@ -1841,7 +1841,7 @@ envsubst:
            {
              const char *str = pattern;

-             if (str[0] == '\0')
+             if (str && str[0] == '\0')
                str = _("parameter null or not set");

              __fxprintf (NULL, "%s: %s\n", env, str);


An interesting side question: is wordexp supposed to print anything?

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/18096] null deref in wordexp/parse_dollars/parse_arith

[carlos at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=18096

Carlos O'Donell <carlos at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |carlos at redhat dot com

--- Comment #3 from Carlos O'Donell <carlos at redhat dot com> ---
(In reply to Kostya Serebryany from comment #2)
> An interesting side question: is wordexp supposed to print anything?

Never. I don't know why there is an __fxprintf there.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/18096] null deref in wordexp/parse_dollars/parse_arith

[schwab@linux-m68k.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=18096

--- Comment #4 from Andreas Schwab <schwab@linux-m68k.org> ---
${VAR?} is defined to print a message if VAR is unset, so this is as designed.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/18096] null deref in wordexp/parse_dollars/parse_arith

[carlos at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=18096

--- Comment #5 from Carlos O'Donell <carlos at redhat dot com> ---
(In reply to Andreas Schwab from comment #4)
> ${VAR?} is defined to print a message if VAR is unset, so this is as
> designed.

Does that really have to happen for wordexp?

The function as defined by POSIX only requires the result of the expansion to
be returned, it says nothing about the potential shell-like side-effects like
SIGFPE and printing error messages.

I don't see it at all as being useful for wordexp to print anything to the
programs stdout, where it will only get mixed with program output, and you have
no way of knowing exactly when that will be if you run wordexp in a thread.
Worse you'd have to redirect stdin/stdout for the thread, which you can't
easily do.

How is it possibly useful for wordexp to print shell-related side-effects?

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/18096] null deref in wordexp/parse_dollars/parse_arith

[schwab@linux-m68k.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=18096

--- Comment #6 from Andreas Schwab <schwab@linux-m68k.org> ---
Where does the standard require the suppression of the side effects?

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/18096] null deref in wordexp/parse_dollars/parse_arith

[konstantin.s.serebryany at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=18096

--- Comment #7 from Kostya Serebryany <konstantin.s.serebryany at gmail dot com> ---
a related question: is wordexp() really supposed to call setenv()?
It's not just an undocumented side effect, but it also introduces
thread-unsafety
(AFAICT, setenv is not expected to be thread-safe, but wordexp is)

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/18096] null deref in wordexp/parse_dollars/parse_arith

[carlos at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=18096

--- Comment #8 from Carlos O'Donell <carlos at redhat dot com> ---
(In reply to Andreas Schwab from comment #6)
> Where does the standard require the suppression of the side effects?

It doesn't, it leaves it up to the implementation.

POSIX does mention this:
~~~
Unless WRDE_SHOWERR is set in flags, wordexp() shall redirect stderr to
/dev/null for any utilities executed as a result of command substitution while
expanding words. If WRDE_SHOWERR is set, wordexp() may write messages to stderr
if syntax errors are detected while expanding words; however, it is unspecified
whether any write errors encountered while outputting such messages will affect
the stderr error indicator or the value of errno.
~~~

It would still seem to me that the principle of least surprise is that when I
call wordexp to do trivial shell expansion it should not print anything by
default, like is done for utilities.

POSIX says:
~~~
The expansions shall be the same as would be performed by the command line
interpreter if words were the part of a command line representing the arguments
to a utility.
~~~
Which in my opinion means that the *expansion* is the only part that will be
the *same* was would be performed on the command line. The side-effect of
${VAR?} expanding and printing an error is potentially relevant, but I think
should be guarded by WRDE_SHOWERR.

Therefore the suggested fix would be:

diff --git a/posix/wordexp.c b/posix/wordexp.c
index e3d8d6b..e66b459 100644
--- a/posix/wordexp.c
+++ b/posix/wordexp.c
@@ -1836,11 +1836,11 @@ envsubst:
          if (!colon_seen && value)
            /* Substitute NULL */
            ;
-         else
+         else if (flags & WRDE_SHOWERR)
            {
              const char *str = pattern;

-             if (str[0] == '\0')
+             if (str == NULL || str[0] == '\0')
                str = _("parameter null or not set");

              __fxprintf (NULL, "%s: %s\n", env, str);
---


#include <stdlib.h>
#include <stdio.h>
#include <wordexp.h>
#include <string.h>

int main() {
  int i;
  char *p = strdup("${VAR?}");
  char **res;
  wordexp_t w;
  wordexp(p, &w, 0);
  res = w.we_wordv;
  for (i = 0; i < w.we_wordc; i++)
     printf("%s\n", res[i]);
  wordfree(&w);
  return 0;
}

* Before the fix crashes.

* After the fix, but without checking WRDE_SHOWERR it prints:

  VAR: parameter null or not set

* After the fix, and honouring WRDE_SHOERR it prints nothing.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/18096] null deref in wordexp/parse_dollars/parse_arith

[carlos at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=18096

--- Comment #9 from Carlos O'Donell <carlos at redhat dot com> ---
(In reply to Kostya Serebryany from comment #7)
> a related question: is wordexp() really supposed to call setenv()?
> It's not just an undocumented side effect, but it also introduces
> thread-unsafety
> (AFAICT, setenv is not expected to be thread-safe, but wordexp is)

It is documented.

The manual already list wordexp as MT-unsafe, see glibc/manual/pattern.texi:

@deftypefun int wordexp (const char *@var{words}, wordexp_t
*@var{word-vector-ptr}, int @var{flags})
@safety{@prelim{}@mtunsafe{@mtasurace{:utent} @mtasuconst{:@mtsenv{}} @mtsenv{}
@mtascusig{:ALRM} @mtascutimer{} @mtslocale{}}@asunsafe{@ascudlopen{}
@ascuplugin{} @ascuintl{} @ascuheap{} @asucorrupt{}
@asulock{}}@acunsafe{@acucorrupt{} @aculock{} @acsfd{} @acsmem{}}}

Which translates into:

Preliminary: | MT-Unsafe race:utent const:env env sig:ALRM timer locale | AS-
Unsafe dlopen plugin i18n heap corrupt lock | AC-Unsafe corrupt lock fd mem |
See Section 1.2.2.1 [POSIX Safety Concepts], page 2.

So some of the MT-safety issues can be mitigated, others can't.

It needs to call setenv() to correctly handle expansions that might use that
value. This is all again a QoI issues, it's likely the only safe way to handle
any this is in a child process, which solves the SIGFPE issue also. Keeping the
side-effects in the child process.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/18096] null deref in wordexp/parse_dollars/parse_arith

[bugdal at aerifal dot cx]

https://sourceware.org/bugzilla/show_bug.cgi?id=18096

Rich Felker <bugdal at aerifal dot cx> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bugdal at aerifal dot cx

--- Comment #10 from Rich Felker <bugdal at aerifal dot cx> ---
With WRDE_SHOWERR, wordexp is permitted to write to stderr, but I don't see any
allowance for it to write to stdout, and short of explicit text allowing this,
I don't think it's permitted.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/18096] null deref in wordexp/parse_dollars/parse_arith

[fweimer at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=18096

Florian Weimer <fweimer at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |fweimer at redhat dot com
              Flags|                            |security+

--- Comment #11 from Florian Weimer <fweimer at redhat dot com> ---
Happens with RDE_NOCMD, so it's a denial-of-service security issue.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/18096] null deref in wordexp/parse_dollars/parse_arith

[neleai at seznam dot cz]

https://sourceware.org/bugzilla/show_bug.cgi?id=18096

Ondrej Bilka <neleai at seznam dot cz> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |neleai at seznam dot cz

--- Comment #12 from Ondrej Bilka <neleai at seznam dot cz> ---
Florian, there is no additional security impact as allowing user select wordexp
pattern is DoS, just use {1,2}{1,2}{1,2}...

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/18096] null deref in wordexp/parse_dollars/parse_arith

[fweimer at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=18096

Florian Weimer <fweimer at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
              Flags|security+                   |security-

--- Comment #13 from Florian Weimer <fweimer at redhat dot com> ---
(In reply to Ondrej Bilka from comment #12)
> Florian, there is no additional security impact as allowing user select
> wordexp pattern is DoS, just use {1,2}{1,2}{1,2}...

Thanks, I've updated the exceptions wiki page:
<https://sourceware.org/glibc/wiki/Security%20Exceptions>

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug glob/18096] null deref in wordexp/parse_dollars/parse_arith

[jsm28 at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=18096

Joseph Myers <jsm28 at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|libc                        |glob

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug glob/18096] null deref in wordexp/parse_dollars/parse_arith

[adhemerval.zanella at linaro dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=18096

Adhemerval Zanella <adhemerval.zanella at linaro dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
                 CC|                            |adhemerval.zanella at linaro dot o
                   |                            |rg
   Target Milestone|---                         |2.38
             Status|NEW                         |RESOLVED

--- Comment #15 from Adhemerval Zanella <adhemerval.zanella at linaro dot org> ---
Fixed on 2.38.

-- 
You are receiving this mail because:
You are on the CC list for the bug.