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