[libvirt] [PATCH 0/4] Couple of random and crypto adjustments
Patches 1&2 alter the virRandomBytes to accomodate the lone holdout code that was using /dev/urandom directly to add a range of bytes that are acceptible returns for the caller. Patches 3&4 modify the virCryptoEncryptData algorithm to "A" (hopefully) build right in a non gnutls environment and "B" make it so there future adjustments won't be doing cut, copy, & paste based on the key size and whether the iv is required. If AES192CBC were added the first two parameters to the DO_CRYPT macro would be 24, 16, while AES192CBC would have 16, 16. John Ferlan (4): util: Add range parameters to virRandomBytes storage: Use virRandomBytes for virStorageGenerateQcowPassphrase util: Alter virCryptoEncryptData for non GNUTLS builds util: Adjust virCryptoEncryptData code to use macros src/storage/storage_backend.c | 10 +++-- src/util/vircrypto.c | 88 +++++++++++++++++++++++++---------------- src/util/virrandom.c | 13 ++++-- src/util/virrandom.h | 3 +- src/util/virstorageencryption.c | 42 +++++++------------- src/util/virstorageencryption.h | 4 +- src/util/viruuid.c | 2 +- tests/qemuxml2argvmock.c | 2 +- tests/vircryptotest.c | 4 +- tests/virrandommock.c | 10 +++-- tests/virrandomtest.c | 32 +++++++++++---- 11 files changed, 124 insertions(+), 86 deletions(-) -- 2.5.5
Add a minval and maxval range for acceptible values from /dev/urandom Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/vircrypto.c | 2 +- src/util/virrandom.c | 13 ++++++++++--- src/util/virrandom.h | 3 ++- src/util/viruuid.c | 2 +- tests/qemuxml2argvmock.c | 2 +- tests/vircryptotest.c | 4 ++-- tests/virrandommock.c | 10 +++++++--- tests/virrandomtest.c | 32 ++++++++++++++++++++++++-------- 8 files changed, 48 insertions(+), 20 deletions(-) diff --git a/src/util/vircrypto.c b/src/util/vircrypto.c index 4f288f0..4125230 100644 --- a/src/util/vircrypto.c +++ b/src/util/vircrypto.c @@ -301,7 +301,7 @@ virCryptoGenerateRandom(size_t nbytes) /* If we don't have gnutls_rnd(), we will generate a less cryptographically * strong master buf from /dev/urandom. */ - if ((ret = virRandomBytes(buf, nbytes)) < 0) { + if ((ret = virRandomBytes(buf, nbytes, 0x00, 0xff)) < 0) { virReportSystemError(ret, "%s", _("failed to generate byte stream")); VIR_FREE(buf); return NULL; diff --git a/src/util/virrandom.c b/src/util/virrandom.c index 62a0e31..256819a 100644 --- a/src/util/virrandom.c +++ b/src/util/virrandom.c @@ -164,13 +164,17 @@ uint32_t virRandomInt(uint32_t max) * virRandomBytes * @buf: Pointer to location to store bytes * @buflen: Number of bytes to store + * @minval: Minimum value acceptable + * @maxval: Minimum value acceptable * * Generate a stream of random bytes from /dev/urandom * into @buf of size @buflen */ int virRandomBytes(unsigned char *buf, - size_t buflen) + size_t buflen, + uint8_t minval, + uint8_t maxval) { int fd; @@ -187,8 +191,11 @@ virRandomBytes(unsigned char *buf, return n < 0 ? errno : ENODATA; } - buf += n; - buflen -= n; + /* Compare to acceptable range */ + if (*buf >= minval && *buf <= maxval) { + buf += n; + buflen -= n; + } } VIR_FORCE_CLOSE(fd); diff --git a/src/util/virrandom.h b/src/util/virrandom.h index f457d2d..6b3fb0f 100644 --- a/src/util/virrandom.h +++ b/src/util/virrandom.h @@ -27,7 +27,8 @@ uint64_t virRandomBits(int nbits); double virRandom(void); uint32_t virRandomInt(uint32_t max); -int virRandomBytes(unsigned char *buf, size_t buflen) +int virRandomBytes(unsigned char *buf, size_t buflen, + uint8_t minval, uint8_t maxval) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; int virRandomGenerateWWN(char **wwn, const char *virt_type); diff --git a/src/util/viruuid.c b/src/util/viruuid.c index 3cbaae0..f494adf 100644 --- a/src/util/viruuid.c +++ b/src/util/viruuid.c @@ -76,7 +76,7 @@ virUUIDGenerate(unsigned char *uuid) if (uuid == NULL) return -1; - if ((err = virRandomBytes(uuid, VIR_UUID_BUFLEN))) { + if ((err = virRandomBytes(uuid, VIR_UUID_BUFLEN, 0x00, 0xff))) { char ebuf[1024]; VIR_WARN("Falling back to pseudorandom UUID," " failed to generate random bytes: %s", diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c index c6a1f98..76511af 100644 --- a/tests/qemuxml2argvmock.c +++ b/tests/qemuxml2argvmock.c @@ -157,7 +157,7 @@ virCryptoGenerateRandom(size_t nbytes) if (VIR_ALLOC_N(buf, nbytes) < 0) return NULL; - ignore_value(virRandomBytes(buf, nbytes)); + ignore_value(virRandomBytes(buf, nbytes, 0x00, 0xff)); return buf; } diff --git a/tests/vircryptotest.c b/tests/vircryptotest.c index 72265d9..0dd2cf4 100644 --- a/tests/vircryptotest.c +++ b/tests/vircryptotest.c @@ -87,8 +87,8 @@ testCryptoEncrypt(const void *opaque) VIR_ALLOC_N(iv, ivlen) < 0) goto cleanup; - if (virRandomBytes(enckey, enckeylen) < 0 || - virRandomBytes(iv, ivlen) < 0) + if (virRandomBytes(enckey, enckeylen, 0x00, 0xff) < 0 || + virRandomBytes(iv, ivlen, 0x00, 0xff) < 0) goto cleanup; if (virCryptoEncryptData(data->algorithm, enckey, enckeylen, iv, ivlen, diff --git a/tests/virrandommock.c b/tests/virrandommock.c index 6df5e20..d988bab 100644 --- a/tests/virrandommock.c +++ b/tests/virrandommock.c @@ -28,12 +28,16 @@ int virRandomBytes(unsigned char *buf, - size_t buflen) + size_t buflen, + uint8_t minval, + uint8_t maxval) { size_t i; - for (i = 0; i < buflen; i++) - buf[i] = i; + for (i = 0; i < buflen; i++) { + if (i >= minval && i <= maxval) + buf[i] = i; + } return 0; } diff --git a/tests/virrandomtest.c b/tests/virrandomtest.c index 367bdc7..be148d2 100644 --- a/tests/virrandomtest.c +++ b/tests/virrandomtest.c @@ -29,27 +29,31 @@ # define VIR_FROM_THIS VIR_FROM_NONE +struct testRandomData { + uint8_t minval; + uint8_t maxval; +}; + static int -testRandomBytes(const void *unused ATTRIBUTE_UNUSED) +testRandomBytes(const void *opaque) { int ret = -1; size_t i; uint8_t *data; size_t datalen = 32; + const struct testRandomData *range = opaque; if (VIR_ALLOC_N(data, datalen) < 0) return -1; - if (virRandomBytes(data, datalen) < 0) { + if (virRandomBytes(data, datalen, range->minval, range->maxval) < 0) { fprintf(stderr, "Failed to generate random bytes"); goto cleanup; } - for (i = 0; i < datalen; i++) { + for (i = range->minval; i < datalen; i++) { if (data[i] != i) { - fprintf(stderr, - "virRandomBytes data[%zu]='%x' not in sequence\n", - i, data[i]); + fprintf(stderr, "data[%zu]='%x' not in sequence\n", i, data[i]); goto cleanup; } } @@ -67,8 +71,20 @@ mymain(void) { int ret = 0; - if (virtTestRun("RandomBytes", testRandomBytes, NULL) < 0) - ret = -1; +# define DO_TEST(name, min, max) \ + do { \ + struct testRandomData range = { \ + .minval = min, \ + .maxval = max, \ + }; \ + if (virtTestRun("Random " name, testRandomBytes, &range) < 0) \ + ret = -1; \ + } while (0); + + DO_TEST("AllBytes", 0x00, 0xff); + DO_TEST("Printable", 0x20, 0x7f); + +# undef DO_TEST return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.5.5
Use the common API Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend.c | 10 +++++++--- src/util/virstorageencryption.c | 42 +++++++++++++++-------------------------- src/util/virstorageencryption.h | 4 ++-- 3 files changed, 24 insertions(+), 32 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 3a23cd7..637b59a 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -597,7 +597,7 @@ virStorageGenerateQcowEncryption(virConnectPtr conn, virStorageEncryptionSecretPtr enc_secret = NULL; virSecretPtr secret = NULL; char *xml; - unsigned char value[VIR_STORAGE_QCOW_PASSPHRASE_SIZE]; + unsigned char *value = NULL; int ret = -1; if (conn->secretDriver == NULL || @@ -641,10 +641,13 @@ virStorageGenerateQcowEncryption(virConnectPtr conn, } VIR_FREE(xml); - if (virStorageGenerateQcowPassphrase(value) < 0) + if (!(value = + virStorageGenerateQcowPassphrase(VIR_STORAGE_QCOW_PASSPHRASE_SIZE))) goto cleanup; - if (conn->secretDriver->secretSetValue(secret, value, sizeof(value), 0) < 0) + if (conn->secretDriver->secretSetValue(secret, value, + VIR_STORAGE_QCOW_PASSPHRASE_SIZE, + 0) < 0) goto cleanup; enc_secret->type = VIR_STORAGE_ENCRYPTION_SECRET_TYPE_PASSPHRASE; @@ -666,6 +669,7 @@ virStorageGenerateQcowEncryption(virConnectPtr conn, virBufferFreeAndReset(&buf); virSecretDefFree(def); VIR_FREE(enc_secret); + VIR_FREE(value); return ret; } diff --git a/src/util/virstorageencryption.c b/src/util/virstorageencryption.c index ec4a8cb..52d5c45 100644 --- a/src/util/virstorageencryption.c +++ b/src/util/virstorageencryption.c @@ -1,7 +1,7 @@ /* * virstorageencryption.c: volume encryption information * - * Copyright (C) 2009-2014 Red Hat, Inc. + * Copyright (C) 2009-2014, 2016 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -34,6 +34,7 @@ #include "virerror.h" #include "viruuid.h" #include "virfile.h" +#include "virrandom.h" #define VIR_FROM_THIS VIR_FROM_STORAGE @@ -288,36 +289,23 @@ virStorageEncryptionFormat(virBufferPtr buf, return 0; } -int -virStorageGenerateQcowPassphrase(unsigned char *dest) +unsigned char * +virStorageGenerateQcowPassphrase(size_t nbytes) { - int fd; - size_t i; + int ret; + uint8_t *value; + + if (VIR_ALLOC_N(value, nbytes) < 0) + return NULL; /* A qcow passphrase is up to 16 bytes, with any data following a NUL ignored. Prohibit control and non-ASCII characters to avoid possible unpleasant surprises with the qemu monitor input mechanism. */ - fd = open("/dev/urandom", O_RDONLY); - if (fd < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Cannot open /dev/urandom")); - return -1; - } - i = 0; - while (i < VIR_STORAGE_QCOW_PASSPHRASE_SIZE) { - ssize_t r; - - while ((r = read(fd, dest + i, 1)) == -1 && errno == EINTR) - ; - if (r <= 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Cannot read from /dev/urandom")); - VIR_FORCE_CLOSE(fd); - return -1; - } - if (dest[i] >= 0x20 && dest[i] <= 0x7E) - i++; /* Got an acceptable character */ + if ((ret = virRandomBytes(value, nbytes, 0x20, 0x7E)) < 0) { + virReportSystemError(ret, "%s", _("failed to generate passphrase")); + VIR_FREE(value); + return NULL; } - VIR_FORCE_CLOSE(fd); - return 0; + + return value; } diff --git a/src/util/virstorageencryption.h b/src/util/virstorageencryption.h index 04641b1..bdfaa15 100644 --- a/src/util/virstorageencryption.h +++ b/src/util/virstorageencryption.h @@ -1,7 +1,7 @@ /* * virstorageencryption.h: volume encryption information * - * Copyright (C) 2009-2011, 2014 Red Hat, Inc. + * Copyright (C) 2009-2011, 2014, 2016 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -76,6 +76,6 @@ enum { VIR_STORAGE_QCOW_PASSPHRASE_SIZE = 16 }; -int virStorageGenerateQcowPassphrase(unsigned char *dest); +unsigned char *virStorageGenerateQcowPassphrase(size_t nbytes); #endif /* __VIR_STORAGE_ENCRYPTION_H__ */ -- 2.5.5
Rather than intermixing the ATTRIBUTE_UNUSED - use HAVE_GNUTLS_CIPHER_ENCRYPT for the whole function instead. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/vircrypto.c | 36 +++++++++++++++++++++++++----------- 1 file changed, 25 insertions(+), 11 deletions(-) diff --git a/src/util/vircrypto.c b/src/util/vircrypto.c index 4125230..27a3d1d 100644 --- a/src/util/vircrypto.c +++ b/src/util/vircrypto.c @@ -200,7 +200,6 @@ virCryptoEncryptDataAESgnutls(gnutls_cipher_algorithm_t gnutls_enc_alg, memset(&iv_buf, 0, sizeof(gnutls_datum_t)); return -1; } -#endif /* virCryptoEncryptData: @@ -221,14 +220,14 @@ virCryptoEncryptDataAESgnutls(gnutls_cipher_algorithm_t gnutls_enc_alg, */ int virCryptoEncryptData(virCryptoCipher algorithm, - uint8_t *enckey ATTRIBUTE_UNUSED, + uint8_t *enckey, size_t enckeylen, - uint8_t *iv ATTRIBUTE_UNUSED, + uint8_t *iv, size_t ivlen, - uint8_t *data ATTRIBUTE_UNUSED, - size_t datalen ATTRIBUTE_UNUSED, - uint8_t **ciphertext ATTRIBUTE_UNUSED, - size_t *ciphertextlen ATTRIBUTE_UNUSED) + uint8_t *data, + size_t datalen, + uint8_t **ciphertext, + size_t *ciphertextlen) { switch (algorithm) { case VIR_CRYPTO_CIPHER_AES256CBC: @@ -246,7 +245,6 @@ virCryptoEncryptData(virCryptoCipher algorithm, return -1; } -#ifdef HAVE_GNUTLS_CIPHER_ENCRYPT /* * Encrypt the data buffer using an encryption key and * initialization vector via the gnutls_cipher_encrypt API @@ -256,9 +254,6 @@ virCryptoEncryptData(virCryptoCipher algorithm, enckey, enckeylen, iv, ivlen, data, datalen, ciphertext, ciphertextlen); -#else - break; -#endif case VIR_CRYPTO_CIPHER_NONE: case VIR_CRYPTO_CIPHER_LAST: @@ -270,6 +265,25 @@ virCryptoEncryptData(virCryptoCipher algorithm, return -1; } +#else + +int +virCryptoEncryptData(virCryptoCipher algorithm, + uint8_t *enckey ATTRIBUTE_UNUSED, + size_t enckeylen ATTRIBUTE_UNUSED, + uint8_t *iv ATTRIBUTE_UNUSED, + size_t ivlen ATTRIBUTE_UNUSED, + uint8_t *data ATTRIBUTE_UNUSED, + size_t datalen ATTRIBUTE_UNUSED, + uint8_t **ciphertext ATTRIBUTE_UNUSED, + size_t *ciphertextlen ATTRIBUTE_UNUSED) +{ + virReportError(VIR_ERR_INVALID_ARG, + _("algorithm=%d is not supported"), algorithm); + return -1; +} +#endif + /* virCryptoGenerateRandom: * @nbytes: Size in bytes of random byte stream to generate * -- 2.5.5
Will make it easier to add new key lengths Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/vircrypto.c | 50 +++++++++++++++++++++++++++----------------------- 1 file changed, 27 insertions(+), 23 deletions(-) diff --git a/src/util/vircrypto.c b/src/util/vircrypto.c index 27a3d1d..f50ac6a 100644 --- a/src/util/vircrypto.c +++ b/src/util/vircrypto.c @@ -229,36 +229,40 @@ virCryptoEncryptData(virCryptoCipher algorithm, uint8_t **ciphertext, size_t *ciphertextlen) { + /* + * Encrypt the data buffer using an encryption key and + * initialization vector via the gnutls_cipher_encrypt API + * for the specific cipher algorithm. + */ +# define DO_CRYPT(ekl, ivl, alg, nam) \ + do { \ + if (enckeylen != ekl) { \ + virReportError(VIR_ERR_INVALID_ARG, \ + _("'%s' encryption invalid keylen=%d"), \ + nam, ekl); \ + return -1; \ + } \ + if (ivlen != ivl) { \ + virReportError(VIR_ERR_INVALID_ARG, \ + _("'%s' initialization vector invalid len=%d"), \ + nam, ivl); \ + return -1; \ + } \ + return virCryptoEncryptDataAESgnutls(alg, enckey, enckeylen, \ + iv, ivlen, data, datalen, \ + ciphertext, ciphertextlen); \ + } while (0); + switch (algorithm) { case VIR_CRYPTO_CIPHER_AES256CBC: - if (enckeylen != 32) { - virReportError(VIR_ERR_INVALID_ARG, - _("AES256CBC encryption invalid keylen=%zu"), - enckeylen); - return -1; - } - - if (ivlen != 16) { - virReportError(VIR_ERR_INVALID_ARG, - _("AES256CBC initialization vector invalid len=%zu"), - ivlen); - return -1; - } - - /* - * Encrypt the data buffer using an encryption key and - * initialization vector via the gnutls_cipher_encrypt API - * for GNUTLS_CIPHER_AES_256_CBC. - */ - return virCryptoEncryptDataAESgnutls(GNUTLS_CIPHER_AES_256_CBC, - enckey, enckeylen, iv, ivlen, - data, datalen, - ciphertext, ciphertextlen); + DO_CRYPT(32, 16, GNUTLS_CIPHER_AES_256_CBC, "AES256CBC"); case VIR_CRYPTO_CIPHER_NONE: case VIR_CRYPTO_CIPHER_LAST: break; } +# undef DO_CRYPT + virReportError(VIR_ERR_INVALID_ARG, _("algorithm=%d is not supported"), algorithm); -- 2.5.5
participants (1)
-
John Ferlan