Hello, Eric Blake <ebl...@redhat.com> 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
