[PATCH] gc-random: Replace implementation with call to getrandom.

[PATCH] gc-random: Replace implementation with call to getrandom.

[simon--- via Gnulib discussion list]

[-- Attachment #1.1: Type: text/plain, Size: 278 bytes --]

Hi.  I re-read the discussion around getrandom vs gc-random and didn't
see any point in keeping the duplicated code.  I believe the
getrandom-approach is better than what was in gc-gnulib.c today, so this
patch make it use that function.  I have pushed the patch below.

/Simon

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-gc-random-Replace-implementation-with-call-to-getran.patch --]
[-- Type: text/x-diff, Size: 10772 bytes --]

From 44ed0db8c93f6a81fd996f1f10e93051291fbf1d Mon Sep 17 00:00:00 2001
From: Simon Josefsson <simon@josefsson.org>
Date: Wed, 20 Jan 2021 11:50:21 +0100
Subject: [PATCH] gc-random: Replace implementation with call to getrandom.

* lib/gc-gnulib.c [GNULIB_GC_RANDOM]: Replace #include's with
those needed for getrandom.
(gc_init): Remove old randomness code.
(gc_done): Likewise.
(randomize): Rewrite using getrandom, inspired by getentropy.
* m4/gc-random.m4: Remove file.
* modules/crypto/gc-random: Drop gc-random.m4, gl_GC_RANDOM, and
LIB_GC_RANDOM.  Add conditional dependency on getrandom.
* modules/crypto/gc-tests (test_gc_LDADD): Drop LIB_GC_RANDOM.
---
 ChangeLog                |  13 ++++
 lib/gc-gnulib.c          | 138 +++++++--------------------------------
 m4/gc-random.m4          |  89 -------------------------
 modules/crypto/gc-random |   6 +-
 modules/crypto/gc-tests  |   2 +-
 5 files changed, 39 insertions(+), 209 deletions(-)
 delete mode 100644 m4/gc-random.m4

diff --git a/ChangeLog b/ChangeLog
index 92db4972c..0cedb2dc8 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,16 @@
+2021-01-20  Simon Josefsson  <simon@josefsson.org>
+
+	gc-random: Replace implementation with call to getrandom.
+	* lib/gc-gnulib.c [GNULIB_GC_RANDOM]: Replace #include's with
+	those needed for getrandom.
+	(gc_init): Remove old randomness code.
+	(gc_done): Likewise.
+	(randomize): Rewrite using getrandom, inspired by getentropy.
+	* m4/gc-random.m4: Remove file.
+	* modules/crypto/gc-random: Drop gc-random.m4, gl_GC_RANDOM, and
+	LIB_GC_RANDOM.  Add conditional dependency on getrandom.
+	* modules/crypto/gc-tests (test_gc_LDADD): Drop LIB_GC_RANDOM.
+
 2021-01-20  Bruno Haible  <bruno@clisp.org>
 
 	exec*e tests: Avoid test failures on Cygwin.
diff --git a/lib/gc-gnulib.c b/lib/gc-gnulib.c
index 9f361be0e..c6f201bd2 100644
--- a/lib/gc-gnulib.c
+++ b/lib/gc-gnulib.c
@@ -28,11 +28,9 @@
 
 /* For randomize. */
 #if GNULIB_GC_RANDOM
-# include <unistd.h>
-# include <sys/types.h>
-# include <sys/stat.h>
-# include <fcntl.h>
-# include <errno.h>
+#include <unistd.h>
+#include <errno.h>
+#include <sys/random.h>
 #endif
 
 /* Hashes. */
@@ -75,150 +73,62 @@
 # include "rijndael-api-fst.h"
 #endif
 
