diff mbox

ICMP redirect issue

Message ID 20110928171952.0c0d2d05@asterix.rh
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Flavio Leitner Sept. 28, 2011, 8:19 p.m. UTC 
On Wed, 28 Sep 2011 14:06:32 -0400 (EDT)
David Miller <davem@davemloft.net> wrote:

> From: Flavio Leitner <fbl@redhat.com>
> 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 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

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

Comments

David Miller Sept. 28, 2011, 10:56 p.m. UTC | #1 
From: Flavio Leitner <fbl@redhat.com>
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.
--
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
David Miller Sept. 28, 2011, 11:12 p.m. UTC | #2 
From: David Miller <davem@davemloft.net>
Date: Wed, 28 Sep 2011 18:56:54 -0400 (EDT)

> From: Flavio Leitner <fbl@redhat.com>
> 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.

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.
--
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
Simon Horman Oct. 1, 2011, 3:22 a.m. UTC | #3 
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)
> 
> > From: Flavio Leitner <fbl@redhat.com>
> > 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.
> 
> 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?

--
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/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;