Bug 18096

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;

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?

(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.

${VAR?} is defined to print a message if VAR is unset, so this is as designed.

(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?

Where does the standard require the suppression of the side effects?

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)

(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.

(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.

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.

Happens with RDE_NOCMD, so it's a denial-of-service security issue.

Florian, there is no additional security impact as allowing user select wordexp pattern is DoS, just use {1,2}{1,2}{1,2}...

(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>

I still see this, as found by oss-fuzz, on glibc2.30

Quick reproducer is 
```
#include <wordexp.h>

int main() {
wordexp_t p;
int ret = wordexp("$[]", &p, 0);
return ret;
}
```

Fixed on 2.38.