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

* Re: [PATCH - alsa-lib 1/1] snd_user_file: avoid use wordexp
       [not found] <20170712075308.29347-1-ncopa@alpinelinux.org>
@ 2017-07-12 10:02 ` Takashi Iwai
  2017-07-12 12:54   ` Natanael Copa
  2017-07-14 16:47   ` [PATCH v2 - alsa-lib] " Natanael Copa
  0 siblings, 2 replies; 6+ messages in thread
From: Takashi Iwai @ 2017-07-12 10:02 UTC (permalink / raw)
  To: Natanael Copa; +Cc: alsa-devel

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
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread