Re: $RANDOM not Cryptographically secure pseudorandom number generator

2018-12-15 Thread Ole Tange
On Mon, Dec 3, 2018 at 9:18 PM Chet Ramey  wrote:
> On 12/3/18 11:31 AM, Ole Tange wrote:
> > On Mon, Dec 3, 2018 at 3:56 PM Chet Ramey  wrote:
> >
> >> There has to be a compelling reason to change this, especially at a point
> >> so close to a major release.

I would think that a major release would be the perfect opportunity to
change this: Major releases in general are known for not being 100%
compatible with earlier releases.

> > The reason for my submission was that I needed a bunch of random
> > numbers in a shell script, but I needed them to be high quality.
> > Luckily I did not just assume that Bash delivers high quality random
> > numbers, but I read the source code, and then found that the quality
> > was low. I do not think must users would do that.
>
> This is always requirements-driven. Nobody expects to get cryptographic-
> quality PRNGs out of the shell (or any of the libc interfaces, tbh),

While I did not *expect* it, I honestly had hoped for it. Otherwise I
would never have raised this.

I feel a bit as if I am saying: "Hey this using environment variables
to store function definitions seems like it could be a problem, but I
do not have an exploit. I do, however, have an easy fix so that it
will not be a problem in the future."

And you replying: "Come back when you have an exploit."

And then we simply wait for Shellshock to happen.

> that's never been promised or expected. You can't really expect that from
> something that only promises 16 bits.

The naive user may assume that he can simply concatenate values and
get 128 bits:

echo $RANDOM-$RANDOM-$RANDOM-$RANDOM-$RANDOM-$RANDOM-$RANDOM-$RANDOM

But I hope we agree that he will not get 128 bits of randomness no
matter how many values he concatenates.

Or he might expect that this is not an infinite loop:

  while [ ! $RANDOM = $RANDOM ] ; do true; done

just like this is not:

  while [ ! $RANDOM = $(( 1+$RANDOM )) ] ; do true; done

(This one came as a surprise to me - I had totally expected $RANDOM
would give the same value twice 1 time in 65536 tries on average.
Tested on 4.4.19)

At the very least make it clear from the documentation what $RANDOM
can be used for. The man page does not warn about the low quality
either, and it does not point to a way to get high quality numbers.
Somehow we expect the user to simply know this without giving him even
a hint about this.

> However, for common scripting tasks like generating temporary filenames,
> it's perfectly adequate.

I hope that we agree that you should never use $RANDOM for generating
temporary file names in a dir that an attacker has write access to.
mktemp is made to do that in a secure fashion.

But your comment actually emphasizes my point: We _will_ have users
who are naive enough to use $RANDOM in ways you and I would not do,
because we know it is unsafe.

Let's make those usages a little safer.


/Ole



Re: $RANDOM not Cryptographically secure pseudorandom number generator

2018-12-15 Thread Eduardo Bustamante
On Sat, Dec 15, 2018 at 6:08 PM Ole Tange  wrote:
(...)
> But your comment actually emphasizes my point: We _will_ have users
> who are naive enough to use $RANDOM in ways you and I would not do,
> because we know it is unsafe.
>
> Let's make those usages a little safer.

You know no one is stopping you from submitting a patch to actually
fix the documentation right? (or maybe, you know, submitting an actual
working patch to change the random generator, not just drop some
irrelevant code snippet you got from Wikipedia).

> And then we simply wait for Shellshock to happen.

Also, comparing this to shellshock is a huge strawman. Please don't do
that :), we all know better than that.



Bash build issues in `devel' branch due to -Werror compiler flag

2018-12-15 Thread Eduardo A . Bustamante López
Commit 9d80be9ab5cc17011c634e0348c64c15fcba95bf adds the following compiler 
flag:

  dualbus@debian:~/src/gnu/bash$ cat -n configure.ac | grep Werror -C3
1159CFLAGS="$CFLAGS -Wno-parentheses -Wno-format-security"
1160if test -n "$DEBUG"
1161then
  ~>1162CFLAGS="$CFLAGS -Werror"
1163fi
1164fi
1165

During build (with gcc):

  dualbus@debian:~/src/gnu/bash$ make -j$(nproc) -s
  make[1]: warning: -j4 forced in submake: resetting jobserver mode.
  ./parse.y: warning: 1 shift/reduce conflict [-Wconflicts-sr]
  make[1]: warning: -j4 forced in submake: resetting jobserver mode.
  expr.c:217:17: error: conflicting types for built-in function ‘exp2’ 
[-Werror=builtin-declaration-mismatch]
   static intmax_t exp2 __P((void));
   ^~~~
  cc1: all warnings being treated as errors


I know the `exp2' function here has nothing to do with the built-in exponential
function, and that it has had that name for a long time, but the build breaks 
due to that.

