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

[Mathieu Lirzin]

Hello,

Eric Blake <address@hidden> writes:

> 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.

I agree with Eric reasoning.

Thanks for the report.

-- 
Mathieu Lirzin
GPG: F2A3 8D7E EB2B 6640 5761  070D 0ADE E100 9460 4D37