[PATCH 1/3] Merge _GL_UNUSED C23 patch from Gnulib

[PATCH 1/3] Merge _GL_UNUSED C23 patch from Gnulib

[Paul Eggert]

* posix/getopt.c (_getopt_initialize):
* sysdeps/posix/tempname.c (try_dir, try_nocreate):
Put _GL_UNUSED before args instead of after.
This makes no difference for glibc.
It is needed for Gnulib when being compiled on
non-GCC C23 compilers.
---
 posix/getopt.c           | 4 ++--
 sysdeps/posix/tempname.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/posix/getopt.c b/posix/getopt.c
index e9661c79fa..a160a4e3bd 100644
--- a/posix/getopt.c
+++ b/posix/getopt.c
@@ -377,8 +377,8 @@ process_long_option (int argc, char **argv, const char *optstring,
 /* Initialize internal data upon the first call to getopt.  */
 
 static const char *
-_getopt_initialize (int argc _GL_UNUSED,
-		    char **argv _GL_UNUSED, const char *optstring,
+_getopt_initialize (_GL_UNUSED int argc,
+		    _GL_UNUSED char **argv, const char *optstring,
 		    struct _getopt_data *d, int posixly_correct)
 {
   /* Start processing options with ARGV-element 1 (since ARGV-element 0
diff --git a/sysdeps/posix/tempname.c b/sysdeps/posix/tempname.c
index ebd2a43951..60f8541085 100644
--- a/sysdeps/posix/tempname.c
+++ b/sysdeps/posix/tempname.c
@@ -181,13 +181,13 @@ try_file (char *tmpl, void *flags)
 }
 
 static int
-try_dir (char *tmpl, void *flags _GL_UNUSED)
+try_dir (char *tmpl, _GL_UNUSED void *flags)
 {
   return __mkdir (tmpl, S_IRUSR | S_IWUSR | S_IXUSR);
 }
 
 static int
-try_nocreate (char *tmpl, void *flags _GL_UNUSED)
+try_nocreate (char *tmpl, _GL_UNUSED void *flags)
 {
   struct_stat64 st;
 
-- 
2.37.2

[PATCH 2/3] Merge getopt patch from Gnulib

[Paul Eggert]

* posix/getopt.c [!_LIBC]: Merge _WIN32 patch from Gnulib
so that these source files are identical.
This makes no difference for glibc.
---
 posix/getopt.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/posix/getopt.c b/posix/getopt.c
index a160a4e3bd..128dc7fcf5 100644
--- a/posix/getopt.c
+++ b/posix/getopt.c
@@ -45,7 +45,8 @@
 # define _(msgid) gettext (msgid)
 /* When used standalone, flockfile and funlockfile might not be
    available.  */
-# ifndef _POSIX_THREAD_SAFE_FUNCTIONS
+# if (!defined _POSIX_THREAD_SAFE_FUNCTIONS \
+      || (defined _WIN32 && ! defined __CYGWIN__))
 #  define flockfile(fp) /* nop */
 #  define funlockfile(fp) /* nop */
 # endif
-- 
2.37.2

Re: [PATCH 2/3] Merge getopt patch from Gnulib

[Florian Weimer]

* Paul Eggert:

> * posix/getopt.c [!_LIBC]: Merge _WIN32 patch from Gnulib
> so that these source files are identical.
> This makes no difference for glibc.
> ---
>  posix/getopt.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/posix/getopt.c b/posix/getopt.c
> index a160a4e3bd..128dc7fcf5 100644
> --- a/posix/getopt.c
> +++ b/posix/getopt.c
> @@ -45,7 +45,8 @@
>  # define _(msgid) gettext (msgid)
>  /* When used standalone, flockfile and funlockfile might not be
>     available.  */
> -# ifndef _POSIX_THREAD_SAFE_FUNCTIONS
> +# if (!defined _POSIX_THREAD_SAFE_FUNCTIONS \
> +      || (defined _WIN32 && ! defined __CYGWIN__))
>  #  define flockfile(fp) /* nop */
>  #  define funlockfile(fp) /* nop */
>  # endif

Okay.

Reviewed-by: Florian Weimer <fweimer@redhat.com>

Thanks,
Florian

[PATCH 3/3] Merge tempname ASLR etc. patch from Gnulib

[Paul Eggert]

Merge patch from Gnulib that fixes problems with ASLR info leak
and entropy loss.  This syncs tempname.c with Gnulib commit
564b523fe97a8d63493aa68acb627b8c40744fb9 (2022-08-22) and
fixes <https://bugs.gnu.org/57129> which was reported against
Emacs and uncovered some unlikely Glibc bugs.
* sysdeps/posix/tempname.c: Don't include stdalign.h.
(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.
---
 sysdeps/posix/tempname.c | 94 +++++++++++++++++++++++++---------------
 1 file changed, 59 insertions(+), 35 deletions(-)

diff --git a/sysdeps/posix/tempname.c b/sysdeps/posix/tempname.c
index 60f8541085..0e2f29f5de 100644
--- a/sysdeps/posix/tempname.c
+++ b/sysdeps/posix/tempname.c
@@ -48,7 +48,6 @@
 #include <string.h>
 
 #include <fcntl.h>
-#include <stdalign.h>
 #include <stdint.h>
 #include <sys/random.h>
 #include <sys/stat.h>
@@ -77,20 +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)
 
+/* 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 _LIBC || (defined CLOCK_MONOTONIC && HAVE_CLOCK_GETTIME)
-  /* 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 = s;
+
+#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
@@ -213,7 +247,7 @@ static const char letters[] =
                         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
@@ -261,25 +295,17 @@ 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);
+  /* A random variable.  */
+  random_value v = 0;
 
-  /* How many random base-62 digits can currently be extracted from V.  */
+  /* A value derived from the random variable, and how many random
+     base-62 digits can currently be extracted from VDIGBUF.  */
+  random_value vdigbuf;
   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
-     less 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);
@@ -299,18 +325,16 @@ 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;
 
+              vdigbuf = v;
               vdigits = BASE_62_DIGITS;
             }
 
-          XXXXXX[i] = letters[v % 62];
-          v /= 62;
+          XXXXXX[i] = letters[vdigbuf % 62];
+          vdigbuf /= 62;
           vdigits--;
         }
 
-- 
2.37.2

Re: [PATCH 3/3] Merge tempname ASLR etc. patch from Gnulib

[Florian Weimer]

* Paul Eggert:

> @@ -299,18 +325,16 @@ 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;

Should glibc simply use arc4random_uniform here?

Thanks,
Florian

Re: [PATCH 3/3] Merge tempname ASLR etc. patch from Gnulib

[Paul Eggert]

On 8/23/22 02:19, Florian Weimer wrote:
> Should glibc simply use arc4random_uniform here?

Unfortunately not, because tempname must use getrandom with 
GRND_NONBLOCK and arc4random_uniform does not do that.

Re: [PATCH 1/3] Merge _GL_UNUSED C23 patch from Gnulib

[Florian Weimer]

* Paul Eggert:

> * posix/getopt.c (_getopt_initialize):
> * sysdeps/posix/tempname.c (try_dir, try_nocreate):
> Put _GL_UNUSED before args instead of after.
> This makes no difference for glibc.
> It is needed for Gnulib when being compiled on
> non-GCC C23 compilers.

This looks okay.

Reviewed-by: Florian Weimer <fweimer@redhat.com>

Thanks,
Florian