-#if GNULIB_GC_RANDOM
-# if defined _WIN32 && ! defined __CYGWIN__
-#  include <windows.h>
-#  include <wincrypt.h>
-HCRYPTPROV g_hProv = 0;
-#  ifndef PROV_INTEL_SEC
-#   define PROV_INTEL_SEC 22
-#  endif
-#  ifndef CRYPT_VERIFY_CONTEXT
-#   define CRYPT_VERIFY_CONTEXT 0xF0000000
-#  endif
-# endif
-#endif
-
-#if defined _WIN32 && ! defined __CYGWIN__
-/* Don't assume that UNICODE is not defined.  */
-# undef CryptAcquireContext
-# define CryptAcquireContext CryptAcquireContextA
-#endif
-
 Gc_rc
 gc_init (void)
 {
-#if GNULIB_GC_RANDOM
-# if defined _WIN32 && ! defined __CYGWIN__
-  if (g_hProv)
-    CryptReleaseContext (g_hProv, 0);
-
-  /* There is no need to create a container for just random data, so
-     we can use CRYPT_VERIFY_CONTEXT (one call) see:
-     https://web.archive.org/web/20070314163712/http://blogs.msdn.com/dangriff/archive/2003/11/19/51709.aspx */
-
-  /* We first try to use the Intel PIII RNG if drivers are present */
-  if (!CryptAcquireContext (&g_hProv, NULL, NULL,
-                            PROV_INTEL_SEC, CRYPT_VERIFY_CONTEXT))
-    {
-      /* not a PIII or no drivers available, use default RSA CSP */
-      if (!CryptAcquireContext (&g_hProv, NULL, NULL,
-                                PROV_RSA_FULL, CRYPT_VERIFY_CONTEXT))
-        return GC_RANDOM_ERROR;
-    }
-# endif
-#endif
-
   return GC_OK;
 }
 
 void
 gc_done (void)
 {
-#if GNULIB_GC_RANDOM
-# if defined _WIN32 && ! defined __CYGWIN__
-  if (g_hProv)
-    {
-      CryptReleaseContext (g_hProv, 0);
-      g_hProv = 0;
-    }
-# endif
-#endif
-
   return;
 }
 
 #if GNULIB_GC_RANDOM
 
-/* Randomness. */
-
-static Gc_rc
-randomize (int level, char *data, size_t datalen)
+/* Overwrite BUFFER with random data, under the control of getrandom
+   FLAGS.  BUFFER contains LENGTH bytes.  Inspired by getentropy,
+   however LENGTH is not restricted to 256.  Return 0 on success, -1
+   (setting errno) on failure.  */
+static int
+randomize (void *buffer, size_t length, unsigned int flags)
 {
-#if defined _WIN32 && ! defined __CYGWIN__
-  if (!g_hProv)
-    return GC_RANDOM_ERROR;
-  CryptGenRandom (g_hProv, (DWORD) datalen, data);
-#else
-  int fd;
-  const char *device;
-  size_t len = 0;
-  int rc;
-
-  switch (level)
-    {
-    case 0:
-      device = NAME_OF_NONCE_DEVICE;
-      break;
-
-    case 1:
-      device = NAME_OF_PSEUDO_RANDOM_DEVICE;
-      break;
-
-    default:
-      device = NAME_OF_RANDOM_DEVICE;
-      break;
-    }
-
-  if (strcmp (device, "no") == 0)
-    return GC_RANDOM_ERROR;
+  char *buf = buffer;
 
-  fd = open (device, O_RDONLY | O_CLOEXEC);
-  if (fd < 0)
-    return GC_RANDOM_ERROR;
-
-  do
+  for (;;)
     {
-      ssize_t tmp;
-
-      tmp = read (fd, data, datalen);
-
-      if (tmp < 0)
-        {
-          int save_errno = errno;
-          close (fd);
-          errno = save_errno;
+      ssize_t bytes;
+      if (length == 0)
+        return GC_OK;
+      while ((bytes = getrandom (buf, length, flags)) < 0)
+        if (errno != EINTR)
           return GC_RANDOM_ERROR;
-        }
-
-      len += tmp;
+      if (bytes == 0)
+        break;
+      buf += bytes;
+      length -= bytes;
     }
-  while (len < datalen);
-
-  rc = close (fd);
-  if (rc < 0)
-    return GC_RANDOM_ERROR;
-#endif
 
-  return GC_OK;
+  return GC_RANDOM_ERROR;
 }
 
 Gc_rc
 gc_nonce (char *data, size_t datalen)
 {
-  return randomize (0, data, datalen);
+  return randomize (data, datalen, 0);
 }
 
 Gc_rc
 gc_pseudo_random (char *data, size_t datalen)
 {
-  return randomize (1, data, datalen);
+  return randomize (data, datalen, 0);
 }
 
 Gc_rc
 gc_random (char *data, size_t datalen)
 {
-  return randomize (2, data, datalen);
+  return randomize (data, datalen, GRND_RANDOM);
 }
 
 #endif
