The struct doesn't contain any secrets to clear before freeing and even if it did VIR_DISPOSE_N wouldn't help as the struct contains only pointers thus the actual memory pointing to isn't sanitized. Just free the params array pointer and then the struct itself. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/hyperv/hyperv_wmi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/hyperv/hyperv_wmi.c b/src/hyperv/hyperv_wmi.c index a28bb0e815..3bb75392a7 100644 --- a/src/hyperv/hyperv_wmi.c +++ b/src/hyperv/hyperv_wmi.c @@ -184,8 +184,8 @@ hypervFreeInvokeParams(hypervInvokeParamsListPtr params) } } - VIR_DISPOSE_N(params->params, params->nbAvailParams); - VIR_FREE(params); + g_free(params->params); + g_free(params); } -- 2.29.2

The code pretends that it cares about clearing the secret values, but passes the secret value to a realloc, which may copy the value somewhere else and doesn't sanitize the original location when it does so. Since we want to construct a string from the value, let's copy it to a new piece of memory which has the space for the 'NUL' byte ourselves, to prevent a random realloc keeping the data around. While at it, use memset of VIR_DISPOSE_N since it's being phased out. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/storage/storage_backend_iscsi.c | 16 +++++++++------- src/storage/storage_backend_iscsi_direct.c | 17 +++++++++-------- 2 files changed, 18 insertions(+), 15 deletions(-) diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index 45167e4490..b24b8d54b1 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -256,8 +256,9 @@ static int virStorageBackendISCSISetAuth(const char *portal, virStoragePoolSourcePtr source) { - unsigned char *secret_value = NULL; + g_autofree unsigned char *secret_value = NULL; size_t secret_size; + g_autofree char *secret_str = NULL; virStorageAuthDefPtr authdef = source->auth; int ret = -1; virConnectPtr conn = NULL; @@ -282,10 +283,10 @@ virStorageBackendISCSISetAuth(const char *portal, &secret_value, &secret_size) < 0) goto cleanup; - if (VIR_REALLOC_N(secret_value, secret_size + 1) < 0) - goto cleanup; - - secret_value[secret_size] = '\0'; + secret_str = g_new0(char, secret_size + 1); + memcpy(secret_str, secret_value, secret_size); + memset(secret_value, 0, secret_size); + secret_str[secret_size] = '\0'; if (virISCSINodeUpdate(portal, source->devices[0].path, @@ -298,13 +299,14 @@ virStorageBackendISCSISetAuth(const char *portal, virISCSINodeUpdate(portal, source->devices[0].path, "node.session.auth.password", - (const char *)secret_value) < 0) + secret_str) < 0) goto cleanup; ret = 0; cleanup: - VIR_DISPOSE_N(secret_value, secret_size); + if (secret_str) + memset(secret_str, 0, secret_size); virObjectUnref(conn); return ret; } diff --git a/src/storage/storage_backend_iscsi_direct.c b/src/storage/storage_backend_iscsi_direct.c index 12b075db0b..78b12f057f 100644 --- a/src/storage/storage_backend_iscsi_direct.c +++ b/src/storage/storage_backend_iscsi_direct.c @@ -87,8 +87,9 @@ static int virStorageBackendISCSIDirectSetAuth(struct iscsi_context *iscsi, virStoragePoolSourcePtr source) { - unsigned char *secret_value = NULL; + g_autofree unsigned char *secret_value = NULL; size_t secret_size; + g_autofree char *secret_str = NULL; virStorageAuthDefPtr authdef = source->auth; int ret = -1; virConnectPtr conn = NULL; @@ -113,14 +114,13 @@ virStorageBackendISCSIDirectSetAuth(struct iscsi_context *iscsi, &secret_value, &secret_size) < 0) goto cleanup; - if (VIR_REALLOC_N(secret_value, secret_size + 1) < 0) - goto cleanup; - - secret_value[secret_size] = '\0'; + secret_str = g_new0(char, secret_size + 1); + memcpy(secret_str, secret_value, secret_size); + memset(secret_value, 0, secret_size); + secret_str[secret_size] = '\0'; if (iscsi_set_initiator_username_pwd(iscsi, - authdef->username, - (const char *)secret_value) < 0) { + authdef->username, secret_str) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to set credential: %s"), iscsi_get_error(iscsi)); @@ -129,7 +129,8 @@ virStorageBackendISCSIDirectSetAuth(struct iscsi_context *iscsi, ret = 0; cleanup: - VIR_DISPOSE_N(secret_value, secret_size); + if (secret_str) + memset(secret_str, 0, secret_size); virObjectUnref(conn); return ret; } -- 2.29.2

When virRandomBytes fails we don't get any random bytes and even if we did they don't have to be treated as secret as they weren't used in any way. Add a temporary variable with automatic freeing for the secret buffer and assign it only on success. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/qemu/qemu_domain.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 0c078a9388..2c34307c82 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -562,18 +562,19 @@ int qemuDomainMasterKeyCreate(virDomainObjPtr vm) { qemuDomainObjPrivatePtr priv = vm->privateData; + g_autofree uint8_t *key = NULL; /* If we don't have the capability, then do nothing. */ if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_SECRET)) return 0; - priv->masterKey = g_new0(uint8_t, QEMU_DOMAIN_MASTER_KEY_LEN); - priv->masterKeyLen = QEMU_DOMAIN_MASTER_KEY_LEN; + key = g_new0(uint8_t, QEMU_DOMAIN_MASTER_KEY_LEN); - if (virRandomBytes(priv->masterKey, priv->masterKeyLen) < 0) { - VIR_DISPOSE_N(priv->masterKey, priv->masterKeyLen); + if (virRandomBytes(key, QEMU_DOMAIN_MASTER_KEY_LEN) < 0) return -1; - } + + priv->masterKey = g_steal_pointer(&key); + priv->masterKeyLen = QEMU_DOMAIN_MASTER_KEY_LEN; return 0; } -- 2.29.2

