[pacman-dev] [PATCH] makepkg: do not eval dlcmd

[Dave Reisner]

On Sat, Sep 06, 2014 at 01:30:54AM +0800, lolilolicon wrote:
> Something like `source=('$pkgname-$pkgver.tar.gz')` in the PKGBUILD
> would "just work" with this eval. This could be a security issue since
> URLs are arbitrary external data.

Sounds somewhat contrived. External data, yes, but then you might
consider that the *entire* PKGBUILD is external data which is fed to
makepkg. What makes this particular case special? Is there an immediate
problem that this fixes?

> Instead, expand the dlagent command line into an array, replace the %o,
> %u place holders, and run the resultant command line as is.
> 
> A shortcoming of this is the dlagent command line cannot have embedded
> spaces. If this turns out to be a problem, we can do an early eval
> before substituting %o and %u.
> ---
>  scripts/makepkg.sh.in | 30 ++++++++++++++----------------
>  1 file changed, 14 insertions(+), 16 deletions(-)
> 
> diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
> index 8e8a64c..a134453 100644
> --- a/scripts/makepkg.sh.in
> +++ b/scripts/makepkg.sh.in
> @@ -288,12 +288,12 @@ get_protocol() {
>  get_downloadclient() {
>  	local proto=$1
>  
> -	# loop through DOWNLOAD_AGENTS variable looking for protocol
> +	# loop through DLAGENTS variable looking for protocol
>  	local i
>  	for i in "${DLAGENTS[@]}"; do
>  		local handler="${i%%::*}"
>  		if [[ $proto = "$handler" ]]; then
> -			local agent="${i##*::}"
> +			local agent="${i#*::}"

While I agree with this change (and other similar fixups), it seems
unrelated to the rest of the commit...

>  			break
>  		fi
>  	done
> @@ -308,8 +308,7 @@ get_downloadclient() {
>  	# ensure specified program is installed
>  	local program="${agent%% *}"
>  	if [[ ! -x $program ]]; then
> -		local baseprog="${program##*/}"
> -		error "$(gettext "The download program %s is not installed.")" "$baseprog"
> +		error "$(gettext "The download program '%s' is not installed.")" "$program"
>  		plain "$(gettext "Aborting...")"
>  		exit 1 # $E_MISSING_PROGRAM
>  	fi
> @@ -342,15 +341,16 @@ download_file() {
>  	local proto=$(get_protocol "$netfile")
>  
>  	# find the client we should use for this URL
> -	local dlcmd
> -	dlcmd=$(get_downloadclient "$proto") || exit $?
> +	local -a cmdline
> +	local IFS=' '

How sure are you that this won't affect some later operation in the
function?

> +	cmdline=($(get_downloadclient "$proto")) || exit

Why not use read to split the result?

  IFS=' ' read -a cmdline < <(get_downloadclient "$proto")
  [[ $cmdline ]] || exit

>  	local filename=$(get_filename "$netfile")
>  	local url=$(get_url "$netfile")
>  
>  	if [[ $proto = "scp" ]]; then
>  		# scp downloads should not pass the protocol in the url
> -		url="${url##*://}"
> +		url="${url#*://}"
>  	fi
>  
>  	msg2 "$(gettext "Downloading %s...")" "$filename"
> @@ -359,20 +359,18 @@ download_file() {
>  	local dlfile="${url##*/}"
>  
>  	# replace %o by the temporary dlfile if it exists
> -	if [[ $dlcmd = *%o* ]]; then
> -		dlcmd=${dlcmd//\%o/\"$filename.part\"}
> -		dlfile="$filename.part"
> +	if [[ ${cmdline[*]} = *%o* ]]; then
> +		dlfile=$filename.part
> +		cmdline=("${cmdline[@]//%o/"$dlfile"}")
>  	fi
>  	# add the URL, either in place of %u or at the end
> -	if [[ $dlcmd = *%u* ]]; then
> -		dlcmd=${dlcmd//\%u/\"$url\"}
> +	if [[ ${cmdline[*]} = *%u* ]]; then
> +		cmdline=("${cmdline[@]//%u/"$url"}")
>  	else
> -		dlcmd="$dlcmd \"$url\""
> +		cmdline+=("$url")
>  	fi
>  
> -	local ret=0
> -	eval "$dlcmd >&2 || ret=\$?"
> -	if (( ret )); then
> +	if ! command -- "${cmdline[@]}" >&2; then
>  		[[ ! -s $dlfile ]] && rm -f -- "$dlfile"
>  		error "$(gettext "Failure while downloading %s")" "$filename"
>  		plain "$(gettext "Aborting...")"
> -- 
> 2.1.0
>