[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 3/3] util: Introduce encryption APIs



On Wed, May 18, 2016 at 19:52:32 -0400, John Ferlan wrote:
> Introduce virCryptoHaveEncrypt and virCryptoEncryptSecret to handle
> performing encryption.
> 
>  virCryptoHaveEncrypt:
>    Boolean function to determine whether the requested encryption
>    API is available. It's expected this API will be called prior to
>    virCryptoEncryptSecret. It will return true/false.
> 
>  virCryptoEncryptSecret:
>    Based on the requested encryption 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 redhat com>
> ---
>  configure.ac             |   1 +
>  src/libvirt_private.syms |   2 +
>  src/util/vircrypto.c     | 189 ++++++++++++++++++++++++++++++++++++++++++++++-
>  src/util/vircrypto.h     |  20 ++++-
>  tests/vircryptotest.c    |  86 +++++++++++++++++++++
>  5 files changed, 296 insertions(+), 2 deletions(-)
> 

[...]

> diff --git a/src/util/vircrypto.c b/src/util/vircrypto.c
> index 39a479a..05bd167 100644
> --- a/src/util/vircrypto.c
> +++ b/src/util/vircrypto.c

[...]

> @@ -76,3 +85,181 @@ virCryptoHashString(virCryptoHash hash,
>  
>      return 0;
>  }
> +
> +
> +/* virCryptoHaveEncrypt:
> + * @algorithm: Specific encryption 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
> +virCryptoHaveEncrypt(virCryptoEncrypt algorithm)

Cipher rather than Encrypt

> +{
> +    switch (algorithm) {
> +
> +    case VIR_CRYPTO_ENCRYPT_AES256CBC:
> +#ifdef HAVE_GNUTLS_CIPHER_ENCRYPT
> +    return true;
> +#else
> +    return false;
> +#endif
> +
> +    case VIR_CRYPTO_ENCRYPT_NONE:
> +    case VIR_CRYPTO_ENCRYPT_LAST:
> +        break;
> +    };
> +
> +    return false;
> +}
> +
> +
> +/* virCryptoEncryptDataAESgntuls:
> + *
> + * Performs the AES gnutls encryption - parameters essentially the
> + * same as virCryptoEncryptSecret, except the libvirt algorithm is
> + * converted to the gnutls_cipher_algorithm_t
> + *
> + * Same input as virCryptoEncryptData, but ensures encryption key and
> + * initialization vector lengths are correct.
> + *
> + * The data buffer will be cleared as soon as it has been prepared for
> + * encryption.
> + *
> + * 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_key;

As pointed out last time, even here it won't compile with gnutls.

> +    uint8_t *ciphertext;
> +    size_t ciphertextlen;
> +
> +    /* Allocate a padded buffer, copy in the data, padding the buffer with
> +     * the size of the padding size which is required for decryption, then
> +     * clear the data buffer when we're done. */
> +    ciphertextlen = VIR_ROUND_UP(datalen, 16);
> +    if (VIR_ALLOC_N(ciphertext, ciphertextlen) < 0)
> +        return -1;
> +    memcpy(ciphertext, data, datalen);
> +    for (i = datalen; i < ciphertextlen; i++)

And this still isn't documented.

> +        ciphertext[i] = ciphertextlen - datalen;
> +    memset(data, 0, datalen);

The caller should ensure sanitization. Especially sinc its _NOT_ done if
the above allocation fails. In such case the caller still needs to do
it.

