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@xxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
