[PATCH] route: fix ICMP secure_redirects

[PATCH] route: fix ICMP secure_redirects

[Flavio Leitner]

It should accept ICMP redirects from any host and not
just from gateways when secure_redirects is disabled.

Signed-off-by: Flavio Leitner <fbl@redhat.com>
---
 net/ipv4/route.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 155138d..dd6937ec 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -1347,7 +1347,8 @@ void ip_rt_redirect(__be32 old_gw, __be32 daddr, __be32 new_gw,
 				continue;
 
 			if (rt->dst.error || rt->dst.dev != dev ||
-			    rt->rt_gateway != old_gw) {
+			    (IN_DEV_SEC_REDIRECTS(in_dev) &&
+			    rt->rt_gateway != old_gw)) {
 				ip_rt_put(rt);
 				continue;
 			}
-- 
1.7.6

Re: [PATCH] route: fix ICMP secure_redirects

[David Miller]

From: Flavio Leitner <fbl@redhat.com>
Date: Mon,  7 Nov 2011 13:41:45 -0200

> It should accept ICMP redirects from any host and not
> just from gateways when secure_redirects is disabled.
> 
> Signed-off-by: Flavio Leitner <fbl@redhat.com>

This is changing the default behavior, and could break things for people.

We have sort-of discussed this already, and agreed that the tests made in
this code before my inetpeer reworking had to be reinstated exactly as it
was.

Re: [PATCH] route: fix ICMP secure_redirects

[Flavio Leitner]

On Mon, 07 Nov 2011 13:35:41 -0500 (EST)
David Miller <davem@davemloft.net> wrote:

> From: Flavio Leitner <fbl@redhat.com>
> Date: Mon,  7 Nov 2011 13:41:45 -0200
> 
> > It should accept ICMP redirects from any host and not
> > just from gateways when secure_redirects is disabled.
> > 
> > Signed-off-by: Flavio Leitner <fbl@redhat.com>
> 
> This is changing the default behavior, and could break things for
> people.
> 
> We have sort-of discussed this already, and agreed that the tests
> made in this code before my inetpeer reworking had to be reinstated
> exactly as it was.

Right, so I cannot change either values 0 or 1 then. For some
reason I thought I couldn't change only the default behavior.
I will think on something else then.
thanks,
fbl

[PATCH] route: add more relaxed option for secure_redirects

[Flavio Leitner]

When the host uses a gateway IP address that is actually an alias
address, the ICMP redirect message source address can be the gateway's
main IP address, so the message is ignored by the host regardless of
the secure_redirects setup.

The new value (2) allows that ICMP message to be processed.
The possible values are:

 0 - Accept ICMP redirect messages only if its source address is the
     previous gateway address.
 1 - The same as above. However, if shared_media is FALSE, it has to
     be for gateways listed in default gateway list as well.
 2 - Accept ICMP redirects messages ignoring the conditions above.
 default value is 1.

Signed-off-by: Flavio Leitner <fbl@redhat.com>
---
 Documentation/networking/ip-sysctl.txt |   17 ++++++++++-------
 include/linux/icmp.h                   |    5 +++++
 include/linux/inetdevice.h             |    2 +-
 net/ipv4/route.c                       |    6 ++++--
 4 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/Documentation/networking/ip-sysctl.txt b/Documentation/networking/ip-sysctl.txt
index cb7f314..f17727d 100644
--- a/Documentation/networking/ip-sysctl.txt
+++ b/Documentation/networking/ip-sysctl.txt
@@ -805,13 +805,16 @@ shared_media - BOOLEAN
 	it will be disabled otherwise
 	default TRUE
 
-secure_redirects - BOOLEAN
-	Accept ICMP redirect messages only for gateways,
-	listed in default gateway list.
-	secure_redirects for the interface will be enabled if at least one of
-	conf/{all,interface}/secure_redirects is set to TRUE,
-	it will be disabled otherwise
-	default TRUE
+secure_redirects - INTEGER
+	0 - Accept ICMP redirect messages only if its source address is the
+	    previous gateway address.
+	1 - The same as above. However, if shared_media is FALSE, it has to
+	    be for gateways listed in default gateway list as well.
+	2 - Accept ICMP redirects messages ignoring the conditions above.
+	default value is 1.
+
+	The max value from conf/{all,interface}/secure_redirects is used
+	when doing the ICMP redirect message validation on the {interface}.
 
 send_redirects - BOOLEAN
 	Send redirects, if router.
diff --git a/include/linux/icmp.h b/include/linux/icmp.h
index 474f2a5..77f0c2d 100644
--- a/include/linux/icmp.h
+++ b/include/linux/icmp.h
@@ -64,6 +64,11 @@
 #define ICMP_EXC_TTL		0	/* TTL count exceeded		*/
 #define ICMP_EXC_FRAGTIME	1	/* Fragment Reass time exceeded	*/
 
+/* secure_redirects sysctl */
+#define ICMP_SEC_REDIR_OLDGW	0	/* accept only from old gw	*/
+#define ICMP_SEC_REDIR_TOGW	1	/* accept only from old gw and
+					 * to known listed gw */
+#define ICMP_SEC_REDIR_ANY	2	/* accept from any host		*/
 
 struct icmphdr {
   __u8		type;
diff --git a/include/linux/inetdevice.h b/include/linux/inetdevice.h
index 5f81466..eca2cfb 100644
--- a/include/linux/inetdevice.h
+++ b/include/linux/inetdevice.h
@@ -124,7 +124,7 @@ static inline void ipv4_devconf_setall(struct in_device *in_dev)
 #define IN_DEV_PROXY_ARP_PVLAN(in_dev)	IN_DEV_CONF_GET(in_dev, PROXY_ARP_PVLAN)
 #define IN_DEV_SHARED_MEDIA(in_dev)	IN_DEV_ORCONF((in_dev), SHARED_MEDIA)
 #define IN_DEV_TX_REDIRECTS(in_dev)	IN_DEV_ORCONF((in_dev), SEND_REDIRECTS)
-#define IN_DEV_SEC_REDIRECTS(in_dev)	IN_DEV_ORCONF((in_dev), \
+#define IN_DEV_SEC_REDIRECTS(in_dev)	IN_DEV_MAXCONF((in_dev), \
 						      SECURE_REDIRECTS)
 #define IN_DEV_IDTAG(in_dev)		IN_DEV_CONF_GET(in_dev, TAG)
 #define IN_DEV_MEDIUM_ID(in_dev)	IN_DEV_CONF_GET(in_dev, MEDIUM_ID)
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 155138d..ec3ce37 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -1329,7 +1329,8 @@ void ip_rt_redirect(__be32 old_gw, __be32 daddr, __be32 new_gw,
 	if (!IN_DEV_SHARED_MEDIA(in_dev)) {
 		if (!inet_addr_onlink(in_dev, new_gw, old_gw))
 			goto reject_redirect;
-		if (IN_DEV_SEC_REDIRECTS(in_dev) && ip_fib_check_default(new_gw, dev))
+		if (IN_DEV_SEC_REDIRECTS(in_dev) == ICMP_SEC_REDIR_TOGW &&
+		    ip_fib_check_default(new_gw, dev))
 			goto reject_redirect;
 	} else {
 		if (inet_addr_type(net, new_gw) != RTN_UNICAST)
@@ -1347,7 +1348,8 @@ void ip_rt_redirect(__be32 old_gw, __be32 daddr, __be32 new_gw,
 				continue;
 
 			if (rt->dst.error || rt->dst.dev != dev ||
-			    rt->rt_gateway != old_gw) {
+			    (IN_DEV_SEC_REDIRECTS(in_dev) != ICMP_SEC_REDIR_ANY &&
+			    rt->rt_gateway != old_gw)) {
 				ip_rt_put(rt);
 				continue;
 			}
-- 
1.7.6

Re: [PATCH] route: add more relaxed option for secure_redirects

[David Miller]

From: Flavio Leitner <fbl@redhat.com>
Date: Mon,  7 Nov 2011 22:03:50 -0200

> When the host uses a gateway IP address that is actually an alias
> address, the ICMP redirect message source address can be the gateway's
> main IP address, so the message is ignored by the host regardless of
> the secure_redirects setup.
> 
> The new value (2) allows that ICMP message to be processed.
> The possible values are:
> 
>  0 - Accept ICMP redirect messages only if its source address is the
>      previous gateway address.
>  1 - The same as above. However, if shared_media is FALSE, it has to
>      be for gateways listed in default gateway list as well.
>  2 - Accept ICMP redirects messages ignoring the conditions above.
>  default value is 1.
> 
> Signed-off-by: Flavio Leitner <fbl@redhat.com>

The more I look at this the less I like it.

Look, if IPVS or whatever is translating addresses and this is what
causes the problem then this entity can very well translate the damn
addresses right back in the redirect so it looks legitimate to the
sender.

You can't translate people's addresses, and them let them see that
intenal remapping in ICMP errors.  The redirect is dropped by the
sender because it not only looks like crap, it is crap.

This is fundamentally not the correct way to handle this.

I'm not applying this patch.

Re: [PATCH] route: add more relaxed option for secure_redirects

[Flavio Leitner]

On Fri, 11 Nov 2011 20:33:21 -0500 (EST)
David Miller <davem@davemloft.net> wrote:

> From: Flavio Leitner <fbl@redhat.com>
> Date: Mon,  7 Nov 2011 22:03:50 -0200
> 
> > When the host uses a gateway IP address that is actually an alias
> > address, the ICMP redirect message source address can be the
> > gateway's main IP address, so the message is ignored by the host
> > regardless of the secure_redirects setup.
> > 
> > The new value (2) allows that ICMP message to be processed.
> > The possible values are:
> > 
> >  0 - Accept ICMP redirect messages only if its source address is the
> >      previous gateway address.
> >  1 - The same as above. However, if shared_media is FALSE, it has to
> >      be for gateways listed in default gateway list as well.
> >  2 - Accept ICMP redirects messages ignoring the conditions above.
> >  default value is 1.
> > 
> > Signed-off-by: Flavio Leitner <fbl@redhat.com>
> 
> The more I look at this the less I like it.
> 
> Look, if IPVS or whatever is translating addresses and this is what
> causes the problem then this entity can very well translate the damn
> addresses right back in the redirect so it looks legitimate to the
> sender.
> 
I thought about that, see below.

> You can't translate people's addresses, and them let them see that
> intenal remapping in ICMP errors.  The redirect is dropped by the
> sender because it not only looks like crap, it is crap.
> 
> This is fundamentally not the correct way to handle this.
> 

I agree. The problem is that the communication is between the host
and another external host, so it's not with the gateway. Therefore,
the original packet has the saddr of the origin host and the daddr
is an external host.  When the gateway receives that packet, there
is no way to tell which IP address should be used to reply with.

Today Linux picks the primary address of an existing interface.
There is the sysctl icmp_errors_use_inbound_ifaddr to use the
primary address of the interface that _received_ the packet that
caused the icmp error. Yet, it doesn't help if the host used
the alias address (i.e. secondary address) as the gw address.

It may possible to promote the secondary address to be the primary
one in the gateway, but this can't be done if you use more than
just one address in the same subnet as gw for some reason.

Thus, the only option at the sender side would be using iptables
to change the ICMP redirect source address to be the float address,
but that is not working as well. (It isn't passing through -t nat)

fbl

Re: [PATCH] route: add more relaxed option for secure_redirects

[David Miller]

From: Flavio Leitner <fbl@redhat.com>
Date: Wed, 16 Nov 2011 18:46:12 -0200

> Thus, the only option at the sender side would be using iptables
> to change the ICMP redirect source address to be the float address,
> but that is not working as well. (It isn't passing through -t nat)

If it's going to mangle the packet in one direct, the only option
for sane operation is to make the exact reverse transformation in
the other direction for ICMP messages.

I'm sorry to be so difficult about this, but this is the only way to
handle this problem.  If packet mangling is performed to change the
world, that mangling entity has taken on the responsibility to make
everything look correct to all entities for the mangled packets
and any packets generated in response to such mangled packets.

Re: [PATCH] route: add more relaxed option for secure_redirects

[Flavio Leitner]

On Wed, 16 Nov 2011 17:02:13 -0500 (EST)
David Miller <davem@davemloft.net> wrote:

> From: Flavio Leitner <fbl@redhat.com>
> Date: Wed, 16 Nov 2011 18:46:12 -0200
> 
> > Thus, the only option at the sender side would be using iptables
> > to change the ICMP redirect source address to be the float address,
> > but that is not working as well. (It isn't passing through -t nat)
> 
> If it's going to mangle the packet in one direct, the only option
> for sane operation is to make the exact reverse transformation in
> the other direction for ICMP messages.
> 
> I'm sorry to be so difficult about this, but this is the only way to
> handle this problem.  If packet mangling is performed to change the
> world, that mangling entity has taken on the responsibility to make
> everything look correct to all entities for the mangled packets
> and any packets generated in response to such mangled packets.
>

I'm sorry, I lost you there. There is no transformation happening in
any side. The iptables is just a work around to force the outgoing
ICMP redirect to use the correct source address (secondary or alias). 

The whole problem is the linux gateway sending ICMP redirects using
*always* the primary address.

fbl

Re: [PATCH] route: add more relaxed option for secure_redirects

[Flavio Leitner]

On Wed, 16 Nov 2011 21:17:38 -0200
Flavio Leitner <fbl@redhat.com> wrote:

> On Wed, 16 Nov 2011 17:02:13 -0500 (EST)
> David Miller <davem@davemloft.net> wrote:
> 
> > From: Flavio Leitner <fbl@redhat.com>
> > Date: Wed, 16 Nov 2011 18:46:12 -0200
> > 
> > > Thus, the only option at the sender side would be using iptables
> > > to change the ICMP redirect source address to be the float
> > > address, but that is not working as well. (It isn't passing
> > > through -t nat)
> > 
> > If it's going to mangle the packet in one direct, the only option
> > for sane operation is to make the exact reverse transformation in
> > the other direction for ICMP messages.
> > 
> > I'm sorry to be so difficult about this, but this is the only way to
> > handle this problem.  If packet mangling is performed to change the
> > world, that mangling entity has taken on the responsibility to make
> > everything look correct to all entities for the mangled packets
> > and any packets generated in response to such mangled packets.
> >
> 
> I'm sorry, I lost you there. There is no transformation happening in
> any side. The iptables is just a work around to force the outgoing
> ICMP redirect to use the correct source address (secondary or alias). 
> 
> The whole problem is the linux gateway sending ICMP redirects using
> *always* the primary address.
> 

To make sure we are in the same page, this simple setup reproduces
the issue.

IP: 10.0.0.1
gw: 10.0.0.100
+--------+          +-----+ primary: 10.0.0.2
| client |----+-----| GW1 |   alias: 10.0.0.100
+--------+    |     +-----+      gw: 10.0.0.254
           +--+--+
           | GW2 |---> internet
           +-----+
         10.0.0.254

1. Client sends TCP SYN to an internet host using
   GW1 alias address as default gw address

2. Then GW1 sends the ICMP redirect back to client
   using the primary address as source address.

3. GW1 forwards the original packet to GW2

4. client ignores the ICMP redirect because
   client.gw != gw1.primary.

Regards,
fbl

Re: [PATCH] route: add more relaxed option for secure_redirects

[David Miller]

From: Flavio Leitner <fbl@redhat.com>
Date: Wed, 16 Nov 2011 23:40:42 -0200

> To make sure we are in the same page, this simple setup reproduces
> the issue.

Thanks for the description, I'll think this over.

Re: [PATCH] route: add more relaxed option for secure_redirects

[David Miller]

From: Flavio Leitner <fbl@redhat.com>
Date: Wed, 16 Nov 2011 23:40:42 -0200

> To make sure we are in the same page, this simple setup reproduces
> the issue.
> 
> IP: 10.0.0.1
> gw: 10.0.0.100
> +--------+          +-----+ primary: 10.0.0.2
> | client |----+-----| GW1 |   alias: 10.0.0.100
> +--------+    |     +-----+      gw: 10.0.0.254
>            +--+--+
>            | GW2 |---> internet
>            +-----+
>          10.0.0.254
> 
> 1. Client sends TCP SYN to an internet host using
>    GW1 alias address as default gw address
> 
> 2. Then GW1 sends the ICMP redirect back to client
>    using the primary address as source address.
> 
> 3. GW1 forwards the original packet to GW2
> 
> 4. client ignores the ICMP redirect because
>    client.gw != gw1.primary.

GW1 must respond using a source address matching 'alias', ie.
10.0.0.100 and I would accept a mechinsm to make sure that happens,
if not by default then via a sysctl or similar control.