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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list