bug#28317: Non-portable sh script - $RANDOM

[Eric Blake]

tag 28317 notabug
thanks

On 09/01/2017 07:17 AM, Neven Sajko wrote:
> automake version 1.15.1, also in latest git master.
> 
> See
> 
> https://git.savannah.gnu.org/cgit/automake.git/tree/lib/install-sh#n327

Let's look at it in context:

          *)
            tmpdir=${TMPDIR-/tmp}/ins$RANDOM-$$
            trap 'ret=$?; rmdir "$tmpdir/d" "$tmpdir" 2>/dev/null; exit
$ret' 0

            if (umask $mkdir_umask &&
                exec $mkdirprog $mkdir_mode -p -- "$tmpdir/d")
>/dev/null 2>&1
            then
              if test -z "$dir_arg" || {
                   # Check for POSIX incompatibilities with -m.
                   # HP-UX 11.23 and IRIX 6.5 mkdir -m -p sets group- or
                   # other-writable bit of parent directory when it
shouldn't.
                   # FreeBSD 6.1 mkdir -m -p sets mode of existing
directory.
                   ls_ld_tmpdir=`ls -ld "$tmpdir"`
                   case $ls_ld_tmpdir in
                     d????-?r-*) different_mode=700;;
                     d????-?--*) different_mode=755;;
                     *) false;;
                   esac &&
                   $mkdirprog -m$different_mode -p -- "$tmpdir" && {
                     ls_ld_tmpdir_1=`ls -ld "$tmpdir"`
                     test "$ls_ld_tmpdir" = "$ls_ld_tmpdir_1"
                   }
                 }
              then posix_mkdir=:
              fi
              rmdir "$tmpdir/d" "$tmpdir"
            else
              # Remove any dirs left behind by ancient mkdir
implementations.
              rmdir ./$mkdir_mode ./-p ./-- 2>/dev/null
            fi
            trap '' 0;;
        esac;;

> 
> The RANDOM variable giving pseudo-random numbers is not a POSIX sh
> feature. Dash, for example, does not implement it.

Correct. But for shells that do not implement it, it will expand to the
empty string, at which point we are merely naming our directory ins-$$,
which is still a (somewhat) random name, because it depends on the pid.
True, when $RANDOM is not supported, it's easier to guess the name being
used, but the REAL test is whether the code correctly handles the case
where an attacker races with your probe of an available name and your
subsequent use of the name.  _This_ code is specifically calling mkdir
(which is race-free) as the only use of $tmpdir, and therefore, even
when $RANDOM is not supported, we are not opening ourselves to attack.

Therefore, even though we know it is not POSIX, we also don't care.
This is not a shortcoming that needs to be patched.

> Maybe this would work instead:
> 
> random=`dd 'if=/dev/urandom' 'count=1' 'bs=256' 2>/dev/null | cksum | sed 
> "$r"`\
> `date -u | cksum | sed "$r"`

No, there's no need to furrther complicate something that is already
correct even when $RANDOM is empty.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature