Re: [libvirt] [PATCH v5 2/6] util: Introduce encryption APIs

On Thu, May 19, 2016 at 16:29:01 -0400, John Ferlan wrote: > Introduce virCryptoHaveCipher and virCryptoEncryptData to handle > performing encryption. > > virCryptoHaveCipher: > Boolean function to determine whether the requested cipher algorithm > is available. It's expected this API will be called prior to > virCryptoEncryptdata. It will return true/false. > > virCryptoEncryptData: > Based on the requested cipher type, call the specific encryption > API to encrypt the data. > > Currently the only algorithm support is the AES 256 CBC encryption. > > Adjust tests for the API's > > Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; > --- > configure.ac | 1 + > src/libvirt_private.syms | 2 + > src/util/vircrypto.c | 192 ++++++++++++++++++++++++++++++++++++++++++++++- > src/util/vircrypto.h | 20 ++++- > tests/vircryptotest.c | 100 +++++++++++++++++++++++- > 5 files changed, 312 insertions(+), 3 deletions(-) [...] > diff --git a/src/util/vircrypto.c b/src/util/vircrypto.c > index 39a479a..b8f5554 100644 > --- a/src/util/vircrypto.c > +++ b/src/util/vircrypto.c [...] > @@ -76,3 +85,184 @@ virCryptoHashString(virCryptoHash hash, > > return 0; > } > + > + > +/* virCryptoHaveCipher: > + * @algorithm: Specific cipher algorithm desired > + * > + * Expected to be called prior to virCryptoEncryptData in order > + * to determine whether the requested encryption option is available, > + * so that "other" alternatives can be taken if the algorithm is > + * not available. > + * > + * Returns true if we can support the encryption. > + */ > +bool > +virCryptoHaveCipher(virCryptoCipher algorithm) > +{ > + switch (algorithm) { > + > + case VIR_CRYPTO_CIPHER_AES256CBC: > +#ifdef HAVE_GNUTLS_CIPHER_ENCRYPT > + return true; > +#else > + return false; > +#endif > + > + case VIR_CRYPTO_CIPHER_NONE: > + case VIR_CRYPTO_CIPHER_LAST: > + break; > + }; > + > + return false; > +} > + > + > +#ifdef HAVE_GNUTLS_CIPHER_ENCRYPT > +/* virCryptoEncryptDataAESgntuls: > + * > + * Performs the AES gnutls encryption > + * > + * Same input as virCryptoEncryptData, except the algoritm is replaced > + * by the specific gnutls algorithm. > + * > + * Encrypts the @data buffer using the @enckey and if available the @iv > + * > + * Returns 0 on success with the ciphertext being filled. It is the > + * caller's responsibility to clear and free it. Returns -1 on failure > + * w/ error set. > + */ > +static int > +virCryptoEncryptDataAESgnutls(gnutls_cipher_algorithm_t gnutls_enc_alg, > + uint8_t *enckey, > + size_t enckeylen, > + uint8_t *iv, > + size_t ivlen, > + uint8_t *data, > + size_t datalen, > + uint8_t **ciphertextret, > + size_t *ciphertextlenret) > +{ > + int rc; > + size_t i; > + gnutls_cipher_hd_t handle = NULL; > + gnutls_datum_t enc_key; > + gnutls_datum_t iv_buf; > + uint8_t *ciphertext; > + size_t ciphertextlen; > + > + /* Allocate a padded buffer, copy in the data */ > + ciphertextlen = VIR_ROUND_UP(datalen, 16); > + if (VIR_ALLOC_N(ciphertext, ciphertextlen) < 0) > + return -1; > + memcpy(ciphertext, data, datalen); > + > + /* Fill in the padding of the buffer with the size of the padding > + * which is required for decryption. */ > + for (i = datalen; i < ciphertextlen; i++) > + ciphertext[i] = ciphertextlen - datalen; > + > + /* Initialize the gnutls cipher */ > + enc_key.size = enckeylen; > + enc_key.data = enckey; > + if (iv) { > + iv_buf.size = ivlen; > + iv_buf.data = iv; > + } > + if ((rc = gnutls_cipher_init(&handle, gnutls_enc_alg, > + &enc_key, &iv_buf)) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("failed to initialize cipher: '%s'"), > + gnutls_strerror(rc)); > + goto error; > + } > + > + /* Encrypt the data and free the memory for cipher operations */ > + rc = gnutls_cipher_encrypt(handle, ciphertext, ciphertextlen); > + gnutls_cipher_deinit(handle);

> + if (rc < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("failed to encrypt the data: '%s'"), > + gnutls_strerror(rc)); > + goto error; > + } > + > + *ciphertextret = ciphertext; > + *ciphertextlenret = ciphertextlen; > + return 0; > + > + error: > + VIR_DISPOSE_N(ciphertext, ciphertextlen); > + return -1; In both cases you should clear the stack'd copy of the encryption key. > +} > +#endif > + > + > +/* virCryptoEncryptData: > + * @algorithm: algoritm desired for encryption > + * @enckey: encryption key > + * @enckeylen: encription key length > + * @iv: initialization vector > + * @ivlen: length of initialization vector > + * @data: data to encrypt > + * @datalen: length of data > + * @ciphertext: stream of bytes allocated to store ciphertext > + * @ciphertextlen: size of the stream of bytes > + * > + * If available, attempt and return the requested encryption type > + * using the parameters passed. > + * > + * Returns 0 on success, -1 on failure with error set > + */ > +int > +virCryptoEncryptData(virCryptoCipher algorithm, > + uint8_t *enckey, > + size_t enckeylen, > + uint8_t *iv, > + size_t ivlen, > + uint8_t *data, > + size_t datalen, > + uint8_t **ciphertext, > + size_t *ciphertextlen) > +{ > + 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; > + } > + > +#ifdef HAVE_GNUTLS_CIPHER_ENCRYPT > + /* > + * 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); > +#else > + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", > + _("no encryption algorithm exists")); That is quite a strong message. I'd say that the chosen algorithm is not supported.

> + return -1; > +#endif > + > + case VIR_CRYPTO_CIPHER_NONE: > + case VIR_CRYPTO_CIPHER_LAST: > + break; > + } > + > + virReportError(VIR_ERR_INVALID_ARG, > + _("algorithm=%d is not supported"), algorithm); e.g. as you do here. [...] > --- a/src/util/vircrypto.h > +++ b/src/util/vircrypto.h [...] > @@ -37,4 +45,14 @@ virCryptoHashString(virCryptoHash hash, > ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) > ATTRIBUTE_RETURN_CHECK; > > +bool virCryptoHaveCipher(virCryptoCipher algorithm); > + > +int virCryptoEncryptData(virCryptoCipher algorithm, > + uint8_t *enckey, size_t enckeylen, > + uint8_t *iv, size_t ivlen, > + uint8_t *data, size_t datalen, > + uint8_t **ciphertext, size_t *ciphertextlen) > + ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(6) > + ATTRIBUTE_NONNULL(8) ATTRIBUTE_RETURN_CHECK; ciphertext and ciphertextlen must be nonnull too.

> + > #endif /* __VIR_CRYPTO_H__ */ > diff --git a/tests/vircryptotest.c b/tests/vircryptotest.c > index bfc47db..1d719d9 100644 > --- a/tests/vircryptotest.c > +++ b/tests/vircryptotest.c [...] > @@ -56,10 +58,82 @@ testCryptoHash(const void *opaque) > } > > > +struct testCryptoEncryptData { > + virCryptoCipher algorithm; > + uint8_t *input; > + size_t inputlen; > + uint8_t *ciphertext; > + size_t ciphertextlen; > +}; > + > +static int > +testCryptoEncrypt(const void *opaque) > +{ > + const struct testCryptoEncryptData *data = opaque; > + size_t i; > + uint8_t *enckey = NULL; > + size_t enckeylen = 32; > + uint8_t *iv = NULL; > + size_t ivlen = 16; > + uint8_t *ciphertext = NULL; > + size_t ciphertextlen = 0; > + int ret = -1; > + > + if (!virCryptoHaveCipher(data->algorithm)) { > + fprintf(stderr, "Invalid cipher algorithm=%d\n", data->algorithm);

> + return -1; > + } > + > + if (VIR_ALLOC_N(enckey, enckeylen) < 0 || > + VIR_ALLOC_N(iv, ivlen) < 0) > + goto cleanup; > + > + if (virRandomBytes(enckey, enckeylen) < 0 || > + virRandomBytes(iv, ivlen) < 0) > + goto cleanup; > + > + if (virCryptoEncryptData(data->algorithm, enckey, enckeylen, iv, ivlen, > + data->input, data->inputlen, > + &ciphertext, &ciphertextlen) < 0) > + goto cleanup; > + > + if (data->ciphertextlen != ciphertextlen) { > + fprintf(stderr, "Expected ciphertextlen(%zu) doesn't match (%zu)\n", > + data->ciphertextlen, ciphertextlen); > + goto cleanup; > + } > + > + if (memcmp(data->ciphertext, ciphertext, ciphertextlen)) { > + fprintf(stderr, "Expected ciphertext doesn't match\n"); > + for (i = 0; i < ciphertextlen; i++) { > + if (data->ciphertext[i] != ciphertext[i]) { > + fprintf(stderr, "expected[%zu]=%x ciphertext[%zu]=%x\n", > + i, data->ciphertext[i], > + i, ciphertext[i]); Last time I was pointing out that it doesn't make much sense to output the difference whether encoded to base64 or not. [...]

> @@ -84,7 +158,31 @@ mymain(void) > VIR_CRYPTO_HASH(VIR_CRYPTO_HASH_MD5, "The quick brown fox", "a2004f37730b9445670a738fa0fc9ee5"); > VIR_CRYPTO_HASH(VIR_CRYPTO_HASH_SHA256, "The quick brown fox", "5cac4f980fedc3d3f1f99b4be3472c9b30d56523e632d151237ec9309048bda9"); > > +#undef VIR_CRYPTO_HASH > + > +#define VIR_CRYPTO_ENCRYPT(a, n, i, il, c, cl) \ > + do { \ > + struct testCryptoEncryptData data = { \ > + .algorithm = a, \ > + .input = i, \ > + .inputlen = il, \ > + .ciphertext = c, \ > + .ciphertextlen = cl, \ > + }; \ > + if (virtTestRun("Encrypt " n, testCryptoEncrypt, &data) < 0) \ > + ret = -1; \ > + } while (0) > + > + memset(&secretdata, 0, 8); > + memcpy(&secretdata, "letmein", 7); > + > + VIR_CRYPTO_ENCRYPT(VIR_CRYPTO_CIPHER_AES256CBC, "aes265cbc", > + secretdata, 7, expected_ciphertext, 16); > + This test will fail on builds without gnutls.

ACK with the nits fixed.