Re: [PATCH 3/7] virStorageBackendISCSISetAuth: Use g_strndup to '\0' terminate data
On Tue, Jan 31, 2023 at 17:35:59 +0100, Martin Kletzander wrote:
On Tue, Jan 31, 2023 at 05:02:15PM +0100, Peter Krempa wrote:
> Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
> ---
> src/storage/storage_backend_iscsi.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/src/storage/storage_backend_iscsi.c
b/src/storage/storage_backend_iscsi.c
> index e4fa49d05f..01900f6809 100644
> --- a/src/storage/storage_backend_iscsi.c
> +++ b/src/storage/storage_backend_iscsi.c
> @@ -283,10 +283,8 @@ virStorageBackendISCSISetAuth(const char *portal,
> &secret_value, &secret_size) < 0)
> return -1;
>
> - secret_str = g_new0(char, secret_size + 1);
> - memcpy(secret_str, secret_value, secret_size);
> + secret_str = g_strndup((char *) secret_value, secret_size);
Unfortunately secrets can contain zero bytes in which case this function
would pad everything after the first zero byte with more zero bytes.
Fortunately (?) the functions that are called below do not take
secret_size, so it won't affect this particular code block, but we might
have other problems already existing in the code with this.
Indeed. If the secret itself contains NUL bytes it would indeed not work
properly, but that's pre-existing.
But with this patch and a NUL byte in a secret we'd actually write
beyond the end of the buffer below when cleaning up as the cleanup is
done via
virSecureErase(secret_str, secret_size);
thus attempting to clear more than the string allocated via g_strndup.
at this point I think I can simply drop this + the other patch doing the
same, as the difference is negligible.
> virSecureErase(secret_value, secret_size);
> - secret_str[secret_size] = '\0';
>
> if (virISCSINodeUpdate(portal,
> source->devices[0].path,
> --
> 2.39.1
>