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