[PATCH] Fix leak in getaddrinfo introduced by the fix for CVE-2023-4806.

[PATCH] Fix leak in getaddrinfo introduced by the fix for CVE-2023-4806.

[Romain Geissler]

This patch fixes a very recently added leak in getaddrinfo (which was
backported on release branches too).

I didn't spend much more than 5 minutes on investigating the code to end
up with this patch, so it may be wrong. Quickly testing it on my side,
it seems to work for me, but it definitely needs review from people who
actually know this part of the code ;)

Running a stripped down version of the newly added test
nss/nss_test_gai_hv2_canonname.c with valgrind results in exposure of
the leak:

> cat test.c

int main()
{
    char aHostName[256];
    gethostname(aHostName,255);

    struct addrinfo hints = {};
    struct addrinfo *result = NULL;

    hints.ai_family = AF_INET6;
    hints.ai_flags = AI_ALL | AI_V4MAPPED | AI_CANONNAME;

    int ret = getaddrinfo(aHostName, NULL, &hints, &result);

    if (ret != 0)
        return 1;
    freeaddrinfo(result);
    return 0;
}

> /opt/1A/toolchain/x86_64-v19/bin/gcc -g -o test test.c
> /opt/1A/toolchain/x86_64-v19/build-pack/default/bin/valgrind --leak-check=full ./test
   ... (snapped)
==68017== 37 bytes in 1 blocks are definitely lost in loss record 1 of 1
==68017==    at 0x4840745: malloc (in /remote/tools/Linux/2.6/1A/toolchain/x86_64-v19.0.81/build-pack/19.0.81.0/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==68017==    by 0x48E7CDA: strdup (in /remote/tools/Linux/2.6/1A/toolchain/x86_64-v19.0.81/lib/libc.so.6)
==68017==    by 0x4936582: convert_hostent_to_gaih_addrtuple.isra.0 (in /remote/tools/Linux/2.6/1A/toolchain/x86_64-v19.0.81/lib/libc.so.6)
==68017==    by 0x4936787: gethosts (in /remote/tools/Linux/2.6/1A/toolchain/x86_64-v19.0.81/lib/libc.so.6)
==68017==    by 0x4938F37: getaddrinfo (in /remote/tools/Linux/2.6/1A/toolchain/x86_64-v19.0.81/lib/libc.so.6)
==68017==    by 0x4011A5: main (test.c:17)
   ... (snapped)
---
 sysdeps/posix/getaddrinfo.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sysdeps/posix/getaddrinfo.c b/sysdeps/posix/getaddrinfo.c
index b4e8ea3880a..5f5bc3fd51f 100644
--- a/sysdeps/posix/getaddrinfo.c
+++ b/sysdeps/posix/getaddrinfo.c
@@ -1199,6 +1199,7 @@ free_and_return:
   if (res.free_at)
     free (res.at);
   free (res.canon);
+  free (res.h_name);
 
   return result;
 }
-- 
2.39.3

Re: [PATCH] Fix leak in getaddrinfo introduced by the fix for CVE-2023-4806.

[DJ Delorie]

I think it may need an "if != NULL" check on it, in case we're exiting
due to error.  The only time h_name is set is via __strdup but that
__strdup may fail.

Re: [PATCH] Fix leak in getaddrinfo introduced by the fix for CVE-2023-4806.

[Romain GEISSLER]

> Le 22 sept. 2023 à 22:26, DJ Delorie <dj@redhat.com> a écrit :
> 
> 
> I think it may need an "if != NULL" check on it, in case we're exiting
> due to error.  The only time h_name is set is via __strdup but that
> __strdup may fail.
> 

Isn’t it fine to call free(NULL) (which should normally be a no-op) ? Or you suggest this to save a jump to a function for optimization reasons ?

Re: [PATCH] Fix leak in getaddrinfo introduced by the fix for CVE-2023-4806.

[DJ Delorie]

Romain GEISSLER <romain.geissler@amadeus.com> writes:
> Isn’t it fine to call free(NULL) (which should normally be a no-op) ?
> Or you suggest this to save a jump to a function for optimization
> reasons ?

Nope, I'm just showing my age again.  Of course it's fine to free(NULL)
these days ;-)

There's a bunch of conditionals around that code, but they're checking
some other flag variable.

LGTM
Reviewed-by: DJ Delorie <dj@redhat.com>

