Hi, On Wed, Apr 24, 2024 at 06:11:23PM +0200, Erik Auerswald wrote: > On Wed, Apr 24, 2024 at 02:55:41PM +0200, Simon Josefsson wrote: > > Erik Auerswald <auers...@unix-ag.uni-kl.de> writes: > > > > > > 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. > > This first check tests if strtol() found a leading digit, i.e., if the > value of "n" comes from the textual representation of a number. > > If there were no digits at all, strtol() stores the original value > of nptr in *endptr (and returns 0). > > The other two checks then verify that the number is in the valid range. > Thus I prefer this order of checks. > > This still allows wrong input, e.g., "192.0.2.47/3k". I'll consider > tightening the checks, possibly based on the following from the strtol(3) > man page: > > In particular, if *nptr is not '\0' but **endptr is '\0' on return, > the entire string is valid.
I have done this in the attached patch. > This would still allow strange values like "192.0.2.47/ +24", but I do > not think it is worth the effort to detect this. > > > Maybe add some other odd values in the self-test? Like > > 1212237832782387238723823782 and -1238912x1298129. Thanks for adding > > regression checks! > > I can do that, no problem. I have done that in the attached patch. I plan to push the changes in a couple of days, unless I receive negative feedback. Best regards, Erik
>From e07feed005c591a2c53ee0067b40e3d9e7d3f8a5 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. It also accepted non-numeric values starting with a number. 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 non-numeric prefix length values. 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 | 12 ++++++++++++ 3 files changed, 17 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..13f12e15 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 (!(*netlen && !*end) || 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..3358e461 100755 --- a/tests/ifconfig.sh +++ b/tests/ifconfig.sh @@ -142,6 +142,18 @@ else EOT fi +# Check that unusable prefix length values are rejected when using -A +# +for preflen in p 33 -1 1212237832782387238723823782 -1238912x1298129 3k \ + 1.2 2e3 '' ' '; +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 -- 2.25.1
