[PATCH 1/2] Sync tempname with gnulib

[PATCH 1/2] Sync tempname with gnulib

[Adhemerval Zanella]

It sync with commit b58bf6ee39a6a114550a6bb68e7db5262c17f8bf.
---
 sysdeps/posix/tempname.c | 260 ++++++++++++++++++++++-----------------
 1 file changed, 147 insertions(+), 113 deletions(-)

diff --git a/sysdeps/posix/tempname.c b/sysdeps/posix/tempname.c
index cd48385a40..9219ee66af 100644
--- a/sysdeps/posix/tempname.c
+++ b/sysdeps/posix/tempname.c
@@ -16,7 +16,7 @@
    <https://www.gnu.org/licenses/>.  */
 
 #if !_LIBC
-# include <config.h>
+# include <libc-config.h>
 # include "tempname.h"
 #endif
 
@@ -24,9 +24,6 @@
 #include <assert.h>
 
 #include <errno.h>
-#ifndef __set_errno
-# define __set_errno(Val) errno = (Val)
-#endif
 
 #include <stdio.h>
 #ifndef P_tmpdir
@@ -36,12 +33,12 @@
 # define TMP_MAX 238328
 #endif
 #ifndef __GT_FILE
-# define __GT_FILE	0
-# define __GT_DIR	1
-# define __GT_NOCREATE	2
+# define __GT_FILE      0
+# define __GT_DIR       1
+# define __GT_NOCREATE  2
 #endif
-#if !_LIBC && (GT_FILE != __GT_FILE || GT_DIR != __GT_DIR	\
-	       || GT_NOCREATE != __GT_NOCREATE)
+#if !_LIBC && (GT_FILE != __GT_FILE || GT_DIR != __GT_DIR       \
+               || GT_NOCREATE != __GT_NOCREATE)
 # error report this to bug-gnulib@gnu.org
 #endif
 
@@ -50,10 +47,8 @@
 #include <string.h>
 
 #include <fcntl.h>
-#include <time.h>
 #include <stdint.h>
-#include <unistd.h>
-
+#include <sys/random.h>
 #include <sys/stat.h>
 
 #if _LIBC
@@ -62,32 +57,29 @@
 #else
 # define struct_stat64 struct stat
 # define __gen_tempname gen_tempname
-# define __getpid getpid
 # define __mkdir mkdir
 # define __open open
 # define __lxstat64(version, file, buf) lstat (file, buf)
-# define __secure_getenv secure_getenv
 #endif
 
 #ifdef _LIBC
 # include <random-bits.h>
 # define RANDOM_BITS(Var) ((Var) = random_bits ())
-# else
+typedef uint32_t random_value;
+# define RANDOM_VALUE_MAX UINT32_MAX
+# define BASE_62_DIGITS 5 /* 62**5 < UINT32_MAX */
+# define BASE_62_POWER (62 * 62 * 62 * 62 * 62) /* 2**BASE_62_DIGITS */
+#else
+/* Use getrandom if it works, falling back on a 64-bit linear
+   congruential generator that starts with whatever Var's value
+   happens to be.  */
 # define RANDOM_BITS(Var) \
-    {                                                                         \
-      struct timespec ts;                                                     \
-      clock_gettime (CLOCK_REALTIME, &ts);                                    \
-      (Var) = ((uint64_t) tv.tv_nsec << 16) ^ tv.tv_sec;                      \
-    }
-#endif
-
-/* Use the widest available unsigned type if uint64_t is not
-   available.  The algorithm below extracts a number less than 62**6
-   (approximately 2**35.725) from uint64_t, so ancient hosts where
-   uintmax_t is only 32 bits lose about 3.725 bits of randomness,
-   which is better than not having mkstemp at all.  */
-#if !defined UINT64_MAX && !defined uint64_t
-# define uint64_t uintmax_t
+    ((void) (getrandom (&(Var), sizeof (Var), 0) == sizeof (Var) \
+             || ((Var) = 2862933555777941757 * (Var) + 3037000493)))
+typedef uint_fast64_t random_value;
+# define RANDOM_VALUE_MAX UINT_FAST64_MAX
+# define BASE_62_DIGITS 10 /* 62**10 < UINT_FAST64_MAX */
+# define BASE_62_POWER (62LL * 62 * 62 * 62 * 62 * 62 * 62 * 62 * 62 * 62)
 #endif
 
 #if _LIBC
