[PATCH v2 00/27] util: Remove VIR_DISPOSE(_N) and VIR_DISPOSE_STRING

Patches 1-6 are pure refactors, other patches then convert handling to the newly introduced functions. Unfortunately quite a lot of the supposedly "secure" handling of secrets isn't really secure as we e.g. copy the secret into another buffer which isn't cleared properly or format it directly onto the commadline ... I've kept them so that they are still marked as secure despite the handling being pointless. v2: - use a new wrapper virSecureErase to mark places really needing secure handling instead of just plain memset (but the wrapper still uses memset) - converted other memset calls to virSecureErase in virCryptoEncryptDataAESgnutls, probably the only function worthy of this change - added patches removing the string disposal code too Peter Krempa (27): hypervFreeInvokeParams: Don't use VIR_DISPOSE_N for freeing 'params' libxlMakeDomBuildInfo: Don't use VIR_DISPOSE_N for USB device list qemuDomainMasterKeyCreate: Don't use VIR_DISPOSE_N on failure cmdSecretSetValue: Make it obvious that --file, --base64 and --interactive are exlcusive virNetLibsshSessionAuthAddPrivKeyAuth: Don't unlock unlocked 'sess' on error virNetLibsshSessionAuthAddPrivKeyAuth: Refactor cleanup util: Introduce virsecureerase module virsh: cmdSecretSetValue: Rework handling of the secret value storage_backend_iscsi(_direct): Properly clear secrets libxlMakeNetworkDiskSrc: Avoid use of VIR_DISPOSE_N qemu: domain: Use virSecureErase for clearing secrets instead of VIR_DISPOSE_N virsh: cmdSecretGetValue: Use virSecureErase instead of VIR_DISPOSE_N virStorageBackendRBDOpenRADOSConn: Use virSecureErase instead of VIR_DISPOSE_N virCryptoEncryptDataAESgnutls: Use virSecureErase instead of VIR_DISPOSE_N virCryptoEncryptDataAESgnutls: Use virSecureErase instead of memset storageBackendCreateQemuImgSecretPath: Use virSecureErase instead of VIR_DISPOSE_N tests: viralloc: Remove testDispose case util: viralloc: Remove VIR_DISPOSE(_N) util: virsecureerase: Introduce virSecureEraseString libxlMakeNetworkDiskSrc: Use virSecureEraseString instead of VIR_AUTODISPOSE_STR qemuBuildRBDSecinfoURI: Use virSecureEraseString instead of VIR_AUTODISPOSE_STR virStorageBackendRBDOpenRADOSConn: Use virSecureEraseString instead of VIR_AUTODISPOSE_STR cmdSecretGetValue: Use virSecureEraseString instead of VIR_AUTODISPOSE_STR virNetLibsshAuthenticatePassword: Use virSecureEraseString instead of VIR_AUTODISPOSE_STR util: alloc: Remove VIR_AUTODISPOSE_STR virnetlibsshsession: Replace VIR_DISPOSE_STRING with virSecureEraseString util: alloc: Remove VIR_DISPOSE_STRING src/hyperv/hyperv_wmi.c | 4 +- src/libvirt_private.syms | 7 ++- src/libxl/libxl_conf.c | 16 ++++-- src/qemu/qemu_command.c | 4 +- src/qemu/qemu_domain.c | 22 +++++--- src/rpc/virnetlibsshsession.c | 50 +++++++---------- src/storage/storage_backend_iscsi.c | 16 +++--- src/storage/storage_backend_iscsi_direct.c | 17 +++--- src/storage/storage_backend_rbd.c | 16 ++++-- src/storage/storage_util.c | 4 +- src/util/meson.build | 1 + src/util/viralloc.c | 52 ------------------ src/util/viralloc.h | 51 ----------------- src/util/vircrypto.c | 12 ++-- src/util/virsecureerase.c | 57 +++++++++++++++++++ src/util/virsecureerase.h | 28 ++++++++++ tests/viralloctest.c | 34 ------------ tools/virsh-secret.c | 64 ++++++++++------------ 18 files changed, 206 insertions(+), 249 deletions(-) create mode 100644 src/util/virsecureerase.c create mode 100644 src/util/virsecureerase.h -- 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 c14ff0e64a..8bb6f591f1 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

