[v2] nss: reject invalid port passed to getaddrinfo [BZ #16208]

When passing a numeric string as port/service to getaddrinfo()
representing a number larger than 65535, the function will not
complain and will simply extract the lowest 16bits of the
converted number.

Specifically, the string is converted to int by calling strtoul()
and later it is moved to gaih_servtuple.num after passing through
htons().

For example, invoking getaddrinfo() with service equal to "70000"
will result in no error and ai_addr->sin_port set to 4464.

This issue was (re)discovered by researcher Anqi Chen while stress
testing OpenVPN with invalid config parameters.

Thanks a lot to Arne Schwabe for finding out that the root
cause was hidden inside glibc.

Similarly, also when passing 0 or an out of range value, no error
is thrown while it should.

Add proper checks to reject invalid values immediately.

The 'num' member has been converted to unsigned long in order
to properly store the result of strtoul(). Then, while checking for
'num' being greater than USHRT_MAX, we also catch overflows/out-of-range
values (strtoul() returns ULONG_MAX in that case).

Cc: Arne Schwabe <arne@rfc2549.org>
Cc: Cristina Nita-Rotaru <crisn@ccs.neu.edu>
Reported-by: Anqi Chen <chen.anqi3@northeastern.edu>
Signed-off-by: Antonio Quartulli <a@unstable.cc>
---
 nss/Makefile           |  1 +
 nss/getaddrinfo.c      | 18 ++++++++++--
 nss/tst-getaddrinfo6.c | 64 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 80 insertions(+), 3 deletions(-)
 create mode 100644 nss/tst-getaddrinfo6.c

Hi all,

I am hoping this is the right time to poke at this patch.
It's been idling for around one month.

Best Regards,

On 09/09/2024 10:27, Antonio Quartulli wrote:
> When passing a numeric string as port/service to getaddrinfo()
> representing a number larger than 65535, the function will not
> complain and will simply extract the lowest 16bits of the
> converted number.
> 
> Specifically, the string is converted to int by calling strtoul()
> and later it is moved to gaih_servtuple.num after passing through
> htons().
> 
> For example, invoking getaddrinfo() with service equal to "70000"
> will result in no error and ai_addr->sin_port set to 4464.
> 
> This issue was (re)discovered by researcher Anqi Chen while stress
> testing OpenVPN with invalid config parameters.
> 
> Thanks a lot to Arne Schwabe for finding out that the root
> cause was hidden inside glibc.
> 
> Similarly, also when passing 0 or an out of range value, no error
> is thrown while it should.
> 
> Add proper checks to reject invalid values immediately.
> 
> The 'num' member has been converted to unsigned long in order
> to properly store the result of strtoul(). Then, while checking for
> 'num' being greater than USHRT_MAX, we also catch overflows/out-of-range
> values (strtoul() returns ULONG_MAX in that case).
> 
> Cc: Arne Schwabe <arne@rfc2549.org>
> Cc: Cristina Nita-Rotaru <crisn@ccs.neu.edu>
> Reported-by: Anqi Chen <chen.anqi3@northeastern.edu>
> Signed-off-by: Antonio Quartulli <a@unstable.cc>
> ---
>   nss/Makefile           |  1 +
>   nss/getaddrinfo.c      | 18 ++++++++++--
>   nss/tst-getaddrinfo6.c | 64 ++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 80 insertions(+), 3 deletions(-)
>   create mode 100644 nss/tst-getaddrinfo6.c
> 
> diff --git a/nss/Makefile b/nss/Makefile
> index 9331b3308c..94acfd0e38 100644
> --- a/nss/Makefile
> +++ b/nss/Makefile
> @@ -321,6 +321,7 @@ tests := \
>     tst-getaddrinfo \
>     tst-getaddrinfo2 \
>     tst-getaddrinfo3 \
> +  tst-getaddrinfo6 \
>     tst-gethnm \
>     tst-getpw \
>     tst-gshadow \
> diff --git a/nss/getaddrinfo.c b/nss/getaddrinfo.c
> index 3ccd3905fa..0ec6f4cfd0 100644
> --- a/nss/getaddrinfo.c
> +++ b/nss/getaddrinfo.c
> @@ -95,7 +95,7 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>   struct gaih_service
>     {
>       const char *name;
> -    int num;
> +    unsigned long num;
>     };
>   
>   struct gaih_servtuple
> @@ -414,7 +414,7 @@ get_servtuples (const struct gaih_service *service, const struct addrinfo *req,
>     if (service != NULL && (tp->protoflag & GAI_PROTO_NOSERVICE) != 0)
>       return -EAI_SERVICE;
>   
> -  if (service == NULL || service->num >= 0)
> +  if (service == NULL || service->num > 0)
>       {
>         int port = service != NULL ? htons (service->num) : 0;
>   
> @@ -2375,7 +2375,19 @@ getaddrinfo (const char *name, const char *service,
>   	      return EAI_NONAME;
>   	    }
>   
> -	  gaih_service.num = -1;
> +	  /* 0 is not a valid port number, therefore we use it
> +	     as marker for "no numeric port provided" */
> +	  gaih_service.num = 0;
> +	}
> +      /* port number must be in range [1, USHRT_MAX].
> +         Any other value is invalid.
> +         strtoul returns ULONG_MAX in case of out-of-range
> +         input and it is caught by this check */
> +      else if (gaih_service.num == 0 || gaih_service.num > USHRT_MAX)
> +	{
> +	  /* provided numeric string is invalid */
> +	  __free_in6ai (in6ai);
> +	  return EAI_NONAME;
>   	}
>   
>         pservice = &gaih_service;
> diff --git a/nss/tst-getaddrinfo6.c b/nss/tst-getaddrinfo6.c
> new file mode 100644
> index 0000000000..6f9c5aa546
> --- /dev/null
> +++ b/nss/tst-getaddrinfo6.c
> @@ -0,0 +1,64 @@
> +/* Test invalid port/service lookup [BZ #16208].
> +   Copyright (C) 2014-2024 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#include <stdio.h>
> +#include <string.h>
> +#include <sys/types.h>
> +#include <sys/socket.h>
> +#include <netinet/in.h>
> +#include <netdb.h>
> +
> +static int
> +do_one_test (const char *service)
> +{
> +  const int family[2] = { AF_INET, AF_INET6 };
> +  struct addrinfo hints, *aitop;
> +  int index, gaierr;
> +  int result = 0;
> +
> +  for (index = 0; index < sizeof (family) / sizeof (family[0]); ++index)
> +    {
> +      memset (&hints, '\0', sizeof (hints));
> +      hints.ai_family = family[index];
> +
> +      gaierr = getaddrinfo (NULL, service, &hints, &aitop);
> +      if (gaierr != EAI_NONAME)
> +	{
> +	  printf ("FAIL getaddrinfo returned %d, should return %d\n",
> +		  gaierr, EAI_NONAME);
> +	  result = 1;
> +	}
> +    }
> +
> +  return result;
> +}
> +
> +
> +static int
> +do_test (void)
> +{
> +  int err = 0;
> +
> +  err |= do_one_test ("0");
> +  err |= do_one_test ("70000");
> +
> +  return err;
> +}
> +#define TEST_FUNCTION do_test ()
> +
> +#include "../test-skeleton.c"

* Antonio Quartulli:

> @@ -2375,7 +2375,19 @@ getaddrinfo (const char *name, const char *service,
>  	      return EAI_NONAME;
>  	    }
>  
> -	  gaih_service.num = -1;
> +	  /* 0 is not a valid port number, therefore we use it
> +	     as marker for "no numeric port provided" */
> +	  gaih_service.num = 0;
> +	}
> +      /* port number must be in range [1, USHRT_MAX].
> +         Any other value is invalid.
> +         strtoul returns ULONG_MAX in case of out-of-range
> +         input and it is caught by this check */
> +      else if (gaih_service.num == 0 || gaih_service.num > USHRT_MAX)
> +	{
> +	  /* provided numeric string is invalid */
> +	  __free_in6ai (in6ai);
> +	  return EAI_NONAME;
>  	}
>  
>        pservice = &gaih_service;

I think we should retain 0 as a valid port number.  Some applications
use it as an indicator that they do not want to lookup the service, or
perhaps to request port assignment from the kernel.

There's this test case in dbus:

  res = getaddrinfo ("127.0.0.1", "0", &hints, &addrs);
  saved_errno = errno;

  if (res != 0)
    {
      const gchar *system_message;
      gchar *skip_message;

#ifdef EAI_SYSTEM
      if (res == EAI_SYSTEM)
        system_message = g_strerror (saved_errno);
      else
#endif
        system_message = gai_strerror (res);

      skip_message = g_strdup_printf ("Name resolution does not work here: "
                                      "getaddrinfo(\"127.0.0.1\", \"0\", "
                                      "{flags=ADDRCONFIG, family=INET,"
                                      "socktype=STREAM, protocol=TCP}): "
                                      "%s",
                                      system_message);
      g_test_skip (skip_message);
      free (skip_message);
    }

Or this code from the FTP code in Debian's APT:

   // Get the information for a listening socket.
   struct addrinfo *BindAddr = NULL;
   struct addrinfo Hints;
   memset(&Hints,0,sizeof(Hints));
   Hints.ai_socktype = SOCK_STREAM;
   Hints.ai_flags |= AI_PASSIVE;
   Hints.ai_family = ((struct sockaddr *)&ServerAddr)->sa_family;
   if (getaddrinfo(0,"0",&Hints,&BindAddr) != 0 || BindAddr == NULL)
      return _error->Error(_("getaddrinfo was unable to get a listening socket"));

I just don't see a compelling reason to break such scenarios.
Checking the range and for negative values should be fine, though.

On 10/10/2024 16:20, Florian Weimer wrote:
> * Antonio Quartulli:
> 
>> @@ -2375,7 +2375,19 @@ getaddrinfo (const char *name, const char *service,
>>   	      return EAI_NONAME;
>>   	    }
>>   
>> -	  gaih_service.num = -1;
>> +	  /* 0 is not a valid port number, therefore we use it
>> +	     as marker for "no numeric port provided" */
>> +	  gaih_service.num = 0;
>> +	}
>> +      /* port number must be in range [1, USHRT_MAX].
>> +         Any other value is invalid.
>> +         strtoul returns ULONG_MAX in case of out-of-range
>> +         input and it is caught by this check */
>> +      else if (gaih_service.num == 0 || gaih_service.num > USHRT_MAX)
>> +	{
>> +	  /* provided numeric string is invalid */
>> +	  __free_in6ai (in6ai);
>> +	  return EAI_NONAME;
>>   	}
>>   
>>         pservice = &gaih_service;
> 
> I think we should retain 0 as a valid port number.  Some applications
> use it as an indicator that they do not want to lookup the service, or
> perhaps to request port assignment from the kernel.
> 
> There's this test case in dbus:
> 
>    res = getaddrinfo ("127.0.0.1", "0", &hints, &addrs);
>    saved_errno = errno;
> 
>    if (res != 0)
>      {
>        const gchar *system_message;
>        gchar *skip_message;
> 
> #ifdef EAI_SYSTEM
>        if (res == EAI_SYSTEM)
>          system_message = g_strerror (saved_errno);
>        else
> #endif
>          system_message = gai_strerror (res);
> 
>        skip_message = g_strdup_printf ("Name resolution does not work here: "
>                                        "getaddrinfo(\"127.0.0.1\", \"0\", "
>                                        "{flags=ADDRCONFIG, family=INET,"
>                                        "socktype=STREAM, protocol=TCP}): "
>                                        "%s",
>                                        system_message);
>        g_test_skip (skip_message);
>        free (skip_message);
>      }
> 
> Or this code from the FTP code in Debian's APT:
> 
>     // Get the information for a listening socket.
>     struct addrinfo *BindAddr = NULL;
>     struct addrinfo Hints;
>     memset(&Hints,0,sizeof(Hints));
>     Hints.ai_socktype = SOCK_STREAM;
>     Hints.ai_flags |= AI_PASSIVE;
>     Hints.ai_family = ((struct sockaddr *)&ServerAddr)->sa_family;
>     if (getaddrinfo(0,"0",&Hints,&BindAddr) != 0 || BindAddr == NULL)
>        return _error->Error(_("getaddrinfo was unable to get a listening socket"));
> 
> I just don't see a compelling reason to break such scenarios.
> Checking the range and for negative values should be fine, though.

It definitely makes sense.
Even more if we already have known users of this specific case.

I will drop the check for '== 0' and send v3 accordingly.

Thanks a lot.

Best Regards,