Re: [PATCH - alsa-lib 1/1] snd_user_file: avoid use wordexp

Re: [PATCH - alsa-lib 1/1] snd_user_file: avoid use wordexp

[Takashi Iwai]

On Wed, 12 Jul 2017 09:53:08 +0200,
Natanael Copa wrote:
> 
> As suggested in POSIX[1], wordexp might execute the shell. If the libc
> implementation does so, it will break the firefox sandbox which does
> not allow exec. This happened on Alpine Linux with musl libc[2].
> 
> Since we cannot guarantee that the system wordexp implementation does
> not execute shell, we cannot really use it, and need to implement the
> ~/ expansion ourselves.
> 
> Generally, wordexp is a large attack vector and it is better to avoid it
> since only tilde expansion is needed.
> 
> [1]: http://pubs.opengroup.org/onlinepubs/9699919799/functions/wordexp.html#tag_16_684_08
> [2]: http://bugs.alpinelinux.org/issues/7454#note-2
> 
> Signed-off-by: Natanael Copa <ncopa@alpinelinux.org>

Well, this may bring a functional regression, so I don't want to
introduce this unconditionally.  Instead, we may introduce configure
option to explicitly control the usage of wordexp(), and add your
extension to the fallback code, for example.  If the security issue
really matters, we can leave it disabled as default, too.


thanks,

Takashi

