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