diff mbox

[v4] ipv6:introduce function to find route for redirect

Message ID 52242CE1.5050501@cn.fujitsu.com
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Duan Jiong Sept. 2, 2013, 6:14 a.m. UTC 
From: Duan Jiong <duanj.fnst@cn.fujitsu.com>

RFC 4861 says that the IP source address of the Redirect is the
same as the current first-hop router for the specified ICMP
Destination Address, so the gateway should be taken into
consideration when we find the route for redirect.

There was once a check in commit
a6279458c534d01ccc39498aba61c93083ee0372 ("NDISC: Search over
all possible rules on receipt of redirect.") and the check
went away in commit b94f1c0904da9b8bf031667afc48080ba7c3e8c9
("ipv6: Use icmpv6_notify() to propagate redirect, instead of
rt6_redirect()").

The bug is only "exploitable" on layer-2 because the source
address of the redirect is checked to be a valid link-local
address but it makes spoofing a lot easier in the same L2
domain nonetheless.

Thanks very much for Hannes's help.

Signed-off-by: Duan Jiong <duanj.fnst@cn.fujitsu.com>
---
 Changes for v4:
 1.Fix the intended problems
 2.Change the ordering of the arguments in function
   ip6_route_redirect
 3.Drop the unnecessary flag RT6_LOOKUP_F_IFACE
 4.Check the dst.error when look up route for redirect

 net/ipv6/ah6.c     |  2 +-
 net/ipv6/esp6.c    |  2 +-
 net/ipv6/icmp.c    |  2 +-
 net/ipv6/ipcomp6.c |  2 +-
 net/ipv6/ndisc.c   |  3 ++-
 net/ipv6/route.c   | 77 +++++++++++++++++++++++++++++++++++++++++++++++++-----
 6 files changed, 77 insertions(+), 11 deletions(-)

Comments

Hannes Frederic Sowa Sept. 2, 2013, 7:50 p.m. UTC | #1 
On Mon, Sep 02, 2013 at 02:14:57PM +0800, Duan Jiong wrote:
> +static struct rt6_info *__ip6_route_redirect(struct net *net,
> +					     struct fib6_table *table,
> +					     struct flowi6 *fl6,
> +					     int flags)
> +{
> +	struct ip6rd_flowi *rdfl = (struct ip6rd_flowi *)fl6;
> +	struct rt6_info *rt;
> +	struct fib6_node *fn;
> +
> +	/* Get the "current" route for this destination and
> +	 * check if the redirect has come from approriate router.
> +	 *
> +	 * RFC 4861 specifies that redirects should only be
> +	 * accepted if they come from the nexthop to the target.
> +	 * Due to the way the routes are chosen, this notion
> +	 * is a bit fuzzy and one might need to check all possible
> +	 * routes.
> +	 */
> +
> +	read_lock_bh(&table->tb6_lock);
> +	fn = fib6_lookup(&table->tb6_root, &fl6->daddr, &fl6->saddr);
> +restart:
> +	for (rt = fn->leaf; rt; rt = rt->dst.rt6_next) {
> +		if (rt6_check_expired(rt))
> +			continue;
> +		if (rt->dst.error)
> +			continue;

Sorry, I should have been more clear what I meant with failing early:

I considered a setup like this:

ip -6 r a default nexthop via fe80::1 dev eth0
ip -6 r a prohibit 2002:1::/64

If the kernel receives a redirect for a destination e.g. 2002:1::1 we
would backtrack above the prohibit rule and return the dst of the default
route and would insert a new cached route which could circumvent the
prohibit rule. We have to try to lock down the tree below 2002:1::/64
in that case. A possible solution for that would be to do something
like this:

	/* We don't accept a redirect in case a more specific route is
	 * installed with dst.error and stop backtracking.
	 */
	if (rt->dst.error)
		break;

Either we have to replace the rt with net->ipv6.ip6_null_entry in that
case or check dst->error before calling rt6_do_redirect below.

> +		if (!(rt->rt6i_flags & RTF_GATEWAY))
> +			continue;
> +		if (fl6->flowi6_oif != rt->dst.dev->ifindex)
> +			continue;
> +		if (!ipv6_addr_equal(&rdfl->gateway, &rt->rt6i_gateway))
> +			continue;
> +		break;
> +	}
> +
> +	if (!rt)
> +		rt = net->ipv6.ip6_null_entry;
> +	BACKTRACK(net, &fl6->saddr);
> +out:
> +	dst_hold(&rt->dst);
> +
> +	read_unlock_bh(&table->tb6_lock);
> +
> +	return rt;
> +};
>
> [...]
>
> @@ -1171,9 +1238,8 @@ void ip6_redirect(struct sk_buff *skb, struct net *net, int oif, u32 mark)
>  	fl6.saddr = iph->saddr;
>  	fl6.flowlabel = ip6_flowinfo(iph);
>  
> -	dst = ip6_route_output(net, NULL, &fl6);
> -	if (!dst->error)
> -		rt6_do_redirect(dst, NULL, skb);
> +	dst = ip6_route_redirect(net, &fl6, &ipv6_hdr(skb)->saddr);
> +	rt6_do_redirect(dst, NULL, skb);
>  	dst_release(dst);
>  }
>  EXPORT_SYMBOL_GPL(ip6_redirect);
> @@ -1193,9 +1259,8 @@ void ip6_redirect_no_header(struct sk_buff *skb, struct net *net, int oif,
>  	fl6.daddr = msg->dest;
>  	fl6.saddr = iph->daddr;
>  
> -	dst = ip6_route_output(net, NULL, &fl6);
> -	if (!dst->error)
> -		rt6_do_redirect(dst, NULL, skb);
> +	dst = ip6_route_redirect(net, &fl6, &iph->saddr);
> +	rt6_do_redirect(dst, NULL, skb);
>  	dst_release(dst);
>  }

Btw. I still think it should be possible to eliminate
ip6_redirect_no_header:

We could always use ip6_redirect_no_header and use the data of the redirected
header option just for finding the socket to be notified. We can do the whole
verification and route updating in ndisc layer and then just call into icmpv6
layer if upper protocols need a notification of the redirect. But that should
go into another patch. ;)