> 
> diff --git a/configure.ac b/configure.ac
> index 26e5d125..076c6bab 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -304,7 +304,7 @@ fi
>  AC_SUBST(ALSA_DEPLIBS)
>  
>  dnl Check for headers
> -AC_CHECK_HEADERS([wordexp.h endian.h sys/endian.h sys/shm.h])
> +AC_CHECK_HEADERS([endian.h sys/endian.h sys/shm.h])
>  
>  dnl Check for resmgr support...
>  AC_MSG_CHECKING(for resmgr support)
> diff --git a/src/userfile.c b/src/userfile.c
> index 72779da4..0e3f5fae 100644
> --- a/src/userfile.c
> +++ b/src/userfile.c
> @@ -21,6 +21,11 @@
>  #include <config.h>
>  #include <string.h>
>  #include <errno.h>
> +#include <sys/types.h>
> +#include <unistd.h>
> +#include <pwd.h>
> +#include <stdio.h>
> +#include <stdlib.h>
>  
>  /**
>   * \brief Get the full file name
> @@ -28,46 +33,58 @@
>   * \param result The pointer to store the resultant file name
>   * \return 0 if successful, or a negative error code
>   *
> - * Parses the given file name with POSIX-Shell-like expansion and
> - * stores the first matchine one.  The returned string is strdup'ed.
> + * Parses the given file name with POSIX-Shell-like expansion for ~/.
> + * The returned string is strdup'ed.
>   */
>  
> -#ifdef HAVE_WORDEXP_H
> -#include <wordexp.h>
>  #include <assert.h>
>  int snd_user_file(const char *file, char **result)
>  {
> -	wordexp_t we;
>  	int err;
> -	
> +	size_t len;
> +	char *buf = NULL;
> +
>  	assert(file && result);
> -	err = wordexp(file, &we, WRDE_NOCMD);
> -	switch (err) {
> -	case WRDE_NOSPACE:
> -		wordfree(&we);
> -		return -ENOMEM;
> -	case 0:
> -		if (we.we_wordc == 1)
> -			break;
> -		wordfree(&we);
> -		/* fall thru */
> -	default:
> -		return -EINVAL;
> +	*result = NULL;
> +
> +	/* expand ~/ if needed */
> +	if (file[0] == '~' && file[1] == '/') {
> +		const char *home = getenv("HOME");
> +		if (home == NULL) {
> +			struct passwd pwent, *p = NULL;
> +			uid_t id = getuid();
> +			size_t bufsize = 1024;
> +
> +			buf = malloc(bufsize);
> +			if (buf == NULL)
> +				goto out;
> +
> +			while ((err = getpwuid_r(id, &pwent, buf, bufsize, &p)) == ERANGE) {
> +				char *newbuf;
> +				bufsize += 1024;
> +				if (bufsize < 1024)
> +					break;
> +				newbuf = realloc(buf, bufsize);
> +				if (newbuf == NULL)
> +					goto out;
> +				buf = newbuf;
> +			}
> +			home = err ? "" : pwent.pw_dir;
> +		}
> +		len = strlen(home) + strlen(&file[2]) + 2;
> +		*result = malloc(len);
> +		if (*result)
> +			snprintf(*result, len, "%s/%s", home, &file[2]);
> +	} else {
> +		*result = strdup(file);
>  	}
> -	*result = strdup(we.we_wordv[0]);
> -	wordfree(&we);
> +
> +out:
> +	if (buf)
> +		free(buf);
> +
>  	if (*result == NULL)
>  		return -ENOMEM;
>  	return 0;
>  }
>  
> -#else /* !HAVE_WORDEXP_H */
> -/* just copy the string - would be nicer to expand by ourselves, though... */
> -int snd_user_file(const char *file, char **result)
> -{
> -	*result = strdup(file);
> -	if (! *result)
> -		return -ENOMEM;
> -	return 0;
> -}
> -#endif /* HAVE_WORDEXP_H */
> -- 
> 2.13.2
> 

Re: [PATCH - alsa-lib 1/1] snd_user_file: avoid use wordexp

[Natanael Copa]

Hi,

Thanks for your quick response!

On Wed, 12 Jul 2017 12:02:28 +0200
Takashi Iwai <tiwai@suse.de> wrote:

> On Wed, 12 Jul 2017 09:53:08 +0200,
> Natanael Copa wrote:
> > 
> > As suggested in POSIX[1], wordexp might execute the shell. If the libc
> > implementation does so, it will break the firefox sandbox which does
> > not allow exec. This happened on Alpine Linux with musl libc[2].
> > 
> > Since we cannot guarantee that the system wordexp implementation does
> > not execute shell, we cannot really use it, and need to implement the
> > ~/ expansion ourselves.
> > 
> > Generally, wordexp is a large attack vector and it is better to avoid it
> > since only tilde expansion is needed.
> > 
> > [1]: http://pubs.opengroup.org/onlinepubs/9699919799/functions/wordexp.html#tag_16_684_08
> > [2]: http://bugs.alpinelinux.org/issues/7454#note-2
> > 
> > Signed-off-by: Natanael Copa <ncopa@alpinelinux.org>  
> 
> Well, this may bring a functional regression, so I don't want to
> introduce this unconditionally.  Instead, we may introduce configure
> option to explicitly control the usage of wordexp(), and add your
> extension to the fallback code, for example.  If the security issue
> really matters, we can leave it disabled as default, too.

I just tested and can confirm that it does break "$HOME/.asoundrc"
which previously worked. I don't know if it is needed/desired but I
would guess that environment variables should be expanded with "@func
getenv" and that expansion of environment vars without @func getenv was
unintentional.

The command expansion is explicitly disabled with WRDE_NOCMD so that
means that things like "$(pwd)/.asoundrc" never worked.

Wildcard expansion did work sort of, but would only pick the first
match and not return all matches, so it was not really useful or
reliable.

So my conclusion was that the only needed expansion was ~/. This could
been implemented with glob() and GLOB_TILDE, but it is not supported in
POSIX (and ironically, POSIX says that you should use wordexp for tilde
expansion :)

I understand your worry for a functional regression, even if that
functionality is weird and unintended so I will prepare a patch with a
configure option for it.

Another option would be to drop support for ~/ expansion and use
@func getenv to expand $HOME, but not sure how that is done. I suppose
expanding ~/ directly like my patches does is fine.


Thanks!

-nc

Re: [PATCH - alsa-lib 1/1] snd_user_file: avoid use wordexp

[Takashi Iwai]

On Wed, 12 Jul 2017 14:54:08 +0200,
Natanael Copa wrote:
> 
> Hi,
> 
> Thanks for your quick response!
> 
> On Wed, 12 Jul 2017 12:02:28 +0200
> Takashi Iwai <tiwai@suse.de> wrote:
> 
> > On Wed, 12 Jul 2017 09:53:08 +0200,
> > Natanael Copa wrote:
> > > 
> > > As suggested in POSIX[1], wordexp might execute the shell. If the libc
> > > implementation does so, it will break the firefox sandbox which does
> > > not allow exec. This happened on Alpine Linux with musl libc[2].
> > > 
> > > Since we cannot guarantee that the system wordexp implementation does
> > > not execute shell, we cannot really use it, and need to implement the
> > > ~/ expansion ourselves.
> > > 
> > > Generally, wordexp is a large attack vector and it is better to avoid it
> > > since only tilde expansion is needed.
> > > 
> > > [1]: http://pubs.opengroup.org/onlinepubs/9699919799/functions/wordexp.html#tag_16_684_08
> > > [2]: http://bugs.alpinelinux.org/issues/7454#note-2
> > > 
> > > Signed-off-by: Natanael Copa <ncopa@alpinelinux.org>  
> > 
> > Well, this may bring a functional regression, so I don't want to
> > introduce this unconditionally.  Instead, we may introduce configure
> > option to explicitly control the usage of wordexp(), and add your
> > extension to the fallback code, for example.  If the security issue
> > really matters, we can leave it disabled as default, too.
> 
> I just tested and can confirm that it does break "$HOME/.asoundrc"
> which previously worked. I don't know if it is needed/desired but I
> would guess that environment variables should be expanded with "@func
> getenv" and that expansion of environment vars without @func getenv was
> unintentional.
> 
> The command expansion is explicitly disabled with WRDE_NOCMD so that
> means that things like "$(pwd)/.asoundrc" never worked.
> 
> Wildcard expansion did work sort of, but would only pick the first
> match and not return all matches, so it was not really useful or
> reliable.
> 
> So my conclusion was that the only needed expansion was ~/. This could
> been implemented with glob() and GLOB_TILDE, but it is not supported in
> POSIX (and ironically, POSIX says that you should use wordexp for tilde
> expansion :)
> 
> I understand your worry for a functional regression, even if that
> functionality is weird and unintended so I will prepare a patch with a
> configure option for it.
> 
> Another option would be to drop support for ~/ expansion and use
> @func getenv to expand $HOME, but not sure how that is done. I suppose
> expanding ~/ directly like my patches does is fine.

Yeah, I *guess* it suffices in most cases, but who knows what kind of
weird setup users have :)


