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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.