Re: [Qemu-devel] [PATCH v3 4/5] crypto: add QCryptoSecret object class for password/key handling

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3581 bytes --]

On 11/27/2015 09:30 AM, Daniel P. Berrange wrote:
> Introduce a new QCryptoSecret object class which will be used
> for providing passwords and keys to other objects which need
> sensitive credentials.
> 

> More examples are shown in the updated docs.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---

> +++ b/crypto/secret.c

> +static void
> +qcrypto_secret_load_data(QCryptoSecret *secret,
> +                         uint8_t **output,
> +                         size_t *outputlen,
> +                         Error **errp)
> +{

> +        if (!g_file_get_contents(secret->file, &data, &length, &gerr)) {
> +            error_setg(errp,
> +                       "Unable to read %s: %s",
> +                       secret->file, gerr->message);
> +            g_error_free(gerr);
> +            return;
> +        }
> +        if (length) {
> +            /* Even though data is raw 8-bit, so may contain
> +             * arbitrary NULs, ensure it is explicitly NUL
> +             * terminated */
> +            *output = g_renew(uint8_t, data, length + 1);
> +            (*output)[length] = '\0';

These two lines are dead code. g_file_get_contents() guarantees that on
success, contents is malloc'd large enough and that contents[length] == 0.

https://developer.gnome.org/glib/stable/glib-File-Utilities.html#g-file-get-contents

> +            *outputlen = length;
> +        } else {
> +            error_setg(errp, "Secret file %s is empty",
> +                       secret->file);

Is there any technical reason why we must forbid a 0-length password?
(Sometimes, having the empty string as a password can be a useful for
development tests).  I'm not opposed to rejecting it, especially if
doing so now avoids a more cryptic error message later because there is
indeed a technical reason; but just want to make sure it is not an
arbitrary limitation.

> +            g_free(data);
> +        }
> +    } else if (secret->data) {
> +        *outputlen = strlen(secret->data);
> +        *output = g_new(uint8_t, *outputlen + 1);
> +        memcpy(*output, secret->data, *outputlen + 1);

These two lines could be shortened to:
*output = g_strdup(secret->data);

> +
> +static void qcrypto_secret_decrypt(QCryptoSecret *secret,
> +                                   const uint8_t *input,
> +                                   size_t inputlen,
> +                                   uint8_t **output,
> +                                   size_t *outputlen,
> +                                   Error **errp)
> +{

> +    if (secret->format == QCRYPTO_SECRET_FORMAT_BASE64) {
> +        ciphertext = qbase64_decode((const gchar*)input,
> +                                    inputlen,
> +                                    &ciphertextlen,
> +                                    errp);
> +        if (!ciphertext) {
> +            goto cleanup;
> +        }
> +        plaintext = g_new0(uint8_t, ciphertextlen + 1);
> +    } else {
> +        ciphertextlen = inputlen;
> +        plaintext = g_new0(uint8_t, inputlen + 1);

g_new0(uint8_t, value) is the same as g_malloc0(value); I don't know if
it is worth the distinction.  But not worth a respin on its own.

I found some style or efficiency things you might want to touch up, but
no actual bugs that would prevent this patch from being usable as-is.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]