[PATCH v4] nss: reject invalid port passed to getaddrinfo [BZ #16208]
Antonio Quartulli
a@unstable.cc
Wed Oct 30 00:09:03 GMT 2024
Subject was supposed to say "PATCH v5" - sorry for the mistake.
On 30/10/2024 01:07, 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 when passing 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>
>
> ---
> Changes from v4:
> * used USHRT_MAX + 1 to report parsing error
> * added unit test for port parsing error ("10haha")
>
> Changes from v3:
> * extended unit test with more notorius integer constants
> * rephrased comment
> * removed mention of 0 being invalid from commit message
>
> Changes from v2:
> * allowed specifying 0 as port number (equivalent to not passing any
> port)
> * changed unit-test to check success when passing 0 as port
> ---
> nss/Makefile | 1 +
> nss/getaddrinfo.c | 17 +++++++--
> nss/tst-getaddrinfo6.c | 85 ++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 100 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..0e5984ef1b 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 && service->num <= USHRT_MAX))
> {
> int port = service != NULL ? htons (service->num) : 0;
>
> @@ -2375,7 +2375,18 @@ getaddrinfo (const char *name, const char *service,
> return EAI_NONAME;
> }
>
> - gaih_service.num = -1;
> + /* use an out-of-range value to report the parsing error */
> + gaih_service.num = USHRT_MAX + 1;
> + }
> + /* port number must be in range [0, 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 > USHRT_MAX)
> + {
> + /* the provided port number 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..2ecd0f07e0
> --- /dev/null
> +++ b/nss/tst-getaddrinfo6.c
> @@ -0,0 +1,85 @@
> +/* 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, int expected_err)
> +{
> + 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 != expected_err)
> + {
> + printf ("FAIL getaddrinfo returned %d, should return %d\n",
> + gaierr, expected_err);
> + result = 1;
> + }
> + }
> +
> + return result;
> +}
> +
> +
> +static int
> +do_test (void)
> +{
> + int err = 0;
> +
> + // 0 is accepted, test should succeed
> + err |= do_one_test ("0", 0);
> + err |= do_one_test ("70000", EAI_NONAME);
> + err |= do_one_test ("-1", EAI_NONAME);
> + err |= do_one_test ("10haha", EAI_SERVICE);
> +
> + // INT_MIN
> + err |= do_one_test ("-2147483648", EAI_NONAME);
> + // INT_MAX
> + err |= do_one_test ("2147483647", EAI_NONAME);
> + // UINT_MAX / ULONG_MAX 32bit
> + err |= do_one_test ("4294967295", EAI_NONAME);
> + // ULONG_MAX 64bit / ULLONG_MAX
> + err |= do_one_test ("18446744073709551615", EAI_NONAME);
> + // LONG_MAX 64bit / LLONG_MAX
> + err |= do_one_test ("9223372036854775807", EAI_NONAME);
> + // LONG_MIN 64bit / LLONG_MIN
> + err |= do_one_test ("-9223372036854775808", EAI_NONAME);
> +
> + // ULONG_MAX 64bit +1 / ULLONG_MAX +1
> + err |= do_one_test ("18446744073709551616", EAI_NONAME);
> + // LONG_MIN 64bit -1 / LLONG_MIN -1
> + err |= do_one_test ("-9223372036854775809", EAI_NONAME);
> +
> + return err;
> +}
> +#define TEST_FUNCTION do_test ()
> +
> +#include "../test-skeleton.c"
--
Antonio Quartulli
More information about the Libc-alpha
mailing list