On 03/09/2011 01:54 AM, Philipp Hahn wrote:
> At least protect the $uri variable against further expansion by properly
> quoting it. While doing that, also quote all other variables to protect
> against shell meta characters.
Hmm. Valid URIs can contain '?', which _is_ a glob character, so I
agree that we need to quote "$uri" everywhere that we have already
correctly obtatined it in order to avoid inadvertent globbing.
Meanwhile, $URIS is indeed a user-settable variable, via
/etc/sysconfig/libvirt-guests, so we should want to protect ourselves
from malicious input. (On the other hand, we already blindly source
/etc/sysconfig/libvirt-guests without validating that it contains _only_
shell assignments, so a malicious user could already put arbitrary shell
code in there. The real way to be safe would be to parse out _just_ the
assignments we care about, using sed or read, and eval those assignments
rather than sourcing the entire file. I guess we have to draw a line
somewhere at how much paranoia we want to express. If we can trust that
/etc/sysconfig/libvirt-guests can't be subverted by a malicious user,
then we can trust that it won't contain a malicious $URIS and instead
chalk up bugs in that file as system administration shortcomings.)
However, that points out a shortcoming in your patch - we already do
constructs like
for uri in $URIS; do
which _already_ corrupted any globbing and performed IFS splitting, at
which point $uri would not contain any further globbing (except in the
weird case of a glob that expanded to an existing file name which
happens to also be a valid glob), so quoting just "$uri" as in your
patch is a lost cause unless we address the problem from the point of
the user input to begin with.
Meanwhile, we already know that a valid URI does not contain whitespace
(a valid URI can represent whitespace via encoding, where needed), so
it's already acceptable to use IFS splitting on $URIS to break it into
individual URIs, it's just that we need to suppress globbing in the
process. 'set -f' can be used for this, if needed.
If we could use bash shell arrays, it would be a matter of creating an
array variable ${URI[]} that contains the individual URIs, doing the
work of safely splitting things only once. But libvirt-guests has to
run under /bin/sh == dash for Debian, which lacks shell arrays.
I'm still thinking about how to handle this one...
Meanwhile, I'm not a fan of blindly quoting everything; there are
documented cases where you can trigger shell bugs by doing too much
quoting. For example:
foo=`some_command "with quotes"`
is portable, but
foo="`some_command "with quotes"`"
is not. So I prefer to quote variables that come from external source,
but not internal variables that are obviously safe (that said, quoting
generally doesn't hurt, so even if something is overkill does not meant
that I am rejecting the patch).
> +++ b/tools/libvirt-guests.init.sh
> @@ -66,12 +66,10 @@ run_virsh() {
> shift
>
> if [ "x$uri" = xdefault ]; then
> - conn=
> + virsh "$@" </dev/null
> else
> - conn="-c $uri"
> + virsh -c "$uri" "$@" </dev/null
> fi
> -
> - virsh $conn "$@" </dev/null
This part's good.
> @@ -89,7 +87,7 @@ list_guests() {
>
> uuids=
> for id in $(echo "$list" | awk 'NR > 2 {print $1}'); do
> - uuid=$(run_virsh_c $uri dominfo $id | awk '/^UUID:/{print $2}')
> + uuid=$(run_virsh_c "$uri" dominfo "$id" | awk '/^UUID:/{print $2}')
Quoting "$id" is overkill; we know it's numeric. But it probably
doesn't hurt.
> - name=$(run_virsh_c $uri dominfo $uuid 2>/dev/null | \
> + name=$(run_virsh_c "$uri" dominfo "$uuid" 2>/dev/null | \
Likewise, if we know $uuid is valid, then it contains neither $IFS nor
globbing characters. Should we instead be tightening up the list_guests
function to guarantee that we never encounter garbage data?
> @@ -146,25 +144,25 @@ start() {
> while read uri list; do
> configured=false
> for confuri in $URIS; do
> - if [ $confuri = $uri ]; then
> + if [ "$confuri" = "$uri" ]; then
To be really robust (for example, $confuri could be '(', which is known
to trip up some buggy test(1) implementations), this should be:
[ "x$confuri" = "x$uri" ]
> configured=true
> break
> fi
> done
> - if ! $configured; then
> + if ! "$configured"; then
This one's overkill - we explicitly set $configured to either 'true' or
'false'; no external data possible.
> virsh_pid=$!
> while true; do
> sleep 1
> - kill -0 $virsh_pid >/dev/null 2>&1 || break
> - progress=$(run_virsh_c $uri domjobinfo $guest 2>/dev/null | \
> + kill -0 "$virsh_pid" >/dev/null 2>&1 || break
> + progress=$(run_virsh_c "$uri" domjobinfo "$guest" 2>/dev/null | \
Overkill - we know $virsh_pid is numeric.
> timeout=$SHUTDOWN_TIMEOUT
> - while [ $timeout -gt 0 ]; do
> + while [ "$timeout" -gt 0 ]; do
Reasonable, since SHUTDOWN_TIMEOUT might come from external sources.
> sleep 1
> timeout=$((timeout - 1))
> - guest_is_on $uri $guest || return
> - $guest_running || break
> - printf '\r%s%-12d ' "$label" $timeout
> + guest_is_on "$uri" "$guest" || return
> + "$guest_running" || break
> + printf '\r%s%-12d ' "$label" "$timeout"
Overkill, since once you are inside the while loop, you are guaranteed
that $timeout is now numeric.
> done
>
> - if guest_is_on $uri $guest; then
> - if $guest_running; then
> + if guest_is_on "$uri" "$guest"; then
> + if "$guest_running"; then
Overkill, since $guest_running is under our control.
>
> - list=$(list_guests $uri)
> + list=$(list_guests "$uri")
> if [ $? -eq 0 ]; then
> empty=true
> for uuid in $list; do
> - $empty || printf ", "
> - printf %s "$(guest_name $uri $uuid)"
> + "$empty" || printf ", "
Overkill, since $empty is under our control.
>
> while read uri list; do
> - if $suspending; then
> + if "$suspending"; then
Likewise.
> @@ -330,7 +328,7 @@ case "$1" in
> usage 0
> ;;
> start|stop|gueststatus)
> - $1
> + "$1"
Overkill, since we know that $1 is one of three safe values.
I think I'll apply your patch as-is, in spite of the overkill, then
worry about how to safely split $URIS as a separate patch.
--
Eric Blake eblake@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list