Re: [alsa-devel] [PATCH - alsa-lib 1/1] snd_user_file: avoid use wordexp
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.
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
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.
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
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.
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
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_1... [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 */
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.
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
participants (2)
-
Natanael Copa -
Takashi Iwai