Re: [PATCH] ifconfig: prefix length handling fixes for -A

[Simon Josefsson]

Erik Auerswald <auerswal@unix-ag.uni-kl.de> writes:

> Hi,
>
> while "ifconfig -A" now accepts CIDR notation, it does not reject prefix
> length values outside of [0,32].  Also, with a prefix length of 0,
> undefined begavior is invoked, and at least on x86_64 a wrong netmask
> is computed.
>
> I think the attached patch fixes this.
>
> If it is OK, I can commit the changes.  What do you think?

Makes sense to me -- when using strtol() one ought to check for LONG_MIN
and LONG_MAX too, but your comparison address that.

       The  strtol() function returns the result of the conversion, unless the
       value would underflow or overflow.  If an  underflow  occurs,  strtol()
       returns  LONG_MIN.

Given that, I would re-order the if statement to compare "n" before
comparing (stale) "end", although I suppose this is somewhat cosmetic.

Maybe add some other odd values in the self-test?  Like
1212237832782387238723823782 and -1238912x1298129.  Thanks for adding
regression checks!

/Simon

> Br,
> Erik
>
> From d4d56a3d36be612d22a9a2146e53698c967ec5e9 Mon Sep 17 00:00:00 2001
> From: Erik Auerswald <auerswal@unix-ag.uni-kl.de>
> Date: Tue, 23 Apr 2024 22:28:19 +0200
> Subject: [PATCH] ifconfig: prefix length handling fixes for -A
>
> With option -A, ifconfig accepted too small and too large prefix
> length values.  Depending on the given prefix length value, this
> could result in undefined behavior.  Using a valid prefix length
> of 0 also resulted in undefined behavior and a wrong result on at
> least the x86_64 architecture.
>
> * NEWS: Mention ifconfig fixes.
> * ifconfig/options.c (parse_opt): Reject too small and too large
> prefix length values for IPv4 addresses.  Avoid undefined behavior
> with a prefix length of 0.
> * tests/ifconfig.sh: Test rejection of invalid prefix length value
> examples.
> ---
>  NEWS               |  3 +++
>  ifconfig/options.c |  4 ++--
>  tests/ifconfig.sh  | 11 +++++++++++
>  3 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 535fccb8..bb3f5f1d 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -10,6 +10,9 @@ Thanks to Rui Chen and Caleb Xu, see
>  Thanks to Collin Funk:
>  https://lists.gnu.org/archive/html/bug-inetutils/2024-03/msg00000.html
>  
> +** ifconfig: With -A, reject invalid prefix length specifications, and
> +correctly handle a prefix length of 0.
> +
>  * Noteworthy changes in release 2.5 (2023-12-29) [stable]
>  
>  ** ftpd, rcp, rlogin, rsh, rshd, uucpd
> diff --git a/ifconfig/options.c b/ifconfig/options.c
> index e4a56369..d22a349d 100644
> --- a/ifconfig/options.c
> +++ b/ifconfig/options.c
> @@ -535,10 +535,10 @@ parse_opt (int key, char *arg, struct argp_state *state)
>           *netlen++ = 0;
>  
>           n = strtol (netlen, &end, 10);
> -         if (end == netlen)
> +         if (end == netlen || n < 0 || n > 32)
>             error (EXIT_FAILURE, 0, "Wrong netmask length %s", netlen);
>  
> -         addr.s_addr = htonl (INADDR_BROADCAST << (32 - n));
> +         addr.s_addr = n ? htonl (INADDR_BROADCAST << (32 - n)) : INADDR_ANY;
>           str = strdup (inet_ntoa (addr));
>           parse_opt_set_netmask (ifp, str);
>         }
> diff --git a/tests/ifconfig.sh b/tests/ifconfig.sh
> index 048f1ff5..bed7f043 100755
> --- a/tests/ifconfig.sh
> +++ b/tests/ifconfig.sh
> @@ -142,6 +142,17 @@ else
>  EOT
>  fi
>  
> +# Check that unusable prefix length values are rejected when using -A
> +#
> +for preflen in p 33 -1;
> +do
> +    $IFCONFIG -i $LO -A 192.0.2.1/$preflen 2>&1 | \
> +        $GREP 'Wrong netmask length' >/dev/null 2>&1 ||
> +            { errno=1;
> +              echo >&2 "Failed to reject invalid prefix length '$preflen'."
> +            }
> +done
> +
>  test $errno -ne 0 || $silence echo "Successful testing".
>  
>  exit $errno

signature.asc
Description: PGP signature