[libvirt] [PATCH 04/10] virCryptoGenerateRandom: Don't allocate return buffer

[Michal Privoznik]

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 at 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