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