diff --git a/m4/gc-random.m4 b/m4/gc-random.m4
deleted file mode 100644
index d27031b27..000000000
--- a/m4/gc-random.m4
+++ /dev/null
@@ -1,89 +0,0 @@
-# gc-random.m4 serial 9
-dnl Copyright (C) 2005-2021 Free Software Foundation, Inc.
-dnl This file is free software; the Free Software Foundation
-dnl gives unlimited permission to copy and/or distribute it,
-dnl with or without modifications, as long as this notice is preserved.
-
-AC_DEFUN([gl_GC_RANDOM],
-[
-  # Devices with randomness.
-  # FIXME: Are these the best defaults?
-
-  AC_REQUIRE([AC_CANONICAL_HOST])
-
-  case "$host_os" in
-    *mirbsd*)
-      NAME_OF_RANDOM_DEVICE="/dev/srandom"
-      NAME_OF_PSEUDO_RANDOM_DEVICE="/dev/prandom"
-      NAME_OF_NONCE_DEVICE="/dev/urandom"
-      ;;
-
-    *irix* | *dec-osf* )
-      NAME_OF_RANDOM_DEVICE="/dev/random"
-      NAME_OF_PSEUDO_RANDOM_DEVICE="/dev/random"
-      NAME_OF_NONCE_DEVICE="/dev/random"
-      ;;
-
-    *)
-      NAME_OF_RANDOM_DEVICE="/dev/random"
-      NAME_OF_PSEUDO_RANDOM_DEVICE="/dev/urandom"
-      NAME_OF_NONCE_DEVICE="/dev/urandom"
-      ;;
-  esac
-
-  AC_MSG_CHECKING([device with (strong) random data...])
-  AC_ARG_ENABLE([random-device],
-        AS_HELP_STRING([--enable-random-device],
-                [device with (strong) randomness (for Nettle)]),
-        NAME_OF_RANDOM_DEVICE=$enableval)
-  AC_MSG_RESULT([$NAME_OF_RANDOM_DEVICE])
-
-  AC_MSG_CHECKING([device with pseudo random data...])
-  AC_ARG_ENABLE([pseudo-random-device],
-        AS_HELP_STRING([--enable-pseudo-random-device],
-                [device with pseudo randomness (for Nettle)]),
-        NAME_OF_PSEUDO_RANDOM_DEVICE=$enableval)
-  AC_MSG_RESULT([$NAME_OF_PSEUDO_RANDOM_DEVICE])
-
-  AC_MSG_CHECKING([device with unpredictable data for nonces...])
-  AC_ARG_ENABLE([nonce-device],
-        AS_HELP_STRING([--enable-nonce-device],
-                [device with unpredictable nonces (for Nettle)]),
-        NAME_OF_NONCE_DEVICE=$enableval)
-  AC_MSG_RESULT([$NAME_OF_NONCE_DEVICE])
-
-  if test "$cross_compiling" != yes; then
-    if test "$NAME_OF_RANDOM_DEVICE" != "no"; then
-      AC_CHECK_FILE([$NAME_OF_RANDOM_DEVICE],,
-        AC_MSG_WARN([[Device '$NAME_OF_RANDOM_DEVICE' does not exist, consider to use --enable-random-device]]))
-    fi
-    if test "$NAME_OF_PSEUDO_RANDOM_DEVICE" != "no"; then
-      AC_CHECK_FILE([$NAME_OF_PSEUDO_RANDOM_DEVICE],,
-        AC_MSG_WARN([[Device '$NAME_OF_PSEUDO_RANDOM_DEVICE' does not exist, consider to use --enable-pseudo-random-device]]))
-    fi
-    if test "$NAME_OF_NONCE_DEVICE" != "no"; then
-      AC_CHECK_FILE([$NAME_OF_NONCE_DEVICE],,
-        AC_MSG_WARN([[Device '$NAME_OF_NONCE_DEVICE' does not exist, consider to use --enable-nonce-device]]))
-    fi
-  else
-    AC_MSG_NOTICE([[Cross compiling, assuming random devices exists on the target host...]])
-  fi
-
-  # FIXME?: Open+read 42 bytes+close twice and compare data.  Should differ.
-
-  AC_DEFINE_UNQUOTED([NAME_OF_RANDOM_DEVICE], ["$NAME_OF_RANDOM_DEVICE"],
-                   [defined to the name of the (strong) random device])
-  AC_DEFINE_UNQUOTED([NAME_OF_PSEUDO_RANDOM_DEVICE],
-                         "$NAME_OF_PSEUDO_RANDOM_DEVICE",
-                   [defined to the name of the pseudo random device])
-  AC_DEFINE_UNQUOTED([NAME_OF_NONCE_DEVICE], ["$NAME_OF_NONCE_DEVICE"],
-                   [defined to the name of the unpredictable nonce device])
-
-  case $host_os in
-    mingw*)
-      LIB_GC_RANDOM='-ladvapi32' ;;
-    *)
-      LIB_GC_RANDOM= ;;
-  esac
-  AC_SUBST([LIB_GC_RANDOM])
-])
diff --git a/modules/crypto/gc-random b/modules/crypto/gc-random
index 5158f8576..8a5ae462e 100644
--- a/modules/crypto/gc-random
+++ b/modules/crypto/gc-random
@@ -2,13 +2,12 @@ Description:
 Generic crypto random number functions.
 
 Files:
