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

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Neven Sajko
automake version 1.15.1, also in latest git master.

See

https://git.savannah.gnu.org/cgit/automake.git/tree/lib/install-sh#n327

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

So line 327 is probably wrong.

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"`

If this fix is correct, maybe you would also like to update
https://git.savannah.gnu.org/cgit/automake.git/tree/lib/config.guess#n104


Regards



Reply | Threaded
Open this post in threaded view
|

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

Eric Blake-3
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 (632 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Mathieu Lirzin
Hello,

Eric Blake <[hidden email]> 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