netdev - Re: [iproute PATCH] ip-route: Propagate errors from parse_one

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180123144442.1500f35a@xeon-e3>
Date:   Tue, 23 Jan 2018 14:44:42 -0800
From:   Stephen Hemminger <stephen@...workplumber.org>
To:     Phil Sutter <phil@....cc>
Cc:     netdev@...r.kernel.org
Subject: Re: [iproute PATCH] ip-route: Propagate errors from parse_one_nh()

On Tue, 23 Jan 2018 17:40:47 +0100
Phil Sutter <phil@....cc> wrote:

> The following command segfaults if enp0s31f6 does not exist:
> 
> | # ip -6 route add default proto ra metric 20100 \
> | 	nexthop via fe80:52:0:2040::1fc dev enp0s31f6 weight 1 \
> | 	nexthop via fe80:52:0:2040::1fe dev enp0s31f6 weight 1
> 
> Since the non-zero return code from parse_one_nh() is ignored,
> parse_nexthops() continues iterating over the the same fields in argv
> until buffer space is exhausted and eventually accesses unallocated
> memory.
> 
> Fix this by aborting on error in parse_nexthops() and make
> iproute_modify() fail if parse_nexthops() did.
> 
> Reported-by: Lennart Poettering <lpoetter@...hat.com>
> Fixes: 2f406f2d0b4ef ("ip route: replace exits with returns")
> Signed-off-by: Phil Sutter <phil@....cc>
> ---
>  ip/iproute.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/ip/iproute.c b/ip/iproute.c
> index bf886fda9d761..d7accf57ac8d1 100644
> --- a/ip/iproute.c
> +++ b/ip/iproute.c
> @@ -871,7 +871,8 @@ static int parse_nexthops(struct nlmsghdr *n, struct rtmsg *r,
>  		memset(rtnh, 0, sizeof(*rtnh));
>  		rtnh->rtnh_len = sizeof(*rtnh);
>  		rta->rta_len += rtnh->rtnh_len;
> -		parse_one_nh(n, r, rta, rtnh, &argc, &argv);
> +		if (parse_one_nh(n, r, rta, rtnh, &argc, &argv) < 0)
> +			return -1;
>  		rtnh = RTNH_NEXT(rtnh);
>  	}
>  
> @@ -1318,8 +1319,8 @@ static int iproute_modify(int cmd, unsigned int flags, int argc, char **argv)
>  		addattr_l(&req.n, sizeof(req), RTA_METRICS, RTA_DATA(mxrta), RTA_PAYLOAD(mxrta));
>  	}
>  
> -	if (nhs_ok)
> -		parse_nexthops(&req.n, &req.r, argc, argv);
> +	if (nhs_ok && parse_nexthops(&req.n, &req.r, argc, argv) < 0)
> +		return -1;
>  
>  	if (req.r.rtm_family == AF_UNSPEC)
>  		req.r.rtm_family = AF_INET;


The real issue is that handling of invalid device is different than all the other
possible semantic errors.

My recommendations are:
	* change bad device to use invarg() which does exit
	* make functions that only return 0 void including
		parse_one_nh
		lwt_parse_encap
		get_addr

Also, it looks like read_family converts any address family it doesn't know about to unspec
that is stupid behavior as well.

The original commit 2f406f2d0b4ef ("ip route: replace exits with returns")
looks like well intentioned but suspect. Most of the errors in ip route
indicate real issues where continuing is not a good plan.