-m4/gc-random.m4
 
 Depends-on:
 crypto/gc
+getrandom [test "$ac_cv_libgcrypt" != yes]
 
 configure.ac:
-gl_GC_RANDOM
 gl_MODULE_INDICATOR([gc-random])
 
 Makefile.am:
@@ -16,9 +15,6 @@ Makefile.am:
 Include:
 "gc.h"
 
-Link:
-$(LIB_GC_RANDOM)
-
 License:
 LGPLv2+
 
diff --git a/modules/crypto/gc-tests b/modules/crypto/gc-tests
index 214db7a4e..7b153d18d 100644
--- a/modules/crypto/gc-tests
+++ b/modules/crypto/gc-tests
@@ -8,4 +8,4 @@ configure.ac:
 Makefile.am:
 TESTS += test-gc
 check_PROGRAMS += test-gc
-test_gc_LDADD = $(LDADD) @LIB_CRYPTO@ $(LIB_GC_RANDOM)
+test_gc_LDADD = $(LDADD) @LIB_CRYPTO@
-- 
2.20.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

Re: [PATCH] gc-random: Replace implementation with call to getrandom.

[Bruno Haible]

Hi Simon,

> Hi.  I re-read the discussion around getrandom vs gc-random and didn't
> see any point in keeping the duplicated code.  I believe the
> getrandom-approach is better than what was in gc-gnulib.c today, so this
> patch make it use that function.  I have pushed the patch below.

The change produces a link error when building for native Windows with MSVC:

libgnu.a(getrandom.obj) : error LNK2019: unresolved external symbol BCryptGenRandom referenced in function getrandom
test-gc.exe : fatal error LNK1120: 1 unresolved externals

This patch fixes it.


2021-01-20  Bruno Haible  <bruno@clisp.org>

	gc-random: Fix link error in tests.
	* modules/crypto/gc-random (Link): New section.
	* modules/crypto/gc-tests (Makefile.am): Link test-gc with
	$(LIB_GETRANDOM).

diff --git a/modules/crypto/gc-random b/modules/crypto/gc-random
index 8a5ae46..0e1bd76 100644
--- a/modules/crypto/gc-random
+++ b/modules/crypto/gc-random
@@ -15,6 +15,9 @@ Makefile.am:
 Include:
 "gc.h"
 
+Link:
+$(LIB_GETRANDOM)
+
 License:
 LGPLv2+
 
diff --git a/modules/crypto/gc-tests b/modules/crypto/gc-tests
index 7b153d1..5a16ba3 100644
--- a/modules/crypto/gc-tests
+++ b/modules/crypto/gc-tests
@@ -8,4 +8,4 @@ configure.ac:
 Makefile.am:
 TESTS += test-gc
 check_PROGRAMS += test-gc
-test_gc_LDADD = $(LDADD) @LIB_CRYPTO@
+test_gc_LDADD = $(LDADD) @LIB_CRYPTO@ $(LIB_GETRANDOM)

Re: [PATCH] gc-random: Replace implementation with call to getrandom.

[simon--- via Gnulib discussion list]

