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.
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 >
Attachment:
signature.asc
Description: PGP signature
