diff mbox series

[2/2] posix: Improve randomness on try_tempname_len

Message ID 20210104170349.3681241-2-adhemerval.zanella@linaro.org
State Committed
Headers show
Series [1/2] posix: Sync tempname with gnulib [BZ #26648] | expand

Commit Message

Adhemerval Zanella Jan. 4, 2021, 5:03 p.m. UTC
For __GT_NOCREATE (mktemp, tempnam, tmpnam) getrandom is also used
on first try, otherwise randomness is obtained using the clock plus
a linear congruential generator.

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 | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Comments

Paul Eggert Jan. 9, 2021, 2:20 a.m. UTC | #1
On 1/4/21 9:03 AM, Adhemerval Zanella wrote:
> For __GT_NOCREATE (mktemp, tempnam, tmpnam) getrandom is also used
> on first try, otherwise randomness is obtained using the clock plus
> a linear congruential generator.

Why not use getrandom in the first try also for __GT_DIR (mkdtemp) and 
__GT_FILE (mkostemp, mkostemps, mkstemp, mkstemps, tmpfile)? That is 
what Gnulib tempname.c is doing now. This not only simplifies the code, 
it improves resistance to some (admittedly less-likely) attacks.

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

Thanks, I installed that part of the proposal into Gnulib by installing 
the attached. The idea is for tempname.c to be identical after we get 
the abovementioned issue worked out.
Adhemerval Zanella Jan. 11, 2021, 12:29 p.m. UTC | #2
On 08/01/2021 23:20, Paul Eggert wrote:
> On 1/4/21 9:03 AM, Adhemerval Zanella wrote:
>> For __GT_NOCREATE (mktemp, tempnam, tmpnam) getrandom is also used
>> on first try, otherwise randomness is obtained using the clock plus
>> a linear congruential generator.
> 
> Why not use getrandom in the first try also for __GT_DIR (mkdtemp) and __GT_FILE (mkostemp, mkostemps, mkstemp, mkstemps, tmpfile)? That is what Gnulib tempname.c is doing now. This not only simplifies the code, it improves resistance to some (admittedly less-likely) attacks.

The idea is to always issue getrandom for __GT_DIR or __GT_FILE on first try,
as you suggested initially [1].  I followed your idea [2]:

  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.

The current code should address Jakub concerns of using getrandom without 
GRND_NONBLOCK and not using on on first try (to avoid deplete the random 
entropy pool) and use getrandom only when a collision if found. I will merge
the code, close the bug, and we can work whether use getrandom only for
__GT_DIR/__GT_FILE is an improvement or not.

> 
>> Also for getrandom GRND_NONBLOCK is used to avoid blocking indefinitely
>> on some older kernels.
> 
> Thanks, I installed that part of the proposal into Gnulib by installing the attached. The idea is for tempname.c to be identical after we get the abovementioned issue worked out.

[1] https://sourceware.org/pipermail/libc-alpha/2020-September/117535.html
[2] https://sourceware.org/pipermail/libc-alpha/2020-September/117539.html
Paul Eggert Jan. 12, 2021, 1:06 a.m. UTC | #3
On 1/11/21 4:29 AM, Adhemerval Zanella wrote:

> The idea is to always issue getrandom for __GT_DIR or __GT_FILE on first try,
> as you suggested initially [1].  I followed your idea [2]:... > [1] 
https://sourceware.org/pipermail/libc-alpha/2020-September/117535.html
> [2] https://sourceware.org/pipermail/libc-alpha/2020-September/117539.html

Ah, thanks, I'd forgotten about that conversation.

I looked at the patch 
<https://sourceware.org/pipermail/libc-alpha/2021-January/121302.html> 
again. A couple of small things. First, it uses bool so needs to include 
stdbool.h. Second, the generated code's a bit smaller if we call 
random_bits only once. I added those two changes and installed the 
attached patch to Gnulib master on savannah, with the idea being that 
Gnulib's tempname.c can be identical to glibc's.
diff mbox series

Patch

diff --git a/sysdeps/posix/tempname.c b/sysdeps/posix/tempname.c
index 193d791103..06db694181 100644
--- a/sysdeps/posix/tempname.c
+++ b/sysdeps/posix/tempname.c
@@ -76,10 +76,11 @@  typedef uint_fast64_t random_value;
 #define BASE_62_POWER (62LL * 62 * 62 * 62 * 62 * 62 * 62 * 62 * 62 * 62)
 
 static random_value
-random_bits (random_value var)
+random_bits (random_value var, bool use_getrandom)
 {
   random_value r;
-  if (__getrandom (&r, sizeof r, 0) == sizeof 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)
   /* Add entropy if getrandom is not supported.  */
@@ -263,9 +264,10 @@  try_tempname_len (char *tmpl, int suffixlen, void *args,
      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.  */
@@ -290,7 +292,7 @@  try_tempname_len (char *tmpl, int suffixlen, void *args,
           if (vdigits == 0)
             {
               do
-                v = random_bits (v);
+                v = random_bits (v, true);
               while (unfair_min <= v);
 
               vdigits = BASE_62_DIGITS;