Re: [PATCH] Fix leak in getaddrinfo introduced by the fix for CVE-2023-4806.

[Siddhesh Poyarekar]

On 2023-09-22 18:11, Romain Geissler wrote:
> This patch fixes a very recently added leak in getaddrinfo (which was
> backported on release branches too).
> 
> I didn't spend much more than 5 minutes on investigating the code to end
> up with this patch, so it may be wrong. Quickly testing it on my side,
> it seems to work for me, but it definitely needs review from people who
> actually know this part of the code ;)
> 

Nice catch, thank you for noticing.

> Running a stripped down version of the newly added test
> nss/nss_test_gai_hv2_canonname.c with valgrind results in exposure of
> the leak:
> 
>> cat test.c
> 
> int main()
> {
>      char aHostName[256];
>      gethostname(aHostName,255);
> 
>      struct addrinfo hints = {};
>      struct addrinfo *result = NULL;
> 
>      hints.ai_family = AF_INET6;
>      hints.ai_flags = AI_ALL | AI_V4MAPPED | AI_CANONNAME;
> 
>      int ret = getaddrinfo(aHostName, NULL, &hints, &result);
> 
>      if (ret != 0)
>          return 1;
>      freeaddrinfo(result);
>      return 0;
> }
> 
>> /opt/1A/toolchain/x86_64-v19/bin/gcc -g -o test test.c
>> /opt/1A/toolchain/x86_64-v19/build-pack/default/bin/valgrind --leak-check=full ./test
>     ... (snapped)
> ==68017== 37 bytes in 1 blocks are definitely lost in loss record 1 of 1
> ==68017==    at 0x4840745: malloc (in /remote/tools/Linux/2.6/1A/toolchain/x86_64-v19.0.81/build-pack/19.0.81.0/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==68017==    by 0x48E7CDA: strdup (in /remote/tools/Linux/2.6/1A/toolchain/x86_64-v19.0.81/lib/libc.so.6)
> ==68017==    by 0x4936582: convert_hostent_to_gaih_addrtuple.isra.0 (in /remote/tools/Linux/2.6/1A/toolchain/x86_64-v19.0.81/lib/libc.so.6)
> ==68017==    by 0x4936787: gethosts (in /remote/tools/Linux/2.6/1A/toolchain/x86_64-v19.0.81/lib/libc.so.6)
> ==68017==    by 0x4938F37: getaddrinfo (in /remote/tools/Linux/2.6/1A/toolchain/x86_64-v19.0.81/lib/libc.so.6)
> ==68017==    by 0x4011A5: main (test.c:17)
>     ... (snapped)
> ---
>   sysdeps/posix/getaddrinfo.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/sysdeps/posix/getaddrinfo.c b/sysdeps/posix/getaddrinfo.c
> index b4e8ea3880a..5f5bc3fd51f 100644
> --- a/sysdeps/posix/getaddrinfo.c
> +++ b/sysdeps/posix/getaddrinfo.c
> @@ -1199,6 +1199,7 @@ free_and_return:
>     if (res.free_at)
>       free (res.at);
>     free (res.canon);
> +  free (res.h_name);

Could you please consolidate all of this into a gaih_result_reset (&res) 
call?  There's an additional memset, but that should be negligible 
overhead for a cleaner abstraction.

Thanks,
Sid

Re: [PATCH] Fix leak in getaddrinfo introduced by the fix for CVE-2023-4806.

[Siddhesh Poyarekar]

On 2023-09-22 23:21, Siddhesh Poyarekar wrote:
>> diff --git a/sysdeps/posix/getaddrinfo.c b/sysdeps/posix/getaddrinfo.c
>> index b4e8ea3880a..5f5bc3fd51f 100644
>> --- a/sysdeps/posix/getaddrinfo.c
>> +++ b/sysdeps/posix/getaddrinfo.c
>> @@ -1199,6 +1199,7 @@ free_and_return:
>>     if (res.free_at)
>>       free (res.at);
>>     free (res.canon);
>> +  free (res.h_name);
> 
> Could you please consolidate all of this into a gaih_result_reset (&res) 
> call?  There's an additional memset, but that should be negligible 
> overhead for a cleaner abstraction.
> 

Oh, and it would be fabulous if you could quote the original bug number 
(BZ #30843) in the v2 and add a leak check test case (see 
resolv/tst-leaks* for an example) but I won't block the fix on that.

Thanks!
Sid