On Thu, Nov 20, 2025 at 22:23:42 +0530, Arun Menon via Devel wrote:
> Adds `virCryptoDecryptDataAESgnutls` and `virCryptoDecryptData`
> as wrapper functions for GnuTLS decryption.
>
> These functions are the inverse of the existing GnuTLS encryption wrappers.
> This commit also includes a corresponding test case to validate data
> decryption.
>
> Signed-off-by: Arun Menon <[email protected]>
> ---
> src/libvirt_private.syms | 1 +
> src/util/vircrypto.c | 128 ++++++++++++++++++++++++++++++++++++++-
> src/util/vircrypto.h | 8 +++
> tests/vircryptotest.c | 65 ++++++++++++++++++++
> 4 files changed, 201 insertions(+), 1 deletion(-)
> @@ -233,3 +233,129 @@ virCryptoEncryptData(virCryptoCipher algorithm,
> _("algorithm=%1$d is not supported"), algorithm);
> return -1;
> }
> +
> +/* virCryptoDecryptDataAESgnutls:
> + *
> + * Performs the AES gnutls decryption
> + *
> + * Same input as virCryptoDecryptData, except the algorithm is replaced
> + * by the specific gnutls algorithm.
> + *
> + * Decrypts the @data buffer using the @deckey and if available the @iv
> + *
> + * Returns 0 on success with the plaintext being filled. It is the
> + * caller's responsibility to clear and free it. Returns -1 on failure
> + * w/ error set.
> + */
> +static int
> +virCryptoDecryptDataAESgnutls(gnutls_cipher_algorithm_t gnutls_dec_alg,
> + uint8_t *deckey,
> + size_t deckeylen,
> + uint8_t *iv,
> + size_t ivlen,
> + uint8_t *data,
> + size_t datalen,
> + uint8_t **plaintextret,
> + size_t *plaintextlenret)
> +{
> + int rc;
> + size_t i;
The 'i' variable isn't used as an iterator in a loop. Please give it a
proper name.
> + gnutls_cipher_hd_t handle = NULL;
> + gnutls_datum_t dec_key = { .data = deckey, .size = deckeylen };
> + gnutls_datum_t iv_buf = { .data = iv, .size = ivlen };
> + g_autofree uint8_t *plaintext = NULL;
> + size_t plaintextlen;
> +
> + if ((rc = gnutls_cipher_init(&handle, gnutls_dec_alg,
> + &dec_key, &iv_buf)) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("failed to initialize cipher: '%1$s'"),
> + gnutls_strerror(rc));
> + return -1;
> + }
> +
> + plaintext = g_memdup2(data, datalen);
> + plaintextlen = datalen;
So 'plaintextlen' is assigned from 'datalen'. 'datalen' wasn't modified
so is the same as the argument.
> + rc = gnutls_cipher_decrypt(handle, plaintext, plaintextlen);
'plaintextlen' is passed by value so it can't be modified by
'gnutls_cipher_decrypt'.
> + gnutls_cipher_deinit(handle);
> + if (rc < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("failed to decrypt the data: '%1$s'"),
> + gnutls_strerror(rc));
> + goto error;
> + }
> + if (plaintextlen == 0) {
'plaintextlen' still wasn't modified and the value you are checking
against was known from the beginning, so do this check right at the
beginning.
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("decrypted data has zero length"));
> + goto error;
> + }
> + i = plaintext[plaintextlen - 1];
So 'i' should be called 'paddinglen' and also be of 'uint8_t' type.
Also here it'd be worth commenting why we're looking at the last byte to
determine the padding lenght.
> + if (i > plaintextlen) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
Failed padding isn't IMO an internal error any more since it's data that
can originate from somewhere else.
> + _("decrypted data has invalid padding"));
> + goto error;
> + }
> + *plaintextlenret = plaintextlen - i;
This at the first glance looked like plaintextlen - 1. So 'i' is not
only wrong semantically but also can look confusing.
> + *plaintextret = g_steal_pointer(&plaintext);
> + return 0;
> + error:
> + virSecureErase(plaintext, plaintextlen);
> + return -1;
> +}
> +
> +/* virCryptoDecryptData:
> + * @algorithm: algorithm desired for decryption
> + * @deckey: decryption key
> + * @deckeylen: decryption key length
> + * @iv: initialization vector
> + * @ivlen: length of initialization vector
> + * @data: data to decrypt
> + * @datalen: length of data
> + * @plaintext: stream of bytes allocated to store plaintext
> + * @plaintextlen: size of the stream of bytes
> + * Returns 0 on success, -1 on failure with error set
> + */
> +int
> +virCryptoDecryptData(virCryptoCipher algorithm,
> + uint8_t *deckey,
> + size_t deckeylen,
> + uint8_t *iv,
> + size_t ivlen,
> + uint8_t *data,
> + size_t datalen,
> + uint8_t **plaintext,
> + size_t *plaintextlen)
> +{
> + switch (algorithm) {
> + case VIR_CRYPTO_CIPHER_AES256CBC:
> + if (deckeylen != 32) {
> + virReportError(VIR_ERR_INVALID_ARG,
> + _("AES256CBC decryption invalid keylen=%1$zu"),
> + deckeylen);
> + return -1;
> + }
> + if (ivlen != 16) {
> + virReportError(VIR_ERR_INVALID_ARG,
> + _("AES256CBC initialization vector invalid
> len=%1$zu"),
> + ivlen);
> + return -1;
> + }
> + /*
> + * Decrypt the data buffer using a decryption key and
> + * initialization vector via the gnutls_cipher_decrypt API
> + * for GNUTLS_CIPHER_AES_256_CBC.
> + */
This comment doesn't really explain anything which wouldn't be obvious
from what you're calling here. In contrast e.g. the padding lenght
determination may not be obvious to the reader which would make a much
better use of a comment.
> + return virCryptoDecryptDataAESgnutls(GNUTLS_CIPHER_AES_256_CBC,
> + deckey, deckeylen, iv, ivlen,
> + data, datalen,
> + plaintext, plaintextlen);
> + case VIR_CRYPTO_CIPHER_NONE:
> + case VIR_CRYPTO_CIPHER_LAST:
> + break;
> + }
> +
> + virReportError(VIR_ERR_INVALID_ARG,
> + _("algorithm=%1$d is not supported"), algorithm);
> + return -1;
> +}
