[PATCH net-next] ipv4: one more case for non-local saddr in ICMP

[PATCH net-next] ipv4: one more case for non-local saddr in ICMP

[Julian Anastasov]

	May be there is one more case that we can avoid using
non-local source for ICMP errors: xfrm_lookup, num_xfrms = 0 when
reverse "Flow passes untransformed". Avoid using the input route
if xfrm_lookup returns same dst.

Signed-off-by: Julian Anastasov <ja@ssi.bg>
---

	In fact, should we use local IP in all cases when
sending ICMP? I'm asking it for the following case:

	Large packet is forwarded but is rejected with ICMP FRAG
NEEDED. We usually send ICMP with local saddr instead of the
original non-local destination. What is the role of
this reverse check? May be after xfrm_decode_session_reverse
we should use 'fl4_dec.saddr = fl4->saddr;' so that xfrm_lookup
works with ICMP from local IP? What is right thing to do here?
I don't see code that looks in the embedded header...

--- net-next/net/ipv4/icmp.c	2011-08-15 18:42:21.289899686 +0300
+++ linux/net/ipv4/icmp.c	2011-08-15 18:38:25.816244111 +0300
@@ -379,7 +379,7 @@ static struct rtable *icmp_route_lookup(
 					int type, int code,
 					struct icmp_bxm *param)
 {
-	struct rtable *rt, *rt2;
+	struct rtable *rt, *rt2, *rt3;
 	struct flowi4 fl4_dec;
 	int err;
 
@@ -440,13 +440,18 @@ static struct rtable *icmp_route_lookup(
 	if (err)
 		goto relookup_failed;
 
+	rt3 = rt2;
 	rt2 = (struct rtable *) xfrm_lookup(net, &rt2->dst,
 					    flowi4_to_flowi(&fl4_dec), NULL,
 					    XFRM_LOOKUP_ICMP);
 	if (!IS_ERR(rt2)) {
-		dst_release(&rt->dst);
-		memcpy(fl4, &fl4_dec, sizeof(*fl4));
-		rt = rt2;
+		if (rt2 == rt3 && rt) {
+			dst_release(&rt2->dst);
+		} else {
+			dst_release(&rt->dst);
+			memcpy(fl4, &fl4_dec, sizeof(*fl4));
+			rt = rt2;
+		}
 	} else if (PTR_ERR(rt2) == -EPERM) {
 		if (rt)
 			dst_release(&rt->dst);

Re: [PATCH net-next] ipv4: one more case for non-local saddr in ICMP

[David Miller]

From: Julian Anastasov <ja@ssi.bg>
Date: Mon, 15 Aug 2011 19:21:23 +0300 (EEST)

> 
> 	May be there is one more case that we can avoid using
> non-local source for ICMP errors: xfrm_lookup, num_xfrms = 0 when
> reverse "Flow passes untransformed". Avoid using the input route
> if xfrm_lookup returns same dst.
> 
> Signed-off-by: Julian Anastasov <ja@ssi.bg>
> ---
> 
> 	In fact, should we use local IP in all cases when
> sending ICMP? I'm asking it for the following case:
> 
> 	Large packet is forwarded but is rejected with ICMP FRAG
> NEEDED. We usually send ICMP with local saddr instead of the
> original non-local destination. What is the role of
> this reverse check? May be after xfrm_decode_session_reverse
> we should use 'fl4_dec.saddr = fl4->saddr;' so that xfrm_lookup
> works with ICMP from local IP? What is right thing to do here?
> I don't see code that looks in the embedded header...

Well.. this relookup behavior is guided by a special transform state
XFRM_STATE_ICMP that the user must explicitly create IPSEC rules for.

Presumably they are going to add real transforms to such special IPSEC
rules, not create NOP ones with no transforms.  And if they do create
such IPSEC state with no transforms, perhaps the intention is to trigger
to use of the non-local source.

The whole thing revolves around how Herbert envisions people implementing
RFC4301 support using this new XFRM_STATE_ICMP thing.

Right?

Re: [PATCH net-next] ipv4: one more case for non-local saddr in ICMP

[Herbert Xu]

On Fri, Aug 19, 2011 at 03:43:54AM -0700, David Miller wrote:
> From: Julian Anastasov <ja@ssi.bg>
> Date: Mon, 15 Aug 2011 19:21:23 +0300 (EEST)
> 
> > 
> > 	May be there is one more case that we can avoid using
> > non-local source for ICMP errors: xfrm_lookup, num_xfrms = 0 when
> > reverse "Flow passes untransformed". Avoid using the input route
> > if xfrm_lookup returns same dst.
> > 
> > Signed-off-by: Julian Anastasov <ja@ssi.bg>
> > ---
> > 
> > 	In fact, should we use local IP in all cases when
> > sending ICMP? I'm asking it for the following case:
> > 
> > 	Large packet is forwarded but is rejected with ICMP FRAG
> > NEEDED. We usually send ICMP with local saddr instead of the
> > original non-local destination. What is the role of
> > this reverse check? May be after xfrm_decode_session_reverse
> > we should use 'fl4_dec.saddr = fl4->saddr;' so that xfrm_lookup
> > works with ICMP from local IP? What is right thing to do here?
> > I don't see code that looks in the embedded header...
> 
> Well.. this relookup behavior is guided by a special transform state
> XFRM_STATE_ICMP that the user must explicitly create IPSEC rules for.
> 
> Presumably they are going to add real transforms to such special IPSEC
> rules, not create NOP ones with no transforms.  And if they do create
> such IPSEC state with no transforms, perhaps the intention is to trigger
> to use of the non-local source.
> 
> The whole thing revolves around how Herbert envisions people implementing
> RFC4301 support using this new XFRM_STATE_ICMP thing.
> 
> Right?

The intention of XFRM_STATE_ICMP is to automatically allow inbound
IPsec-protected ICMP packets (remember that IPsec tunnels are not
automatically allowed, as that opens room for address spoofing).

Imagine if you have a policy P that allows IPsec packets with inner
addresses going from S to D.  The purpose of this is to ensure that
ICMP packets from D to S are automatically allowed.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH net-next] ipv4: one more case for non-local saddr in ICMP

[Julian Anastasov]

	Hello,

On Sat, 20 Aug 2011, Herbert Xu wrote:

> On Fri, Aug 19, 2011 at 03:43:54AM -0700, David Miller wrote:
> > From: Julian Anastasov <ja@ssi.bg>
> > Date: Mon, 15 Aug 2011 19:21:23 +0300 (EEST)
> > 
> > > 
> > > 	May be there is one more case that we can avoid using
> > > non-local source for ICMP errors: xfrm_lookup, num_xfrms = 0 when
> > > reverse "Flow passes untransformed". Avoid using the input route
> > > if xfrm_lookup returns same dst.
> > > 
> > > Signed-off-by: Julian Anastasov <ja@ssi.bg>
> > > ---
> > > 
> > > 	In fact, should we use local IP in all cases when
> > > sending ICMP? I'm asking it for the following case:
> > > 
> > > 	Large packet is forwarded but is rejected with ICMP FRAG
> > > NEEDED. We usually send ICMP with local saddr instead of the
> > > original non-local destination. What is the role of
> > > this reverse check? May be after xfrm_decode_session_reverse
> > > we should use 'fl4_dec.saddr = fl4->saddr;' so that xfrm_lookup
> > > works with ICMP from local IP? What is right thing to do here?
> > > I don't see code that looks in the embedded header...
> > 
> > Well.. this relookup behavior is guided by a special transform state
> > XFRM_STATE_ICMP that the user must explicitly create IPSEC rules for.
> > 
> > Presumably they are going to add real transforms to such special IPSEC
> > rules, not create NOP ones with no transforms.  And if they do create
> > such IPSEC state with no transforms, perhaps the intention is to trigger
> > to use of the non-local source.
> > 
> > The whole thing revolves around how Herbert envisions people implementing
> > RFC4301 support using this new XFRM_STATE_ICMP thing.
> > 
> > Right?

	I see, RFC 4301 prescribes reverse check for ICMP errors
to reuse SA as a fallback mechanism. Even if we send ICMP packet via
some tunnel to the previous secure gateway, shouldn't the ICMP
receiver prefer to see our local IP as ICMP sender?

> The intention of XFRM_STATE_ICMP is to automatically allow inbound
> IPsec-protected ICMP packets (remember that IPsec tunnels are not
> automatically allowed, as that opens room for address spoofing).
> 
> Imagine if you have a policy P that allows IPsec packets with inner
> addresses going from S to D.  The purpose of this is to ensure that
> ICMP packets from D to S are automatically allowed.

	OK, thanks for the information! I don't have setup for
further tests, I hope things still work somehow, even if we
fallback to unprotected ICMP with non-local source which was
the only concern for me. I think, using local IP for ICMP packets
is preferred to help firewalling.

	I just notice that if original packet (say TCP) hits
"fwd" policy, for the locally generated ICMP error we call
xfrm_lookup with sk = NULL and it always uses "out" dir
for the flow_cache_lookup call to search for this reverse SA.
So, we must be prepared with rules in "out" dir even for
the forwarding case? icmp flag in "fwd" rules is not considered?

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [PATCH net-next] ipv4: one more case for non-local saddr in ICMP

[Herbert Xu]

On Tue, Aug 23, 2011 at 09:34:45AM +0300, Julian Anastasov wrote:
>
> 	I just notice that if original packet (say TCP) hits
> "fwd" policy, for the locally generated ICMP error we call
> xfrm_lookup with sk = NULL and it always uses "out" dir
> for the flow_cache_lookup call to search for this reverse SA.
> So, we must be prepared with rules in "out" dir even for
> the forwarding case? icmp flag in "fwd" rules is not considered?

This is a common question about our IPsec stack.  Both fwd
and in are used for ingress, with the only difference being
whether the packet is locally destined or not.  Only out is
used for egress traffic.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH net-next] ipv4: one more case for non-local saddr in ICMP

[David Miller]

From: Julian Anastasov <ja@ssi.bg>
Date: Mon, 15 Aug 2011 19:21:23 +0300 (EEST)

> 	May be there is one more case that we can avoid using
> non-local source for ICMP errors: xfrm_lookup, num_xfrms = 0 when
> reverse "Flow passes untransformed". Avoid using the input route
> if xfrm_lookup returns same dst.
> 
> Signed-off-by: Julian Anastasov <ja@ssi.bg>

Julian I'm going to defer on this patch unless you think there is
a real pressing reason to apply it.

Thanks.

Re: [PATCH net-next] ipv4: one more case for non-local saddr in ICMP

[Julian Anastasov]

	Hello,

On Wed, 24 Aug 2011, David Miller wrote:

> From: Julian Anastasov <ja@ssi.bg>
> Date: Mon, 15 Aug 2011 19:21:23 +0300 (EEST)
> 
> > 	May be there is one more case that we can avoid using
> > non-local source for ICMP errors: xfrm_lookup, num_xfrms = 0 when
> > reverse "Flow passes untransformed". Avoid using the input route
> > if xfrm_lookup returns same dst.
> > 
> > Signed-off-by: Julian Anastasov <ja@ssi.bg>
> 
> Julian I'm going to defer on this patch unless you think there is
> a real pressing reason to apply it.

	No such reason, it solves a very very rare case.

Regards

--
Julian Anastasov <ja@ssi.bg>