[-- Attachment #1: Type: text/plain, Size: 1258 bytes --]

Bruno Haible <bruno@clisp.org> writes:

> Hi Simon,
>
>> Hi.  I re-read the discussion around getrandom vs gc-random and didn't
>> see any point in keeping the duplicated code.  I believe the
>> getrandom-approach is better than what was in gc-gnulib.c today, so this
>> patch make it use that function.  I have pushed the patch below.
>
> The change produces a link error when building for native Windows with MSVC:

Thanks for testing!

> diff --git a/modules/crypto/gc-random b/modules/crypto/gc-random
> index 8a5ae46..0e1bd76 100644
> --- a/modules/crypto/gc-random
> +++ b/modules/crypto/gc-random
> @@ -15,6 +15,9 @@ Makefile.am:
>  Include:
>  "gc.h"
>  
> +Link:
> +$(LIB_GETRANDOM)
> +
>  License:
>  LGPLv2+

Is this part needed?  The module depends on getrandom.  Is it because
that is only a conditional dependency?

> diff --git a/modules/crypto/gc-tests b/modules/crypto/gc-tests
> index 7b153d1..5a16ba3 100644
> --- a/modules/crypto/gc-tests
> +++ b/modules/crypto/gc-tests
> @@ -8,4 +8,4 @@ configure.ac:
>  Makefile.am:
>  TESTS += test-gc
>  check_PROGRAMS += test-gc
> -test_gc_LDADD = $(LDADD) @LIB_CRYPTO@
> +test_gc_LDADD = $(LDADD) @LIB_CRYPTO@ $(LIB_GETRANDOM)

Looks good to me.

/Simon

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

Re: link dependencies

[Bruno Haible]

Simon Josefsson wrote:
> >  
> > +Link:
> > +$(LIB_GETRANDOM)
> > +
> >  License:
> >  LGPLv2+
> 
> Is this part needed?  The module depends on getrandom.  Is it because
> that is only a conditional dependency?
> 
> > diff --git a/modules/crypto/gc-tests b/modules/crypto/gc-tests
> > index 7b153d1..5a16ba3 100644
> > --- a/modules/crypto/gc-tests
> > +++ b/modules/crypto/gc-tests
> > @@ -8,4 +8,4 @@ configure.ac:
> >  Makefile.am:
> >  TESTS += test-gc
> >  check_PROGRAMS += test-gc
> > -test_gc_LDADD = $(LDADD) @LIB_CRYPTO@
> > +test_gc_LDADD = $(LDADD) @LIB_CRYPTO@ $(LIB_GETRANDOM)

In test_gc_LDADD, $(LIB_GETRANDOM) is needed, to avoid the link error on MSVC.

The 'Link' section in the module description is only informative.

Originally, we thought that it would be good to note, in the 'Link' section,
only the immediate link dependencies of the module, and let the user use
  ./gnulib-tool --extract-recursive-link-directive MODULE
for the rest.

But in the past few years, it has turned out that this does not work well:
As a user of a module, we don't look at the recursive dependencies closure
any more (partly because this closure is so large, often). The user of a
module looks at the module description _only_.

And so, the 'Link' section needs to contain the recursive link directive.
And when changing some module to use 'getrandom', all direct and indirect
callers of this module must get $(LIB_GETRANDOM) added to their module
descriptions.

Note that we don't (yet) follow this rule strictly for $(LIBINTL) and
$(LIBUNISTRING). But for more rarely used libraries like $(LIB_GETRANDOM)
it's worth it.

Bruno

Re: link dependencies

[simon--- via Gnulib discussion list]

> The 'Link' section in the module description is only informative.
> 
> Originally, we thought that it would be good to note, in the 'Link'
> section,
> only the immediate link dependencies of the module, and let the user
> use
>   ./gnulib-tool --extract-recursive-link-directive MODULE
> for the rest.
> 
> But in the past few years, it has turned out that this does not work
> well:
> As a user of a module, we don't look at the recursive dependencies
> closure
> any more (partly because this closure is so large, often). The user
> of a
> module looks at the module description _only_.
> 
> And so, the 'Link' section needs to contain the recursive link
> directive.
> And when changing some module to use 'getrandom', all direct and
> indirect
> callers of this module must get $(LIB_GETRANDOM) added to their
> module
> descriptions.
> 
> Note that we don't (yet) follow this rule strictly for $(LIBINTL) and
> $(LIBUNISTRING). But for more rarely used libraries like
> $(LIB_GETRANDOM)
> it's worth it.

Thanks for explaining!  It seems sub-optimal, but I can't think of what
a better solution would be.

/Simon