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

On Tue, Jan 31, 2023 at 05:41:57PM +0100, Peter Krempa wrote: > 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&gt; > > > --- > > > 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. > no, that's fine, g_strndup will allocate secret_size + 1.