> +
> +    /* Initialize the gnutls cipher */
> +    enc_key.size = enckeylen;
> +    enc_key.data = enckey;
> +    if (iv) {
> +        iv_key.size = ivlen;
> +        iv_key.data = iv;
> +    }
> +    if ((rc = gnutls_cipher_init(&handle, gnutls_enc_alg,
> +                                 &enc_key, &iv_key)) < 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;
> +}
> +
> +
> +/* 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(virCryptoEncrypt 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_ENCRYPT_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,
> +                           _("AES246CBC initialization vector invalid len=%zu"),

AES256CBC

> +                           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);
> +
> +    case VIR_CRYPTO_ENCRYPT_NONE:
> +    case VIR_CRYPTO_ENCRYPT_LAST:
> +        break;
> +    }
> +
> +    virReportError(VIR_ERR_INVALID_ARG,
> +                   _("algorithm=%d is not supported"), algorithm);
> +    return -1;
> +}
> diff --git a/src/util/vircrypto.h b/src/util/vircrypto.h
> index f67d49d..7d0829e 100644
> --- a/src/util/vircrypto.h
> +++ b/src/util/vircrypto.h

[...]

> @@ -30,6 +30,14 @@ typedef enum {
>      VIR_CRYPTO_HASH_LAST
>  } virCryptoHash;
>  
> +
> +typedef enum {
> +    VIR_CRYPTO_ENCRYPT_NONE = 0,
> +    VIR_CRYPTO_ENCRYPT_AES256CBC,
> +
> +    VIR_CRYPTO_ENCRYPT_LAST
> +} virCryptoEncrypt;

This can be used for decryption too. Thus virCryptoCipher.

> +
>  int
>  virCryptoHashString(virCryptoHash hash,
>                      const char *input,

[...]

> diff --git a/tests/vircryptotest.c b/tests/vircryptotest.c
> index bfc47db..038eb7a 100644
> --- a/tests/vircryptotest.c
> +++ b/tests/vircryptotest.c

[...]

> @@ -56,10 +57,74 @@ testCryptoHash(const void *opaque)
>  }
>  
>  
> +struct testCryptoEncryptData {
> +    virCryptoEncrypt algorithm;
> +    uint8_t *input;
> +    size_t inputlen;
> +    const char *base64;
> +};
> +
> +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;
> +    char *actual = NULL;
> +    int ret = -1;
> +
> +    if (!virCryptoHaveEncrypt(data->algorithm)) {
> +        fprintf(stderr, "Invalid encryption algorithm=%d\n", data->algorithm);
> +        return -1;
> +    }
> +
> +    if (VIR_ALLOC_N(enckey, enckeylen) < 0 ||
> +        VIR_ALLOC_N(iv, ivlen) < 0)
> +        goto cleanup;
> +
> +    /* To be replaced by mock if I can get it to work */

Why not keep it this way? This is testing the encryption not the key
generation.

> +    for (i = 0; i < enckeylen; i++)
> +        enckey[i] = i;
> +    for (i = 0; i < ivlen; i++)
> +        iv[i] = i;
> +
> +    if (virCryptoEncryptData(data->algorithm, enckey, enckeylen, iv, ivlen,
> +                             data->input, data->inputlen,
> +                             &ciphertext, &ciphertextlen) < 0)
> +        goto cleanup;
> +
> +    /* Comparing the encoded ciphertext would be what is desired in
> +     * the long run and easier to read/format the expected ciphertext */

Read? nobody is going to read it if it doesn't match.

> +    if (!(actual = virStringEncodeBase64(ciphertext, ciphertextlen)))
> +        goto cleanup;
> +
> +    if (STRNEQ(data->base64, actual)) {
> +        fprintf(stderr, "Expected encoded encryption '%s' but got '%s'\n",
> +                data->base64, NULLSTR(actual));

and it definitely doesn't make sense to print it. It's just binary
garbage.

> +        goto cleanup;
> +    }
> +
> +    ret = 0;
> + cleanup:
> +    VIR_DISPOSE_N(enckey, enckeylen);
> +    VIR_DISPOSE_N(iv, ivlen);
> +    VIR_DISPOSE_N(ciphertext, ciphertextlen);

This isn't really necessary for demo keys. You can also lose the lenght
variables.

> +    VIR_FREE(actual);
> +

I won't review further versions until you fix the issue with compiling
without gnutls. I've pointed it out far too many times.

Peter

Attachment: signature.asc
Description: Digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]