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

[Martin Kletzander]

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

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
>> >
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20230131/e34131f7/attachment.sig>