Thanks,

  Hannes

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Duan Jiong Sept. 3, 2013, 5:37 a.m. UTC | #2 
于 2013年09月03日 03:50, Hannes Frederic Sowa 写道:
> On Mon, Sep 02, 2013 at 02:14:57PM +0800, Duan Jiong wrote:
>> +static struct rt6_info *__ip6_route_redirect(struct net *net,
>> +					     struct fib6_table *table,
>> +					     struct flowi6 *fl6,
>> +					     int flags)
>> +{
>> +	struct ip6rd_flowi *rdfl = (struct ip6rd_flowi *)fl6;
>> +	struct rt6_info *rt;
>> +	struct fib6_node *fn;
>> +
>> +	/* Get the "current" route for this destination and
>> +	 * check if the redirect has come from approriate router.
>> +	 *
>> +	 * RFC 4861 specifies that redirects should only be
>> +	 * accepted if they come from the nexthop to the target.
>> +	 * Due to the way the routes are chosen, this notion
>> +	 * is a bit fuzzy and one might need to check all possible
>> +	 * routes.
>> +	 */
>> +
>> +	read_lock_bh(&table->tb6_lock);
>> +	fn = fib6_lookup(&table->tb6_root, &fl6->daddr, &fl6->saddr);
>> +restart:
>> +	for (rt = fn->leaf; rt; rt = rt->dst.rt6_next) {
>> +		if (rt6_check_expired(rt))
>> +			continue;
>> +		if (rt->dst.error)
>> +			continue;
> 
> Sorry, I should have been more clear what I meant with failing early:
> 
> I considered a setup like this:
> 
> ip -6 r a default nexthop via fe80::1 dev eth0
> ip -6 r a prohibit 2002:1::/64
> 
> If the kernel receives a redirect for a destination e.g. 2002:1::1 we
> would backtrack above the prohibit rule and return the dst of the default
> route and would insert a new cached route which could circumvent the
> prohibit rule. We have to try to lock down the tree below 2002:1::/64
> in that case. A possible solution for that would be to do something
> like this:
> 
> 	/* We don't accept a redirect in case a more specific route is
> 	 * installed with dst.error and stop backtracking.
> 	 */
> 	if (rt->dst.error)
> 		break;
> 
> Either we have to replace the rt with net->ipv6.ip6_null_entry in that
> case or check dst->error before calling rt6_do_redirect below.
>

Thanks for you comment, i understand what you mean.
 
>> +		if (!(rt->rt6i_flags & RTF_GATEWAY))
>> +			continue;
>> +		if (fl6->flowi6_oif != rt->dst.dev->ifindex)
>> +			continue;
>> +		if (!ipv6_addr_equal(&rdfl->gateway, &rt->rt6i_gateway))
>> +			continue;
>> +		break;
>> +	}
>> +
>> +	if (!rt)
>> +		rt = net->ipv6.ip6_null_entry;
>> +	BACKTRACK(net, &fl6->saddr);
>> +out:
>> +	dst_hold(&rt->dst);
>> +
>> +	read_unlock_bh(&table->tb6_lock);
>> +
>> +	return rt;
>> +};
>>
>> [...]
>>
>> @@ -1171,9 +1238,8 @@ void ip6_redirect(struct sk_buff *skb, struct net *net, int oif, u32 mark)
>>  	fl6.saddr = iph->saddr;
>>  	fl6.flowlabel = ip6_flowinfo(iph);
>>  
>> -	dst = ip6_route_output(net, NULL, &fl6);
>> -	if (!dst->error)
>> -		rt6_do_redirect(dst, NULL, skb);
>> +	dst = ip6_route_redirect(net, &fl6, &ipv6_hdr(skb)->saddr);
>> +	rt6_do_redirect(dst, NULL, skb);
>>  	dst_release(dst);
>>  }
>>  EXPORT_SYMBOL_GPL(ip6_redirect);
>> @@ -1193,9 +1259,8 @@ void ip6_redirect_no_header(struct sk_buff *skb, struct net *net, int oif,
>>  	fl6.daddr = msg->dest;
>>  	fl6.saddr = iph->daddr;
>>  
>> -	dst = ip6_route_output(net, NULL, &fl6);
>> -	if (!dst->error)
>> -		rt6_do_redirect(dst, NULL, skb);
>> +	dst = ip6_route_redirect(net, &fl6, &iph->saddr);
>> +	rt6_do_redirect(dst, NULL, skb);
>>  	dst_release(dst);
>>  }
> 
> Btw. I still think it should be possible to eliminate
> ip6_redirect_no_header:
> 
> We could always use ip6_redirect_no_header and use the data of the redirected
> header option just for finding the socket to be notified. We can do the whole
> verification and route updating in ndisc layer and then just call into icmpv6
> layer if upper protocols need a notification of the redirect. But that should
> go into another patch. ;)
> 