thanks,

Takashi

[PATCH v2 - alsa-lib] snd_user_file: avoid use wordexp

[Natanael Copa]

As suggested in POSIX[1], wordexp might execute the shell. If the libc
implementation does so, it will break the firefox sandbox which does
not allow exec. This happened on Alpine Linux with musl libc[2].

Since we cannot guarantee that the system wordexp implementation does
not execute shell, we cannot really use it, and need to implement the
~/ expansion ourselves.

We provide a configure option --with-wordexp for users that still may
need it, but we leave this off by default because wordexp is a large
large attack vector and it is better to avoid it.

[1]: http://pubs.opengroup.org/onlinepubs/9699919799/functions/wordexp.html#tag_16_684_08
[2]: http://bugs.alpinelinux.org/issues/7454#note-2

Signed-off-by: Natanael Copa <ncopa@alpinelinux.org>
---
changes v2:
 - add configure option to enable old behaviour which uses wordexp.
   this is off by default.

I was not sure if I should use --with-wordexp or --enable-wordexp but
went with --with-wordexp similar to --with-softfloat.

 configure.ac   | 19 ++++++++++++++++-
 src/userfile.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 75 insertions(+), 9 deletions(-)

diff --git a/configure.ac b/configure.ac
index 26e5d125..fbcfa829 100644
--- a/configure.ac
+++ b/configure.ac
@@ -303,8 +303,25 @@ fi
 
 AC_SUBST(ALSA_DEPLIBS)
 