@@ -107,7 +99,7 @@ direxists (const char *dir)
    enough space in TMPL. */
 int
 __path_search (char *tmpl, size_t tmpl_len, const char *dir, const char *pfx,
-	       int try_tmpdir)
+               int try_tmpdir)
 {
   const char *d;
   size_t dlen, plen;
@@ -121,35 +113,35 @@ __path_search (char *tmpl, size_t tmpl_len, const char *dir, const char *pfx,
     {
       plen = strlen (pfx);
       if (plen > 5)
-	plen = 5;
+        plen = 5;
     }
 
   if (try_tmpdir)
     {
       d = __secure_getenv ("TMPDIR");
       if (d != NULL && direxists (d))
-	dir = d;
+        dir = d;
       else if (dir != NULL && direxists (dir))
-	/* nothing */ ;
+        /* nothing */ ;
       else
-	dir = NULL;
+        dir = NULL;
     }
   if (dir == NULL)
     {
       if (direxists (P_tmpdir))
-	dir = P_tmpdir;
+        dir = P_tmpdir;
       else if (strcmp (P_tmpdir, "/tmp") != 0 && direxists ("/tmp"))
-	dir = "/tmp";
+        dir = "/tmp";
       else
-	{
-	  __set_errno (ENOENT);
-	  return -1;
-	}
+        {
+          __set_errno (ENOENT);
+          return -1;
+        }
     }
 
   dlen = strlen (dir);
   while (dlen > 1 && dir[dlen - 1] == '/')
-    dlen--;			/* remove trailing slashes */
+    dlen--;                     /* remove trailing slashes */
 
   /* check we have room for "${dir}/${pfx}XXXXXX\0" */
   if (tmpl_len < dlen + 1 + plen + 6 + 1)
@@ -163,39 +155,91 @@ __path_search (char *tmpl, size_t tmpl_len, const char *dir, const char *pfx,
 }
 #endif /* _LIBC */
 
+#if _LIBC
+static int try_tempname_len (char *, int, void *, int (*) (char *, void *),
+                             size_t);
+#endif
+
+static int
+try_file (char *tmpl, void *flags)
+{
+  int *openflags = flags;
+  return __open (tmpl,
+                 (*openflags & ~O_ACCMODE)
+                 | O_RDWR | O_CREAT | O_EXCL, S_IRUSR | S_IWUSR);
+}
+
+static int
+try_dir (char *tmpl, void *flags _GL_UNUSED)
+{
+  return __mkdir (tmpl, S_IRUSR | S_IWUSR | S_IXUSR);
+}
+
+static int
+try_nocreate (char *tmpl, void *flags _GL_UNUSED)
+{
+  struct_stat64 st;
+
+  if (__lxstat64 (_STAT_VER, tmpl, &st) == 0 || errno == EOVERFLOW)
+    __set_errno (EEXIST);
+  return errno == ENOENT ? 0 : -1;
+}
+
 /* These are the characters used in temporary file names.  */
 static const char letters[] =
 "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789";
 
 /* Generate a temporary file name based on TMPL.  TMPL must match the
-   rules for mk[s]temp (i.e. end in "XXXXXX", possibly with a suffix).
+   rules for mk[s]temp (i.e., end in at least X_SUFFIX_LEN "X"s,
+   possibly with a suffix).
    The name constructed does not exist at the time of the call to
-   __gen_tempname.  TMPL is overwritten with the result.
+   this function.  TMPL is overwritten with the result.
 
    KIND may be one of:
-   __GT_NOCREATE:	simply verify that the name does not exist
-			at the time of the call.
-   __GT_FILE:		create the file using open(O_CREAT|O_EXCL)
-			and return a read-write fd.  The file is mode 0600.
-   __GT_DIR:		create a directory, which will be mode 0700.
+   __GT_NOCREATE:       simply verify that the name does not exist
+                        at the time of the call.
+   __GT_FILE:           create the file using open(O_CREAT|O_EXCL)
+                        and return a read-write fd.  The file is mode 0600.
+   __GT_DIR:            create a directory, which will be mode 0700.
 
    We use a clever algorithm to get hard-to-predict names. */
+#ifdef _LIBC
+static
+#endif
 int
-__gen_tempname (char *tmpl, int suffixlen, int flags, int kind)
+gen_tempname_len (char *tmpl, int suffixlen, int flags, int kind,
+                  size_t x_suffix_len)
+{
+  static int (*const tryfunc[]) (char *, void *) =
+    {
+      [__GT_FILE] = try_file,
+      [__GT_DIR] = try_dir,
+      [__GT_NOCREATE] = try_nocreate
+    };
+  return try_tempname_len (tmpl, suffixlen, &flags, tryfunc[kind],
+                           x_suffix_len);
+}
+
+#ifdef _LIBC
+static
+#endif
+int
+try_tempname_len (char *tmpl, int suffixlen, void *args,
+                  int (*tryfunc) (char *, void *), size_t x_suffix_len)
 {
-  int len;
+  size_t len;
   char *XXXXXX;
   unsigned int count;
   int fd = -1;
   int save_errno = errno;
-  struct_stat64 st;
 
   /* A lower bound on the number of temporary files to attempt to
      generate.  The maximum total number of temporary file names that
      can exist for a given template is 62**6.  It should never be
      necessary to try all of these combinations.  Instead if a reasonable
      number of names is tried (we define reasonable as 62**3) fail to
-     give the system administrator the chance to remove the problems.  */
+     give the system administrator the chance to remove the problems.
+     This value requires that X_SUFFIX_LEN be at least 3.  */
 #define ATTEMPTS_MIN (62 * 62 * 62)
 
   /* The number of times to attempt to generate a temporary file.  To
@@ -206,82 +250,72 @@ __gen_tempname (char *tmpl, int suffixlen, int flags, int kind)
   unsigned int attempts = ATTEMPTS_MIN;
 #endif
 
+  /* A random variable.  */
+  random_value v;
+
+  /* How many random base-62 digits can currently be extracted from V.  */
+  int vdigits = 0;
+
+  /* Least unfair value for V.  If V is less than this, V can generate
+     BASE_62_DIGITS digits fairly.  Otherwise it might be biased.  */
+  random_value const unfair_min
+    = RANDOM_VALUE_MAX - RANDOM_VALUE_MAX % BASE_62_POWER;
+
   len = strlen (tmpl);
-  if (len < 6 + suffixlen || memcmp (&tmpl[len - 6 - suffixlen], "XXXXXX", 6))
+  if (len < x_suffix_len + suffixlen
+      || strspn (&tmpl[len - x_suffix_len - suffixlen], "X") < x_suffix_len)
     {
       __set_errno (EINVAL);
       return -1;
     }
 
   /* This is where the Xs start.  */
-  XXXXXX = &tmpl[len - 6 - suffixlen];
+  XXXXXX = &tmpl[len - x_suffix_len - suffixlen];
 
-  uint64_t pid = (uint64_t) __getpid () << 32;
   for (count = 0; count < attempts; ++count)
     {
-      uint64_t v;
-      /* Get some more or less random data.  */
-      RANDOM_BITS (v);
-      v ^= pid;
-
-      /* Fill in the random bits.  */
-      XXXXXX[0] = letters[v % 62];
-      v /= 62;
-      XXXXXX[1] = letters[v % 62];
-      v /= 62;
-      XXXXXX[2] = letters[v % 62];
-      v /= 62;
-      XXXXXX[3] = letters[v % 62];
-      v /= 62;
-      XXXXXX[4] = letters[v % 62];
-      v /= 62;
-      XXXXXX[5] = letters[v % 62];
-
-      switch (kind)
-	{
-	case __GT_FILE:
-	  fd = __open (tmpl,
-		       (flags & ~O_ACCMODE)
-		       | O_RDWR | O_CREAT | O_EXCL, S_IRUSR | S_IWUSR);
-	  break;
-
-	case __GT_DIR:
-	  fd = __mkdir (tmpl, S_IRUSR | S_IWUSR | S_IXUSR);
-	  break;
-
-	case __GT_NOCREATE:
-	  /* This case is backward from the other three.  __gen_tempname
-	     succeeds if __xstat fails because the name does not exist.
-	     Note the continue to bypass the common logic at the bottom
-	     of the loop.  */
-	  if (__lxstat64 (_STAT_VER, tmpl, &st) < 0)
-	    {
-	      if (errno == ENOENT)
-		{
-		  __set_errno (save_errno);
-		  return 0;
-		}
-	      else
-		/* Give up now. */
-		return -1;
-	    }
-	  continue;
-
-	default:
-	  assert (! "invalid KIND in __gen_tempname");
-	  abort ();
-	}
-
+      for (size_t i = 0; i < x_suffix_len; i++)
+        {
+          if (vdigits == 0)
+            {
+              do
+                RANDOM_BITS (v);
+              while (unfair_min <= v);
+
+              vdigits = BASE_62_DIGITS;
+            }
+
+          XXXXXX[i] = letters[v % 62];
+          v /= 62;
+          vdigits--;
+        }
+
+      fd = tryfunc (tmpl, args);
       if (fd >= 0)
-	{
-	  __set_errno (save_errno);
-	  return fd;
-	}
+        {
+          __set_errno (save_errno);
+          return fd;
+        }
       else if (errno != EEXIST)
-	return -1;
+        return -1;
     }
 
   /* We got out of the loop because we ran out of combinations to try.  */
   __set_errno (EEXIST);
   return -1;
 }
+
+int
+__gen_tempname (char *tmpl, int suffixlen, int flags, int kind)
+{
+  return gen_tempname_len (tmpl, suffixlen, flags, kind, 6);
+}
+
+#if !_LIBC
+int
+try_tempname (char *tmpl, int suffixlen, void *args,
+              int (*tryfunc) (char *, void *))
+{
+  return try_tempname_len (tmpl, suffixlen, args, tryfunc, 6);
+}
+#endif
-- 
2.25.1

[PATCH 2/2] Use getrandom on try_tempname_len [BZ #15813]

[Adhemerval Zanella]

Remove the usage of random-bits.h for _LIBC and use getrandom instead.

Also, the fallback relies on UB (the LCG might use unitialized value
from the stack variable) so initialize the initial state using some ASLR
entropy.

The fallback will be always used on Linux with kernels older than 3.17,
so it also adds some extra entropy based on the clock for the linear
congruential generator (LCG).

Checked on x86_64-linux-gnu.
---
 sysdeps/posix/tempname.c | 41 +++++++++++++++++++++++-----------------
 1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/sysdeps/posix/tempname.c b/sysdeps/posix/tempname.c
index 9219ee66af..1ef5536814 100644
--- a/sysdeps/posix/tempname.c
+++ b/sysdeps/posix/tempname.c
@@ -60,27 +60,31 @@
 # define __mkdir mkdir
 # define __open open
 # define __lxstat64(version, file, buf) lstat (file, buf)
+# define __getrandom getrandom
 #endif
 
-#ifdef _LIBC
-# include <random-bits.h>
-# define RANDOM_BITS(Var) ((Var) = random_bits ())
-typedef uint32_t random_value;
-# define RANDOM_VALUE_MAX UINT32_MAX
-# define BASE_62_DIGITS 5 /* 62**5 < UINT32_MAX */
-# define BASE_62_POWER (62 * 62 * 62 * 62 * 62) /* 2**BASE_62_DIGITS */
-#else
 /* Use getrandom if it works, falling back on a 64-bit linear
    congruential generator that starts with whatever Var's value
    happens to be.  */
-# define RANDOM_BITS(Var) \
-    ((void) (getrandom (&(Var), sizeof (Var), 0) == sizeof (Var) \
-             || ((Var) = 2862933555777941757 * (Var) + 3037000493)))
 typedef uint_fast64_t random_value;
-# define RANDOM_VALUE_MAX UINT_FAST64_MAX
-# define BASE_62_DIGITS 10 /* 62**10 < UINT_FAST64_MAX */
-# define BASE_62_POWER (62LL * 62 * 62 * 62 * 62 * 62 * 62 * 62 * 62 * 62)
+#define RANDOM_VALUE_MAX UINT_FAST64_MAX
+#define BASE_62_DIGITS 10 /* 62**10 < UINT_FAST64_MAX */
+#define BASE_62_POWER (62LL * 62 * 62 * 62 * 62 * 62 * 62 * 62 * 62 * 62)
+
+static random_value
+random_bits (random_value var)
+{
+  random_value r;
+  if (__getrandom (&r, sizeof (r), 0) == sizeof (r))
+    return r;
+#if _LIBC
+  /* Add some more entropy if getrandom is not supported.  */
+  struct __timespec64 tv;
+  __clock_gettime64 (CLOCK_MONOTONIC, &tv);
+  var = var ^ tv.tv_nsec;
 #endif
+  return 2862933555777941757 * var + 3037000493;
+}
 
 #if _LIBC
 /* Return nonzero if DIR is an existent directory.  */
@@ -250,8 +254,11 @@ try_tempname_len (char *tmpl, int suffixlen, void *args,
   unsigned int attempts = ATTEMPTS_MIN;
 #endif
 
-  /* A random variable.  */
-  random_value v;
+  /* A random variable.  The initial value is used only the for fallback path
+     on 'random_bits' on 'getrandom' failure.  Its initial value tries to use
+     some entropy from the ASLR and ignore possible bits from the stack
+     alignment.  */
+  random_value v = ((uintptr_t) &v) / 16;
 
   /* How many random base-62 digits can currently be extracted from V.  */
   int vdigits = 0;
@@ -279,7 +286,7 @@ try_tempname_len (char *tmpl, int suffixlen, void *args,
           if (vdigits == 0)
             {
               do
-                RANDOM_BITS (v);
+                v = random_bits (v);
               while (unfair_min <= v);
 
               vdigits = BASE_62_DIGITS;
-- 
2.25.1

Re: [PATCH 2/2] Use getrandom on try_tempname_len [BZ #15813]

[Paul Eggert]

Thanks for looking into this. I merged those proposed changes into the Gnulib 
tempname module, taking the following comments into account. With luck you can 
sync with the updated Gnulib tempname.c.

On 9/9/20 10:53 AM, Adhemerval Zanella wrote:

> +# define __getrandom getrandom

This also needs to #define __clock_gettime64 and __timespec64 in the !_LIBC case.

>   /* Use getrandom if it works, falling back on a 64-bit linear
>      congruential generator that starts with whatever Var's value
>      happens to be.  */

This comment should be updated to match the updated code.

> +  if (__getrandom (&r, sizeof (r), 0) == sizeof (r))

Minor style nit: "sizeof r" is just as clear, and is shorter.

> +#if _LIBC
> +  /* Add some more entropy if getrandom is not supported.  */
> +  struct __timespec64 tv;
> +  __clock_gettime64 (CLOCK_MONOTONIC, &tv);

For the Gnulib case, the "#if _LIBC" should be changed to "#if _LIBC || (defined 
CLOCK_MONOTONIC && HAVE_CLOCK_GETTIME)".

> +  var = var ^ tv.tv_nsec;

Might as well use '^='.

> +  random_value v = ((uintptr_t) &v) / 16;

Instead of the magic number 16, use "alignof (max_align_t)" for better portability.

The file should include <time.h> for clock_gettime and CLOCK_MONOTONIC, and 
stdalign.h for alignof.

Re: [PATCH 2/2] Use getrandom on try_tempname_len [BZ #15813]

[Jakub Jelinek]

On Wed, Sep 09, 2020 at 02:53:55PM -0300, Adhemerval Zanella wrote:
> +static random_value
> +random_bits (random_value var)
> +{
> +  random_value r;
> +  if (__getrandom (&r, sizeof (r), 0) == sizeof (r))
> +    return r;

Is this a good idea?  Without GRND_NONBLOCK it will block until there is
sufficient entropy available, at least with older kernel and can be blocked
for minutes.

As I wrote in bugzilla, I think it would be better to use clock_gettime64 ^
pid based "random" source for the initial randomness value, so that it
wouldn't deplete the random entropy pool, and use it only for the retries
(so only in the unlikely case the file exists already).

> +#if _LIBC
> +  /* Add some more entropy if getrandom is not supported.  */
> +  struct __timespec64 tv;
> +  __clock_gettime64 (CLOCK_MONOTONIC, &tv);
> +  var = var ^ tv.tv_nsec;
>  #endif
> +  return 2862933555777941757 * var + 3037000493;
> +}
>  
>  #if _LIBC
>  /* Return nonzero if DIR is an existent directory.  */
> @@ -250,8 +254,11 @@ try_tempname_len (char *tmpl, int suffixlen, void *args,
>    unsigned int attempts = ATTEMPTS_MIN;
>  #endif
>  
> -  /* A random variable.  */
> -  random_value v;
> +  /* A random variable.  The initial value is used only the for fallback path
> +     on 'random_bits' on 'getrandom' failure.  Its initial value tries to use
> +     some entropy from the ASLR and ignore possible bits from the stack
> +     alignment.  */
> +  random_value v = ((uintptr_t) &v) / 16;
>  
>    /* How many random base-62 digits can currently be extracted from V.  */
>    int vdigits = 0;
> @@ -279,7 +286,7 @@ try_tempname_len (char *tmpl, int suffixlen, void *args,
>            if (vdigits == 0)
>              {
>                do
> -                RANDOM_BITS (v);
> +                v = random_bits (v);
>                while (unfair_min <= v);
>  
>                vdigits = BASE_62_DIGITS;
> -- 
> 2.25.1

	Jakub

Re: [PATCH 2/2] Use getrandom on try_tempname_len [BZ #15813]

[Adhemerval Zanella]

On 10/09/2020 05:57, Jakub Jelinek wrote:
> On Wed, Sep 09, 2020 at 02:53:55PM -0300, Adhemerval Zanella wrote:
>> +static random_value
>> +random_bits (random_value var)
>> +{
>> +  random_value r;
>> +  if (__getrandom (&r, sizeof (r), 0) == sizeof (r))
>> +    return r;
> 
> Is this a good idea?  Without GRND_NONBLOCK it will block until there is
> sufficient entropy available, at least with older kernel and can be blocked
> for minutes.

Ok, I forgot about this issue on some kernels.  It indeed makes it using
getrandom cumbersome (at least with default flags).

> 
> As I wrote in bugzilla, I think it would be better to use clock_gettime64 ^
> pid based "random" source for the initial randomness value, so that it
> wouldn't deplete the random entropy pool, and use it only for the retries
> (so only in the unlikely case the file exists already).
 
What about the patch below? It tries to get the initial state randomness
from the ASLR (which may provide vary bits depending of the architecture
or even none), plus the clock, and the template address itself.  I try to
avoid using PID to be more costly (requires a syscall) and make it easier
to share code with gnulib.

I also added GRND_NONBLOCK on getrandom call, since it can potentially
block for a long time.  The maximum number of attempts along with the
LCG should be passable fallback.

I have added the suggested changes from Paul Eggert.

---

[PATCH v2 2/2] Improve randomness on try_tempname_len [BZ #15813]

Remove the usage of random-bits.h for _LIBC and use as initial state
the ASLR (which may provide vary bits depending of the architecture
or even none), plus the clock, and the template address itself.  It
call getrandom or the fallback routine iff the file already exists.

The getrandom fallback will be always used on Linux with kernels older
than 3.17, so it also adds some extra entropy based on the clock for
the linear congruential generator.

Checked on x86_64-linux-gnu.
---
 sysdeps/posix/tempname.c | 65 ++++++++++++++++++++++++++++------------
 1 file changed, 46 insertions(+), 19 deletions(-)

diff --git a/sysdeps/posix/tempname.c b/sysdeps/posix/tempname.c
index 9219ee66af..2867276dcb 100644
--- a/sysdeps/posix/tempname.c
+++ b/sysdeps/posix/tempname.c
@@ -47,9 +47,11 @@
 #include <string.h>
 
 #include <fcntl.h>
+#include <stdalign.h>
 #include <stdint.h>
 #include <sys/random.h>
 #include <sys/stat.h>
+#include <time.h>
 
 #if _LIBC
 # define struct_stat64 struct stat64
@@ -60,27 +62,51 @@
 # define __mkdir mkdir
 # define __open open
 # define __lxstat64(version, file, buf) lstat (file, buf)
+# define __getrandom getrandom
+# define __clock_gettime64 clock_gettime
+# define __timespec64 timespec
 #endif
 
-#ifdef _LIBC
-# include <random-bits.h>
-# define RANDOM_BITS(Var) ((Var) = random_bits ())
-typedef uint32_t random_value;
-# define RANDOM_VALUE_MAX UINT32_MAX
-# define BASE_62_DIGITS 5 /* 62**5 < UINT32_MAX */
-# define BASE_62_POWER (62 * 62 * 62 * 62 * 62) /* 2**BASE_62_DIGITS */
-#else
 /* Use getrandom if it works, falling back on a 64-bit linear
-   congruential generator that starts with whatever Var's value
-   happens to be.  */
-# define RANDOM_BITS(Var) \
-    ((void) (getrandom (&(Var), sizeof (Var), 0) == sizeof (Var) \
-             || ((Var) = 2862933555777941757 * (Var) + 3037000493)))
+   congruential generator that starts with Var's value
+   mixed in with a clock's low-order bits if available.  */
 typedef uint_fast64_t random_value;
-# define RANDOM_VALUE_MAX UINT_FAST64_MAX
-# define BASE_62_DIGITS 10 /* 62**10 < UINT_FAST64_MAX */
-# define BASE_62_POWER (62LL * 62 * 62 * 62 * 62 * 62 * 62 * 62 * 62 * 62)
+#define RANDOM_VALUE_MAX UINT_FAST64_MAX
+#define BASE_62_DIGITS 10 /* 62**10 < UINT_FAST64_MAX */
+#define BASE_62_POWER (62LL * 62 * 62 * 62 * 62 * 62 * 62 * 62 * 62 * 62)
+
+/* Initial state of random variable.  It tries to use some entropy from
+   the ASLR (ignoring the possible bits from the stack alignment), plus
+   some bits from the clock (if available), and from the template input
+   itself.  */
+static int
+random_bits_init (random_value *var, const char *tmpl)
+{
+  *var = ((uintptr_t) var) / alignof (max_align_t);
+#if _LIBC || (defined CLOCK_MONOTONIC && HAVE_CLOCK_GETTIME)
+  struct __timespec64 tv;
+  __clock_gettime64 (CLOCK_MONOTONIC, &tv);
+  *var ^= tv.tv_nsec;
 #endif
+  *var += (uintptr_t) tmpl;
+  return BASE_62_DIGITS;
+}
+
+static random_value
+random_bits (random_value var)
+{
+  random_value r;
+  /* Without GRND_NONBLOCK it can be blocked for minutes on some systems.  */
+  if (__getrandom (&r, sizeof r, GRND_NONBLOCK) == sizeof r)
+    return r;
+#if _LIBC || (defined CLOCK_MONOTONIC && HAVE_CLOCK_GETTIME)
+  /* Add entropy if getrandom is not supported.  */
+  struct __timespec64 tv;
+  __clock_gettime64 (CLOCK_MONOTONIC, &tv);
+  var ^= tv.tv_nsec;
+#endif
+  return 2862933555777941757 * var + 3037000493;
+}
 
 #if _LIBC
 /* Return nonzero if DIR is an existent directory.  */
@@ -250,11 +276,12 @@ try_tempname_len (char *tmpl, int suffixlen, void *args,
   unsigned int attempts = ATTEMPTS_MIN;
 #endif
 
-  /* A random variable.  */
+  /* A random variable.  Try to get some entropy to avoid call random_bits
+     (which might be expensive).  */
   random_value v;
 
   /* How many random base-62 digits can currently be extracted from V.  */
-  int vdigits = 0;
+  int vdigits = random_bits_init (&v, tmpl);
 
   /* Least unfair value for V.  If V is less than this, V can generate
      BASE_62_DIGITS digits fairly.  Otherwise it might be biased.  */
@@ -279,7 +306,7 @@ try_tempname_len (char *tmpl, int suffixlen, void *args,
           if (vdigits == 0)
             {
               do
-                RANDOM_BITS (v);
+                v = random_bits (v);
               while (unfair_min <= v);
 
               vdigits = BASE_62_DIGITS;
-- 
2.25.1

Re: [PATCH 2/2] Use getrandom on try_tempname_len [BZ #15813]

[Jakub Jelinek]

On Thu, Sep 10, 2020 at 09:27:44AM -0300, Adhemerval Zanella wrote:
> > As I wrote in bugzilla, I think it would be better to use clock_gettime64 ^
> > pid based "random" source for the initial randomness value, so that it
> > wouldn't deplete the random entropy pool, and use it only for the retries
> > (so only in the unlikely case the file exists already).
>  
> What about the patch below? It tries to get the initial state randomness

LGTM (but not an official patch review for glibc).

	Jakub

Re: [PATCH 2/2] Use getrandom on try_tempname_len [BZ #15813]

[Paul Eggert]

>> As I wrote in bugzilla, I think it would be better to use clock_gettime64 ^
>> pid based "random" source for the initial randomness value, so that it
>> wouldn't deplete the random entropy pool, and use it only for the retries
>> (so only in the unlikely case the file exists already).

Isn't part of the goal to avoid collisions even in the first try, to avoid 
attacks by name-guessers on not-so-well-written callers? If so, we should use 
getrandom even for the first try (with GRND_NONBLOCK of course).

Generating a file name ought to be a reasonably-rare action, and I wouldn't 
worry too much about entropy pool exhaustion from such a small request.

> +  *var = ((uintptr_t) var) / alignof (max_align_t);
> +#if _LIBC || (defined CLOCK_MONOTONIC && HAVE_CLOCK_GETTIME)
> +  struct __timespec64 tv;
> +  __clock_gettime64 (CLOCK_MONOTONIC, &tv);
> +  *var ^= tv.tv_nsec;
>   #endif
> +  *var += (uintptr_t) tmpl;

This should also use ^=.

> +#if _LIBC || (defined CLOCK_MONOTONIC && HAVE_CLOCK_GETTIME)
> +  /* Add entropy if getrandom is not supported.  */
> +  struct __timespec64 tv;
> +  __clock_gettime64 (CLOCK_MONOTONIC, &tv);
> +  var ^= tv.tv_nsec;
> +#endif

This is duplicate code and should be refactored out, assuming you don't follow 
my suggestion above (which should remove the code duplication anyway).

> +  /* A random variable.  Try to get some entropy to avoid call random_bits
> +     (which might be expensive).  */

I don't quite follow the comment, which has grammar problems with that "avoid call".

Re: [PATCH 2/2] Use getrandom on try_tempname_len [BZ #15813]

[Jakub Jelinek]

On Thu, Sep 10, 2020 at 02:21:33PM -0700, Paul Eggert wrote:
> > > As I wrote in bugzilla, I think it would be better to use clock_gettime64 ^
> > > pid based "random" source for the initial randomness value, so that it
> > > wouldn't deplete the random entropy pool, and use it only for the retries
> > > (so only in the unlikely case the file exists already).
> 
> Isn't part of the goal to avoid collisions even in the first try, to avoid
> attacks by name-guessers on not-so-well-written callers? If so, we should
> use getrandom even for the first try (with GRND_NONBLOCK of course).
> 
> Generating a file name ought to be a reasonably-rare action, and I wouldn't
> worry too much about entropy pool exhaustion from such a small request.

Given that the file is (attempted to be) opened with O_CREAT | O_EXCL, the
only harm I can see is DDOS, but for that one needs to create all the
TMP_MAX files in the sequence, not just the first one.
So it really doesn't matter how much unpredictable the first attempt is, as
long as the following filenames aren't (easily) predictable.

	Jakub

Re: [PATCH 2/2] Use getrandom on try_tempname_len [BZ #15813]

[Adhemerval Zanella]

On 10/09/2020 18:21, Paul Eggert wrote:
>>> As I wrote in bugzilla, I think it would be better to use clock_gettime64 ^
>>> pid based "random" source for the initial randomness value, so that it
>>> wouldn't deplete the random entropy pool, and use it only for the retries
>>> (so only in the unlikely case the file exists already).
> 
> Isn't part of the goal to avoid collisions even in the first try, to avoid attacks by name-guessers on not-so-well-written callers? If so, we should use getrandom even for the first try (with GRND_NONBLOCK of course).
> 
> Generating a file name ought to be a reasonably-rare action, and I wouldn't worry too much about entropy pool exhaustion from such a small request.

I don't have a strong opinion here, but I see that using GRND_NONBLOCK
with current glibc code results in a slight simples code with better
guaranties specially on recent kernels.  I will send an updated version
using gnulib code with GRND_NONBLOCK change.

> 
>> +  *var = ((uintptr_t) var) / alignof (max_align_t);
>> +#if _LIBC || (defined CLOCK_MONOTONIC && HAVE_CLOCK_GETTIME)
>> +  struct __timespec64 tv;
>> +  __clock_gettime64 (CLOCK_MONOTONIC, &tv);
>> +  *var ^= tv.tv_nsec;
>>   #endif
>> +  *var += (uintptr_t) tmpl;
> 
> This should also use ^=.> 
>> +#if _LIBC || (defined CLOCK_MONOTONIC && HAVE_CLOCK_GETTIME)
>> +  /* Add entropy if getrandom is not supported.  */
>> +  struct __timespec64 tv;
>> +  __clock_gettime64 (CLOCK_MONOTONIC, &tv);
>> +  var ^= tv.tv_nsec;
>> +#endif
> 
> This is duplicate code and should be refactored out, assuming you don't follow my suggestion above (which should remove the code duplication anyway).> 
>> +  /* A random variable.  Try to get some entropy to avoid call random_bits
>> +     (which might be expensive).  */
> 
> I don't quite follow the comment, which has grammar problems with that "avoid call"

Re: [PATCH 2/2] Use getrandom on try_tempname_len [BZ #15813]

[Jakub Jelinek]

On Thu, Sep 10, 2020 at 06:53:27PM -0300, Adhemerval Zanella wrote:
> On 10/09/2020 18:21, Paul Eggert wrote:
> >>> As I wrote in bugzilla, I think it would be better to use clock_gettime64 ^
> >>> pid based "random" source for the initial randomness value, so that it
> >>> wouldn't deplete the random entropy pool, and use it only for the retries
> >>> (so only in the unlikely case the file exists already).
> > 
> > Isn't part of the goal to avoid collisions even in the first try, to avoid attacks by name-guessers on not-so-well-written callers? If so, we should use getrandom even for the first try (with GRND_NONBLOCK of course).
> > 
> > Generating a file name ought to be a reasonably-rare action,

For some programs like gcc, it is certainly not a rare action, it can create
thousands of them e.g. with LTO.
So not wasting time and entropy in the common case seems desirable to me.

> and I wouldn't worry too much about entropy pool exhaustion from such a small request.

	Jakub

Re: [PATCH 2/2] Use getrandom on try_tempname_len [BZ #15813]

[Paul Eggert]

On 9/10/20 2:53 PM, Jakub Jelinek wrote:
>> Generating a file name ought to be a reasonably-rare action, and I wouldn't
>> worry too much about entropy pool exhaustion from such a small request.

> Given that the file is (attempted to be) opened with O_CREAT | O_EXCL,

That's not always the case unfortunately, because mktemp, tempnam, tmpnam, and 
tmpnam_r don't do that because they all use this code's __GT_NOCREATE case. Of 
course these functions are all deprecated for good reason, but I expect that 
plenty of substandard legacy code still uses them (without also using O_CREAT | 
O_EXCL). Case in point: GNU Emacs's make-temp-name function, which is still used 
all-too-often by Elisp code despite the security warnings in the Elisp 
documentation, is implemented via the __GT_NOCREATE branch and does not use 
O_CREAT | O_EXCL.

Here's an idea: use getrandom in the first try only for the __GT_NOCREATE case. 
Although a bit more complicated, I expect this would address both your entropy 
and my security concerns.

> > Generating a file name ought to be a reasonably-rare action,
> 
> For some programs like gcc, it is certainly not a rare action, it can create
> thousands of them e.g. with LTO.

Such a program should be generating plenty of entropy as part of its other 
activity, no? Wouldn't that suffice to generate more entropy than it consumes? 
In the usual case we're talking only 64 bits per file name, and if necessary we 
can easily cut that to 40 bits without hurting security, because the log base 2 
of 62**6 is less than 36.

Re: [PATCH 2/2] Use getrandom on try_tempname_len [BZ #15813]

[Jakub Jelinek]

On Thu, Sep 10, 2020 at 06:37:56PM -0700, Paul Eggert wrote:
> On 9/10/20 2:53 PM, Jakub Jelinek wrote:
> > > Generating a file name ought to be a reasonably-rare action, and I wouldn't
> > > worry too much about entropy pool exhaustion from such a small request.
> 
> > Given that the file is (attempted to be) opened with O_CREAT | O_EXCL,
> 
> That's not always the case unfortunately, because mktemp, tempnam, tmpnam,
> and tmpnam_r don't do that because they all use this code's __GT_NOCREATE
> case. Of course these functions are all deprecated for good reason, but I
> expect that plenty of substandard legacy code still uses them (without also
> using O_CREAT | O_EXCL). Case in point: GNU Emacs's make-temp-name function,
> which is still used all-too-often by Elisp code despite the security
> warnings in the Elisp documentation, is implemented via the __GT_NOCREATE
> branch and does not use O_CREAT | O_EXCL.
> 
> Here's an idea: use getrandom in the first try only for the __GT_NOCREATE
> case. Although a bit more complicated, I expect this would address both your
> entropy and my security concerns.

That looks fine to me.

	Jakub

Re: [PATCH 2/2] Use getrandom on try_tempname_len [BZ #15813]

[Adhemerval Zanella]

On 10/09/2020 22:37, Paul Eggert wrote:
> On 9/10/20 2:53 PM, Jakub Jelinek wrote:
>>> Generating a file name ought to be a reasonably-rare action, and I wouldn't
>>> worry too much about entropy pool exhaustion from such a small request.
> 
>> Given that the file is (attempted to be) opened with O_CREAT | O_EXCL,
> 
> That's not always the case unfortunately, because mktemp, tempnam, tmpnam, and tmpnam_r don't do that because they all use this code's __GT_NOCREATE case. Of course these functions are all deprecated for good reason, but I expect that plenty of substandard legacy code still uses them (without also using O_CREAT | O_EXCL). Case in point: GNU Emacs's make-temp-name function, which is still used all-too-often by Elisp code despite the security warnings in the Elisp documentation, is implemented via the __GT_NOCREATE branch and does not use O_CREAT | O_EXCL.
> 
> Here's an idea: use getrandom in the first try only for the __GT_NOCREATE case. Although a bit more complicated, I expect this would address both your entropy and my security concerns.
> 
>> > Generating a file name ought to be a reasonably-rare action,
>>
>> For some programs like gcc, it is certainly not a rare action, it can create
>> thousands of them e.g. with LTO.
> 
> Such a program should be generating plenty of entropy as part of its other activity, no? Wouldn't that suffice to generate more entropy than it consumes? In the usual case we're talking only 64 bits per file name, and if necessary we can easily cut that to 40 bits without hurting security, because the log base 2 of 62**6 is less than 36.

What about the following:

---

[PATCH v2 2/2] Improve randomness on try_tempname_len [BZ #15813]

Remove the usage of random-bits.h for _LIBC.  The initial random
state is initialized using ASLR randomness (which may provide vary
bits depending of the architecture or even none).

For __GT_NOCREATE getrandom is also used on first try, otherwise
randomness is obtained using the clock plus a linear congruential
generator.  The clock plus LCG is also used as the fallback in the
case getrandom fails (for instance on kernel older than 3.17).

Also for getrandom GRND_NONBLOCK is used to avoid blocking indefinitely
on some older kernels.

Checked on x86_64-linux-gnu.
---
 sysdeps/posix/tempname.c | 52 ++++++++++++++++++++++++----------------
 1 file changed, 32 insertions(+), 20 deletions(-)

diff --git a/sysdeps/posix/tempname.c b/sysdeps/posix/tempname.c
index 9219ee66af..c2680605b4 100644
--- a/sysdeps/posix/tempname.c
+++ b/sysdeps/posix/tempname.c
@@ -47,9 +47,11 @@
 #include <string.h>
 
 #include <fcntl.h>
+#include <stdalign.h>
 #include <stdint.h>
 #include <sys/random.h>
 #include <sys/stat.h>
+#include <time.h>
 
 #if _LIBC
 # define struct_stat64 struct stat64
@@ -60,27 +62,33 @@
 # define __mkdir mkdir
 # define __open open
 # define __lxstat64(version, file, buf) lstat (file, buf)
+# define __getrandom getrandom
+# define __clock_gettime64 clock_gettime
+# define __timespec64 timespec
 #endif
 
-#ifdef _LIBC
-# include <random-bits.h>
-# define RANDOM_BITS(Var) ((Var) = random_bits ())
-typedef uint32_t random_value;
-# define RANDOM_VALUE_MAX UINT32_MAX
-# define BASE_62_DIGITS 5 /* 62**5 < UINT32_MAX */
-# define BASE_62_POWER (62 * 62 * 62 * 62 * 62) /* 2**BASE_62_DIGITS */
-#else
 /* Use getrandom if it works, falling back on a 64-bit linear
-   congruential generator that starts with whatever Var's value
-   happens to be.  */
-# define RANDOM_BITS(Var) \
-    ((void) (getrandom (&(Var), sizeof (Var), 0) == sizeof (Var) \
-             || ((Var) = 2862933555777941757 * (Var) + 3037000493)))
+   congruential generator that starts with Var's value
+   mixed in with a clock's low-order bits if available.  */
 typedef uint_fast64_t random_value;
-# define RANDOM_VALUE_MAX UINT_FAST64_MAX
-# define BASE_62_DIGITS 10 /* 62**10 < UINT_FAST64_MAX */
-# define BASE_62_POWER (62LL * 62 * 62 * 62 * 62 * 62 * 62 * 62 * 62 * 62)
+#define RANDOM_VALUE_MAX UINT_FAST64_MAX
+#define BASE_62_DIGITS 10 /* 62**10 < UINT_FAST64_MAX */
+#define BASE_62_POWER (62LL * 62 * 62 * 62 * 62 * 62 * 62 * 62 * 62 * 62)
+
+static random_value
+random_bits (random_value var, bool use_getrandom)
+{
+  random_value r;
+  /* Without GRND_NONBLOCK it can be blocked for minutes on some systems.  */
+  if (use_getrandom && __getrandom (&r, sizeof r, GRND_NONBLOCK) == sizeof r)
+    return r;
+#if _LIBC || (defined CLOCK_MONOTONIC && HAVE_CLOCK_GETTIME)
+  struct __timespec64 tv;
+  __clock_gettime64 (CLOCK_MONOTONIC, &tv);
+  var ^= tv.tv_nsec;
 #endif
+  return 2862933555777941757 * var + 3037000493;
+}
 
 #if _LIBC
 /* Return nonzero if DIR is an existent directory.  */
@@ -250,11 +258,15 @@ try_tempname_len (char *tmpl, int suffixlen, void *args,
   unsigned int attempts = ATTEMPTS_MIN;
 #endif
 
-  /* A random variable.  */
-  random_value v;
+  /* A random variable.  The initial value is used only the for fallback path
+     on 'random_bits' on 'getrandom' failure.  Its initial value tries to use
+     some entropy from the ASLR and ignore possible bits from the stack
+     alignment.  */
+  random_value v = ((uintptr_t) &v) / alignof (max_align_t);
+  v = random_bits (v, tryfunc == try_nocreate);
 
   /* How many random base-62 digits can currently be extracted from V.  */
-  int vdigits = 0;
+  int vdigits = BASE_62_DIGITS;
 
   /* Least unfair value for V.  If V is less than this, V can generate
      BASE_62_DIGITS digits fairly.  Otherwise it might be biased.  */
@@ -279,7 +291,7 @@ try_tempname_len (char *tmpl, int suffixlen, void *args,
           if (vdigits == 0)
             {
               do
-                RANDOM_BITS (v);
+                v = random_bits (v, true);
               while (unfair_min <= v);
 
               vdigits = BASE_62_DIGITS;
-- 
2.25.1

Re: [PATCH 2/2] Use getrandom on try_tempname_len [BZ #15813]

[Florian Weimer]

* Adhemerval Zanella via Libc-alpha:

> [PATCH v2 2/2] Improve randomness on try_tempname_len [BZ #15813]

I haven't looked at the patch, but I think this needs a new bug in
Bugzilla.

If you can post patches based on master, I can review them.

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill

Re: [PATCH 2/2] Use getrandom on try_tempname_len [BZ #15813]

[Carlos O'Donell]

On 9/15/20 9:39 AM, Florian Weimer via Libc-alpha wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> [PATCH v2 2/2] Improve randomness on try_tempname_len [BZ #15813]
> 
> I haven't looked at the patch, but I think this needs a new bug in
> Bugzilla.
> 
> If you can post patches based on master, I can review them.

My reading of this thread is that we have consensus on a solution and
the next step would be for Adhemerval to post patches against master
for review.

I had a chance to review this thread since we had a downstream report
about this bug in Fedora, but there it was a misconfigured VM that
resulted in the bug being triggered.

We should still fix this and backport to stable release branch.

Thanks for the patches Adhmerval!

-- 
Cheers,
Carlos.