I think this is good, but i have a question below:

  if the socket type is connection-based, the dst information is stored in related
sock struct, so there is no need to look up the route for redirect in ip6_redirect
or ip6_redirect_no_header, in this case, we do the verification and route 
updating in the upper protocols' err_handler is better. 

How do you think of this?

Thanks,
  Duan
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hannes Frederic Sowa Sept. 3, 2013, 7:17 p.m. UTC | #3 
On Tue, Sep 03, 2013 at 01:37:19PM +0800, Duan Jiong wrote:
> > Btw. I still think it should be possible to eliminate
> > ip6_redirect_no_header:
> > 
> > We could always use ip6_redirect_no_header and use the data of the redirected
> > header option just for finding the socket to be notified. We can do the whole
> > verification and route updating in ndisc layer and then just call into icmpv6
> > layer if upper protocols need a notification of the redirect. But that should
> > go into another patch. ;)
> > 
> 
> I think this is good, but i have a question below:
> 
>   if the socket type is connection-based, the dst information is stored in related
> sock struct, so there is no need to look up the route for redirect in ip6_redirect
> or ip6_redirect_no_header, in this case, we do the verification and route 
> updating in the upper protocols' err_handler is better. 
> 
> How do you think of this?

This should not be a problem, because every cached dst should be validated
with ip6_dst_check before it is used. It uses the fib6_node serial number
which is incremented for all fib6_nodes on the path to the new installed
node by fib6_add_1. So we are safe here.