To hack around it, I renamed it from `exp2' to `exp22', and then it broke here:

  dualbus@debian:~/src/gnu/bash$ make -j$(nproc) -s
  make[1]: warning: -j4 forced in submake: resetting jobserver mode.
  
***
* *
* GNU bash, version 5.0.0(1)-rc1 (x86_64-pc-linux-gnu)
* *
***
  
  making lib/glob/libglob.a in ./lib/glob
  make[1]: warning: -j4 forced in submake: resetting jobserver mode.
  smatch.c: In function ‘_fnmatch_fallback_wc’:
  smatch.c:318:11: error: implicit declaration of function ‘fnmatch’; did you 
mean ‘gmatch’? [-Werror=implicit-function-declaration]
 return (fnmatch ((const char *)w2, (const char *)w1, 0));
 ^~~
 gmatch


Which I believe is an actual issue and I assume the fix is along the lines of:

  dualbus@debian:~/src/gnu/bash$ git diff -- lib/
  diff --git a/lib/glob/smatch.c b/lib/glob/smatch.c
  index 3826c93e..9150a28e 100644
  --- a/lib/glob/smatch.c
  +++ b/lib/glob/smatch.c
  @@ -25,6 +25,7 @@
  
   #include "strmatch.h"
   #include 
  +#include 
   
   #include "bashansi.h"
   #include "shmbutil.h"


I'm not sure what to do about `exp2' though. Personally, I'd love for these
functions to have slighly more meaningful names.


I also tried clang version 7.0.1-+rc3-2, which gives an additional error:

  making lib/sh/libsh.a in ./lib/sh
  (...)
  getenv.c:55:7: error: comparison of nonnull parameter 'name' equal to a null 
pointer is 'false' on first encounter [-Werror,-Wtautological-pointer-compare]
if (name == 0 || *name == '\0')
^~~~~
  /usr/include/stdlib.h:631:50: note: declared 'nonnull' here
  extern char *getenv (const char *__name) __THROW __nonnull ((1)) __wur;
   ^
  /usr/include/x86_64-linux-gnu/sys/cdefs.h:293:44: note: expanded from macro 
'__nonnull'
  # define __nonnull(params) __attribute__ ((__nonnull__ params))


This is due to the stdlib.h header being pulled by bashansi.h in
lib/sh/getenv.c, thus, providing a function signature that doesn't match Bash's
re-definition of getenv():

  dualbus@debian:~/src/gnu/bash$ grep bashansi.h lib/sh/getenv.c 
  #include 
  
  dualbus@debian:~/src/gnu/bash$ grep stdlib.h bashansi.h 
  #  include 
  #  include "ansi_stdlib.h"


~~~


Additional information:

gcc:

  dualbus@debian:~/src/gnu/bash$ cc -v
  Using built-in specs.
  COLLECT_GCC=cc
  COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-linux-gnu/8/lto-wrapper
  OFFLOAD_TARGET_NAMES=nvptx-none
  OFFLOAD_TARGET_DEFAULT=1
  Target: x86_64-linux-gnu
  Configured with: ../src/configure -v --with-pkgversion='Debian 8.2.0-12' 
--with-bugurl=file:///usr/share/doc/gcc-8/README.Bugs 
--enable-languages=c,ada,c++,go,brig,d,fortran,objc,obj-c++ --prefix=/usr 
--with-gcc-major-version-only --program-suffix=-8 
--program-prefix=x86_64-linux-gnu- --enable-shared --enable-linker-build-id 
--libexecdir=/usr/lib --without-included-gettext --enable-threads=posix 
--libdir=/usr/lib --enable-nls --with-sysroot=/ --enable-clocale=gnu 
--enable-libstdcxx-debug --enable-libstdcxx-time=yes 
--with-default-libstdcxx-abi=new --enable-gnu-unique-object 
--disable-vtable-verify --enable-libmpx --enable-plugin --enable-default-pie 
--with-system-zlib --with-target-system-zlib --enable-objc-gc=auto 
--enable-multiarch --disable-werror --with-arch-32=i686 --with-abi=m64 
--with-multilib-list=m32,m64,mx32 --enable-multilib --with-tune=generic 
--enable-offload-targets=nvptx-none --without-cuda-driver 
--enable-checking=release --build=x86_64-linux-gnu --host=x86_64-linux-gnu 
--target=x86_64-linux-gnu
  Thread model: posix
  gcc version 8.2.0 (Debia