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 :|