GNU bug report logs - #28317
Non-portable sh script - $RANDOM

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 28317 in the body.
You can then email your comments to 28317 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Neven Sajko <nsajko <at> gmail.com>
To: bug-automake <at> gnu.org
Subject: Non-portable sh script - $RANDOM
Date: Fri, 1 Sep 2017 14:17:38 +0200

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

Message #12 received at 28317-done <at> debbugs.gnu.org (full text, mbox):

From: Eric Blake <eblake <at> redhat.com>
To: Neven Sajko <nsajko <at> gmail.com>, 28317-done <at> debbugs.gnu.org
Subject: Re: bug#28317: Non-portable sh script - $RANDOM
Date: Fri, 1 Sep 2017 12:13:39 -0500

[Message part 1 (text/plain, inline)]

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 (application/pgp-signature, attachment)]

Message #15 received at 28317 <at> debbugs.gnu.org (full text, mbox):

From: Mathieu Lirzin <mthl <at> gnu.org>
To: nsajko <at> gmail.com
Cc: eblake <at> redhat.com, 28317 <at> debbugs.gnu.org
Subject: Re: bug#28317: Non-portable sh script - $RANDOM
Date: Fri, 01 Sep 2017 19:40:09 +0200

Hello,

Eric Blake <eblake <at> 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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.