Erik Auerswald <auers...@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 <auers...@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