Btw. this is the same logic redirects get currently picked up, too.

Greetings,

  Hannes

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Duan Jiong Sept. 4, 2013, 12:06 p.m. UTC | #4 
于 2013年09月04日 03:17, Hannes Frederic Sowa 写道:
> On Tue, Sep 03, 2013 at 01:37:19PM +0800, Duan Jiong wrote:
>>> Btw. I still think it should be possible to eliminate
>>> ip6_redirect_no_header:
>>>
>>> We could always use ip6_redirect_no_header and use the data of the redirected
>>> header option just for finding the socket to be notified. We can do the whole
>>> verification and route updating in ndisc layer and then just call into icmpv6
>>> layer if upper protocols need a notification of the redirect. But that should
>>> go into another patch. ;)
>>>
>>
>> I think this is good, but i have a question below:
>>
>>   if the socket type is connection-based, the dst information is stored in related
>> sock struct, so there is no need to look up the route for redirect in ip6_redirect
>> or ip6_redirect_no_header, in this case, we do the verification and route 
>> updating in the upper protocols' err_handler is better. 
>>
>> How do you think of this?
> 
> This should not be a problem, because every cached dst should be validated
> with ip6_dst_check before it is used. It uses the fib6_node serial number
> which is incremented for all fib6_nodes on the path to the new installed
> node by fib6_add_1. So we are safe here.
> 
> Btw. this is the same logic redirects get currently picked up, too.
> 

Thanks for you answer, but i still have some questions on dealing with redirect
in ip4ip6_err() and ipip6_err(), and i need some time to learn more about them.
So i only send one patch to fix the bug.

Please forgive me is a newbie.:)

Thanks,
  Duan 

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hannes Frederic Sowa Sept. 8, 2013, 6 p.m. UTC | #5 
On Wed, Sep 04, 2013 at 08:06:11PM +0800, Duan Jiong wrote:
> 于 2013年09月04日 03:17, Hannes Frederic Sowa 写道:
> > On Tue, Sep 03, 2013 at 01:37:19PM +0800, Duan Jiong wrote:
> >>> Btw. I still think it should be possible to eliminate
> >>> ip6_redirect_no_header:
> >>>
> >>> We could always use ip6_redirect_no_header and use the data of the redirected
> >>> header option just for finding the socket to be notified. We can do the whole
> >>> verification and route updating in ndisc layer and then just call into icmpv6
> >>> layer if upper protocols need a notification of the redirect. But that should
> >>> go into another patch. ;)
> >>>
> >>
> >> I think this is good, but i have a question below:
> >>
> >>   if the socket type is connection-based, the dst information is stored in related
> >> sock struct, so there is no need to look up the route for redirect in ip6_redirect
> >> or ip6_redirect_no_header, in this case, we do the verification and route 
> >> updating in the upper protocols' err_handler is better. 
> >>
> >> How do you think of this?
> > 
> > This should not be a problem, because every cached dst should be validated
> > with ip6_dst_check before it is used. It uses the fib6_node serial number
> > which is incremented for all fib6_nodes on the path to the new installed
> > node by fib6_add_1. So we are safe here.
> > 
> > Btw. this is the same logic redirects get currently picked up, too.
> > 
> 
> Thanks for you answer, but i still have some questions on dealing with redirect
> in ip4ip6_err() and ipip6_err(), and i need some time to learn more about them.
> So i only send one patch to fix the bug.

