[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