[PATCH 00/14] util: Remove VIR_DISPOSE(_N)
Most callers are way better off using memset directly additionally few places didn't even use it to clear sensitive data in the first place since the name probably sounded as the right thing to use. Peter Krempa (14): hypervFreeInvokeParams: Don't use VIR_DISPOSE_N for freeing 'params' libxlMakeDomBuildInfo: Don't use VIR_DISPOSE_N for USB device list storage_backend_iscsi(_direct): Properly clear secrets libxlMakeNetworkDiskSrc: Avoid use of VIR_DISPOSE_N qemuDomainMasterKeyCreate: Don't use VIR_DISPOSE_N on failure qemu: domain: Use memset for clearing secrets instead of VIR_DISPOSE_N cmdSecretSetValue: Make it obvious that --file, --base64 and --interactive are exlcusive virsh: cmdSecretSetValue: Rework handling of the secret value virsh: cmdSecretGetValue: Use memset instead of VIR_DISPOSE_N virStorageBackendRBDOpenRADOSConn: Use memset instead of VIR_DISPOSE_N virCryptoEncryptDataAESgnutls: Use memset instead of VIR_DISPOSE_N storageBackendCreateQemuImgSecretPath: Use memset instead of VIR_DISPOSE_N tests: viralloc: Remove testDispose case util: viralloc: Remove VIR_DISPOSE(_N) src/hyperv/hyperv_wmi.c | 4 +- src/libvirt_private.syms | 1 - src/libxl/libxl_conf.c | 8 +-- src/qemu/qemu_domain.c | 24 ++++++--- src/storage/storage_backend_iscsi.c | 16 +++--- src/storage/storage_backend_iscsi_direct.c | 17 ++++--- src/storage/storage_backend_rbd.c | 5 +- src/storage/storage_util.c | 5 +- src/util/viralloc.c | 39 +------------- src/util/viralloc.h | 27 ---------- src/util/vircrypto.c | 3 +- tests/viralloctest.c | 34 ------------- tools/virsh-secret.c | 59 ++++++++++------------ 13 files changed, 76 insertions(+), 166 deletions(-) -- 2.29.2
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@redhat.com> --- 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 list isn't secret which would need being disposed of. Just expand the array and return failure when adding the NULL terminator similarly to how we expand the list for adding devices in a loop. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libxl/libxl_conf.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 6a8ae27f54..cb1fd7df7d 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -686,7 +686,6 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, /* NULL-terminate usbdevice_list */ if (nusbdevice > 0 && VIR_EXPAND_N(b_info->u.hvm.usbdevice_list, nusbdevice, 1) < 0) { - VIR_DISPOSE_N(b_info->u.hvm.usbdevice_list, nusbdevice); return -1; } #endif -- 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@redhat.com> --- 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
Clear the secret right after use with memset. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libxl/libxl_conf.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index cb1fd7df7d..b2fcb21324 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -998,14 +998,15 @@ static int libxlMakeNetworkDiskSrc(virStorageSourcePtr src, char **srcstr) { virConnectPtr conn = NULL; - uint8_t *secret = NULL; VIR_AUTODISPOSE_STR base64secret = NULL; - size_t secretlen = 0; char *username = NULL; int ret = -1; *srcstr = NULL; if (src->auth && src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD) { + g_autofree uint8_t *secret = NULL; + size_t secretlen = 0; + username = src->auth->username; if (!(conn = virConnectOpen("xen:///system"))) goto cleanup; @@ -1017,6 +1018,7 @@ libxlMakeNetworkDiskSrc(virStorageSourcePtr src, char **srcstr) /* RBD expects an encoded secret */ base64secret = g_base64_encode(secret, secretlen); + memset(secret, 0, secretlen); } if (!(*srcstr = libxlMakeNetworkDiskSrcStr(src, username, base64secret))) @@ -1025,7 +1027,6 @@ libxlMakeNetworkDiskSrc(virStorageSourcePtr src, char **srcstr) ret = 0; cleanup: - VIR_DISPOSE_N(secret, secretlen); 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@redhat.com> --- 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
Phase out use of VIR_DISPOSE_N from the qemu driver. Use memset in the appropriate cases. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_domain.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 2c34307c82..7f3f704bf4 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -443,7 +443,10 @@ qemuDomainMasterKeyFree(qemuDomainObjPrivatePtr priv) if (!priv->masterKey) return; - VIR_DISPOSE_N(priv->masterKey, priv->masterKeyLen); + if (priv->masterKey) { + memset(priv->masterKey, 0, priv->masterKeyLen); + g_clear_pointer(&priv->masterKey, g_free); + } } /* qemuDomainMasterKeyReadFile: @@ -584,7 +587,10 @@ static void qemuDomainSecretPlainClear(qemuDomainSecretPlainPtr secret) { VIR_FREE(secret->username); - VIR_DISPOSE_N(secret->secret, secret->secretlen); + if (secret->secret) { + memset(secret->secret, 0, secret->secretlen); + g_clear_pointer(&secret->secret, g_free); + } } @@ -1143,7 +1149,8 @@ qemuDomainSecretAESSetupFromSecret(qemuDomainObjPrivatePtr priv, secinfo = qemuDomainSecretAESSetup(priv, alias, username, secret, secretlen); - VIR_DISPOSE_N(secret, secretlen); + memset(secret, 0, secretlen); + g_free(secret); return secinfo; } -- 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@redhat.com> --- 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
Use a single buffer for the secret to make it easier to follow it's lifecycle. For base64 decoding use a local temporary buffer which will be cleared right away. This also uses memset for clearing the bufer instead of VIR_DISPOSE_N which is being phased out. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-secret.c | 34 ++++++++++++++++------------------ 1 file changed, 16 insertions(+), 18 deletions(-) diff --git a/tools/virsh-secret.c b/tools/virsh-secret.c index 5d656151e8..23bbd61698 100644 --- a/tools/virsh-secret.c +++ b/tools/virsh-secret.c @@ -202,10 +202,8 @@ cmdSecretSetValue(vshControl *ctl, const vshCmd *cmd) g_autoptr(virshSecret) secret = NULL; const char *base64 = NULL; const char *filename = NULL; - char *file_buf = NULL; - size_t file_len = 0; - unsigned char *value; - size_t value_size; + g_autofree char *secret_val = NULL; + size_t secret_len = 0; bool plain = vshCommandOptBool(cmd, "plain"); bool interactive = vshCommandOptBool(cmd, "interactive"); int res; @@ -228,41 +226,41 @@ cmdSecretSetValue(vshControl *ctl, const vshCmd *cmd) 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!")); + secret_val = g_strdup(base64); + secret_len = strlen(secret_val); } else if (filename) { ssize_t read_ret; - if ((read_ret = virFileReadAll(filename, 1024, &file_buf)) < 0) { + if ((read_ret = virFileReadAll(filename, 1024, &secret_val)) < 0) { vshSaveLibvirtError(); return false; } - file_len = read_ret; - base64 = file_buf; + secret_len = read_ret; } else if (interactive) { vshPrint(ctl, "%s", _("Enter new value for secret:")); fflush(stdout); - if (!(file_buf = virGetPassword())) { + if (!(secret_val = virGetPassword())) { vshError(ctl, "%s", _("Failed to read secret")); return false; } - file_len = strlen(file_buf); + secret_len = strlen(secret_val); plain = true; } else { vshError(ctl, _("Input secret value is missing")); return false; } - if (plain) { - value = g_steal_pointer(&file_buf); - value_size = file_len; - file_len = 0; - } else { - value = g_base64_decode(base64, &value_size); + if (!plain) { + g_autofree char *tmp = g_steal_pointer(&secret_val); + size_t tmp_len = secret_len; + + secret_val = (char *) g_base64_decode(tmp, &secret_len); + memset(tmp, 0, tmp_len); } - res = virSecretSetValue(secret, value, value_size, 0); - VIR_DISPOSE_N(value, value_size); - VIR_DISPOSE_N(file_buf, file_len); + res = virSecretSetValue(secret, (unsigned char *) secret_val, secret_len, 0); + memset(secret_val, 0, secret_len); if (res != 0) { vshError(ctl, "%s", _("Failed to set secret value")); -- 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@redhat.com> --- 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
Switch the secret value to 'g_autofree' for handling of the memory and clear it out using memset. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/storage/storage_backend_rbd.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index 22f5c78591..5af6136c4a 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -185,7 +185,7 @@ virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr, int ret = -1; virStoragePoolSourcePtr source = &def->source; virStorageAuthDefPtr authdef = source->auth; - unsigned char *secret_value = NULL; + g_autofree unsigned char *secret_value = NULL; size_t secret_value_size = 0; VIR_AUTODISPOSE_STR rados_key = NULL; g_auto(virBuffer) mon_host = VIR_BUFFER_INITIALIZER; @@ -215,6 +215,7 @@ virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr, goto cleanup; rados_key = g_base64_encode(secret_value, secret_value_size); + memset(secret_value, 0, secret_value_size); if (virStorageBackendRBDRADOSConfSet(ptr->cluster, "key", rados_key) < 0) @@ -325,8 +326,6 @@ virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr, ret = 0; cleanup: - VIR_DISPOSE_N(secret_value, secret_value_size); - virObjectUnref(conn); return ret; } -- 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@redhat.com> --- 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
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@redhat.com> --- src/storage/storage_util.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index 3d8de16341..303ba5fa70 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -1314,7 +1314,10 @@ storageBackendCreateQemuImgSecretPath(virStoragePoolObjPtr pool, cleanup: virObjectUnref(conn); - VIR_DISPOSE_N(secret, secretlen); + if (secret) { + memset(secret, 0, secretlen); + g_free(secret); + } return secretPath; -- 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@redhat.com> --- 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
The macros are unused now and callers who care about clearing the memory they use should use memset() appropriately. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 - src/util/viralloc.c | 39 ++------------------------------------- src/util/viralloc.h | 27 --------------------------- 3 files changed, 2 insertions(+), 65 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 8138780237..45e89eba3c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1726,7 +1726,6 @@ vir_g_strdup_vprintf; # util/viralloc.h virAllocVar; virDeleteElementsN; -virDispose; virDisposeString; virExpandN; virInsertElementsN; diff --git a/src/util/viralloc.c b/src/util/viralloc.c index 0360b8a8aa..036007cb53 100644 --- a/src/util/viralloc.c +++ b/src/util/viralloc.c @@ -295,42 +295,6 @@ int virAllocVar(void *ptrptr, } -/** - * virDispose: - * @ptrptr: pointer to pointer for address of memory to be sanitized and freed - * @count: count of elements in the array to dispose - * @element_size: size of one element - * @countptr: pointer to the count variable to clear (may be NULL) - * - * Clear and release the chunk of memory in the pointer pointed to by 'prtptr'. - * - * If @countptr is provided, it's value is used instead of @count and it's set - * to 0 after clearing and freeing the memory. - * - * After release, 'ptrptr' will be updated to point to NULL. - */ -void virDispose(void *ptrptr, - size_t count, - size_t element_size, - size_t *countptr) -{ - int save_errno = errno; - - if (countptr) - count = *countptr; - - if (*(void**)ptrptr && count > 0) - memset(*(void **)ptrptr, 0, count * element_size); - - g_free(*(void**)ptrptr); - *(void**)ptrptr = NULL; - - if (countptr) - *countptr = 0; - errno = save_errno; -} - - /** * virDisposeString: * @ptrptr: pointer to pointer for a string which should be sanitized and cleared @@ -343,5 +307,6 @@ virDisposeString(char **strptr) if (!*strptr) return; - virDispose(strptr, strlen(*strptr), sizeof(char), NULL); + memset(*strptr, 0, strlen(*strptr)); + g_clear_pointer(strptr, g_free); } diff --git a/src/util/viralloc.h b/src/util/viralloc.h index 1abd94fac4..0173107b87 100644 --- a/src/util/viralloc.h +++ b/src/util/viralloc.h @@ -52,8 +52,6 @@ int virDeleteElementsN(void *ptrptr, size_t size, size_t at, size_t *countptr, int virAllocVar(void *ptrptr, size_t struct_size, size_t element_size, size_t count) G_GNUC_WARN_UNUSED_RESULT ATTRIBUTE_NONNULL(1); -void virDispose(void *ptrptr, size_t count, size_t element_size, size_t *countptr) - ATTRIBUTE_NONNULL(1); void virDisposeString(char **strptr) ATTRIBUTE_NONNULL(1); @@ -342,20 +340,6 @@ void virDisposeString(char **strptr) #define VIR_FREE(ptr) g_clear_pointer(&(ptr), g_free) -/** - * VIR_DISPOSE_N: - * @ptr: pointer holding address to be cleared and freed - * @count: count of elements in @ptr - * - * Clear the memory of the array of elements pointed to by 'ptr' of 'count' - * elements and free it. Update the pointer/count to NULL/0. - * - * This macro is safe to use on arguments with side effects. - */ -#define VIR_DISPOSE_N(ptr, count) virDispose(1 ? (void *) &(ptr) : (ptr), 0, \ - sizeof(*(ptr)), &(count)) - - /** * VIR_DISPOSE_STRING: * @ptr: pointer to a string to be cleared and freed @@ -375,14 +359,3 @@ void virDisposeString(char **strptr) */ #define VIR_AUTODISPOSE_STR \ __attribute__((cleanup(virDisposeString))) char * - -/** - * VIR_DISPOSE: - * @ptr: pointer to memory to be cleared and freed - * - * Clears and frees the corresponding memory. - * - * This macro is safe to be used on arguments with side effects. - */ -#define VIR_DISPOSE(ptr) virDispose(1 ? (void *) &(ptr) : (ptr), 1, \ - sizeof(*(ptr)), NULL) -- 2.29.2
On Mon, Feb 01, 2021 at 02:38:52PM +0100, Peter Krempa wrote:
Most callers are way better off using memset directly additionally few places didn't even use it to clear sensitive data in the first place since the name probably sounded as the right thing to use.
Although virDispose did indeed use memset(), I don't think we should be replacing it with use of memset(). This is well known to be subject to compiler optimization eliminating the call entirely. We shouldn't have used it in virDispose in the first place, instead we need to call the platform specific "safe" method for erasing data. Istead we ought to have been using explicit_bzero or memset_s(), or memset_explicitly, or $whatever. At least with virDispose we would only have one place to fix this problem, but this with series eliminating it, the callers that need the secure erase are no longer distinct/visible from general memset usage. I think we ought to have a 'virSecureErase' function, that we can back with the appropriate platform specific call. If you don't want to get so deeply involved in that, I'd be fine if this series too a minimialist approach and only introduced #define virSecureErase(ptr, len) memset(ptr, 0, len) and then used virSecureErase intead of memset(). That would at least make sure we're no worse than today and callers remain easily identifiable. Actually checking for the platform specific secure erase functions and wiring them up could be a separate patch series at a later time. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Mon, Feb 01, 2021 at 13:52:11 +0000, Daniel Berrange wrote:
On Mon, Feb 01, 2021 at 02:38:52PM +0100, Peter Krempa wrote:
Most callers are way better off using memset directly additionally few places didn't even use it to clear sensitive data in the first place since the name probably sounded as the right thing to use.
Although virDispose did indeed use memset(), I don't think we should be replacing it with use of memset(). This is well known to be subject to compiler optimization eliminating the call entirely.
We shouldn't have used it in virDispose in the first place, instead we need to call the platform specific "safe" method for erasing data. Istead we ought to have been using explicit_bzero or memset_s(), or memset_explicitly, or $whatever.
At least with virDispose we would only have one place to fix this problem, but this with series eliminating it, the callers that need the secure erase are no longer distinct/visible from general memset usage.
I think we ought to have a 'virSecureErase' function, that we can back with the appropriate platform specific call.
If you don't want to get so deeply involved in that, I'd be fine if this series too a minimialist approach and only introduced
#define virSecureErase(ptr, len) memset(ptr, 0, len)
and then used virSecureErase intead of memset(). That would at least make sure we're no worse than today and callers remain easily identifiable.
I will do that but it's worth mentioning that it might give us a false sense of security since there's a lot of memset usage (prior to this series, one place is visible in 11/14, where the encryption key used with gnutls is cleared memset) thus any series wanting to do something else than memset in virSecureErase will need to go through all of memset calls anyways.
participants (2)
-
Daniel P. Berrangé -
Peter Krempa