Coverity discovered that the redirect code in ip6_tunnel.c is logically
dead, which is correct:

    639         }
    640         if (rel_type == ICMP_REDIRECT)
    641                 skb_dst(skb2)->ops->redirect(skb_dst(skb2), NULL, skb2);
    642 
    643         icmp_send(skb2, rel_type, rel_code, htonl(rel_info));

rel_type will never be ICMP_REDIRECT in line 640 because of the updates
to rel_type in ip6_tnl_err.

My guess is that we need to move the call to ->redirect to ip6_tnl_err
and afterwards set rel_msg to 0 or we factor out the calls to ->redirect
into the ndisc layer.

I hope this clears up some confusion you had in ip6ip6_err.

Greetings,

  Hannes

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/net/ipv6/ah6.c b/net/ipv6/ah6.c
index bb02e17..73784c3 100644
--- a/net/ipv6/ah6.c
+++ b/net/ipv6/ah6.c
@@ -628,7 +628,7 @@  static void ah6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
 		return;
 
 	if (type == NDISC_REDIRECT)
-		ip6_redirect(skb, net, 0, 0);
+		ip6_redirect(skb, net, skb->dev->ifindex, 0);
 	else
 		ip6_update_pmtu(skb, net, info, 0, 0);
 	xfrm_state_put(x);
diff --git a/net/ipv6/esp6.c b/net/ipv6/esp6.c
index aeac0dc..d3618a7 100644
--- a/net/ipv6/esp6.c
+++ b/net/ipv6/esp6.c
@@ -447,7 +447,7 @@  static void esp6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
 		return;
 
 	if (type == NDISC_REDIRECT)
-		ip6_redirect(skb, net, 0, 0);
+		ip6_redirect(skb, net, skb->dev->ifindex, 0);
 	else
 		ip6_update_pmtu(skb, net, info, 0, 0);
 	xfrm_state_put(x);
diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
index 7cfc8d2..73681c2 100644
--- a/net/ipv6/icmp.c
+++ b/net/ipv6/icmp.c
@@ -92,7 +92,7 @@  static void icmpv6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
 	if (type == ICMPV6_PKT_TOOBIG)
 		ip6_update_pmtu(skb, net, info, 0, 0);
 	else if (type == NDISC_REDIRECT)
-		ip6_redirect(skb, net, 0, 0);
+		ip6_redirect(skb, net, skb->dev->ifindex, 0);
 
 	if (!(type & ICMPV6_INFOMSG_MASK))
 		if (icmp6->icmp6_type == ICMPV6_ECHO_REQUEST)
diff --git a/net/ipv6/ipcomp6.c b/net/ipv6/ipcomp6.c
index 7af5aee..5636a91 100644
--- a/net/ipv6/ipcomp6.c
+++ b/net/ipv6/ipcomp6.c
@@ -76,7 +76,7 @@  static void ipcomp6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
 		return;
 
 	if (type == NDISC_REDIRECT)
-		ip6_redirect(skb, net, 0, 0);
+		ip6_redirect(skb, net, skb->dev->ifindex, 0);
 	else
 		ip6_update_pmtu(skb, net, info, 0, 0);
 	xfrm_state_put(x);
diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
index 04d31c2..70abece 100644
--- a/net/ipv6/ndisc.c
+++ b/net/ipv6/ndisc.c
@@ -1370,7 +1370,8 @@  static void ndisc_redirect_rcv(struct sk_buff *skb)
 		return;
 
 	if (!ndopts.nd_opts_rh) {
-		ip6_redirect_no_header(skb, dev_net(skb->dev), 0, 0);
+		ip6_redirect_no_header(skb, dev_net(skb->dev),
+					skb->dev->ifindex, 0);
 		return;
 	}
 
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 8d9a93e..a01337f 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -1157,6 +1157,73 @@  void ip6_sk_update_pmtu(struct sk_buff *skb, struct sock *sk, __be32 mtu)
 }
 EXPORT_SYMBOL_GPL(ip6_sk_update_pmtu);
 
