[iproute] iproute: Abort if nexthop cannot be parsed

Message ID 20180411094311.21097-1-jkbs@redhat.com
State Accepted
Delegated to: stephen hemminger
Headers show
Series
  • [iproute] iproute: Abort if nexthop cannot be parsed
Related show

Commit Message

Jakub Sitnicki April 11, 2018, 9:43 a.m.
Attempt to add a multipath route where a nexthop definition refers to a
non-existent device causes 'ip' to crash and burn due to stack buffer
overflow:

  # ip -6 route add fd00::1/64 nexthop dev fake1
  Cannot find device "fake1"
  Cannot find device "fake1"
  Cannot find device "fake1"
  ...
  Segmentation fault (core dumped)

Don't ignore errors from the helper routine that parses the nexthop
definition, and abort immediately if parsing fails.

Signed-off-by: Jakub Sitnicki <jkbs@redhat.com>
---
 ip/iproute.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Stephen Hemminger April 17, 2018, midnight | #1
On Wed, 11 Apr 2018 11:43:11 +0200
Jakub Sitnicki <jkbs@redhat.com> wrote:

> Attempt to add a multipath route where a nexthop definition refers to a
> non-existent device causes 'ip' to crash and burn due to stack buffer
> overflow:
> 
>   # ip -6 route add fd00::1/64 nexthop dev fake1
>   Cannot find device "fake1"
>   Cannot find device "fake1"
>   Cannot find device "fake1"
>   ...
>   Segmentation fault (core dumped)
> 
> Don't ignore errors from the helper routine that parses the nexthop
> definition, and abort immediately if parsing fails.
> 
> Signed-off-by: Jakub Sitnicki <jkbs@redhat.com>

Makes sense. Applied

Patch

diff --git a/ip/iproute.c b/ip/iproute.c
index 1d8fd815..44351bc5 100644
--- a/ip/iproute.c
+++ b/ip/iproute.c
@@ -1038,7 +1038,10 @@  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)) {
+			fprintf(stderr, "Error: cannot parse nexthop\n");
+			exit(-1);
+		}
 		rtnh = RTNH_NEXT(rtnh);
 	}