9:41 a.m.
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