Re: [libvirt] [PATCH 04/10] virCryptoGenerateRandom: Don't allocate return buffer
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(a)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