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

[Eric Blake]

On 11/24/2015 08:02 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.
> 
> The new object can provide secret values directly as properties,
> or indirectly via a file. The latter includes support for file
> descriptor passing syntax on UNIX platforms. Ordinarily passing
> secret values directly as properties is insecure, since they
> are visible in process listings, or in log files showing the
> CLI args / QMP commands. It is possible to use AES-256-CBC to
> encrypt the secret values though, in which case all that is
> visible is the ciphertext.  For adhoc developer testing though,

dictionary.com says it's always 'ad hoc' (since it is literally two
Latin words), even though the two are always spoken together in English.
 And although 'ad-hoc' is starting to gain some traction in modern
usage, squashing it to 'adhoc' just feels like a typo.

> it is fine to provide the secrets directly without encryption
> so this is not explicitly forbidden.
> 
> The anticipated scenario is that libvirtd will create a random
> master key per QEMU instance (eg /var/run/libvirt/qemu/$VMNAME.key)
> and will use that key to encrypt all passwords it provides to
> QEMU via '-object secret,....'.  This avoids the need for libvirt
> (or other mgmt apps) to worry about file descriptor passing.
> 
> It also makes life easier for people who are scripting the
> management of QEMU, for whom FD passing is significantly more
> complex.
> 
> Providing data inline (insecure, only for adhoc dev testing)

Of course, if we touch it up in one place, you should use consistent
spelling throughout :)

> 
>   $QEMU -object secret,id=sec0,data=letmein
> 
> Providing data indirectly in raw format
> 
>   printf "letmein" > mypasswd.txt
>   $QEMU -object secret,id=sec0,file=mypasswd.txt
> 
> Providing data indirectly in base64 format
> 
>   $QEMU -object secret,id=sec0,file=mykey.b64,format=base64
> 
> Providing data with encryption
> 
>   $QEMU -object secret,id=master0,file=mykey.b64,format=base64 \
>         -object secret,id=sec0,data=[base64 ciphertext],\
>                  keyid=master0,iv=[base64 IV],format=base64
> 
> Note that 'format' here refers to the format of the ciphertext
> data. The decrypted data must always be in raw byte format.
> 
> More examples are shown in the updated docs.
> 
> Signed-off-by: Daniel P. Berrange <address@hidden>
> ---