+dnl Check for use of wordexp...
+AC_MSG_CHECKING(for use of wordexp)
+AC_ARG_WITH(wordexp,
+  AS_HELP_STRING([--with-wordexp],
+    [Use wordexp when expanding configs (default = no)]),
+  [case "$withval" in
+	y|yes) wordexp=yes ;;
+	*) wordexp=no ;;
+   esac],)
+if test "$wordexp" = "yes" ; then
+  AC_DEFINE(HAVE_WORDEXP, "1", [Enable use of wordexp])
+  AC_MSG_RESULT(yes)
+  AC_CHECK_HEADER([wordexp.h],[], [AC_MSG_ERROR([Couldn't find wordexp.h])])
+else
+  AC_MSG_RESULT(no)
+fi
+
 dnl Check for headers
-AC_CHECK_HEADERS([wordexp.h endian.h sys/endian.h sys/shm.h])
+AC_CHECK_HEADERS([endian.h sys/endian.h sys/shm.h])
 
 dnl Check for resmgr support...
 AC_MSG_CHECKING(for resmgr support)
diff --git a/src/userfile.c b/src/userfile.c
index 72779da4..f2145470 100644
--- a/src/userfile.c
+++ b/src/userfile.c
@@ -21,6 +21,7 @@
 #include <config.h>
 #include <string.h>
 #include <errno.h>
+#include <assert.h>
 
 /**
  * \brief Get the full file name
@@ -32,14 +33,13 @@
  * stores the first matchine one.  The returned string is strdup'ed.
  */
 
-#ifdef HAVE_WORDEXP_H
+#ifdef HAVE_WORDEXP
 #include <wordexp.h>
-#include <assert.h>
 int snd_user_file(const char *file, char **result)
 {
 	wordexp_t we;
 	int err;
-	
+
 	assert(file && result);
 	err = wordexp(file, &we, WRDE_NOCMD);
 	switch (err) {
@@ -61,13 +61,62 @@ int snd_user_file(const char *file, char **result)
 	return 0;
 }
 
-#else /* !HAVE_WORDEXP_H */
-/* just copy the string - would be nicer to expand by ourselves, though... */
+#else /* !HAVE_WORDEX */
+
+#include <sys/types.h>
+#include <unistd.h>
+#include <pwd.h>
+#include <stdio.h>
+#include <stdlib.h>
+
 int snd_user_file(const char *file, char **result)
 {
-	*result = strdup(file);
-	if (! *result)
+	int err;
+	size_t len;
+	char *buf = NULL;
+
+	assert(file && result);
+	*result = NULL;
+
+	/* expand ~/ if needed */
+	if (file[0] == '~' && file[1] == '/') {
+		const char *home = getenv("HOME");
+		if (home == NULL) {
+			struct passwd pwent, *p = NULL;
+			uid_t id = getuid();
+			size_t bufsize = 1024;
+
+			buf = malloc(bufsize);
+			if (buf == NULL)
+				goto out;
+
+			while ((err = getpwuid_r(id, &pwent, buf, bufsize, &p)) == ERANGE) {
+				char *newbuf;
+				bufsize += 1024;
+				if (bufsize < 1024)
+					break;
+				newbuf = realloc(buf, bufsize);
+				if (newbuf == NULL)
+					goto out;
+				buf = newbuf;
+			}
+			home = err ? "" : pwent.pw_dir;
+		}
+		len = strlen(home) + strlen(&file[2]) + 2;
+		*result = malloc(len);
+		if (*result)
+			snprintf(*result, len, "%s/%s", home, &file[2]);
+	} else {
+		*result = strdup(file);
+	}
+
+out:
+	if (buf)
+		free(buf);
+
+	if (*result == NULL)
 		return -ENOMEM;
 	return 0;
 }
-#endif /* HAVE_WORDEXP_H */
+
+#endif /* HAVE_WORDEXP */
-- 
2.13.2

Re: [PATCH v2 - alsa-lib] snd_user_file: avoid use wordexp

[Takashi Iwai]

On Fri, 14 Jul 2017 18:47:05 +0200,
Natanael Copa wrote:
> 
> As suggested in POSIX[1], wordexp might execute the shell. If the libc
> implementation does so, it will break the firefox sandbox which does
> not allow exec. This happened on Alpine Linux with musl libc[2].
> 
> Since we cannot guarantee that the system wordexp implementation does
> not execute shell, we cannot really use it, and need to implement the
> ~/ expansion ourselves.
> 
> We provide a configure option --with-wordexp for users that still may
> need it, but we leave this off by default because wordexp is a large
> large attack vector and it is better to avoid it.
> 
> [1]: http://pubs.opengroup.org/onlinepubs/9699919799/functions/wordexp.html#tag_16_684_08
> [2]: http://bugs.alpinelinux.org/issues/7454#note-2
> 
> Signed-off-by: Natanael Copa <ncopa@alpinelinux.org>
> ---
> changes v2:
>  - add configure option to enable old behaviour which uses wordexp.
>    this is off by default.
> 
> I was not sure if I should use --with-wordexp or --enable-wordexp but
> went with --with-wordexp similar to --with-softfloat.

That's OK, a matter of taste.

Applied now as is.  Thanks.


Takashi