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

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 input. This also violated the principle of least surprise. Instead, expand the dlagent command line into an array, replace the %o, %u place holders, and run the resultant command line as is. Embedded spaces in the DLAGENT entry can be escaped with a backslash. --- scripts/makepkg.sh.in | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 8e8a64c..2b05468 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -342,8 +342,9 @@ 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 + IFS=' ' read -a cmdline < <(get_downloadclient "$proto") && + (( ${#cmdline[@]} )) || exit local filename=$(get_filename "$netfile") local url=$(get_url "$netfile") @@ -359,20 +360,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

On Sat, Sep 06, 2014 at 04:42:08AM +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 input. This also violated the principle of least surprise.

I still say this is security theater. The point about "least surprise" is more convincing.

Instead, expand the dlagent command line into an array, replace the %o, %u place holders, and run the resultant command line as is.

Embedded spaces in the DLAGENT entry can be escaped with a backslash. --- scripts/makepkg.sh.in | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 8e8a64c..2b05468 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -342,8 +342,9 @@ 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 + IFS=' ' read -a cmdline < <(get_downloadclient "$proto") && + (( ${#cmdline[@]} )) || exit

The exit status of read isn't something you want to rely on here -- it can return non-zero on EOF. Please just rely on the cmdline array being populated or not to determine success. Looks fine otherwise.

local filename=$(get_filename "$netfile") local url=$(get_url "$netfile") @@ -359,20 +360,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

On 08/09/14 02:57, lolilolicon wrote:

This eval enables the following in a PKGBUILD to "just work":

source=('$pkgname-$pkgver.tar.gz'::'https://host/$pkgver.tar.gz')

This has at least two problems:

- It violated the principle of least surprise. - It could be a security issue since URLs are arbitrary input.

Instead, expand the dlagent command line into an array, replace the %o, %u place holders, and run the resultant command line as is.

Embedded spaces in the DLAGENTS entry can be escaped with a backslash.

Fixes FS#41682 ---

This is broken with bash<4.3. There are extra quotes everywhere in the command. bash-4.2 + /usr/bin/curl -fLC - --retry 3 --retry-delay 3 -o '"64.tar.xz.part"' '"http://allanmcrae.com/tmp/64.tar.xz"' bash-4.3 + /usr/bin/curl -fLC - --retry 3 --retry-delay 3 -o 64.tar.xz.part http://allanmcrae.com/tmp/64.tar.xz @Dave: any suggestions on how to fix.

scripts/makepkg.sh.in | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 8e8a64c..446e9b0 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -342,8 +342,9 @@ 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 + IFS=' ' read -a cmdline < <(get_downloadclient "$proto") + (( ${#cmdline[@]} )) || exit

local filename=$(get_filename "$netfile") local url=$(get_url "$netfile") @@ -359,20 +360,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...")"