On Tue, Jan 31, 2023 at 17:56:09 +0100, Martin Kletzander wrote: > 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@xxxxxxxxxx> > > > > --- > > > > 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. Ah right, the documentation says so. Anyways, we can't do anything about non-C-string passwords here because virISCSINodeUpdate actually invokes a command with the password as argument so it must be a C string regardless.
