[PATCH 3/7] virStorageBackendISCSISetAuth: Use g_strndup to '\0' terminate data

[Peter Krempa]

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