Hello Werner, all.
Steffen Nurpmeso wrote in
<20200529155411.tgyu1%stef...@sdaoden.eu>:
|Werner Koch wrote in
|<87sgfjrqf1....@wheatstone.g10code.de>:
||On Thu, 28 May 2020 14:43, Steffen Nurpmeso said:
...
|out for NAME_OF_DEV_*RANDOM at all .. hmm .. i must admit
|random/rndlinux.c:_gcry_rndlinux_gather_random() seems strange to
|me. :) Two possible calls to getpid, could be "((apid = XPID) !=
...
|I still would not do it like that, because if software cannot rely
...
|Anyhow, unless i am mistaken from this five minute looking, that
|random/random-csprng.c:getfnc_gather_random()
|
| #if USE_RNDLINUX
| if ( !access (NAME_OF_DEV_RANDOM, R_OK)
| && !access (NAME_OF_DEV_URANDOM, R_OK))
| {
| fnc = _gcry_rndlinux_gather_random;
| return fnc;
|}
| #endif
|
|i would change, maybe with a new call-in to rndlinux.c which
|should be made responsible for Linux-only environmental detections
|imho. Like that it could solely depend on getrandom, and make all
|the FDs optional, maybe by testing for NOSYS with a one byte read
|or what at first, or by later aborting if collecting random fails
|if that is possible. (For my MUA i use this for seeding only
|anyhow.)
|
||Are you running in FIPS mode?
||
||Can you run the Libgcrypt test suite? In particular
||
||$ libgcrypt/tests/version
||$ libgcrypt/tests/random --verbose --debug
So with the attached patch libgcrypt solely relies upon getentropy
if available, no FD handling is done no more if at all possible.
The test suite passes, a short review makes me think it is alright.
- The setup could block when the OS cannot serve 1 byte of strong
entropy. This is different to before, access(2) does not.
(On the other hand neither on OpenBSD nor on newer Linux (5.4 or
5.6 i think) this should matter. And it is likely it does not
elsewhere, either people seem to have used things like my
entropy-saver or even hammers like haveged which reveal how
strange entropy counting was, imho.)
- Some tests aka code places directly reach into
_gcry_rndlinux_gather_random() and thus only give errors in
open_device() not the warning that initiated this ML thread.
This i did not get at first, the tests suite passed nonetheless.
- P.S.: even if this patch is not used, i would suggest an audit
of this file.
- RANDOM_CONF_ONLY_URANDOM lost its meaning in the past, and this
patch does not reinstantiate that. It cannot be done portably,
except for OSs which provide getrandom(2).
- I shortly thought about using "extern", but i think doing so in
an isolated fashion is surely wrong.
Ciao,
--steffen
|
|Der Kragenbaer, The moon bear,
|der holt sich munter he cheerfully and one by one
|einen nach dem anderen runter wa.ks himself off
|(By Robert Gernhardt)
commit d37a0f8e (HEAD -> refs/heads/master)
Author: Steffen Nurpmeso <stef...@sdaoden.eu>
AuthorDate: 2020-05-29 21:44:59 +0200
Commit: Steffen Nurpmeso <stef...@sdaoden.eu>
CommitDate: 2020-05-29 22:03:43 +0200
random/rndlinux.c: avoid redundant actions on FDs if possible
---
random/rand-internal.h | 5 +-
random/random-csprng.c | 3 +-
random/random-drbg.c | 4 +-
random/rndlinux.c | 216 ++++++++++++++++++++++++++++---------------------
4 files changed, 133 insertions(+), 95 deletions(-)
diff --git a/random/rand-internal.h b/random/rand-internal.h
index d99c6671..4e1298c1 100644
--- a/random/rand-internal.h
+++ b/random/rand-internal.h
@@ -90,10 +90,13 @@ void _gcry_rngsystem_randomize (void *buffer, size_t length,
/*-- rndlinux.c --*/
+#if USE_RNDLINUX
+int _gcry_rndlinux_setup (void);
int _gcry_rndlinux_gather_random (void (*add) (const void *, size_t,
enum random_origins),
- enum random_origins origin,
+ enum random_origins origin,
size_t length, int level);
+#endif
/*-- rndunix.c --*/
int _gcry_rndunix_gather_random (void (*add) (const void *, size_t,
diff --git a/random/random-csprng.c b/random/random-csprng.c
index b06810a0..55557ae8 100644
--- a/random/random-csprng.c
+++ b/random/random-csprng.c
@@ -1120,8 +1120,7 @@ getfnc_gather_random (void))(void (*)(const void*, size_t,
enum random_origins, size_t, int);
#if USE_RNDLINUX
- if ( !access (NAME_OF_DEV_RANDOM, R_OK)
- && !access (NAME_OF_DEV_URANDOM, R_OK))
+ if (_gcry_rndlinux_setup () != -1)
{
fnc = _gcry_rndlinux_gather_random;
return fnc;
diff --git a/random/random-drbg.c b/random/random-drbg.c
index 6124f5fb..7f63fade 100644
--- a/random/random-drbg.c
+++ b/random/random-drbg.c
@@ -1865,11 +1865,11 @@ _gcry_rngdrbg_reinit (const char *flagstr, gcry_buffer_t *pers, int npers)
void
_gcry_rngdrbg_close_fds (void)
{
-#if USE_RNDLINUX
drbg_lock ();
+#if USE_RNDLINUX
_gcry_rndlinux_gather_random (NULL, 0, 0, 0);
- drbg_unlock ();
#endif
+ drbg_unlock ();
}
/* Print some statistics about the RNG. */
diff --git a/random/rndlinux.c b/random/rndlinux.c
index 04e2a464..c710d368 100644
--- a/random/rndlinux.c
+++ b/random/rndlinux.c
@@ -18,7 +18,6 @@
* License along with this program; if not, see <http://www.gnu.org/licenses/>.
*/
-
#include <config.h>
#include <stdio.h>
#include <stdlib.h>
@@ -32,21 +31,27 @@
#include <string.h>
#include <unistd.h>
#include <fcntl.h>
-#if defined(__linux__) || !defined(HAVE_GETENTROPY)
-#ifdef HAVE_SYSCALL
-# include <sys/syscall.h>
-# ifdef __NR_getrandom
-# define getentropy(buf,buflen) syscall (__NR_getrandom, buf, buflen, 0)
+
+#undef HAVE_GETRANDOM_SYSCALL
+#if !defined(HAVE_GETENTROPY)
+# if defined(__linux__) defined(HAVE_SYSCALL)
+# include <sys/syscall.h>
+# ifdef __NR_getrandom
+# define HAVE_GETENTROPY
+# define HAVE_GETRANDOM_SYSCALL
+# define getentropy(buf,buflen) syscall (__NR_getrandom, buf, buflen, 0)
+# endif
# endif
#endif
-#endif
#include "types.h"
#include "g10lib.h"
#include "rand-internal.h"
-static int open_device (const char *name, int retry);
+static int a_rndlinux_have_getentropy;
+static int set_cloexec_flag (int fd);
+static int open_device (const char *name, int retry);
static int
set_cloexec_flag (int fd)
@@ -60,8 +65,6 @@ set_cloexec_flag (int fd)
return fcntl (fd, F_SETFD, oldflags);
}
-
-
/*
* Used to open the /dev/random devices (Linux, xBSD, Solaris (if it
* exists)). If RETRY is true, the function does not terminate with
@@ -107,6 +110,43 @@ open_device (const char *name, int retry)
return fd;
}
+int
+_gcry_rndlinux_setup (void)
+{
+ int rv = 0;
+
+#if HAVE_GETENTROPY
+ if (!a_rndlinux_have_getentropy)
+ {
+ char buf[8];
+ long ret;
+
+ do
+ {
+ _gcry_pre_syscall ();
+ ret =
+# ifdef HAVE_GETRANDOM_SYSCALL
+ syscall (__NR_getrandom, buf, 1, GRND_NONBLOCK
+# else
+ getentropy (buf, 1
+# endif
+ );
+ _gcry_post_syscall ();
+ }
+ while (ret == -1 && errno == EINTR);
+
+ a_rndlinux_have_getentropy = (ret != -1 || errno != ENOSYS) ? ++rv : -1;
+ }
+ else
+ rv = (a_rndlinux_have_getentropy != -1);
+#endif /* HAVE_GETENTROPY */
+
+ if (!rv && !access (NAME_OF_DEV_RANDOM, R_OK)
+ && !access (NAME_OF_DEV_URANDOM, R_OK))
+ ++rv;
+
+ return --rv;
+}
/* Note that the caller needs to make sure that this function is only
* called by one thread at a time. The function returns 0 on success
@@ -137,55 +177,55 @@ _gcry_rndlinux_gather_random (void (*add)(const void*, size_t,
int any_need_entropy = 0;
int delay;
- /* On the first call read the conf file to check whether we want to
- * use only urandom. */
- if (only_urandom == -1)
+ /* XXX Some modules directly address this function whether usable or not */
+ if (!a_rndlinux_have_getentropy)
+ _gcry_rndlinux_setup ();
+
+ if (a_rndlinux_have_getentropy != -1)
{
- my_pid = getpid ();
- if ((_gcry_random_read_conf () & RANDOM_CONF_ONLY_URANDOM))
- only_urandom = 1;
- else
- only_urandom = 0;
+ /* Special destructor call? */
+ if (!add)
+ return 0;
}
-
- if (!add)
+ else
{
- /* Special mode to close the descriptors. */
- if (fd_random != -1)
+ /* Detect a fork and close the devices so that we do not use the old
+ * file descriptors. Note that open_device will be called in retry
+ * mode if the devices was opened by the parent process.
+ * !add is the special destructor call */
+ if (!add || (apid = getpid ()) != my_pid)
{
- close (fd_random);
- fd_random = -1;
- }
- if (fd_urandom != -1)
- {
- close (fd_urandom);
- fd_urandom = -1;
- }
+ if (fd_random != -1)
+ {
+ close (fd_random);
+ fd_random = -1;
+ }
+ if (fd_urandom != -1)
+ {
+ close (fd_urandom);
+ fd_urandom = -1;
+ }
- _gcry_rndjent_fini ();
- return 0;
- }
+ if (!add)
+ {
+ _gcry_rndjent_fini ();
+ return 0;
+ }
- /* Detect a fork and close the devices so that we don't use the old
- * file descriptors. Note that open_device will be called in retry
- * mode if the devices was opened by the parent process. */
- apid = getpid ();
- if (my_pid != apid)
- {
- if (fd_random != -1)
- {
- close (fd_random);
- fd_random = -1;
+ my_pid = apid;
}
- if (fd_urandom != -1)
+
+ /* On the first call read the conf file to check whether we want to
+ * use only urandom. XXX Does not play well with getrandom */
+ if (only_urandom == -1)
{
- close (fd_urandom);
- fd_urandom = -1;
+ if ((_gcry_random_read_conf () & RANDOM_CONF_ONLY_URANDOM))
+ only_urandom = 1;
+ else
+ only_urandom = 0;
}
- my_pid = apid;
}
-
/* First read from a hardware source. However let it account only
for up to 50% (or 25% for RDRAND) of the requested bytes. */
n_hw = _gcry_rndhw_poll_slow (add, origin);
@@ -214,6 +254,39 @@ _gcry_rndlinux_gather_random (void (*add)(const void*, size_t,
length -= n_hw;
}
+ /* If we have a modern kernel, try to use the new getentropy function.
+ * It guarantees that the kernel's RNG has been properly seeded before
+ * returning any data. */
+#if HAVE_GETENTROPY
+ if (a_rndlinux_have_getentropy != -1)
+ {
+ long ret;
+ size_t nbytes;
+
+ for (;;)
+ {
+ if (length == 0)
+ goto jhave_data;
+
+ do
+ {
+ nbytes = length < sizeof(buffer) ? length : sizeof(buffer);
+ if (nbytes > 256)
+ nbytes = 256;
+ _gcry_pre_syscall ();
+ ret = getentropy (buffer, nbytes);
+ _gcry_post_syscall ();
+ }
+ while (ret == -1 && errno == EINTR);
+ if (ret == -1)
+ break;
+
+ (*add)(buffer, nbytes, origin);
+ length -= nbytes;
+ }
+ log_fatal ("unexpected error from getentropy: %s\n", strerror (errno));
+ }
+#endif /* HAVE_GETENTROPY */
/* Open the requested device. The first time a device is to be
opened we fail with a fatal error if the device does not exists.
@@ -252,48 +325,6 @@ _gcry_rndlinux_gather_random (void (*add)(const void*, size_t,
struct timeval tv;
int rc;
- /* If we have a modern operating system, we first try to use the new
- * getentropy function. That call guarantees that the kernel's
- * RNG has been properly seeded before returning any data. This
- * is different from /dev/urandom which may, due to its
- * non-blocking semantics, return data even if the kernel has
- * not been properly seeded. And it differs from /dev/random by never
- * blocking once the kernel is seeded. */
-#if defined(HAVE_GETENTROPY) || defined(__NR_getrandom)
- {
- long ret;
- size_t nbytes;
-
- do
- {
- nbytes = length < sizeof(buffer)? length : sizeof(buffer);
- if (nbytes > 256)
- nbytes = 256;
- _gcry_pre_syscall ();
- ret = getentropy (buffer, nbytes);
- _gcry_post_syscall ();
- }
- while (ret == -1 && errno == EINTR);
- if (ret == -1 && errno == ENOSYS)
- ; /* getentropy is not supported - fallback to pulling from fd. */
- else
- { /* getentropy is supported. Some sanity checks. */
- if (ret == -1)
- log_fatal ("unexpected error from getentropy: %s\n",
- strerror (errno));
-#ifdef __NR_getrandom
- else if (ret != nbytes)
- log_fatal ("getentropy returned only"
- " %ld of %zu requested bytes\n", ret, nbytes);
-#endif
-
- (*add)(buffer, nbytes, origin);
- length -= nbytes;
- continue; /* until LENGTH is zero. */
- }
- }
-#endif
-
/* If we collected some bytes update the progress indicator. We
do this always and not just if the select timed out because
often just a few bytes are gathered within the timeout
@@ -356,6 +387,11 @@ _gcry_rndlinux_gather_random (void (*add)(const void*, size_t,
(*add)(buffer, n, origin);
length -= n;
}
+
+#if HAVE_GETENTROPY
+jhave_data:
+#endif
+
wipememory (buffer, sizeof buffer);
if (any_need_entropy)
_______________________________________________
Gnupg-users mailing list
Gnupg-users@gnupg.org
http://lists.gnupg.org/mailman/listinfo/gnupg-users
