[libvirt] [PATCH 2/3] qemu: Create domain master key

[John Ferlan]

On 03/29/2016 08:56 AM, Peter Krempa wrote:
[,,,]

>>  
>> +/* qemuDomainGetMasterKeyFilePath:
>> + * @libDir: Directory path to domain lib files
>> + *
>> + * This API will generate a path of the domain's libDir (e.g.
>> + * (/var/lib/libvirt/qemu/domain-#-$NAME/) and a name 'master.key'.
>> + *
>> + * This API will check and fail if the libDir is NULL as that results
>> + * in an invalid path generated.
>> + *
>> + * This API does not check if the resulting path exists, that is left
>> + * up to the caller.
>> + *
>> + * Returns path to memory containing the name of the file. It is up to the
>> + * caller to free; otherwise, NULL on failure.
> 
> Whoah, docs longer than the code itself.
> 

Setting future expectations. I'll shorten it though.

>> + */
>> +char *
>> +qemuDomainGetMasterKeyFilePath(const char *libDir)
>> +{
>> +    if (!libDir) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> +                       _("invalid path for master key file"));
>> +        return NULL;
> 
> And the code is rather self-explanatory.
> 
>> +    }
>> +    return virFileBuildPath(libDir, "master.key", NULL);
>> +}
>> +

[...]

>> +qemuDomainMasterKeyReadFile(qemuDomainObjPrivatePtr priv)
>> +{
>> +    char *path;
>> +    char *base64Key = NULL;
>> +    int base64KeySize = 0;
>> +    char *masterKey = NULL;
>> +    size_t masterKeySize = 0;
>> +    int ret = -1;
>> +
>> +    /* If we don't have the capability, then do nothing. */
>> +    if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_SECRET))
>> +        return 0;
>> +
>> +    if (!(path = qemuDomainGetMasterKeyFilePath(priv->libDir)))
>> +        return -1;
>> +
>> +    if (!virFileExists(path)) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                       _("domain master key file doesn't exist in %s"),
>> +                       priv->libDir);
>> +        goto cleanup;
>> +    }
>> +
>> +    /* Read the base64 encooded secret from the file, decode it, and
>> +     * store in the domain private object.
>> +     */
>> +    if ((base64KeySize = virFileReadAll(path, 1024, &base64Key)) < 0)
>> +        goto cleanup;
>> +
>> +    if (!base64_decode_alloc(base64Key, base64KeySize,
>> +                             &masterKey, &masterKeySize)) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> +                       _("invalid base64 in domain master key file"));
>> +        goto cleanup;
>> +    }
>> +
>> +    if (!masterKey || masterKeySize != QEMU_DOMAIN_MASTER_KEY_LEN) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                       _("invalid encoded master key read, size=%zd"),
>> +                       masterKeySize);
>> +        goto cleanup;
>> +    }
>> +
>> +    priv->masterKey = masterKey;
>> +    masterKey = NULL;
> 
> so this is NULL now and masterKeySize > 0
> 

doh.  Yep. and that's what happens when adding the cleanup: late

>> +
>> +    ret = 0;
>> +
>> + cleanup:
>> +    if (masterKeySize > 0)
>> +        memset(masterKey, 0, masterKeySize);
> 
> ... memset(NULL, 0, 32);
> 
> This crashes on success path.
> 
>> +    VIR_FREE(masterKey);
>> +
>> +    if (base64KeySize > 0)
>> +        memset(base64Key, 0, base64KeySize);
>> +    VIR_FREE(base64Key);
>> +
>> +    VIR_FREE(path);
>> +
>> +    return ret;
>> +}
>> +

[...]

>> +void
>> +qemuDomainMasterKeyRemove(qemuDomainObjPrivatePtr priv)
>> +{
>> +    char *path = NULL;
>> +
>> +    if (!priv->masterKey)
>> +        return;
>> +
>> +    /* Clear the heap */
>> +    memset(priv->masterKey, 0, QEMU_DOMAIN_MASTER_KEY_LEN);
>> +    VIR_FREE(priv->masterKey);
>> +
>> +    /* Clear and remove the master key file */
>> +    path = qemuDomainGetMasterKeyFilePath(priv->libDir);
>> +    if (path && virFileExists(path)) {
>> +        /* Write a "0" to the file even though we're about to delete it */
> 
> This still doesn't make any sense. The filesystem needs to guarantee
> this anyways. This is just a false sense of security.
> 

See my note in response to Dan's review. IDC either way.

>> +        ignore_value(virFileWriteStr(path, "0", 0600));
>> +        unlink(path);
>> +    }
>> +    VIR_FREE(path);
>> +}
>> +
>> +
>> +/* qemuDomainMasterKeyCreate:
>> + * @priv: Pointer to the domain private object
>> + *
>> + * As long as the underlying qemu has the secret capability,
>> + * generate and store as a base64 encoded value a random 32-byte
>> + * key to be used as a secret shared with qemu to share sensitive data.
>> + *
>> + * Returns: 0 on success, -1 w/ error message on failure
>> + */
>> +int
>> +qemuDomainMasterKeyCreate(qemuDomainObjPrivatePtr priv)
>> +{
>> +    char *key = NULL;
>> +
>> +    /* If we don't have the capability, then do nothing. */
>> +    if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_SECRET))
>> +        return 0;
>> +
>> +    if (!(key = qemuDomainGenerateRandomKey(QEMU_DOMAIN_MASTER_KEY_LEN)))
>> +        goto error;
>> +
>> +    priv->masterKey = key;
>> +    key = NULL;
> 
> key isn't touched after the previous line, thus it doesn't make much
> sense to clear that variable.
> 

It was at one time, but there's no need for it now. No need for local
either.

>> +
>> +    if (qemuDomainWriteMasterKeyFile(priv) < 0)
>> +        goto error;
>> +
>> +    return 0;
>> +
>> + error:
>> +    qemuDomainMasterKeyRemove(priv);
>> +    return -1;
>> +}
>> +
>> +
>>  static virClassPtr qemuDomainDiskPrivateClass;
>>  
>>  static int
>> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
>> index 573968c..f85f17f 100644
>> --- a/src/qemu/qemu_domain.h
>> +++ b/src/qemu/qemu_domain.h
>> @@ -147,6 +147,7 @@ struct qemuDomainJobObj {
>>  typedef void (*qemuDomainCleanupCallback)(virQEMUDriverPtr driver,
>>                                            virDomainObjPtr vm);
>>  
>> +# define QEMU_DOMAIN_MASTER_KEY_LEN 32  /* For a 32-byte random masterKey */
> 
> 256 bit.
> 

6 of one, half dozen of another.

Tks -

John