ICMP redirect issue

Hi,

While investigating an issue on Red Hat Enterprise Linux, I found that
upstream commit below removed the old_gw check.

commit f39925dbde7788cfb96419c0f092b086aa325c0f
Author: David S. Miller [off-list ref]
Date:   Wed Feb 9 22:00:16 2011 -0800

    ipv4: Cache learned redirect information in inetpeer.

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.

In terms of a security, if someone manages to send ICMP redirect, then
I think it possible to fake the saddr to appear as coming from the
correct gateway.

So, I'm not seeing a problem, but I was told to bring this up to netdev.
Thoughts?

thanks,
fbl

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

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 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.

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;

From: Flavio Leitner <redacted>
Date: Wed, 28 Sep 2011 17:19:52 -0300

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

The previous check was stronger, and served other purposes.

Firstly, it required that the spoofer know the exact gateway
IP address we used previously, whereas your test requires only
knowing the subnet which is easier to figure out.

But more importantly, the old test allowed us to ignore outdated
or erroneous redirects.

We really have to restore the original behavior before my inetpeer
changes (enforce that the old gateway matches), and find another way
to accomodate IPVS.

From: David Miller <davem@davemloft.net>
Date: Wed, 28 Sep 2011 18:56:54 -0400 (EDT)

From: Flavio Leitner <redacted>
Date: Wed, 28 Sep 2011 17:19:52 -0300

quoted

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

The previous check was stronger, and served other purposes.

Firstly, it required that the spoofer know the exact gateway
IP address we used previously, whereas your test requires only
knowing the subnet which is easier to figure out.

But more importantly, the old test allowed us to ignore outdated
or erroneous redirects.

We really have to restore the original behavior before my inetpeer
changes (enforce that the old gateway matches), and find another way
to accomodate IPVS.

BTW, I just double-checked RFC1122 and it explicitly specifies the
old_gw check:

[ RFC1122, section 3.2.2.2 ]

 ...

	A Redirect message SHOULD be silently discarded if the new
        gateway address it specifies is not on the same connected
        (sub-) net through which the Redirect arrived [INTRO:2,
        Appendix A], or if the source of the Redirect is not the
        current first-hop gateway for the specified destination (see
        Section 3.3.1).

In fact, it's saying that we should also validate that saddr == old_gw
too.

So really, we need to put the check back and find a way to accomodate IPVS.

On Wed, Sep 28, 2011 at 07:12:55PM -0400, David Miller wrote:

From: David Miller <davem@davemloft.net>
Date: Wed, 28 Sep 2011 18:56:54 -0400 (EDT)

quoted

From: Flavio Leitner <redacted>
Date: Wed, 28 Sep 2011 17:19:52 -0300

quoted

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

The previous check was stronger, and served other purposes.

Firstly, it required that the spoofer know the exact gateway
IP address we used previously, whereas your test requires only
knowing the subnet which is easier to figure out.

But more importantly, the old test allowed us to ignore outdated
or erroneous redirects.

We really have to restore the original behavior before my inetpeer
changes (enforce that the old gateway matches), and find another way
to accomodate IPVS.

BTW, I just double-checked RFC1122 and it explicitly specifies the
old_gw check:

[ RFC1122, section 3.2.2.2 ]

 ...

	A Redirect message SHOULD be silently discarded if the new
        gateway address it specifies is not on the same connected
        (sub-) net through which the Redirect arrived [INTRO:2,
        Appendix A], or if the source of the Redirect is not the
        current first-hop gateway for the specified destination (see
        Section 3.3.1).

In fact, it's saying that we should also validate that saddr == old_gw
too.

So really, we need to put the check back and find a way to accomodate IPVS.

Hi Dave,

I'm have to admit that this issues is new to me.
But doesn't it affect any setup where a secondary
address is being used as the gateway and the gateway
send an ICMP redirect?

Perhaps an option to weaken the check for these cases
would provide a work-around for those who need it.
Or does that break your inetpeer changes horribly?