On Tue, Feb 02, 2021 at 05:55:38PM +0100, Peter Krempa wrote:
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(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|

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

On Tue, Feb 02, 2021 at 05:55:39PM +0100, Peter Krempa wrote:
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(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|

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

On Tue, Feb 02, 2021 at 05:55:40PM +0100, Peter Krempa wrote:
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(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|

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

On Tue, Feb 02, 2021 at 05:55:41PM +0100, Peter Krempa wrote:
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(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|

The check whether @keyfile is non-NULL is before locking @sess, but uses the 'error' label which unlocks '@sess'. While touching the error path, update the error message to be on one line. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/rpc/virnetlibsshsession.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/rpc/virnetlibsshsession.c b/src/rpc/virnetlibsshsession.c index 959a16a6a9..ed697c7ce4 100644 --- a/src/rpc/virnetlibsshsession.c +++ b/src/rpc/virnetlibsshsession.c @@ -1020,10 +1020,8 @@ virNetLibsshSessionAuthAddPrivKeyAuth(virNetLibsshSessionPtr sess, if (!keyfile) { virReportError(VIR_ERR_LIBSSH, "%s", - _("Key file path must be provided " - "for private key authentication")); - ret = -1; - goto error; + _("Key file path must be provided for private key authentication")); + return -1; } virObjectLock(sess); -- 2.29.2

On Tue, Feb 02, 2021 at 05:55:42PM +0100, Peter Krempa wrote:
The check whether @keyfile is non-NULL is before locking @sess, but uses the 'error' label which unlocks '@sess'.
While touching the error path, update the error message to be on one line.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/rpc/virnetlibsshsession.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|

Shuffle the code around to remove the need for temporary variables and labels for cleaning them. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/rpc/virnetlibsshsession.c | 23 +++++------------------ 1 file changed, 5 insertions(+), 18 deletions(-) diff --git a/src/rpc/virnetlibsshsession.c b/src/rpc/virnetlibsshsession.c index ed697c7ce4..9671a0f98d 100644 --- a/src/rpc/virnetlibsshsession.c +++ b/src/rpc/virnetlibsshsession.c @@ -1013,10 +1013,7 @@ virNetLibsshSessionAuthAddPrivKeyAuth(virNetLibsshSessionPtr sess, const char *keyfile, const char *password) { - int ret; virNetLibsshAuthMethodPtr auth; - VIR_AUTODISPOSE_STR pass = NULL; - char *file = NULL; if (!keyfile) { virReportError(VIR_ERR_LIBSSH, "%s", @@ -1026,28 +1023,18 @@ virNetLibsshSessionAuthAddPrivKeyAuth(virNetLibsshSessionPtr sess, virObjectLock(sess); - file = g_strdup(keyfile); - pass = g_strdup(password); - if (!(auth = virNetLibsshSessionAuthMethodNew(sess))) { - ret = -1; - goto error; + virObjectUnlock(sess); + return -1; } - auth->password = g_steal_pointer(&pass); - auth->filename = file; + auth->password = g_strdup(password); + auth->filename = g_strdup(keyfile); auth->method = VIR_NET_LIBSSH_AUTH_PRIVKEY; auth->ssh_flags = SSH_AUTH_METHOD_PUBLICKEY; - ret = 0; - - cleanup: virObjectUnlock(sess); - return ret; - - error: - VIR_FREE(file); - goto cleanup; + return 0; } int -- 2.29.2

On Tue, Feb 02, 2021 at 05:55:43PM +0100, Peter Krempa wrote:
Shuffle the code around to remove the need for temporary variables and labels for cleaning them.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/rpc/virnetlibsshsession.c | 23 +++++------------------ 1 file changed, 5 insertions(+), 18 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|

The module will provide functions for disposing secrets stored in memory. Note that for now it's implemented using memset, which is not really secure. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 4 ++++ src/util/meson.build | 1 + src/util/virsecureerase.c | 44 +++++++++++++++++++++++++++++++++++++++ src/util/virsecureerase.h | 25 ++++++++++++++++++++++ 4 files changed, 74 insertions(+) create mode 100644 src/util/virsecureerase.c create mode 100644 src/util/virsecureerase.h diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 8138780237..fa0c0887e9 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3175,6 +3175,10 @@ virSecretLookupFormatSecret; virSecretLookupParseSecret; +# util/virsecureerase.h +virSecureErase; + + # util/virsocket.h virSocketRecvFD; virSocketSendFD; diff --git a/src/util/meson.build b/src/util/meson.build index c077c5cc99..e89d32c33d 100644 --- a/src/util/meson.build +++ b/src/util/meson.build @@ -86,6 +86,7 @@ util_sources = [ 'virscsivhost.c', 'virseclabel.c', 'virsecret.c', + 'virsecureerase.c', 'virsocket.c', 'virsocketaddr.c', 'virstoragefile.c', diff --git a/src/util/virsecureerase.c b/src/util/virsecureerase.c new file mode 100644 index 0000000000..1dc3bb476a --- /dev/null +++ b/src/util/virsecureerase.c @@ -0,0 +1,44 @@ +/* + * virsecureerase.c: Secure clearing of memory + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + */ + +#include <config.h> + +#include "virsecureerase.h" + +/** + * virSecureErase: + * @ptr: pointer to memory to clear + * @size: size of memory to clear + * + * Clear @size bytes of memory at @ptr. + * + * Note that for now this is implemented using memset which is not secure as + * it can be optimized out. + * + * Also note that there are possible leftover direct uses of memset. + */ +void +virSecureErase(void *ptr, + size_t size) +{ + if (!ptr || size == 0) + return; + + memset(ptr, 0, size); +} diff --git a/src/util/virsecureerase.h b/src/util/virsecureerase.h new file mode 100644 index 0000000000..66d7e28e8a --- /dev/null +++ b/src/util/virsecureerase.h @@ -0,0 +1,25 @@ +/* + * virsecureerase.h: Secure clearing of memory + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + * + */ + +#pragma once + +#include "internal.h" + +void +virSecureErase(void *ptr, size_t size); -- 2.29.2

On Tue, Feb 02, 2021 at 05:55:44PM +0100, Peter Krempa wrote:
The module will provide functions for disposing secrets stored in memory.
Note that for now it's implemented using memset, which is not really secure.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 4 ++++ src/util/meson.build | 1 + src/util/virsecureerase.c | 44 +++++++++++++++++++++++++++++++++++++++ src/util/virsecureerase.h | 25 ++++++++++++++++++++++ 4 files changed, 74 insertions(+) create mode 100644 src/util/virsecureerase.c create mode 100644 src/util/virsecureerase.h
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|

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 virSecureErase 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 | 35 +++++++++++++++++------------------ 1 file changed, 17 insertions(+), 18 deletions(-) diff --git a/tools/virsh-secret.c b/tools/virsh-secret.c index 5d656151e8..e413af893f 100644 --- a/tools/virsh-secret.c +++ b/tools/virsh-secret.c @@ -31,6 +31,7 @@ #include "virtime.h" #include "vsh-table.h" #include "virenum.h" +#include "virsecureerase.h" static virSecretPtr virshCommandOptSecret(vshControl *ctl, const vshCmd *cmd, const char **name) @@ -202,10 +203,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 +227,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); + virSecureErase(tmp, 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); + virSecureErase(secret_val, secret_len); if (res != 0) { vshError(ctl, "%s", _("Failed to set secret value")); -- 2.29.2

On Tue, Feb 02, 2021 at 05:55:45PM +0100, Peter Krempa wrote:
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 virSecureErase 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 | 35 +++++++++++++++++------------------ 1 file changed, 17 insertions(+), 18 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|

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 virSecureErase instead 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..9127d663b1 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -41,6 +41,7 @@ #include "virsecret.h" #include "storage_util.h" #include "virutil.h" +#include "virsecureerase.h" #define VIR_FROM_THIS VIR_FROM_STORAGE @@ -256,8 +257,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 +284,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); + virSecureErase(secret_value, secret_size); + secret_str[secret_size] = '\0'; if (virISCSINodeUpdate(portal, source->devices[0].path, @@ -298,13 +300,13 @@ 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); + virSecureErase(secret_str, 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

On Tue, Feb 02, 2021 at 05:55:46PM +0100, Peter Krempa wrote:
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 virSecureErase instead 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_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);
virSecureErase surely ? Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|

Clear the secret right after use with virSecureErase. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libxl/libxl_conf.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index cb1fd7df7d..694192e1c3 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -46,6 +46,7 @@ #include "xen_xl.h" #include "virnetdevvportprofile.h" #include "virenum.h" +#include "virsecureerase.h" #define VIR_FROM_THIS VIR_FROM_LIBXL @@ -998,14 +999,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 +1019,7 @@ libxlMakeNetworkDiskSrc(virStorageSourcePtr src, char **srcstr) /* RBD expects an encoded secret */ base64secret = g_base64_encode(secret, secretlen); + virSecureErase(secret, secretlen); } if (!(*srcstr = libxlMakeNetworkDiskSrcStr(src, username, base64secret))) @@ -1025,7 +1028,6 @@ libxlMakeNetworkDiskSrc(virStorageSourcePtr src, char **srcstr) ret = 0; cleanup: - VIR_DISPOSE_N(secret, secretlen); virObjectUnref(conn); return ret; } -- 2.29.2

On Tue, Feb 02, 2021 at 05:55:47PM +0100, Peter Krempa wrote:
Clear the secret right after use with virSecureErase.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libxl/libxl_conf.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|

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 | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 2c34307c82..e60f814e36 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -67,6 +67,7 @@ #include "backup_conf.h" #include "virutil.h" #include "virqemu.h" +#include "virsecureerase.h" #include <sys/time.h> #include <fcntl.h> @@ -443,7 +444,8 @@ qemuDomainMasterKeyFree(qemuDomainObjPrivatePtr priv) if (!priv->masterKey) return; - VIR_DISPOSE_N(priv->masterKey, priv->masterKeyLen); + virSecureErase(priv->masterKey, priv->masterKeyLen); + g_clear_pointer(&priv->masterKey, g_free); } /* qemuDomainMasterKeyReadFile: @@ -584,7 +586,8 @@ static void qemuDomainSecretPlainClear(qemuDomainSecretPlainPtr secret) { VIR_FREE(secret->username); - VIR_DISPOSE_N(secret->secret, secret->secretlen); + virSecureErase(secret->secret, secret->secretlen); + g_clear_pointer(&secret->secret, g_free); } @@ -1131,7 +1134,7 @@ qemuDomainSecretAESSetupFromSecret(qemuDomainObjPrivatePtr priv, g_autoptr(virConnect) conn = virGetConnectSecret(); qemuDomainSecretInfoPtr secinfo; g_autofree char *alias = qemuAliasForSecret(srcalias, secretuse); - uint8_t *secret = NULL; + g_autofree uint8_t *secret = NULL; size_t secretlen = 0; if (!conn) @@ -1143,7 +1146,7 @@ qemuDomainSecretAESSetupFromSecret(qemuDomainObjPrivatePtr priv, secinfo = qemuDomainSecretAESSetup(priv, alias, username, secret, secretlen); - VIR_DISPOSE_N(secret, secretlen); + virSecureErase(secret, secretlen); return secinfo; } -- 2.29.2

On Tue, Feb 02, 2021 at 05:55:48PM +0100, Peter Krempa wrote:
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 | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|

Switch the secret value to 'g_autofree' for handling of the memory and clear it out using virSecureErase. 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 e413af893f..de32f25d64 100644 --- a/tools/virsh-secret.c +++ b/tools/virsh-secret.c @@ -303,7 +303,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"); @@ -315,7 +315,7 @@ cmdSecretGetValue(vshControl *ctl, const vshCmd *cmd) if (plain) { if (fwrite(value, 1, value_size, stdout) != value_size) { - VIR_DISPOSE_N(value, value_size); + virSecureErase(value, value_size); vshError(ctl, "failed to write secret"); return false; } @@ -325,7 +325,7 @@ cmdSecretGetValue(vshControl *ctl, const vshCmd *cmd) vshPrint(ctl, "%s", base64); } - VIR_DISPOSE_N(value, value_size); + virSecureErase(value, value_size); return true; } -- 2.29.2

On Tue, Feb 02, 2021 at 05:55:49PM +0100, Peter Krempa wrote:
Switch the secret value to 'g_autofree' for handling of the memory and clear it out using virSecureErase.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-secret.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|

Switch the secret value to 'g_autofree' for handling of the memory and clear it out using virSecureErase. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/storage/storage_backend_rbd.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index 22f5c78591..1f83205dfa 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -35,6 +35,7 @@ #include "rbd/librbd.h" #include "virsecret.h" #include "storage_util.h" +#include "virsecureerase.h" #define VIR_FROM_THIS VIR_FROM_STORAGE @@ -185,7 +186,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 +216,7 @@ virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr, goto cleanup; rados_key = g_base64_encode(secret_value, secret_value_size); + virSecureErase(secret_value, secret_value_size); if (virStorageBackendRBDRADOSConfSet(ptr->cluster, "key", rados_key) < 0) @@ -325,8 +327,6 @@ virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr, ret = 0; cleanup: - VIR_DISPOSE_N(secret_value, secret_value_size); - virObjectUnref(conn); return ret; } -- 2.29.2

On Tue, Feb 02, 2021 at 05:55:50PM +0100, Peter Krempa wrote:
Switch the secret value to 'g_autofree' for handling of the memory and clear it out using virSecureErase.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/storage/storage_backend_rbd.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|

Clear out the value using virSecureErase 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 | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/util/vircrypto.c b/src/util/vircrypto.c index c4874550af..d2a42d83e2 100644 --- a/src/util/vircrypto.c +++ b/src/util/vircrypto.c @@ -25,6 +25,7 @@ #include "virerror.h" #include "viralloc.h" #include "virrandom.h" +#include "virsecureerase.h" #include <gnutls/gnutls.h> #include <gnutls/crypto.h> @@ -206,7 +207,8 @@ virCryptoEncryptDataAESgnutls(gnutls_cipher_algorithm_t gnutls_enc_alg, return 0; error: - VIR_DISPOSE_N(ciphertext, ciphertextlen); + virSecureErase(ciphertext, 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

On Tue, Feb 02, 2021 at 05:55:51PM +0100, Peter Krempa wrote:
Clear out the value using virSecureErase 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 | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|

Clear the key and IV structs using virSecureErase. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/vircrypto.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/util/vircrypto.c b/src/util/vircrypto.c index d2a42d83e2..78689721c3 100644 --- a/src/util/vircrypto.c +++ b/src/util/vircrypto.c @@ -193,8 +193,8 @@ virCryptoEncryptDataAESgnutls(gnutls_cipher_algorithm_t gnutls_enc_alg, /* Encrypt the data and free the memory for cipher operations */ rc = gnutls_cipher_encrypt(handle, ciphertext, ciphertextlen); gnutls_cipher_deinit(handle); - memset(&enc_key, 0, sizeof(gnutls_datum_t)); - memset(&iv_buf, 0, sizeof(gnutls_datum_t)); + virSecureErase(&enc_key, sizeof(gnutls_datum_t)); + virSecureErase(&iv_buf, sizeof(gnutls_datum_t)); if (rc < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("failed to encrypt the data: '%s'"), @@ -209,8 +209,8 @@ virCryptoEncryptDataAESgnutls(gnutls_cipher_algorithm_t gnutls_enc_alg, error: virSecureErase(ciphertext, ciphertextlen); g_free(ciphertext); - memset(&enc_key, 0, sizeof(gnutls_datum_t)); - memset(&iv_buf, 0, sizeof(gnutls_datum_t)); + virSecureErase(&enc_key, sizeof(gnutls_datum_t)); + virSecureErase(&iv_buf, sizeof(gnutls_datum_t)); return -1; } -- 2.29.2

On Tue, Feb 02, 2021 at 05:55:52PM +0100, Peter Krempa wrote:
Clear the key and IV structs using virSecureErase.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/vircrypto.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|

Clear out the value using virSecureErase 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 | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index 3d8de16341..b5adb05826 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -74,6 +74,7 @@ #include "virxml.h" #include "virfdstream.h" #include "virutil.h" +#include "virsecureerase.h" #define VIR_FROM_THIS VIR_FROM_STORAGE @@ -1314,7 +1315,8 @@ storageBackendCreateQemuImgSecretPath(virStoragePoolObjPtr pool, cleanup: virObjectUnref(conn); - VIR_DISPOSE_N(secret, secretlen); + virSecureErase(secret, secretlen); + g_free(secret); return secretPath; -- 2.29.2

On Tue, Feb 02, 2021 at 05:55:53PM +0100, Peter Krempa wrote:
Clear out the value using virSecureErase 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 | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|

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

On Tue, Feb 02, 2021 at 05:55:54PM +0100, Peter Krempa wrote:
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(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|

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 fa0c0887e9..62a7b8f7b9 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 Tue, Feb 02, 2021 at 05:55:55PM +0100, Peter Krempa wrote:
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(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|

Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virsecureerase.c | 13 +++++++++++++ src/util/virsecureerase.h | 3 +++ 3 files changed, 17 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 62a7b8f7b9..845e749bdf 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3176,6 +3176,7 @@ virSecretLookupParseSecret; # util/virsecureerase.h virSecureErase; +virSecureEraseString; # util/virsocket.h diff --git a/src/util/virsecureerase.c b/src/util/virsecureerase.c index 1dc3bb476a..ead12803da 100644 --- a/src/util/virsecureerase.c +++ b/src/util/virsecureerase.c @@ -42,3 +42,16 @@ virSecureErase(void *ptr, memset(ptr, 0, size); } + +/** + * virSecureEraseString: + * @str: String to securely erase + */ +void +virSecureEraseString(char *str) +{ + if (!str) + return; + + virSecureErase(str, strlen(str)); +} diff --git a/src/util/virsecureerase.h b/src/util/virsecureerase.h index 66d7e28e8a..7aa2f970f7 100644 --- a/src/util/virsecureerase.h +++ b/src/util/virsecureerase.h @@ -23,3 +23,6 @@ void virSecureErase(void *ptr, size_t size); + +void +virSecureEraseString(char *str); -- 2.29.2

On Tue, Feb 02, 2021 at 05:55:56PM +0100, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virsecureerase.c | 13 +++++++++++++ src/util/virsecureerase.h | 3 +++ 3 files changed, 17 insertions(+)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|

Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libxl/libxl_conf.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 694192e1c3..de0fd66842 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -999,7 +999,7 @@ static int libxlMakeNetworkDiskSrc(virStorageSourcePtr src, char **srcstr) { virConnectPtr conn = NULL; - VIR_AUTODISPOSE_STR base64secret = NULL; + g_autofree char *base64secret = NULL; char *username = NULL; int ret = -1; @@ -1022,7 +1022,10 @@ libxlMakeNetworkDiskSrc(virStorageSourcePtr src, char **srcstr) virSecureErase(secret, secretlen); } - if (!(*srcstr = libxlMakeNetworkDiskSrcStr(src, username, base64secret))) + *srcstr = libxlMakeNetworkDiskSrcStr(src, username, base64secret); + virSecureEraseString(base64secret); + + if (!*srcstr) goto cleanup; ret = 0; -- 2.29.2

On Tue, Feb 02, 2021 at 05:55:57PM +0100, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libxl/libxl_conf.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|

In this instance attempting to be correct is really pointless since the secret is formatted into another string which is not erased securely and then put on the commandline. Keep the secure handling for correctness. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index f613aa0201..0320011ced 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -66,6 +66,7 @@ #include "logging/log_manager.h" #include "logging/log_protocol.h" #include "virutil.h" +#include "virsecureerase.h" #include <sys/stat.h> #include <fcntl.h> @@ -776,7 +777,7 @@ static int qemuBuildRBDSecinfoURI(virBufferPtr buf, qemuDomainSecretInfoPtr secinfo) { - VIR_AUTODISPOSE_STR base64secret = NULL; + g_autofree char *base64secret = NULL; if (!secinfo) { virBufferAddLit(buf, ":auth_supported=none"); @@ -791,6 +792,7 @@ qemuBuildRBDSecinfoURI(virBufferPtr buf, virBufferEscape(buf, '\\', ":", ":key=%s:auth_supported=cephx\\;none", base64secret); + virSecureEraseString(base64secret); break; case VIR_DOMAIN_SECRET_INFO_TYPE_AES: -- 2.29.2

On Tue, Feb 02, 2021 at 05:55:58PM +0100, Peter Krempa wrote:
In this instance attempting to be correct is really pointless since the secret is formatted into another string which is not erased securely and then put on the commandline.
Keep the secure handling for correctness.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_command.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|

Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/storage/storage_backend_rbd.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index 1f83205dfa..007c53f7ac 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -188,7 +188,6 @@ virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr, virStorageAuthDefPtr authdef = source->auth; 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; size_t i; const char *client_mount_timeout = "30"; @@ -199,6 +198,9 @@ virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr, g_autofree char *mon_buff = NULL; if (authdef) { + g_autofree char *rados_key = NULL; + int rc; + VIR_DEBUG("Using cephx authorization, username: %s", authdef->username); if (rados_create(&ptr->cluster, authdef->username) < 0) { @@ -218,8 +220,10 @@ virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr, rados_key = g_base64_encode(secret_value, secret_value_size); virSecureErase(secret_value, secret_value_size); - if (virStorageBackendRBDRADOSConfSet(ptr->cluster, - "key", rados_key) < 0) + rc = virStorageBackendRBDRADOSConfSet(ptr->cluster, "key", rados_key); + virSecureEraseString(rados_key); + + if (rc < 0) goto cleanup; if (virStorageBackendRBDRADOSConfSet(ptr->cluster, -- 2.29.2

On Tue, Feb 02, 2021 at 05:55:59PM +0100, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/storage/storage_backend_rbd.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|

Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-secret.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/virsh-secret.c b/tools/virsh-secret.c index de32f25d64..fcfbe5fe9e 100644 --- a/tools/virsh-secret.c +++ b/tools/virsh-secret.c @@ -302,7 +302,6 @@ static bool cmdSecretGetValue(vshControl *ctl, const vshCmd *cmd) { g_autoptr(virshSecret) secret = NULL; - VIR_AUTODISPOSE_STR base64 = NULL; g_autofree unsigned char *value = NULL; size_t value_size; bool plain = vshCommandOptBool(cmd, "plain"); @@ -320,9 +319,10 @@ cmdSecretGetValue(vshControl *ctl, const vshCmd *cmd) return false; } } else { - base64 = g_base64_encode(value, value_size); + g_autofree char *base64 = g_base64_encode(value, value_size); vshPrint(ctl, "%s", base64); + virSecureEraseString(base64); } virSecureErase(value, value_size); -- 2.29.2

On Tue, Feb 02, 2021 at 05:56:00PM +0100, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virsh-secret.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|

Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/rpc/virnetlibsshsession.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/rpc/virnetlibsshsession.c b/src/rpc/virnetlibsshsession.c index 9671a0f98d..73f5e998fc 100644 --- a/src/rpc/virnetlibsshsession.c +++ b/src/rpc/virnetlibsshsession.c @@ -31,6 +31,7 @@ #include "virstring.h" #include "virauth.h" #include "virbuffer.h" +#include "virsecureerase.h" #define VIR_FROM_THIS VIR_FROM_LIBSSH @@ -613,7 +614,7 @@ virNetLibsshAuthenticatePassword(virNetLibsshSessionPtr sess, /* Try the authenticating the set amount of times. The server breaks the * connection if maximum number of bad auth tries is exceeded */ while (true) { - VIR_AUTODISPOSE_STR password = NULL; + g_autofree char *password = NULL; if (!(password = virAuthGetPasswordPath(sess->authPath, sess->cred, "ssh", sess->username, @@ -621,11 +622,12 @@ virNetLibsshAuthenticatePassword(virNetLibsshSessionPtr sess, return SSH_AUTH_ERROR; /* tunnelled password authentication */ - if ((rc = ssh_userauth_password(sess->session, NULL, - password)) == 0) - return SSH_AUTH_SUCCESS; + rc = ssh_userauth_password(sess->session, NULL, password); + virSecureEraseString(password); - if (rc != SSH_AUTH_DENIED) + if (rc == 0) + return SSH_AUTH_SUCCESS; + else if (rc != SSH_AUTH_DENIED) break; } } -- 2.29.2

On Tue, Feb 02, 2021 at 05:56:01PM +0100, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/rpc/virnetlibsshsession.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|

There are no users any more. The replacement is to use g_auto and virSecureEraseString explicitly. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/viralloc.h | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/src/util/viralloc.h b/src/util/viralloc.h index 0173107b87..f9387a00f9 100644 --- a/src/util/viralloc.h +++ b/src/util/viralloc.h @@ -349,13 +349,3 @@ void virDisposeString(char **strptr) * This macro is not safe to be used on arguments with side effects. */ #define VIR_DISPOSE_STRING(ptr) virDisposeString(&(ptr)) - -/** - * VIR_AUTODISPOSE_STR: - * - * Macro to automatically free and clear the memory allocated to - * the string variable declared with it by calling virDisposeString - * when the variable goes out of scope. - */ -#define VIR_AUTODISPOSE_STR \ - __attribute__((cleanup(virDisposeString))) char * -- 2.29.2

On Tue, Feb 02, 2021 at 05:56:02PM +0100, Peter Krempa wrote:
There are no users any more. The replacement is to use g_auto and virSecureEraseString explicitly.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/viralloc.h | 10 ---------- 1 file changed, 10 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|

Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/rpc/virnetlibsshsession.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/rpc/virnetlibsshsession.c b/src/rpc/virnetlibsshsession.c index 73f5e998fc..76934c7c0b 100644 --- a/src/rpc/virnetlibsshsession.c +++ b/src/rpc/virnetlibsshsession.c @@ -114,7 +114,8 @@ virNetLibsshSessionAuthMethodsFree(virNetLibsshSessionPtr sess) size_t i; for (i = 0; i < sess->nauths; i++) { - VIR_DISPOSE_STRING(sess->auths[i]->password); + virSecureEraseString(sess->auths[i]->password); + g_free(sess->auths[i]->password); VIR_FREE(sess->auths[i]->filename); VIR_FREE(sess->auths[i]); } @@ -445,7 +446,8 @@ virNetLibsshAuthenticatePrivkeyCb(const char *prompt, p = virStrncpy(buf, retr_passphrase.result, retr_passphrase.resultlen, len); - VIR_DISPOSE_STRING(retr_passphrase.result); + virSecureEraseString(retr_passphrase.result); + g_free(retr_passphrase.result); if (p < 0) { virReportError(VIR_ERR_LIBSSH, "%s", _("passphrase is too long for the buffer")); @@ -739,7 +741,8 @@ virNetLibsshAuthenticateKeyboardInteractive(virNetLibsshSessionPtr sess, ret = ssh_userauth_kbdint_setanswer(sess->session, iprompt, retr_passphrase.result); - VIR_DISPOSE_STRING(retr_passphrase.result); + virSecureEraseString(retr_passphrase.result); + g_free(retr_passphrase.result); if (ret < 0) { errmsg = ssh_get_error(sess->session); virReportError(VIR_ERR_AUTH_FAILED, -- 2.29.2

On Tue, Feb 02, 2021 at 05:56:03PM +0100, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/rpc/virnetlibsshsession.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|

Users were replaced with virSecureEraseString with explicit freeing of the memory. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 - src/util/viralloc.c | 17 ----------------- src/util/viralloc.h | 14 -------------- 3 files changed, 32 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 845e749bdf..30589c08ac 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1726,7 +1726,6 @@ vir_g_strdup_vprintf; # util/viralloc.h virAllocVar; virDeleteElementsN; -virDisposeString; virExpandN; virInsertElementsN; virReallocN; diff --git a/src/util/viralloc.c b/src/util/viralloc.c index 036007cb53..e4dc13b776 100644 --- a/src/util/viralloc.c +++ b/src/util/viralloc.c @@ -293,20 +293,3 @@ int virAllocVar(void *ptrptr, *(void **)ptrptr = g_malloc0(alloc_size); return 0; } - - -/** - * virDisposeString: - * @ptrptr: pointer to pointer for a string which should be sanitized and cleared - * - * See virDispose. - */ -void -virDisposeString(char **strptr) -{ - if (!*strptr) - return; - - memset(*strptr, 0, strlen(*strptr)); - g_clear_pointer(strptr, g_free); -} diff --git a/src/util/viralloc.h b/src/util/viralloc.h index f9387a00f9..29e3224818 100644 --- a/src/util/viralloc.h +++ b/src/util/viralloc.h @@ -52,9 +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 virDisposeString(char **strptr) - ATTRIBUTE_NONNULL(1); - /** * VIR_REALLOC_N: * @ptr: pointer to hold address of allocated memory @@ -338,14 +335,3 @@ void virDisposeString(char **strptr) * This macro is safe to use on arguments with side effects. */ #define VIR_FREE(ptr) g_clear_pointer(&(ptr), g_free) - - -/** - * VIR_DISPOSE_STRING: - * @ptr: pointer to a string to be cleared and freed - * - * Clears the string and frees the corresponding memory. - * - * This macro is not safe to be used on arguments with side effects. - */ -#define VIR_DISPOSE_STRING(ptr) virDisposeString(&(ptr)) -- 2.29.2

On Tue, Feb 02, 2021 at 05:56:04PM +0100, Peter Krempa wrote:
Users were replaced with virSecureEraseString with explicit freeing of the memory.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/libvirt_private.syms | 1 - src/util/viralloc.c | 17 ----------------- src/util/viralloc.h | 14 -------------- 3 files changed, 32 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
participants (2)
-
Daniel P. Berrangé
-
Peter Krempa