From 9ce573cde017182a69881241e8565ec04e5bc728 Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Mon, 22 Aug 2022 12:07:27 -0700 Subject: [PATCH 2/3] tempname: fix multithreading, ASLR leak etc. Fix problems with tempname and multithreading, entropy loss, and missing clock data (this last on non-GNU platforms). See analysis by Bruno Haible in: https://bugs.gnu.org/57129#149 While looking into this, I noticed that tempname can leak info derived from ASLR into publicly-visible file names, which is a no-no. Fix that too. * lib/tempname.c: Don't include stdalign.h. (HAS_CLOCK_ENTROPY): Remove. (mix_random_values): New function. (random_bits): Use it. Args are now new value address and old value, and this function now returns a success indicator. Omit old USE_GETRANDOM argument: always try getrandom now, as there is no good reason not to now that GRND_NONBLOCK is used. Caller changed. Use CLOCK_REALTIME for for ersatz entropy, as CLOCK_MONOTONIC doesn't work on some platforms. Also, mix in ersatz entropy from tv_sec and from clock (). (try_tempname_len): Do not mix in ASLR-based entropy, as the result is published to the world and ASLR should be private. Do not try to use a static var as that has issues if multithreaded. Instead, simply generate new random bits. Worry about bias only with high-quality random bits. * modules/tempname (Depends-on): Do not depend on stdalign. --- ChangeLog | 25 +++++++++++++ lib/tempname.c | 97 +++++++++++++++++++++++++----------------------- modules/tempname | 1 - 3 files changed, 76 insertions(+), 47 deletions(-) diff --git a/ChangeLog b/ChangeLog index adb445ddcf..ed99c845f7 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,30 @@ 2022-08-22 Paul Eggert + tempname: fix multithreading, ASLR leak etc. + Fix problems with tempname and multithreading, entropy loss, + and missing clock data (this last on non-GNU platforms). + See analysis by Bruno Haible in: + https://bugs.gnu.org/57129#149 + While looking into this, I noticed that tempname can leak + info derived from ASLR into publicly-visible file names, + which is a no-no. Fix that too. + * lib/tempname.c: Don't include stdalign.h. + (HAS_CLOCK_ENTROPY): Remove. + (mix_random_values): New function. + (random_bits): Use it. Args are now new value address and + old value, and this function now returns a success indicator. + Omit old USE_GETRANDOM argument: always try getrandom now, as + there is no good reason not to now that GRND_NONBLOCK is used. + Caller changed. Use CLOCK_REALTIME for for ersatz entropy, + as CLOCK_MONOTONIC doesn't work on some platforms. + Also, mix in ersatz entropy from tv_sec and from clock (). + (try_tempname_len): Do not mix in ASLR-based entropy, as + the result is published to the world and ASLR should be private. + Do not try to use a static var as that has issues if multithreaded. + Instead, simply generate new random bits. + Worry about bias only with high-quality random bits. + * modules/tempname (Depends-on): Do not depend on stdalign. + tempname: merge 64-bit time_t fix from glibc This merges glibc commit 52a5fe70a2c77935afe807fb6e904e512ddd894e "Use 64 bit time_t stat internally". diff --git a/lib/tempname.c b/lib/tempname.c index 4f615b90b9..bc1f7c14dd 100644 --- a/lib/tempname.c +++ b/lib/tempname.c @@ -48,7 +48,6 @@ #include #include -#include #include #include #include @@ -77,26 +76,55 @@ typedef uint_fast64_t random_value; #define BASE_62_DIGITS 10 /* 62**10 < UINT_FAST64_MAX */ #define BASE_62_POWER (62LL * 62 * 62 * 62 * 62 * 62 * 62 * 62 * 62 * 62) -#if _LIBC || (defined CLOCK_MONOTONIC && HAVE_CLOCK_GETTIME) -# define HAS_CLOCK_ENTROPY true -#else -# define HAS_CLOCK_ENTROPY false -#endif +/* Return the result of mixing the entropy from R and S. + Assume that R and S are not particularly random, + and that the result should look randomish to an untrained eye. */ static random_value -random_bits (random_value var, bool use_getrandom) +mix_random_values (random_value r, random_value s) +{ + /* As this code is used only when high-quality randomness is neither + available nor necessary, there is no need for fancier polynomials + such as those in the Linux kernel's 'random' driver. */ + return (2862933555777941757 * r + 3037000493) ^ s; +} + +/* Set *R to a random value. + Return true if *R is set to high-quality value taken from getrandom. + Otherwise return false, falling back to a low-quality *R that might + depend on S. + + This function returns false only when getrandom fails. + On GNU systems this should happen only early in the boot process, + when the fallback should be good enough for programs using tempname + because any attacker likely has root privileges already. */ + +static bool +random_bits (random_value *r, random_value s) { - 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 HAS_CLOCK_ENTROPY - /* Add entropy if getrandom did not work. */ + if (__getrandom (r, sizeof *r, GRND_NONBLOCK) == sizeof *r) + return true; + + /* If getrandom did not work, use ersatz entropy based on low-order + clock bits. On GNU systems getrandom should fail only + early in booting, when ersatz should be good enough. + Do not use ASLR-based entropy, as that would leak ASLR info into + the resulting file name which is typically public. + + Of course we are in a state of sin here. */ + + random_value v = 0; + +#if _LIBC || (defined CLOCK_REALTIME && HAVE_CLOCK_GETTIME) struct __timespec64 tv; - __clock_gettime64 (CLOCK_MONOTONIC, &tv); - var ^= tv.tv_nsec; + __clock_gettime64 (CLOCK_REALTIME, &tv); + v = mix_random_values (v, tv.tv_sec); + v = mix_random_values (v, tv.tv_nsec); #endif - return 2862933555777941757 * var + 3037000493; + + *r = mix_random_values (v, clock ()); + return false; } #if _LIBC @@ -267,32 +295,15 @@ try_tempname_len (char *tmpl, int suffixlen, void *args, unsigned int attempts = ATTEMPTS_MIN; #endif - /* 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); - -#if !HAS_CLOCK_ENTROPY - /* Arrange gen_tempname to return less predictable file names on - systems lacking clock entropy . */ - static random_value prev_v; - v ^= prev_v; -#endif + /* A random variable. */ + random_value v = 0; /* How many random base-62 digits can currently be extracted from V. */ int vdigits = 0; - /* Whether to consume entropy when acquiring random bits. On the - first try it's worth the entropy cost with __GT_NOCREATE, which - is inherently insecure and can use the entropy to make it a bit - more secure. On the (rare) second and later attempts it might - help against DoS attacks. */ - bool use_getrandom = tryfunc == try_nocreate; - - /* 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 + /* Least biased value for V. If V is less than this, V can generate + BASE_62_DIGITS unbiased digits. Otherwise the digits are biased. */ + random_value const biased_min = RANDOM_VALUE_MAX - RANDOM_VALUE_MAX % BASE_62_POWER; len = strlen (tmpl); @@ -312,12 +323,9 @@ try_tempname_len (char *tmpl, int suffixlen, void *args, { if (vdigits == 0) { - do - { - v = random_bits (v, use_getrandom); - use_getrandom = true; - } - while (unfair_min <= v); + /* Worry about bias only if the bits are high quality. */ + while (random_bits (&v, v) && biased_min <= v) + continue; vdigits = BASE_62_DIGITS; } @@ -331,9 +339,6 @@ try_tempname_len (char *tmpl, int suffixlen, void *args, if (fd >= 0) { __set_errno (save_errno); -#if !HAS_CLOCK_ENTROPY - prev_v = v; -#endif return fd; } else if (errno != EEXIST) diff --git a/modules/tempname b/modules/tempname index 4779735d9d..f1fb78e8ff 100644 --- a/modules/tempname +++ b/modules/tempname @@ -16,7 +16,6 @@ getrandom libc-config lstat mkdir -stdalign stdbool stdint sys_stat -- 2.37.2