+/* Handle redirects */
+struct ip6rd_flowi {
+	struct flowi6 fl6;
+	struct in6_addr gateway;
+};
+
+static struct rt6_info *__ip6_route_redirect(struct net *net,
+					     struct fib6_table *table,
+					     struct flowi6 *fl6,
+					     int flags)
+{
+	struct ip6rd_flowi *rdfl = (struct ip6rd_flowi *)fl6;
+	struct rt6_info *rt;
+	struct fib6_node *fn;
+
+	/* Get the "current" route for this destination and
+	 * check if the redirect has come from approriate router.
+	 *
+	 * RFC 4861 specifies that redirects should only be
+	 * accepted if they come from the nexthop to the target.
+	 * Due to the way the routes are chosen, this notion
+	 * is a bit fuzzy and one might need to check all possible
+	 * routes.
+	 */
+
+	read_lock_bh(&table->tb6_lock);
+	fn = fib6_lookup(&table->tb6_root, &fl6->daddr, &fl6->saddr);
+restart:
+	for (rt = fn->leaf; rt; rt = rt->dst.rt6_next) {
+		if (rt6_check_expired(rt))
+			continue;
+		if (rt->dst.error)
+			continue;
+		if (!(rt->rt6i_flags & RTF_GATEWAY))
+			continue;
+		if (fl6->flowi6_oif != rt->dst.dev->ifindex)
+			continue;
+		if (!ipv6_addr_equal(&rdfl->gateway, &rt->rt6i_gateway))
+			continue;
+		break;
+	}
+
+	if (!rt)
+		rt = net->ipv6.ip6_null_entry;
+	BACKTRACK(net, &fl6->saddr);
+out:
+	dst_hold(&rt->dst);
+
+	read_unlock_bh(&table->tb6_lock);
+
+	return rt;
+};
+
+static struct dst_entry *ip6_route_redirect(struct net *net,
+					const struct flowi6 *fl6,
+					const struct in6_addr *gateway)
+{
+	int flags = RT6_LOOKUP_F_HAS_SADDR;
+	struct ip6rd_flowi rdfl;
+
+	rdfl.fl6 = *fl6;
+	rdfl.gateway = *gateway;
+
+	return fib6_rule_lookup(net, &rdfl.fl6,
+				flags, __ip6_route_redirect);
+}
+
 void ip6_redirect(struct sk_buff *skb, struct net *net, int oif, u32 mark)
 {
 	const struct ipv6hdr *iph = (struct ipv6hdr *) skb->data;
@@ -1171,9 +1238,8 @@  void ip6_redirect(struct sk_buff *skb, struct net *net, int oif, u32 mark)
 	fl6.saddr = iph->saddr;
 	fl6.flowlabel = ip6_flowinfo(iph);
 
-	dst = ip6_route_output(net, NULL, &fl6);
-	if (!dst->error)
-		rt6_do_redirect(dst, NULL, skb);
+	dst = ip6_route_redirect(net, &fl6, &ipv6_hdr(skb)->saddr);
+	rt6_do_redirect(dst, NULL, skb);
 	dst_release(dst);
 }
 EXPORT_SYMBOL_GPL(ip6_redirect);
@@ -1193,9 +1259,8 @@  void ip6_redirect_no_header(struct sk_buff *skb, struct net *net, int oif,
 	fl6.daddr = msg->dest;
 	fl6.saddr = iph->daddr;
 
-	dst = ip6_route_output(net, NULL, &fl6);
-	if (!dst->error)
-		rt6_do_redirect(dst, NULL, skb);
+	dst = ip6_route_redirect(net, &fl6, &iph->saddr);
+	rt6_do_redirect(dst, NULL, skb);
 	dst_release(dst);
 }