| Message ID | 1804081611300.19491@stax.localdomain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, 08 Apr 2018 18:13:43 +0200, Mark Hills wrote: > > I just came up against the patch below; it prevents useful snippets of > alsa-conf like this: > > @hooks [ > { > func load > files [ > "~/.asoundrc-$HOSTNAME" > ] > errors false > } > ] > > as the evalutation of all but "~" has been removed. > > Seems like removal of a perfectly good feature in the name of security; > because wordexp() > > 1) is not used (and should not be used) on data originating from an > untrusted source > > 2) is already used with WRDE_NOCMD, which the same POSIX spec documents > as: > > "The WRDE_NOCMD flag is provided for applications that, for security > or other reasons, want to prevent a user from executing shell > commands." > > 3) on glibc can be seen (with strace) not to execute other commands > > If one is to treat the POSIX doc as gospel (as cited by the patch) the > cause of firefox (circa July 2017) not working would actually be that musl > does not honour WRDE_NOCMD to the letter. I agree the spec of wordexp() > could be more useful, though. > > Also, hypothesising the attacks of an already-compromised application > would get into a sticky conversation about the thread safety of > getenv("HOME") (and associated buffer wrangling) vs. a library function > being used for its intended purpose. > > In practice, Firefox may have moved on here (no ALSA support anymore) so > should quirks of its sandbox be driving this? What's wrong with you building the alsa-lib with --with-wordexp if you prefer having that behavior? Takashi
On Mon, 9 Apr 2018, Takashi Iwai wrote: > On Sun, 08 Apr 2018 18:13:43 +0200, > Mark Hills wrote: > > > > I just came up against the patch below; it prevents useful snippets of > > alsa-conf like this: > > > > @hooks [ > > { > > func load > > files [ > > "~/.asoundrc-$HOSTNAME" > > ] > > errors false > > } > > ] > > > > as the evalutation of all but "~" has been removed. > > > > Seems like removal of a perfectly good feature in the name of security; > > because wordexp() > > > > 1) is not used (and should not be used) on data originating from an > > untrusted source > > > > 2) is already used with WRDE_NOCMD, which the same POSIX spec documents > > as: > > > > "The WRDE_NOCMD flag is provided for applications that, for security > > or other reasons, want to prevent a user from executing shell > > commands." > > > > 3) on glibc can be seen (with strace) not to execute other commands > > > > If one is to treat the POSIX doc as gospel (as cited by the patch) the > > cause of firefox (circa July 2017) not working would actually be that musl > > does not honour WRDE_NOCMD to the letter. I agree the spec of wordexp() > > could be more useful, though. > > > > Also, hypothesising the attacks of an already-compromised application > > would get into a sticky conversation about the thread safety of > > getenv("HOME") (and associated buffer wrangling) vs. a library function > > being used for its intended purpose. > > > > In practice, Firefox may have moved on here (no ALSA support anymore) so > > should quirks of its sandbox be driving this? > > What's wrong with you building the alsa-lib with --with-wordexp if you > prefer having that behavior? Practically, I must build custom packages for all machines (some I do not control, eg. my employer's) My case here is both that the default behaviour should not have changed; and that the security rationale offered here is misleading.
On Fri, 13 Apr 2018 13:58:56 +0200, Mark Hills wrote: > > On Mon, 9 Apr 2018, Takashi Iwai wrote: > > > On Sun, 08 Apr 2018 18:13:43 +0200, > > Mark Hills wrote: > > > > > > I just came up against the patch below; it prevents useful snippets of > > > alsa-conf like this: > > > > > > @hooks [ > > > { > > > func load > > > files [ > > > "~/.asoundrc-$HOSTNAME" > > > ] > > > errors false > > > } > > > ] > > > > > > as the evalutation of all but "~" has been removed. > > > > > > Seems like removal of a perfectly good feature in the name of security; > > > because wordexp() > > > > > > 1) is not used (and should not be used) on data originating from an > > > untrusted source > > > > > > 2) is already used with WRDE_NOCMD, which the same POSIX spec documents > > > as: > > > > > > "The WRDE_NOCMD flag is provided for applications that, for security > > > or other reasons, want to prevent a user from executing shell > > > commands." > > > > > > 3) on glibc can be seen (with strace) not to execute other commands > > > > > > If one is to treat the POSIX doc as gospel (as cited by the patch) the > > > cause of firefox (circa July 2017) not working would actually be that musl > > > does not honour WRDE_NOCMD to the letter. I agree the spec of wordexp() > > > could be more useful, though. > > > > > > Also, hypothesising the attacks of an already-compromised application > > > would get into a sticky conversation about the thread safety of > > > getenv("HOME") (and associated buffer wrangling) vs. a library function > > > being used for its intended purpose. > > > > > > In practice, Firefox may have moved on here (no ALSA support anymore) so > > > should quirks of its sandbox be driving this? > > > > What's wrong with you building the alsa-lib with --with-wordexp if you > > prefer having that behavior? > > Practically, I must build custom packages for all machines (some I do not > control, eg. my employer's) > > My case here is both that the default behaviour should not have changed; > and that the security rationale offered here is misleading. How can you assure you'll never hit a badly written wordexp() in the wild old binary? Takashi
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 */
I just came up against the patch below; it prevents useful snippets of alsa-conf like this: @hooks [ { func load files [ "~/.asoundrc-$HOSTNAME" ] errors false } ] as the evalutation of all but "~" has been removed. Seems like removal of a perfectly good feature in the name of security; because wordexp() 1) is not used (and should not be used) on data originating from an untrusted source 2) is already used with WRDE_NOCMD, which the same POSIX spec documents as: "The WRDE_NOCMD flag is provided for applications that, for security or other reasons, want to prevent a user from executing shell commands." 3) on glibc can be seen (with strace) not to execute other commands If one is to treat the POSIX doc as gospel (as cited by the patch) the cause of firefox (circa July 2017) not working would actually be that musl does not honour WRDE_NOCMD to the letter. I agree the spec of wordexp() could be more useful, though. Also, hypothesising the attacks of an already-compromised application would get into a sticky conversation about the thread safety of getenv("HOME") (and associated buffer wrangling) vs. a library function being used for its intended purpose. In practice, Firefox may have moved on here (no ALSA support anymore) so should quirks of its sandbox be driving this?