[libvirt] [PATCH 00/10] Use better PRNG
This is inspired by bug reported here [1]. Even though Eric suggested calling this Linux syscall when building without gnutls [2] I've decided to not implement it. Firstly, we build with gnuls everywhere (even Windows), secondly I see no appealing reason to special case Linux - /dev/urandom is good for both Linux and FreeBSD. Once these are merged I'm probably going to send patch set that makes gnutls mandatory. I'm tired of all those WITH_GNUTLS if-defs (esp. in function arguments). But that is orthogonal to what I'm solving here. Also, I'm not quite sure this is a release material, so I'm fine with merging this after the release. 1: https://www.redhat.com/archives/libvirt-users/2018-May/msg00097.html 2: https://www.redhat.com/archives/libvirt-users/2018-May/msg00100.html Michal Privoznik (10): virRandomBytes: Fix return value virCryptoGenerateRandom: rename ret virCryptoGenerateRandom: Explain gnults error virCryptoGenerateRandom: Don't allocate return buffer virRandomBytes: Prefer saferead over plain read virRandomBytes: Report error virRandomBytes: Use gnutls_rnd whenever possible virrandom: Make virRandomBits better virUUIDGenerate don't fall back to virRandomBits vircrypto: Drop virCryptoGenerateRandom src/libvirt_private.syms | 1 - src/qemu/qemu_domain.c | 13 ++++-- src/util/vircrypto.c | 41 ------------------- src/util/vircrypto.h | 2 - src/util/virrandom.c | 103 ++++++++++++++++------------------------------- src/util/viruuid.c | 25 ++---------- tests/qemuxml2argvmock.c | 13 ------ tests/vircryptotest.c | 4 +- 8 files changed, 48 insertions(+), 154 deletions(-) -- 2.16.1
In libvirt when a function wants to return an error code it should be a negative value. Returning a positive value (or zero) means success. But virRandomBytes() does not follow this rule. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/vircrypto.c | 4 ++-- src/util/virrandom.c | 6 +++--- src/util/viruuid.c | 4 ++-- tests/vircryptotest.c | 4 ++-- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/util/vircrypto.c b/src/util/vircrypto.c index bbc2a01f22..4079013d6d 100644 --- a/src/util/vircrypto.c +++ b/src/util/vircrypto.c @@ -346,8 +346,8 @@ virCryptoGenerateRandom(size_t nbytes) /* If we don't have gnutls_rnd(), we will generate a less cryptographically * strong master buf from /dev/urandom. */ - if ((ret = virRandomBytes(buf, nbytes))) { - virReportSystemError(ret, "%s", _("failed to generate byte stream")); + if ((ret = virRandomBytes(buf, nbytes)) < 0) { + virReportSystemError(-ret, "%s", _("failed to generate byte stream")); VIR_FREE(buf); return NULL; } diff --git a/src/util/virrandom.c b/src/util/virrandom.c index 41daa404b2..9597640840 100644 --- a/src/util/virrandom.c +++ b/src/util/virrandom.c @@ -168,7 +168,7 @@ uint32_t virRandomInt(uint32_t max) * Generate a stream of random bytes from /dev/urandom * into @buf of size @buflen * - * Returns 0 on success or an errno on failure + * Returns 0 on success or an -errno on failure */ int virRandomBytes(unsigned char *buf, @@ -177,7 +177,7 @@ virRandomBytes(unsigned char *buf, int fd; if ((fd = open("/dev/urandom", O_RDONLY)) < 0) - return errno; + return -errno; while (buflen > 0) { ssize_t n; @@ -186,7 +186,7 @@ virRandomBytes(unsigned char *buf, if (errno == EINTR) continue; VIR_FORCE_CLOSE(fd); - return n < 0 ? errno : ENODATA; + return n < 0 ? -errno : -ENODATA; } buf += n; diff --git a/src/util/viruuid.c b/src/util/viruuid.c index 3cbaae0b85..61877aeba4 100644 --- a/src/util/viruuid.c +++ b/src/util/viruuid.c @@ -76,11 +76,11 @@ virUUIDGenerate(unsigned char *uuid) if (uuid == NULL) return -1; - if ((err = virRandomBytes(uuid, VIR_UUID_BUFLEN))) { + if ((err = virRandomBytes(uuid, VIR_UUID_BUFLEN)) < 0) { char ebuf[1024]; VIR_WARN("Falling back to pseudorandom UUID," " failed to generate random bytes: %s", - virStrerror(err, ebuf, sizeof(ebuf))); + virStrerror(-err, ebuf, sizeof(ebuf))); err = virUUIDGeneratePseudoRandomBytes(uuid, VIR_UUID_BUFLEN); } diff --git a/tests/vircryptotest.c b/tests/vircryptotest.c index d9ffc6f34c..b6313e73ad 100644 --- a/tests/vircryptotest.c +++ b/tests/vircryptotest.c @@ -88,8 +88,8 @@ testCryptoEncrypt(const void *opaque) VIR_ALLOC_N(iv, ivlen) < 0) goto cleanup; - if (virRandomBytes(enckey, enckeylen) || - virRandomBytes(iv, ivlen)) { + if (virRandomBytes(enckey, enckeylen) < 0 || + virRandomBytes(iv, ivlen) < 0) { fprintf(stderr, "Failed to generate random bytes\n"); goto cleanup; } -- 2.16.1
On 05/29/2018 03:24 AM, Michal Privoznik wrote:
In libvirt when a function wants to return an error code it should be a negative value. Returning a positive value (or zero) means success. But virRandomBytes() does not follow this rule.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/vircrypto.c | 4 ++-- src/util/virrandom.c | 6 +++--- src/util/viruuid.c | 4 ++-- tests/vircryptotest.c | 4 ++-- 4 files changed, 9 insertions(+), 9 deletions(-)
+++ b/src/util/virrandom.c @@ -168,7 +168,7 @@ uint32_t virRandomInt(uint32_t max) * Generate a stream of random bytes from /dev/urandom * into @buf of size @buflen * - * Returns 0 on success or an errno on failure + * Returns 0 on success or an -errno on failure
"an negative" sounds awkward when pronounced; I'd go with s/an // With that tweak, Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
This function allocates a buffer, fills it in with random bytes and then returns it. However, the buffer is held in @buf variable, therefore having @ret variable which does not hold return value of the function is misleading. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/vircrypto.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/util/vircrypto.c b/src/util/vircrypto.c index 4079013d6d..930fa3b215 100644 --- a/src/util/vircrypto.c +++ b/src/util/vircrypto.c @@ -329,16 +329,16 @@ uint8_t * virCryptoGenerateRandom(size_t nbytes) { uint8_t *buf; - int ret; + int rv; if (VIR_ALLOC_N(buf, nbytes) < 0) return NULL; #if WITH_GNUTLS /* Generate the byte stream using gnutls_rnd() if possible */ - if ((ret = gnutls_rnd(GNUTLS_RND_RANDOM, buf, nbytes)) < 0) { + if ((rv = gnutls_rnd(GNUTLS_RND_RANDOM, buf, nbytes)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("failed to generate byte stream, ret=%d"), ret); + _("failed to generate byte stream, rv=%d"), rv); VIR_FREE(buf); return NULL; } @@ -346,8 +346,8 @@ virCryptoGenerateRandom(size_t nbytes) /* If we don't have gnutls_rnd(), we will generate a less cryptographically * strong master buf from /dev/urandom. */ - if ((ret = virRandomBytes(buf, nbytes)) < 0) { - virReportSystemError(-ret, "%s", _("failed to generate byte stream")); + if ((rv = virRandomBytes(buf, nbytes)) < 0) { + virReportSystemError(-rv, "%s", _("failed to generate byte stream")); VIR_FREE(buf); return NULL; } -- 2.16.1
On 05/29/2018 03:24 AM, Michal Privoznik wrote:
This function allocates a buffer, fills it in with random bytes and then returns it. However, the buffer is held in @buf variable, therefore having @ret variable which does not hold return value of the function is misleading.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/vircrypto.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
When generating random stream using gnults fails an error is reported. However, the error is not helpful as it contains only an integer error code (a negative number). Use gnutls_strerror() to turn the error code into a string explaining what went wrong. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/vircrypto.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/util/vircrypto.c b/src/util/vircrypto.c index 930fa3b215..9879c31555 100644 --- a/src/util/vircrypto.c +++ b/src/util/vircrypto.c @@ -323,7 +323,8 @@ virCryptoEncryptData(virCryptoCipher algorithm, * Since the gnutls_rnd could be missing, provide an alternate less * secure mechanism to at least have something. * - * Returns pointer memory containing byte stream on success, NULL on failure + * Returns pointer memory containing byte stream on success, + * NULL on failure (with error reported) */ uint8_t * virCryptoGenerateRandom(size_t nbytes) @@ -338,7 +339,8 @@ virCryptoGenerateRandom(size_t nbytes) /* Generate the byte stream using gnutls_rnd() if possible */ if ((rv = gnutls_rnd(GNUTLS_RND_RANDOM, buf, nbytes)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("failed to generate byte stream, rv=%d"), rv); + _("failed to generate byte stream: %s"), + gnutls_strerror(rv)); VIR_FREE(buf); return NULL; } -- 2.16.1
To unify our vir*Random() functions we need to make virCryptoGenerateRandom NOT allocate return buffer. It should just fill given buffer with random data. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 12 ++++++++---- src/util/vircrypto.c | 29 ++++++++++++----------------- src/util/vircrypto.h | 3 ++- tests/qemuxml2argvmock.c | 14 ++++---------- 4 files changed, 26 insertions(+), 32 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 47910acb83..2d13a03344 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -930,12 +930,13 @@ qemuDomainMasterKeyCreate(virDomainObjPtr vm) if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_SECRET)) return 0; - if (!(priv->masterKey = - virCryptoGenerateRandom(QEMU_DOMAIN_MASTER_KEY_LEN))) + if (VIR_ALLOC_N(priv->masterKey, QEMU_DOMAIN_MASTER_KEY_LEN) < 0) return -1; - priv->masterKeyLen = QEMU_DOMAIN_MASTER_KEY_LEN; + if (virCryptoGenerateRandom(priv->masterKey, QEMU_DOMAIN_MASTER_KEY_LEN) < 0) + return -1; + return 0; } @@ -1214,8 +1215,11 @@ qemuDomainSecretAESSetup(qemuDomainObjPrivatePtr priv, if (!(secinfo->s.aes.alias = qemuDomainGetSecretAESAlias(srcalias, isLuks))) goto cleanup; + if (VIR_ALLOC_N(raw_iv, ivlen) < 0) + goto cleanup; + /* Create a random initialization vector */ - if (!(raw_iv = virCryptoGenerateRandom(ivlen))) + if (virCryptoGenerateRandom(raw_iv, ivlen) < 0) goto cleanup; /* Encode the IV and save that since qemu will need it */ diff --git a/src/util/vircrypto.c b/src/util/vircrypto.c index 9879c31555..673e1648e8 100644 --- a/src/util/vircrypto.c +++ b/src/util/vircrypto.c @@ -316,44 +316,39 @@ virCryptoEncryptData(virCryptoCipher algorithm, #endif /* virCryptoGenerateRandom: - * @nbytes: Size in bytes of random byte stream to generate + * @buf: Pointer to location to store bytes + * @buflen: Number of bytes to store * - * Generate a random stream of nbytes length and return it. + * Generate a random stream of @buflen length and store it into @buf. * * Since the gnutls_rnd could be missing, provide an alternate less * secure mechanism to at least have something. * - * Returns pointer memory containing byte stream on success, - * NULL on failure (with error reported) + * Returns 0 on success or -1 on failure (with error reported) */ -uint8_t * -virCryptoGenerateRandom(size_t nbytes) +int +virCryptoGenerateRandom(unsigned char *buf, + size_t buflen) { - uint8_t *buf; int rv; - if (VIR_ALLOC_N(buf, nbytes) < 0) - return NULL; - #if WITH_GNUTLS /* Generate the byte stream using gnutls_rnd() if possible */ - if ((rv = gnutls_rnd(GNUTLS_RND_RANDOM, buf, nbytes)) < 0) { + if ((rv = gnutls_rnd(GNUTLS_RND_RANDOM, buf, buflen)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("failed to generate byte stream: %s"), gnutls_strerror(rv)); - VIR_FREE(buf); - return NULL; + return -1; } #else /* If we don't have gnutls_rnd(), we will generate a less cryptographically * strong master buf from /dev/urandom. */ - if ((rv = virRandomBytes(buf, nbytes)) < 0) { + if ((rv = virRandomBytes(buf, buflen)) < 0) { virReportSystemError(-rv, "%s", _("failed to generate byte stream")); - VIR_FREE(buf); - return NULL; + return -1; } #endif - return buf; + return 0; } diff --git a/src/util/vircrypto.h b/src/util/vircrypto.h index 9b5dada53d..649ceff1a1 100644 --- a/src/util/vircrypto.h +++ b/src/util/vircrypto.h @@ -65,6 +65,7 @@ int virCryptoEncryptData(virCryptoCipher algorithm, ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(6) ATTRIBUTE_NONNULL(8) ATTRIBUTE_NONNULL(9) ATTRIBUTE_RETURN_CHECK; -uint8_t *virCryptoGenerateRandom(size_t nbytes) ATTRIBUTE_NOINLINE; +int virCryptoGenerateRandom(unsigned char *buf, + size_t buflen) ATTRIBUTE_NOINLINE; #endif /* __VIR_CRYPTO_H__ */ diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c index 6d78063f00..44b6504de9 100644 --- a/tests/qemuxml2argvmock.c +++ b/tests/qemuxml2argvmock.c @@ -190,17 +190,11 @@ virCommandPassFD(virCommandPtr cmd ATTRIBUTE_UNUSED, /* nada */ } -uint8_t * -virCryptoGenerateRandom(size_t nbytes) +int +virCryptoGenerateRandom(unsigned char *buf, + size_t buflen) { - uint8_t *buf; - - if (VIR_ALLOC_N(buf, nbytes) < 0) - return NULL; - - ignore_value(virRandomBytes(buf, nbytes)); - - return buf; + return virRandomBytes(buf, buflen); } int -- 2.16.1
On 05/29/2018 03:24 AM, Michal Privoznik wrote:
To unify our vir*Random() functions we need to make virCryptoGenerateRandom NOT allocate return buffer. It should just fill given buffer with random data.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 12 ++++++++---- src/util/vircrypto.c | 29 ++++++++++++----------------- src/util/vircrypto.h | 3 ++- tests/qemuxml2argvmock.c | 14 ++++---------- 4 files changed, 26 insertions(+), 32 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 47910acb83..2d13a03344 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -930,12 +930,13 @@ qemuDomainMasterKeyCreate(virDomainObjPtr vm) if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_SECRET)) return 0;
- if (!(priv->masterKey = - virCryptoGenerateRandom(QEMU_DOMAIN_MASTER_KEY_LEN))) + if (VIR_ALLOC_N(priv->masterKey, QEMU_DOMAIN_MASTER_KEY_LEN) < 0) return -1; - priv->masterKeyLen = QEMU_DOMAIN_MASTER_KEY_LEN;
+ if (virCryptoGenerateRandom(priv->masterKey, QEMU_DOMAIN_MASTER_KEY_LEN) < 0) + return -1;
Should this free priv->masterKey and set it back to NULL, so that no other client is tempted to use a half-baked buffer as a key prior to the object being destroyed?
+++ b/tests/qemuxml2argvmock.c @@ -190,17 +190,11 @@ virCommandPassFD(virCommandPtr cmd ATTRIBUTE_UNUSED, /* nada */ }
-uint8_t * -virCryptoGenerateRandom(size_t nbytes) +int +virCryptoGenerateRandom(unsigned char *buf, + size_t buflen)
Indentation looks off.
{ - uint8_t *buf; - - if (VIR_ALLOC_N(buf, nbytes) < 0) - return NULL; - - ignore_value(virRandomBytes(buf, nbytes)); - - return buf; + return virRandomBytes(buf, buflen);
Hmm, my earlier comment about the #if 0 for debugging might be more relevant here - if we are going to mock the random numbers to be reproducible during the testsuite, THIS would be a nice place to fall back to rand() and friends with a reliable sequence when given a fixed seed (rather than directly in src/util/virrandom.c). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On 05/30/2018 02:46 AM, Eric Blake wrote:
On 05/29/2018 03:24 AM, Michal Privoznik wrote:
To unify our vir*Random() functions we need to make virCryptoGenerateRandom NOT allocate return buffer. It should just fill given buffer with random data.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 12 ++++++++---- src/util/vircrypto.c | 29 ++++++++++++----------------- src/util/vircrypto.h | 3 ++- tests/qemuxml2argvmock.c | 14 ++++---------- 4 files changed, 26 insertions(+), 32 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 47910acb83..2d13a03344 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -930,12 +930,13 @@ qemuDomainMasterKeyCreate(virDomainObjPtr vm) if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_SECRET)) return 0; - if (!(priv->masterKey = - virCryptoGenerateRandom(QEMU_DOMAIN_MASTER_KEY_LEN))) + if (VIR_ALLOC_N(priv->masterKey, QEMU_DOMAIN_MASTER_KEY_LEN) < 0) return -1; - priv->masterKeyLen = QEMU_DOMAIN_MASTER_KEY_LEN; + if (virCryptoGenerateRandom(priv->masterKey, QEMU_DOMAIN_MASTER_KEY_LEN) < 0) + return -1;
Should this free priv->masterKey and set it back to NULL, so that no other client is tempted to use a half-baked buffer as a key prior to the object being destroyed?
Good point.
+++ b/tests/qemuxml2argvmock.c @@ -190,17 +190,11 @@ virCommandPassFD(virCommandPtr cmd ATTRIBUTE_UNUSED, /* nada */ } -uint8_t * -virCryptoGenerateRandom(size_t nbytes) +int +virCryptoGenerateRandom(unsigned char *buf, + size_t buflen)
Indentation looks off.
{ - uint8_t *buf; - - if (VIR_ALLOC_N(buf, nbytes) < 0) - return NULL; - - ignore_value(virRandomBytes(buf, nbytes)); - - return buf; + return virRandomBytes(buf, buflen);
Hmm, my earlier comment about the #if 0 for debugging might be more relevant here - if we are going to mock the random numbers to be reproducible during the testsuite, THIS would be a nice place to fall back to rand() and friends with a reliable sequence when given a fixed seed (rather than directly in src/util/virrandom.c).
As I say in my reply to 08/10 I don't think we need to preserve ability to use weak PRNG. I see two reasons for not having fallback in: 1) even though we have gnutls optional we build everywhere with it, so gnutls_rnd() is going to be used. Therefore we can have a small mock: gnutls_rnd() { return 4; /* yes, xkcd reference */ } 2) In case when building without gnutls, one has to modify RANDOM_SOURCE macro in src/util/virrandom.c (which I'm introducing in 06/10). But as I say in the cover letter, I'm planning on making gnutls required. So no workaround like 2) would be needed. Michal
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virrandom.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/util/virrandom.c b/src/util/virrandom.c index 9597640840..ea55fe654d 100644 --- a/src/util/virrandom.c +++ b/src/util/virrandom.c @@ -182,9 +182,7 @@ virRandomBytes(unsigned char *buf, while (buflen > 0) { ssize_t n; - if ((n = read(fd, buf, buflen)) <= 0) { - if (errno == EINTR) - continue; + if ((n = saferead(fd, buf, buflen)) <= 0) { VIR_FORCE_CLOSE(fd); return n < 0 ? -errno : -ENODATA; } -- 2.16.1
Instead of having each caller report error move it into the function. This way we can produce more accurate error messages too. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/vircrypto.c | 6 ++---- src/util/virrandom.c | 18 +++++++++++++----- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/src/util/vircrypto.c b/src/util/vircrypto.c index 673e1648e8..e5f2319720 100644 --- a/src/util/vircrypto.c +++ b/src/util/vircrypto.c @@ -330,9 +330,9 @@ int virCryptoGenerateRandom(unsigned char *buf, size_t buflen) { +#if WITH_GNUTLS int rv; -#if WITH_GNUTLS /* Generate the byte stream using gnutls_rnd() if possible */ if ((rv = gnutls_rnd(GNUTLS_RND_RANDOM, buf, buflen)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -344,10 +344,8 @@ virCryptoGenerateRandom(unsigned char *buf, /* If we don't have gnutls_rnd(), we will generate a less cryptographically * strong master buf from /dev/urandom. */ - if ((rv = virRandomBytes(buf, buflen)) < 0) { - virReportSystemError(-rv, "%s", _("failed to generate byte stream")); + if (virRandomBytes(buf, buflen) < 0) return -1; - } #endif return 0; diff --git a/src/util/virrandom.c b/src/util/virrandom.c index ea55fe654d..230745d311 100644 --- a/src/util/virrandom.c +++ b/src/util/virrandom.c @@ -43,6 +43,8 @@ VIR_LOG_INIT("util.random"); +#define RANDOM_SOURCE "/dev/urandom" + /* The algorithm of virRandomBits relies on gnulib's guarantee that * 'random_r' matches the POSIX requirements on 'random' of being * evenly distributed among exactly [0, 2**31) (that is, we always get @@ -107,7 +109,6 @@ uint64_t virRandomBits(int nbits) if (virRandomInitialize() < 0) { /* You're already hosed, so this particular non-random value * isn't any worse. */ - VIR_WARN("random number generation is broken"); return 0; } @@ -165,10 +166,10 @@ uint32_t virRandomInt(uint32_t max) * @buf: Pointer to location to store bytes * @buflen: Number of bytes to store * - * Generate a stream of random bytes from /dev/urandom + * Generate a stream of random bytes from RANDOM_SOURCE * into @buf of size @buflen * - * Returns 0 on success or an -errno on failure + * Returns 0 on success or -1 (with error reported) */ int virRandomBytes(unsigned char *buf, @@ -176,13 +177,20 @@ virRandomBytes(unsigned char *buf, { int fd; - if ((fd = open("/dev/urandom", O_RDONLY)) < 0) - return -errno; + if ((fd = open(RANDOM_SOURCE, O_RDONLY)) < 0) { + virReportSystemError(errno, + _("unable to open %s"), + RANDOM_SOURCE); + return -1; + } while (buflen > 0) { ssize_t n; if ((n = saferead(fd, buf, buflen)) <= 0) { + virReportSystemError(errno, + _("unable to read from %s"), + RANDOM_SOURCE); VIR_FORCE_CLOSE(fd); return n < 0 ? -errno : -ENODATA; } -- 2.16.1
While /dev/urandom is not terrible source of random data gnutls_rnd is better. Prefer that one. Also, since nearly every platform we build on already has gnutls (if not all of them) this is going to be used by default. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/vircrypto.c | 20 +------------------- src/util/virrandom.c | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+), 19 deletions(-) diff --git a/src/util/vircrypto.c b/src/util/vircrypto.c index e5f2319720..3f3ba0267a 100644 --- a/src/util/vircrypto.c +++ b/src/util/vircrypto.c @@ -330,23 +330,5 @@ int virCryptoGenerateRandom(unsigned char *buf, size_t buflen) { -#if WITH_GNUTLS - int rv; - - /* Generate the byte stream using gnutls_rnd() if possible */ - if ((rv = gnutls_rnd(GNUTLS_RND_RANDOM, buf, buflen)) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("failed to generate byte stream: %s"), - gnutls_strerror(rv)); - return -1; - } -#else - /* If we don't have gnutls_rnd(), we will generate a less cryptographically - * strong master buf from /dev/urandom. - */ - if (virRandomBytes(buf, buflen) < 0) - return -1; -#endif - - return 0; + return virRandomBytes(buf, buflen); } diff --git a/src/util/virrandom.c b/src/util/virrandom.c index 230745d311..444b0f9802 100644 --- a/src/util/virrandom.c +++ b/src/util/virrandom.c @@ -29,6 +29,10 @@ #include <fcntl.h> #include <sys/stat.h> #include <sys/types.h> +#ifdef WITH_GNUTLS +# include <gnutls/gnutls.h> +# include <gnutls/crypto.h> +#endif #include "virrandom.h" #include "virthread.h" @@ -175,6 +179,19 @@ int virRandomBytes(unsigned char *buf, size_t buflen) { +#if WITH_GNUTLS + int rv; + + /* Generate the byte stream using gnutls_rnd() if possible */ + if ((rv = gnutls_rnd(GNUTLS_RND_RANDOM, buf, buflen)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to generate byte stream: %s"), + gnutls_strerror(rv)); + return -1; + } + +#else /* !WITH_GNUTLS */ + int fd; if ((fd = open(RANDOM_SOURCE, O_RDONLY)) < 0) { @@ -200,6 +217,7 @@ virRandomBytes(unsigned char *buf, } VIR_FORCE_CLOSE(fd); +#endif /* !WITH_GNUTLS */ return 0; } -- 2.16.1
Now that we have strong PRNG generator implemented in virRandomBytes() let's use that instead of gnulib's random_r. Problem with the latter is in way we seed it: current UNIX time and libvirtd's PID are not that random as one might think. Imagine two hosts booting at the same time. There's a fair chance that those hosts spawn libvirtds at the same time and with the same PID. This will result in both daemons generating the same sequence of say MAC addresses [1]. 1: https://www.redhat.com/archives/libvirt-users/2018-May/msg00097.html Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virrandom.c | 63 ++-------------------------------------------------- 1 file changed, 2 insertions(+), 61 deletions(-) diff --git a/src/util/virrandom.c b/src/util/virrandom.c index 444b0f9802..01cc82a052 100644 --- a/src/util/virrandom.c +++ b/src/util/virrandom.c @@ -49,53 +49,6 @@ VIR_LOG_INIT("util.random"); #define RANDOM_SOURCE "/dev/urandom" -/* The algorithm of virRandomBits relies on gnulib's guarantee that - * 'random_r' matches the POSIX requirements on 'random' of being - * evenly distributed among exactly [0, 2**31) (that is, we always get - * exactly 31 bits). While this happens to be the value of RAND_MAX - * on glibc, note that POSIX only requires RAND_MAX to be tied to the - * weaker 'rand', so there are platforms where RAND_MAX is smaller - * than the range of 'random_r'. For the results to be evenly - * distributed among up to 64 bits, we also rely on the period of - * 'random_r' to be at least 2**64, which POSIX only guarantees for - * 'random' if you use 256 bytes of state. */ -enum { - RANDOM_BITS_PER_ITER = 31, - RANDOM_BITS_MASK = (1U << RANDOM_BITS_PER_ITER) - 1, - RANDOM_STATE_SIZE = 256, -}; - -static char randomState[RANDOM_STATE_SIZE]; -static struct random_data randomData; -static virMutex randomLock = VIR_MUTEX_INITIALIZER; - - -static int -virRandomOnceInit(void) -{ - unsigned int seed = time(NULL) ^ getpid(); - -#if 0 - /* Normally we want a decent seed. But if reproducible debugging - * of a fixed pseudo-random sequence is ever required, uncomment - * this block to let an environment variable force the seed. */ - const char *debug = virGetEnvBlockSUID("VIR_DEBUG_RANDOM_SEED"); - - if (debug && virStrToLong_ui(debug, NULL, 0, &seed) < 0) - return -1; -#endif - - if (initstate_r(seed, - randomState, - sizeof(randomState), - &randomData) < 0) - return -1; - - return 0; -} - -VIR_ONCE_GLOBAL_INIT(virRandom) - /** * virRandomBits: * @nbits: Number of bits of randommess required @@ -108,26 +61,14 @@ VIR_ONCE_GLOBAL_INIT(virRandom) uint64_t virRandomBits(int nbits) { uint64_t ret = 0; - int32_t bits; - if (virRandomInitialize() < 0) { + if (virRandomBytes((unsigned char *) &ret, sizeof(ret)) < 0) { /* You're already hosed, so this particular non-random value * isn't any worse. */ return 0; } - virMutexLock(&randomLock); - - while (nbits > RANDOM_BITS_PER_ITER) { - random_r(&randomData, &bits); - ret = (ret << RANDOM_BITS_PER_ITER) | (bits & RANDOM_BITS_MASK); - nbits -= RANDOM_BITS_PER_ITER; - } - - random_r(&randomData, &bits); - ret = (ret << nbits) | (bits & ((1 << nbits) - 1)); - - virMutexUnlock(&randomLock); + ret &= (1U << nbits) - 1; return ret; } -- 2.16.1
On Tue, May 29, 2018 at 10:24:44AM +0200, Michal Privoznik wrote:
Now that we have strong PRNG generator implemented in virRandomBytes() let's use that instead of gnulib's random_r.
Problem with the latter is in way we seed it: current UNIX time and libvirtd's PID are not that random as one might think. Imagine two hosts booting at the same time. There's a fair chance that those hosts spawn libvirtds at the same time and with the same PID. This will result in both daemons generating the same sequence of say MAC addresses [1].
1: https://www.redhat.com/archives/libvirt-users/2018-May/msg00097.html
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virrandom.c | 63 ++-------------------------------------------------- 1 file changed, 2 insertions(+), 61 deletions(-)
ACK to patches 1-7. But for this one I'm "concerned" about few things. First of all, just so I don't forget it, random_r can be removed from bootstrap.conf after this patch, right? Before this patch, and without gnutls, we wouldn't deplete the entropy of the kernel, (even though we're just using /dev/urandom and not /dev/random), but now we'd read everything from /dev/urandom. Last but not least, if we completely drop the non-gnutls variants of everything, wouldn't everything be easier anyway? Like the worrying about entropy pool in previous point? And one thing below:
diff --git a/src/util/virrandom.c b/src/util/virrandom.c index 444b0f9802..01cc82a052 100644 --- a/src/util/virrandom.c +++ b/src/util/virrandom.c @@ -108,26 +61,14 @@ VIR_ONCE_GLOBAL_INIT(virRandom) uint64_t virRandomBits(int nbits) { uint64_t ret = 0; - int32_t bits;
- if (virRandomInitialize() < 0) { + if (virRandomBytes((unsigned char *) &ret, sizeof(ret)) < 0) { /* You're already hosed, so this particular non-random value * isn't any worse. */ return 0;
We definitely want to return an error here now IMHO.
On 05/29/2018 03:44 PM, Martin Kletzander wrote:
On Tue, May 29, 2018 at 10:24:44AM +0200, Michal Privoznik wrote:
Now that we have strong PRNG generator implemented in virRandomBytes() let's use that instead of gnulib's random_r.
Problem with the latter is in way we seed it: current UNIX time and libvirtd's PID are not that random as one might think. Imagine two hosts booting at the same time. There's a fair chance that those hosts spawn libvirtds at the same time and with the same PID. This will result in both daemons generating the same sequence of say MAC addresses [1].
1: https://www.redhat.com/archives/libvirt-users/2018-May/msg00097.html
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virrandom.c | 63 ++-------------------------------------------------- 1 file changed, 2 insertions(+), 61 deletions(-)
ACK to patches 1-7. But for this one I'm "concerned" about few things.
First of all, just so I don't forget it, random_r can be removed from bootstrap.conf after this patch, right?
Yes, I was meaning to make that change but then I forgot.
Before this patch, and without gnutls, we wouldn't deplete the entropy of the kernel, (even though we're just using /dev/urandom and not /dev/random), but now we'd read everything from /dev/urandom.
Unless we are built with gnutls. But I don't see much problem with that.
Last but not least, if we completely drop the non-gnutls variants of everything, wouldn't everything be easier anyway? Like the worrying about entropy pool in previous point?
Sure. But requiring gnutls (like I'm suggesting in the cover letter) is orthogonal to these patches IMO.
And one thing below:
diff --git a/src/util/virrandom.c b/src/util/virrandom.c index 444b0f9802..01cc82a052 100644 --- a/src/util/virrandom.c +++ b/src/util/virrandom.c @@ -108,26 +61,14 @@ VIR_ONCE_GLOBAL_INIT(virRandom) uint64_t virRandomBits(int nbits) { uint64_t ret = 0; - int32_t bits;
- if (virRandomInitialize() < 0) { + if (virRandomBytes((unsigned char *) &ret, sizeof(ret)) < 0) { /* You're already hosed, so this particular non-random value * isn't any worse. */ return 0;
We definitely want to return an error here now IMHO.
I'm not quite sure how to achieve that. The only thing I can think about is changing virRandomBits() signature. But since it is pre-existing I think it's safe to do in a follow up patch. Michal
On Wed, May 30, 2018 at 02:16:08PM +0200, Michal Privoznik wrote:
On 05/29/2018 03:44 PM, Martin Kletzander wrote:
On Tue, May 29, 2018 at 10:24:44AM +0200, Michal Privoznik wrote:
Now that we have strong PRNG generator implemented in virRandomBytes() let's use that instead of gnulib's random_r.
Problem with the latter is in way we seed it: current UNIX time and libvirtd's PID are not that random as one might think. Imagine two hosts booting at the same time. There's a fair chance that those hosts spawn libvirtds at the same time and with the same PID. This will result in both daemons generating the same sequence of say MAC addresses [1].
1: https://www.redhat.com/archives/libvirt-users/2018-May/msg00097.html
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virrandom.c | 63 ++-------------------------------------------------- 1 file changed, 2 insertions(+), 61 deletions(-)
ACK to patches 1-7. But for this one I'm "concerned" about few things.
First of all, just so I don't forget it, random_r can be removed from bootstrap.conf after this patch, right?
Yes, I was meaning to make that change but then I forgot.
Before this patch, and without gnutls, we wouldn't deplete the entropy of the kernel, (even though we're just using /dev/urandom and not /dev/random), but now we'd read everything from /dev/urandom.
Unless we are built with gnutls. But I don't see much problem with that.
Yeah, it's not that big of a deal, just an extra point for the next thing I mentioned below.
Last but not least, if we completely drop the non-gnutls variants of everything, wouldn't everything be easier anyway? Like the worrying about entropy pool in previous point?
Sure. But requiring gnutls (like I'm suggesting in the cover letter) is orthogonal to these patches IMO.
My point was that the fixes might be could be cleaner and shorter, but that "not that big of a deal" above would be fixed. It also makes it kind of relevant. Since /dev/urandom cannot be really exhausted in newer Linux kernels (not sure for FreeBSD and others), I don't think that's a problem. We should ensure, however, that it is seeded properly. It might not be when it's early during the boot for Linux (although systemd and others seed it explicitly early enough), that's what getrandom() is for as it ensures the seeding was done properly. But that's Linux-specific. FreeBSD will apparently not give you any data until it's seeded properly. Anyway, I got a bit caught up there. I also learned something new and I think the patch can be used like this. We might also ditch gnutls if we use getrandom() on Linux before using /dev/urandom. That should be fine if we want to take the other approach. But it looks like gnutls will be needed anyway, so...
And one thing below:
diff --git a/src/util/virrandom.c b/src/util/virrandom.c index 444b0f9802..01cc82a052 100644 --- a/src/util/virrandom.c +++ b/src/util/virrandom.c @@ -108,26 +61,14 @@ VIR_ONCE_GLOBAL_INIT(virRandom) uint64_t virRandomBits(int nbits) { uint64_t ret = 0; - int32_t bits;
- if (virRandomInitialize() < 0) { + if (virRandomBytes((unsigned char *) &ret, sizeof(ret)) < 0) { /* You're already hosed, so this particular non-random value * isn't any worse. */ return 0;
We definitely want to return an error here now IMHO.
I'm not quite sure how to achieve that. The only thing I can think about is changing virRandomBits() signature. But since it is pre-existing I think it's safe to do in a follow up patch.
I thought of it differently. The way it failed before was during initialization, once per the daemon running for example, and then all the calls to virRandomBytes were returning 0 all the time. This way (although I have no idea when gnutls_rnd can fail) it can be returning fine random numbers and then, out of nowhere, return 0 few times, then continue with random numbers. I just wanted so that we both understand what the change here is. Since we're logging the error already, maybe just resetting it is fine. Or actually it could be left there. So ACK if you remove random_r from bootstrap.conf.
On Wed, May 30, 2018 at 03:43:31PM +0200, Martin Kletzander wrote:
On Wed, May 30, 2018 at 02:16:08PM +0200, Michal Privoznik wrote:
On 05/29/2018 03:44 PM, Martin Kletzander wrote:
On Tue, May 29, 2018 at 10:24:44AM +0200, Michal Privoznik wrote:
Now that we have strong PRNG generator implemented in virRandomBytes() let's use that instead of gnulib's random_r.
Problem with the latter is in way we seed it: current UNIX time and libvirtd's PID are not that random as one might think. Imagine two hosts booting at the same time. There's a fair chance that those hosts spawn libvirtds at the same time and with the same PID. This will result in both daemons generating the same sequence of say MAC addresses [1].
1: https://www.redhat.com/archives/libvirt-users/2018-May/msg00097.html
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virrandom.c | 63 ++-------------------------------------------------- 1 file changed, 2 insertions(+), 61 deletions(-)
ACK to patches 1-7. But for this one I'm "concerned" about few things.
First of all, just so I don't forget it, random_r can be removed from bootstrap.conf after this patch, right?
Yes, I was meaning to make that change but then I forgot.
Before this patch, and without gnutls, we wouldn't deplete the entropy of the kernel, (even though we're just using /dev/urandom and not /dev/random), but now we'd read everything from /dev/urandom.
Unless we are built with gnutls. But I don't see much problem with that.
Yeah, it's not that big of a deal, just an extra point for the next thing I mentioned below.
Last but not least, if we completely drop the non-gnutls variants of everything, wouldn't everything be easier anyway? Like the worrying about entropy pool in previous point?
Sure. But requiring gnutls (like I'm suggesting in the cover letter) is orthogonal to these patches IMO.
My point was that the fixes might be could be cleaner and shorter, but that "not that big of a deal" above would be fixed. It also makes it kind of relevant.
Since /dev/urandom cannot be really exhausted in newer Linux kernels (not sure for FreeBSD and others), I don't think that's a problem. We should ensure, however, that it is seeded properly. It might not be when it's early during the boot for Linux (although systemd and others seed it explicitly early enough), that's what getrandom() is for as it ensures the seeding was done properly. But that's Linux-specific. FreeBSD will apparently not give you any data until it's seeded properly.
/dev/urandom does not even exist on non-Linux hosts AFAIK, so this has the effect of breaking libvirt on non-Linux hosts when gnutls is disabled. IMHO we should used be using getrandom() as the first fallback, and only then try /dev/urandom or /dev/random if the former doesn't exist Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Fri, Jun 01, 2018 at 11:25:35AM +0100, Daniel P. Berrangé wrote:
On Wed, May 30, 2018 at 03:43:31PM +0200, Martin Kletzander wrote:
On Wed, May 30, 2018 at 02:16:08PM +0200, Michal Privoznik wrote:
On 05/29/2018 03:44 PM, Martin Kletzander wrote:
On Tue, May 29, 2018 at 10:24:44AM +0200, Michal Privoznik wrote:
Now that we have strong PRNG generator implemented in virRandomBytes() let's use that instead of gnulib's random_r.
Problem with the latter is in way we seed it: current UNIX time and libvirtd's PID are not that random as one might think. Imagine two hosts booting at the same time. There's a fair chance that those hosts spawn libvirtds at the same time and with the same PID. This will result in both daemons generating the same sequence of say MAC addresses [1].
1: https://www.redhat.com/archives/libvirt-users/2018-May/msg00097.html
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virrandom.c | 63 ++-------------------------------------------------- 1 file changed, 2 insertions(+), 61 deletions(-)
ACK to patches 1-7. But for this one I'm "concerned" about few things.
First of all, just so I don't forget it, random_r can be removed from bootstrap.conf after this patch, right?
Yes, I was meaning to make that change but then I forgot.
Before this patch, and without gnutls, we wouldn't deplete the entropy of the kernel, (even though we're just using /dev/urandom and not /dev/random), but now we'd read everything from /dev/urandom.
Unless we are built with gnutls. But I don't see much problem with that.
Yeah, it's not that big of a deal, just an extra point for the next thing I mentioned below.
Last but not least, if we completely drop the non-gnutls variants of everything, wouldn't everything be easier anyway? Like the worrying about entropy pool in previous point?
Sure. But requiring gnutls (like I'm suggesting in the cover letter) is orthogonal to these patches IMO.
My point was that the fixes might be could be cleaner and shorter, but that "not that big of a deal" above would be fixed. It also makes it kind of relevant.
Since /dev/urandom cannot be really exhausted in newer Linux kernels (not sure for FreeBSD and others), I don't think that's a problem. We should ensure, however, that it is seeded properly. It might not be when it's early during the boot for Linux (although systemd and others seed it explicitly early enough), that's what getrandom() is for as it ensures the seeding was done properly. But that's Linux-specific. FreeBSD will apparently not give you any data until it's seeded properly.
/dev/urandom does not even exist on non-Linux hosts AFAIK, so this has the effect of breaking libvirt on non-Linux hosts when gnutls is disabled.
On FreeBSD it is a symlink to /dev/random (they both behave like getrandom() on Linux) and I guess on MacOS it is the same. Random search showed it exists there.
IMHO we should used be using getrandom() as the first fallback, and only then try /dev/urandom or /dev/random if the former doesn't exist
Sure, we can do that. It's just some crust (more configure checks and conditional compilations, etc.) in case libvirt would run so early that /dev/urandom was not properly seeded. Is there a modern distribution that doesn't seed /dev/urandom during boot before starting any services?
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Fri, Jun 01, 2018 at 02:10:56PM +0200, Martin Kletzander wrote:
On Fri, Jun 01, 2018 at 11:25:35AM +0100, Daniel P. Berrangé wrote:
On Wed, May 30, 2018 at 03:43:31PM +0200, Martin Kletzander wrote:
On Wed, May 30, 2018 at 02:16:08PM +0200, Michal Privoznik wrote:
On 05/29/2018 03:44 PM, Martin Kletzander wrote:
On Tue, May 29, 2018 at 10:24:44AM +0200, Michal Privoznik wrote:
Now that we have strong PRNG generator implemented in virRandomBytes() let's use that instead of gnulib's random_r.
Problem with the latter is in way we seed it: current UNIX time and libvirtd's PID are not that random as one might think. Imagine two hosts booting at the same time. There's a fair chance that those hosts spawn libvirtds at the same time and with the same PID. This will result in both daemons generating the same sequence of say MAC addresses [1].
1: https://www.redhat.com/archives/libvirt-users/2018-May/msg00097.html
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virrandom.c | 63 ++-------------------------------------------------- 1 file changed, 2 insertions(+), 61 deletions(-)
ACK to patches 1-7. But for this one I'm "concerned" about few things.
First of all, just so I don't forget it, random_r can be removed from bootstrap.conf after this patch, right?
Yes, I was meaning to make that change but then I forgot.
Before this patch, and without gnutls, we wouldn't deplete the entropy of the kernel, (even though we're just using /dev/urandom and not /dev/random), but now we'd read everything from /dev/urandom.
Unless we are built with gnutls. But I don't see much problem with that.
Yeah, it's not that big of a deal, just an extra point for the next thing I mentioned below.
Last but not least, if we completely drop the non-gnutls variants of everything, wouldn't everything be easier anyway? Like the worrying about entropy pool in previous point?
Sure. But requiring gnutls (like I'm suggesting in the cover letter) is orthogonal to these patches IMO.
My point was that the fixes might be could be cleaner and shorter, but that "not that big of a deal" above would be fixed. It also makes it kind of relevant.
Since /dev/urandom cannot be really exhausted in newer Linux kernels (not sure for FreeBSD and others), I don't think that's a problem. We should ensure, however, that it is seeded properly. It might not be when it's early during the boot for Linux (although systemd and others seed it explicitly early enough), that's what getrandom() is for as it ensures the seeding was done properly. But that's Linux-specific. FreeBSD will apparently not give you any data until it's seeded properly.
/dev/urandom does not even exist on non-Linux hosts AFAIK, so this has the effect of breaking libvirt on non-Linux hosts when gnutls is disabled.
On FreeBSD it is a symlink to /dev/random (they both behave like getrandom() on Linux) and I guess on MacOS it is the same. Random search showed it exists there.
Ah that's good to know - I guess they added the symlink due to software having assumptions from linux world :-) Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 05/29/2018 03:24 AM, Michal Privoznik wrote:
Now that we have strong PRNG generator implemented in virRandomBytes() let's use that instead of gnulib's random_r.
Problem with the latter is in way we seed it: current UNIX time and libvirtd's PID are not that random as one might think. Imagine two hosts booting at the same time. There's a fair chance that those hosts spawn libvirtds at the same time and with the same PID. This will result in both daemons generating the same sequence of say MAC addresses [1].
1: https://www.redhat.com/archives/libvirt-users/2018-May/msg00097.html
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virrandom.c | 63 ++-------------------------------------------------- 1 file changed, 2 insertions(+), 61 deletions(-)
-static int -virRandomOnceInit(void) -{ - unsigned int seed = time(NULL) ^ getpid(); - -#if 0 - /* Normally we want a decent seed. But if reproducible debugging - * of a fixed pseudo-random sequence is ever required, uncomment - * this block to let an environment variable force the seed. */ - const char *debug = virGetEnvBlockSUID("VIR_DEBUG_RANDOM_SEED"); - - if (debug && virStrToLong_ui(debug, NULL, 0, &seed) < 0) - return -1;
Are we sure we don't need the ability to quickly compile in a deterministic replacement for debug in the future? I suppose there's always git history, but this particular #if 0 was left in place for a reason, where completely removing it makes it harder to realize that such debugging is even possible. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On 05/29/2018 10:32 PM, Eric Blake wrote:
On 05/29/2018 03:24 AM, Michal Privoznik wrote:
Now that we have strong PRNG generator implemented in virRandomBytes() let's use that instead of gnulib's random_r.
Problem with the latter is in way we seed it: current UNIX time and libvirtd's PID are not that random as one might think. Imagine two hosts booting at the same time. There's a fair chance that those hosts spawn libvirtds at the same time and with the same PID. This will result in both daemons generating the same sequence of say MAC addresses [1].
1: https://www.redhat.com/archives/libvirt-users/2018-May/msg00097.html
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virrandom.c | 63 ++-------------------------------------------------- 1 file changed, 2 insertions(+), 61 deletions(-)
-static int -virRandomOnceInit(void) -{ - unsigned int seed = time(NULL) ^ getpid(); - -#if 0 - /* Normally we want a decent seed. But if reproducible debugging - * of a fixed pseudo-random sequence is ever required, uncomment - * this block to let an environment variable force the seed. */ - const char *debug = virGetEnvBlockSUID("VIR_DEBUG_RANDOM_SEED"); - - if (debug && virStrToLong_ui(debug, NULL, 0, &seed) < 0) - return -1;
Are we sure we don't need the ability to quickly compile in a deterministic replacement for debug in the future? I suppose there's always git history, but this particular #if 0 was left in place for a reason, where completely removing it makes it harder to realize that such debugging is even possible.
Well, what we can now do is to cook a mock library that would implement gnutls_rnd() to return some deterministic number without any need to recompile libvirt at all. Therefore I don't think we should leave #if 0 in. And frankly, until I looked into this file I did not even know we have such option. Michal
If virRandomBytes() fails there is no point calling virRandomBits() because it uses virRandomBytes() internally again. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/viruuid.c | 25 +++---------------------- 1 file changed, 3 insertions(+), 22 deletions(-) diff --git a/src/util/viruuid.c b/src/util/viruuid.c index 61877aeba4..f588a62ec6 100644 --- a/src/util/viruuid.c +++ b/src/util/viruuid.c @@ -48,18 +48,6 @@ VIR_LOG_INIT("util.uuid"); static unsigned char host_uuid[VIR_UUID_BUFLEN]; -static int -virUUIDGeneratePseudoRandomBytes(unsigned char *buf, - int buflen) -{ - while (buflen > 0) { - *buf++ = virRandomBits(8); - buflen--; - } - - return 0; -} - /** * virUUIDGenerate: * @uuid: array of VIR_UUID_BUFLEN bytes to store the new UUID @@ -71,18 +59,11 @@ virUUIDGeneratePseudoRandomBytes(unsigned char *buf, int virUUIDGenerate(unsigned char *uuid) { - int err; - if (uuid == NULL) return -1; - if ((err = virRandomBytes(uuid, VIR_UUID_BUFLEN)) < 0) { - char ebuf[1024]; - VIR_WARN("Falling back to pseudorandom UUID," - " failed to generate random bytes: %s", - virStrerror(-err, ebuf, sizeof(ebuf))); - err = virUUIDGeneratePseudoRandomBytes(uuid, VIR_UUID_BUFLEN); - } + if (virRandomBytes(uuid, VIR_UUID_BUFLEN) < 0) + return -1; /* * Make UUID RFC 4122 compliant. Following form will be used: @@ -103,7 +84,7 @@ virUUIDGenerate(unsigned char *uuid) uuid[6] = (uuid[6] & 0x0F) | (4 << 4); uuid[8] = (uuid[8] & 0x3F) | (2 << 6); - return err; + return 0; } /** -- 2.16.1
Now that virCryptoGenerateRandom() is plain wrapper over virRandomBytes() we can drop it in favour of the latter. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 - src/qemu/qemu_domain.c | 5 +++-- src/util/vircrypto.c | 18 ------------------ src/util/vircrypto.h | 3 --- tests/qemuxml2argvmock.c | 7 ------- 5 files changed, 3 insertions(+), 31 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 8d381ee11b..18c0c3e954 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1649,7 +1649,6 @@ virConfWriteMem; # util/vircrypto.h virCryptoEncryptData; -virCryptoGenerateRandom; virCryptoHashBuf; virCryptoHashString; virCryptoHaveCipher; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 2d13a03344..e49398432f 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -51,6 +51,7 @@ #include "viratomic.h" #include "virprocess.h" #include "vircrypto.h" +#include "virrandom.h" #include "virsystemd.h" #include "secret_util.h" #include "logging/log_manager.h" @@ -934,7 +935,7 @@ qemuDomainMasterKeyCreate(virDomainObjPtr vm) return -1; priv->masterKeyLen = QEMU_DOMAIN_MASTER_KEY_LEN; - if (virCryptoGenerateRandom(priv->masterKey, QEMU_DOMAIN_MASTER_KEY_LEN) < 0) + if (virRandomBytes(priv->masterKey, QEMU_DOMAIN_MASTER_KEY_LEN) < 0) return -1; return 0; @@ -1219,7 +1220,7 @@ qemuDomainSecretAESSetup(qemuDomainObjPrivatePtr priv, goto cleanup; /* Create a random initialization vector */ - if (virCryptoGenerateRandom(raw_iv, ivlen) < 0) + if (virRandomBytes(raw_iv, ivlen) < 0) goto cleanup; /* Encode the IV and save that since qemu will need it */ diff --git a/src/util/vircrypto.c b/src/util/vircrypto.c index 3f3ba0267a..d734ce6ad7 100644 --- a/src/util/vircrypto.c +++ b/src/util/vircrypto.c @@ -314,21 +314,3 @@ virCryptoEncryptData(virCryptoCipher algorithm, return -1; } #endif - -/* virCryptoGenerateRandom: - * @buf: Pointer to location to store bytes - * @buflen: Number of bytes to store - * - * Generate a random stream of @buflen length and store it into @buf. - * - * Since the gnutls_rnd could be missing, provide an alternate less - * secure mechanism to at least have something. - * - * Returns 0 on success or -1 on failure (with error reported) - */ -int -virCryptoGenerateRandom(unsigned char *buf, - size_t buflen) -{ - return virRandomBytes(buf, buflen); -} diff --git a/src/util/vircrypto.h b/src/util/vircrypto.h index 649ceff1a1..e3c70d7d9a 100644 --- a/src/util/vircrypto.h +++ b/src/util/vircrypto.h @@ -65,7 +65,4 @@ int virCryptoEncryptData(virCryptoCipher algorithm, ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(6) ATTRIBUTE_NONNULL(8) ATTRIBUTE_NONNULL(9) ATTRIBUTE_RETURN_CHECK; -int virCryptoGenerateRandom(unsigned char *buf, - size_t buflen) ATTRIBUTE_NOINLINE; - #endif /* __VIR_CRYPTO_H__ */ diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c index 44b6504de9..a4de7f0c46 100644 --- a/tests/qemuxml2argvmock.c +++ b/tests/qemuxml2argvmock.c @@ -190,13 +190,6 @@ virCommandPassFD(virCommandPtr cmd ATTRIBUTE_UNUSED, /* nada */ } -int -virCryptoGenerateRandom(unsigned char *buf, - size_t buflen) -{ - return virRandomBytes(buf, buflen); -} - int virNetDevOpenvswitchGetVhostuserIfname(const char *path ATTRIBUTE_UNUSED, char **ifname) -- 2.16.1
On Tue, May 29, 2018 at 10:24:46AM +0200, Michal Privoznik wrote:
Now that virCryptoGenerateRandom() is plain wrapper over virRandomBytes() we can drop it in favour of the latter.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 - src/qemu/qemu_domain.c | 5 +++-- src/util/vircrypto.c | 18 ------------------ src/util/vircrypto.h | 3 --- tests/qemuxml2argvmock.c | 7 ------- 5 files changed, 3 insertions(+), 31 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 8d381ee11b..18c0c3e954 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1649,7 +1649,6 @@ virConfWriteMem;
# util/vircrypto.h virCryptoEncryptData; -virCryptoGenerateRandom; virCryptoHashBuf; virCryptoHashString; virCryptoHaveCipher; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 2d13a03344..e49398432f 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -51,6 +51,7 @@ #include "viratomic.h" #include "virprocess.h" #include "vircrypto.h" +#include "virrandom.h"
You can also remove this include from vircrypto.c ACK with that changed and also ACK to patch 9.
On 05/29/2018 03:24 AM, Michal Privoznik wrote:
This is inspired by bug reported here [1]. Even though Eric suggested calling this Linux syscall when building without gnutls [2] I've decided to not implement it. Firstly, we build with gnuls everywhere (even Windows), secondly I see no appealing reason to special case Linux - /dev/urandom is good for both Linux and FreeBSD.
Once these are merged I'm probably going to send patch set that makes gnutls mandatory. I'm tired of all those WITH_GNUTLS if-defs (esp. in function arguments). But that is orthogonal to what I'm solving here.
Also, I'm not quite sure this is a release material, so I'm fine with merging this after the release.
1: https://www.redhat.com/archives/libvirt-users/2018-May/msg00097.html 2: https://www.redhat.com/archives/libvirt-users/2018-May/msg00100.html
I'm not sure if we're getting a CVE assigned for this (if Red Hat security gets back to me on that question, and says a CVE is warranted, then maybe it still is a candidate for this release). But if a CVE is assigned, the fact that this issue has been public since 2014 means that one more broken release added to years of neglect regarding the issue won't hurt much. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
participants (4)
-
Daniel P. Berrangé -
Eric Blake -
Martin Kletzander -
Michal Privoznik