> +static void
> +qcrypto_secret_load_data(QCryptoSecret *secret,
> +                         uint8_t **output,
> +                         size_t *outputlen,
> +                         Error **errp)
> +{
> +    int fd;
> +    char *data = NULL;
> +    size_t offset = 0;
> +    size_t length = 0;
> +
> +    *output = NULL;
> +    *outputlen = 0;
> +
> +    if (secret->file) {
> +        if (secret->data) {
> +            error_setg(errp,
> +                       "'file' and 'data' are mutually exclusive");

Is it worth trying to use a qapi flat union to make the mutual exclusion
inherent in the type definition, rather than something we have to
enforce manually?  (I've got more experience with qapi than with Object,
so my question may be nonsensical)

> +            return;
> +        }
> +        fd = qemu_open(secret->file, O_RDONLY);
> +        if (fd < 0) {
> +            error_setg_errno(errp, errno,
> +                             "Unable to open %s", secret->file);

Using error_setg_file_open() makes for a consistent message on open()
failure.

> +            return;
> +        }
> +        while (length < (1024 * 1024)) { /* Limit secrets to 1 MB */
> +            if ((length - offset) < 1024) {
> +                length += 1024;
> +                data = g_renew(char, data, length);
> +            }
> +            ssize_t ret = read(fd, data + offset, length - offset);
> +            if (ret == 0) {
> +                break;
> +            }
> +            if (ret < 0) {
> +                error_setg_errno(errp, errno,
> +                                 "Unable to read from %s", secret->file);

Does glib have a convenience function for reading contents of a file?

> +                close(fd);
> +                g_free(data);
> +                return;
> +            }
> +            offset += ret;
> +        }
> +        close(fd);
> +        if (offset) {
> +            /* Even though data is raw 8-bit, so may contain
> +             * arbitrary NULs, ensure it is explicitly NUL
> +             * terminated */
> +            *output = g_renew(uint8_t, data, offset + 1);
> +            (*output)[offset] = '\0';
> +            *outputlen = offset;
> +        } else {
> +            error_setg(errp, "Secret file %s is empty",
> +                       secret->file);
> +            g_free(data);
> +        }
> +    } else if (secret->data) {
> +        *outputlen = strlen(secret->data);
> +        *output = g_new(uint8_t, *outputlen + 1);
> +        memcpy(*output, secret->data, *outputlen + 1);
> +    } else {
> +        error_setg(errp, "Either 'file' or 'data' must be provided");
> +    }
> +}
> +
> +
> +static void qcrypto_secret_decrypt(QCryptoSecret *secret,
> +                                   const uint8_t *input,
> +                                   size_t inputlen,
> +                                   uint8_t **output,
> +                                   size_t *outputlen,
> +                                   Error **errp)
> +{
> +    uint8_t *key = NULL, *ciphertext = NULL, *iv = NULL;
> +    size_t keylen, ciphertextlen, ivlen;
> +    QCryptoCipher *aes = NULL;
> +    uint8_t *plaintext = NULL;
> +
> +    *output = NULL;
> +    *outputlen = 0;
> +
> +    if (qcrypto_secret_lookup(secret->keyid,
> +                              &key, &keylen,
> +                              errp) < 0) {
> +        goto cleanup;
> +    }
> +
> +    if (keylen != 32) {
> +        error_setg(errp, "Key should be 32 bytes in length");
> +        goto cleanup;
> +    }
> +
> +    if (!secret->iv) {
> +        error_setg(errp, "IV is required to decrypt secret");
> +        goto cleanup;
> +    }
> +
> +    iv = (uint8_t *)g_base64_decode(secret->iv, &ivlen);

Shouldn't this be using qbase64_decode()?


> +++ b/include/crypto/secret.h

> +/**
> + * QCryptoSecret:
> + *
> + * The QCryptoSecret object provides storage of secrets,
> + * which may be user passwords, encryption keys or any
> + * other kind of sensitive data that is represented as
> + * a sequence of bytes.
> + *

> + * Providing data directly, insecurely (suitable for
> + * adhoc developer testing only)

Another instance.

> +++ b/qapi/crypto.json
> @@ -19,3 +19,17 @@
>  { 'enum': 'QCryptoTLSCredsEndpoint',
>    'prefix': 'QCRYPTO_TLS_CREDS_ENDPOINT',
>    'data': ['client', 'server']}
> +
> +
> +##
> +# QCryptoSecretFormat:
> +#
> +# The data format that the secret is provided in
> +#
> +# @raw: raw bytes. When encoded in JSON only valid UTF-8 sequences can be 
> used
> +# @base64: arbitrary base64 encoded binary data
> +# Since: 2.6
> +##
> +{ 'enum': 'QCryptoSecretFormat',
> +  'prefix': 'QCRYPTO_SECRET_FORMAT',
> +  'data': ['raw', 'base64']}

This part is fine.

> diff --git a/qemu-options.hx b/qemu-options.hx
> index 0eea4ee..41c0e59 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -3677,6 +3677,83 @@ Dump the network traffic on netdev @var{dev} to the 
> file specified by
>  The file format is libpcap, so it can be analyzed with tools such as tcpdump
>  or Wireshark.
>  
> address@hidden -object 
> secret,address@hidden,address@hidden,address@hidden|base64}[,address@hidden,address@hidden
> address@hidden -object 
> secret,address@hidden,address@hidden,address@hidden|base64}[,address@hidden,address@hidden
> +
> +Defines a secret to store a password, encryption key, or some other sensitive
> +data. The senstive data can either be passed directly via the @var{data}

s/senstive/sensitive/

> +parameter, or indirectly via the @var{file} parameter. Using the @var{data}
> +parameter is insecure unless the sensitive data is encrypted.
> +

Overall looks fairly reasonable.

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

signature.asc
Description: OpenPGP digital signature