[pacman-dev] [PATCH] makepkg: do not eval dlcmd
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. 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#*::}" 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=' ' + cmdline=($(get_downloadclient "$proto")) || 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
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
On 09/05/14 at 03:35pm, Dave Reisner wrote:
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...
There is also an existing patch that already does this: https://patchwork.archlinux.org/patch/2150/ apg
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
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 --- 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...")" -- 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...")"
On Wed, Jan 07, 2015 at 02:24:19PM +1000, Allan McRae wrote:
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.
I'm not clear on what the problematic quoting actually fixes. If it actually has a purpose, I'm fairly confident that we can just neglect the esoteric edge case it covers, and leave behind a TODO that we should re-fix it once we're comfortable with bash 4.3 being the required minimum.
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...")"
On Sat, Sep 6, 2014 at 3:35 AM, 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?
As I reported in bug #41682, it is surprising to have the URL eval'ed so unexpectedly. Ask a good PKGBUILD writer, what does he expect from that PKGBUILD?
- 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...
I removed this in v2. My bad habit of casually fixing random stuff "in scope"...
+ local IFS=' '
How sure are you that this won't affect some later operation in the function?
I'll bet you a beer and a candy bar that a bad URL will break shit sooner than this will happen.
+ cmdline=($(get_downloadclient "$proto")) || exit
Why not use read to split the result?
IFS=' ' read -a cmdline < <(get_downloadclient "$proto") [[ $cmdline ]] || exit
Good point. Adopted.
participants (4)
-
Allan McRae
-
Andrew Gregory
-
Dave Reisner
-
lolilolicon