Convert the conditions to else if so that it's obvious that only one of the cases will ever be used. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- tools/virsh-secret.c | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/tools/virsh-secret.c b/tools/virsh-secret.c index 16accc8ad2..5d656151e8 100644 --- a/tools/virsh-secret.c +++ b/tools/virsh-secret.c @@ -225,16 +225,10 @@ cmdSecretSetValue(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptStringReq(ctl, cmd, "file", &filename) < 0) return false; - if (!base64 && !filename && !interactive) { - vshError(ctl, _("Input secret value is missing")); - return false; - } - - /* warn users that the --base64 option passed from command line is wrong */ - if (base64) + if (base64) { + /* warn users that the --base64 option passed from command line is wrong */ vshError(ctl, _("Passing secret value as command-line argument is insecure!")); - - if (filename) { + } else if (filename) { ssize_t read_ret; if ((read_ret = virFileReadAll(filename, 1024, &file_buf)) < 0) { vshSaveLibvirtError(); @@ -243,9 +237,7 @@ cmdSecretSetValue(vshControl *ctl, const vshCmd *cmd) file_len = read_ret; base64 = file_buf; - } - - if (interactive) { + } else if (interactive) { vshPrint(ctl, "%s", _("Enter new value for secret:")); fflush(stdout); @@ -255,6 +247,9 @@ cmdSecretSetValue(vshControl *ctl, const vshCmd *cmd) } file_len = strlen(file_buf); plain = true; + } else { + vshError(ctl, _("Input secret value is missing")); + return false; } if (plain) { -- 2.29.2

Switch the secret value to 'g_autofree' for handling of the memory and clear it out using memset. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- tools/virsh-secret.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tools/virsh-secret.c b/tools/virsh-secret.c index 23bbd61698..252219c075 100644 --- a/tools/virsh-secret.c +++ b/tools/virsh-secret.c @@ -302,7 +302,7 @@ cmdSecretGetValue(vshControl *ctl, const vshCmd *cmd) { g_autoptr(virshSecret) secret = NULL; VIR_AUTODISPOSE_STR base64 = NULL; - unsigned char *value; + g_autofree unsigned char *value = NULL; size_t value_size; bool plain = vshCommandOptBool(cmd, "plain"); @@ -314,7 +314,7 @@ cmdSecretGetValue(vshControl *ctl, const vshCmd *cmd) if (plain) { if (fwrite(value, 1, value_size, stdout) != value_size) { - VIR_DISPOSE_N(value, value_size); + memset(value, 0, value_size); vshError(ctl, "failed to write secret"); return false; } @@ -324,7 +324,7 @@ cmdSecretGetValue(vshControl *ctl, const vshCmd *cmd) vshPrint(ctl, "%s", base64); } - VIR_DISPOSE_N(value, value_size); + memset(value, 0, value_size); return true; } -- 2.29.2

Clear out the value using an explicit memset and free it with g_free so that VIR_DISPOSE_N can be phased out. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/util/vircrypto.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/util/vircrypto.c b/src/util/vircrypto.c index c4874550af..82281a070a 100644 --- a/src/util/vircrypto.c +++ b/src/util/vircrypto.c @@ -206,7 +206,8 @@ virCryptoEncryptDataAESgnutls(gnutls_cipher_algorithm_t gnutls_enc_alg, return 0; error: - VIR_DISPOSE_N(ciphertext, ciphertextlen); + memset(ciphertext, 0, ciphertextlen); + g_free(ciphertext); memset(&enc_key, 0, sizeof(gnutls_datum_t)); memset(&iv_buf, 0, sizeof(gnutls_datum_t)); return -1; -- 2.29.2

The VIR_DISPOSE* APIs will be phased out. Additionally the test isn't really doing useful work in ensuring that the values are indeed cleared thus there's no point in keeping it around. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- tests/viralloctest.c | 34 ---------------------------------- 1 file changed, 34 deletions(-) diff --git a/tests/viralloctest.c b/tests/viralloctest.c index 8ea98b8eca..0867be8ce1 100644 --- a/tests/viralloctest.c +++ b/tests/viralloctest.c @@ -312,38 +312,6 @@ testInsertArray(const void *opaque G_GNUC_UNUSED) } -static int -testDispose(const void *opaque G_GNUC_UNUSED) -{ - int *num = NULL; - int *nums = NULL; - size_t nnums = 0; - char *str = NULL; - - VIR_DISPOSE(num); - VIR_DISPOSE_N(nums, nnums); - VIR_DISPOSE_STRING(str); - - nnums = 10; - VIR_DISPOSE_N(nums, nnums); - - num = g_new0(int, 1); - - VIR_DISPOSE(num); - - nnums = 10; - nums = g_new0(int, nnums); - - VIR_DISPOSE_N(nums, nnums); - - str = g_strdup("test"); - - VIR_DISPOSE_STRING(str); - - return 0; -} - - static int mymain(void) { @@ -357,8 +325,6 @@ mymain(void) ret = -1; if (virTestRun("insert array", testInsertArray, NULL) < 0) ret = -1; - if (virTestRun("dispose tests", testDispose, NULL) < 0) - ret = -1; return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.29.2