Re: ICMP redirect issue

On Wed, 28 Sep 2011 14:06:32 -0400 (EDT)
David Miller [off-list ref] wrote:

From: Flavio Leitner <redacted>
Date: Tue, 27 Sep 2011 16:21:20 -0300

quoted

The issue is about the gateway being a LVS, so the servers behind
use the IP alias address as the default gateway.  However, when the
gateway sends an ICMP redirect, it comes from the primary IP
address which is ignored on older kernels because of the old_gw
check:

-                               if (rth->rt_dst != daddr ||
-                                   rth->rt_src != saddr ||
-                                   rth->dst.error ||
-                                   rth->rt_gateway != old_gw ||
-                                   rth->dst.dev != dev)
-                                       break;


Well, the consequence is that the issue doesn't happen in newer
kernels because it happily accepts the ICMP redirect.

The admin can still control using shared_media and secure_redirects
if the host should accept only the ICMP redirects for gateways
listed in default gateway list or not.

Unfortunately, shared_media is on by default which means the default
secure_redirects setting of '1' is ignored.

This means that redirects can be spoofed in the default configuration,
but with the above check they would not be spoofable.

I fail to see what that check is preventing because if someone manages
to inject a redirect packet into the network, then likely the old_gw can
be tweaked to be the network gateway.

I suspect that, because of this, we'll need to add the check back.  Or
do something similar.

We can't "fix" this by turning shared_media off by default because
that changes behavior on input route processing wrt. how we decide
whether to emit a redirect or not.

What about something like below? It will change a bit the
secure_redirects documentation.

shared_media  secure_redirect  behavior:
      0             0          all pass.
      0             1          only from gateways and for gateways.
      1             0          all pass.
      1             1          default, old behavior, only from
                               gateways.

If you agree with the approach, I'll run tests here and post
the patch with a proper changelog, documentation and signed-off.

thanks,
fbl

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 075212e..fa00fcd 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c

@@ -1332,6 +1332,9 @@ void ip_rt_redirect(__be32 old_gw, __be32 daddr, __be32 new_gw,
 			goto reject_redirect;
 	}
 
+	if (IN_DEV_SEC_REDIRECTS(in_dev) && ip_fib_check_default(old_gw, dev))
+		goto reject_redirect;
+
 	peer = inet_getpeer_v4(daddr, 1);
 	if (peer) {
 		peer->redirect_learned.a4 = new_gw;