[NF+IPsec 4/6]: Make IPsec input processing symetrical to output

[NF+IPsec 4/6]: Make IPsec input processing symetrical to output

[Patrick McHardy]

[-- Attachment #1: 04.diff --]
[-- Type: text/x-patch, Size: 2294 bytes --]

[NETFILTER]: Make IPsec input processing symetrical to output

Signed-off-by: Patrick McHardy <kaber@trash.net>

---
commit 9b2748c756fd805c0fe5b37505735e02f06ebb28
tree 440bd33a45b20a01481876c5b379017cceec0388
parent 92dffb4b8138637d28173cc9c10fe5990914cf5d
author Patrick McHardy <kaber@trash.net> Mon, 17 Oct 2005 01:32:19 +0200
committer Patrick McHardy <kaber@trash.net> Mon, 17 Oct 2005 01:32:19 +0200

 net/ipv4/xfrm4_input.c |   23 ++++++++++++++---------
 net/ipv6/xfrm6_input.c |   23 ++++++++++++++---------
 2 files changed, 28 insertions(+), 18 deletions(-)

diff --git a/net/ipv4/xfrm4_input.c b/net/ipv4/xfrm4_input.c
--- a/net/ipv4/xfrm4_input.c
+++ b/net/ipv4/xfrm4_input.c
@@ -137,16 +137,21 @@ int xfrm4_rcv_encap(struct sk_buff *skb,
 	memcpy(skb->sp->x+skb->sp->len, xfrm_vec, xfrm_nr*sizeof(struct sec_decap_state));
 	skb->sp->len += xfrm_nr;
 
-	if (decaps) {
-		if (!(skb->dev->flags&IFF_LOOPBACK)) {
-			dst_release(skb->dst);
-			skb->dst = NULL;
-		}
-		netif_rx(skb);
-		return 0;
-	} else {
-		return -skb->nh.iph->protocol;
+	if (!decaps) {
+		if (skb_cloned(skb) &&
+		    pskb_expand_head(skb, 0, 0, GFP_ATOMIC))
+			goto drop;
+		__skb_push(skb, skb->data - skb->nh.raw);
+		skb->nh.iph->tot_len = htons(skb->len);
+		ip_send_check(skb->nh.iph);
 	}
+	if (!(skb->dev->flags&IFF_LOOPBACK)) {
+		dst_release(skb->dst);
+		skb->dst = NULL;
+	}
+	nf_reset(skb);
+	netif_rx(skb);
+	return 0;
 
 drop_unlock:
 	spin_unlock(&x->lock);
diff --git a/net/ipv6/xfrm6_input.c b/net/ipv6/xfrm6_input.c
--- a/net/ipv6/xfrm6_input.c
+++ b/net/ipv6/xfrm6_input.c
@@ -121,16 +121,21 @@ int xfrm6_rcv_spi(struct sk_buff **pskb,
 	skb->sp->len += xfrm_nr;
 	skb->ip_summed = CHECKSUM_NONE;
 
-	if (decaps) {
-		if (!(skb->dev->flags&IFF_LOOPBACK)) {
-			dst_release(skb->dst);
-			skb->dst = NULL;
-		}
-		netif_rx(skb);
-		return -1;
-	} else {
-		return 1;
+	if (!decaps) {
+		if (skb_cloned(skb) &&
+		    pskb_expand_head(skb, 0, 0, GFP_ATOMIC))
+			goto drop;
+		/* FIXME: Jumbo payload option */
+		skb->nh.ipv6h->payload_len = htons(skb->len);
+		__skb_push(skb, skb->data - skb->nh.raw);
 	}
+	if (!(skb->dev->flags&IFF_LOOPBACK)) {
+		dst_release(skb->dst);
+		skb->dst = NULL;
+	}
+	nf_reset(skb);
+	netif_rx(skb);
+	return -1;
 
 drop_unlock:
 	spin_unlock(&x->lock);

Re: [NF+IPsec 4/6]: Make IPsec input processing symetrical to output

[YOSHIFUJI Hideaki / 吉藤英明]

In article <4352EEC8.9000602@trash.net> (at Mon, 17 Oct 2005 02:22:32 +0200), Patrick McHardy <kaber@trash.net> says:

> [NETFILTER]: Make IPsec input processing symetrical to output

I think this comment is not appropriate.
 1. They are not "NETFILTER" but rather "CORE" IPv4/6 stack.
 2. There are several known bad side effects.
    They should be described.

:
> diff --git a/net/ipv6/xfrm6_input.c b/net/ipv6/xfrm6_input.c
> --- a/net/ipv6/xfrm6_input.c
> +++ b/net/ipv6/xfrm6_input.c
> @@ -121,16 +121,21 @@ int xfrm6_rcv_spi(struct sk_buff **pskb,
>  	skb->sp->len += xfrm_nr;
>  	skb->ip_summed = CHECKSUM_NONE;
>  
> -	if (decaps) {
> -		if (!(skb->dev->flags&IFF_LOOPBACK)) {
> -			dst_release(skb->dst);
> -			skb->dst = NULL;
> -		}
> -		netif_rx(skb);
> -		return -1;
> -	} else {
> -		return 1;
> +	if (!decaps) {
> +		if (skb_cloned(skb) &&
> +		    pskb_expand_head(skb, 0, 0, GFP_ATOMIC))
> +			goto drop;
> +		/* FIXME: Jumbo payload option */
> +		skb->nh.ipv6h->payload_len = htons(skb->len);
> +		__skb_push(skb, skb->data - skb->nh.raw);
>  	}
> +	if (!(skb->dev->flags&IFF_LOOPBACK)) {
> +		dst_release(skb->dst);
> +		skb->dst = NULL;
> +	}
> +	nf_reset(skb);
> +	netif_rx(skb);
> +	return -1;
>  
>  drop_unlock:
>  	spin_unlock(&x->lock);

I diagree.
Stack should process the packet just once if it is of transport mode.
(It is okay to process one twice if it is of tunnel mode.)

--yoshfuji

Re: [NF+IPsec 4/6]: Make IPsec input processing symetrical to output

[Patrick McHardy]

YOSHIFUJI Hideaki / ^[$B5HF#1QL@^[ wrote:
> In article <4352EEC8.9000602@trash.net> (at Mon, 17 Oct 2005 02:22:32 +0200), Patrick McHardy <kaber@trash.net> says:
> 
> 
>>[NETFILTER]: Make IPsec input processing symetrical to output
> 
> 
> I think this comment is not appropriate.
>  1. They are not "NETFILTER" but rather "CORE" IPv4/6 stack.
>  2. There are several known bad side effects.
>     They should be described.

The patches are work in progress, so I didn't spent much time
on descriptive changelog entries. The sideeffects of posting
packets to the stack again are incorrect statistics and
packet sockets see packets on each pass through the stack.
Which is actually not a bad sideeffect because it only affects
pure transport mode bundles with which the plain text packet is
currently not visible for tcpdump at all.

>>diff --git a/net/ipv6/xfrm6_input.c b/net/ipv6/xfrm6_input.c
>>--- a/net/ipv6/xfrm6_input.c
>>+++ b/net/ipv6/xfrm6_input.c
>>@@ -121,16 +121,21 @@ int xfrm6_rcv_spi(struct sk_buff **pskb,
>> 	skb->sp->len += xfrm_nr;
>> 	skb->ip_summed = CHECKSUM_NONE;
>> 
>>-	if (decaps) {
>>-		if (!(skb->dev->flags&IFF_LOOPBACK)) {
>>-			dst_release(skb->dst);
>>-			skb->dst = NULL;
>>-		}
>>-		netif_rx(skb);
>>-		return -1;
>>-	} else {
>>-		return 1;
>>+	if (!decaps) {
>>+		if (skb_cloned(skb) &&
>>+		    pskb_expand_head(skb, 0, 0, GFP_ATOMIC))
>>+			goto drop;
>>+		/* FIXME: Jumbo payload option */
>>+		skb->nh.ipv6h->payload_len = htons(skb->len);
>>+		__skb_push(skb, skb->data - skb->nh.raw);
>> 	}
>>+	if (!(skb->dev->flags&IFF_LOOPBACK)) {
>>+		dst_release(skb->dst);
>>+		skb->dst = NULL;
>>+	}
>>+	nf_reset(skb);
>>+	netif_rx(skb);
>>+	return -1;
>> 
>> drop_unlock:
>> 	spin_unlock(&x->lock);
> 
> 
> I diagree.
> Stack should process the packet just once if it is of transport mode.
> (It is okay to process one twice if it is of tunnel mode.)

For IPv6 this probably can be changed, I just kept the code in sync
for now. For IPv4 it is necessary to pass it through the entire stack
again to make netfilter fully usable without lots of code duplication
or unusual limitations, especially for NAT. So the question is whether
we want to keep the code and behaviour of IPv4 and IPv6 in sync or
not. I think we should.

Re: [NF+IPsec 4/6]: Make IPsec input processing symetrical to output

[Herbert Xu]

On Mon, Oct 17, 2005 at 03:24:25AM +0200, Patrick McHardy wrote:
>
> For IPv6 this probably can be changed, I just kept the code in sync
> for now. For IPv4 it is necessary to pass it through the entire stack
> again to make netfilter fully usable without lots of code duplication
> or unusual limitations, especially for NAT. So the question is whether
> we want to keep the code and behaviour of IPv4 and IPv6 in sync or
> not. I think we should.

I can see the concern of processing pure transport mode packets in IPv6
twice in the stack because IPsec in IPv6 is used for a totally different
purpose compared to IPv4.

So how about this? We let the SA tell us whether they want to go through
netfilter again.  So each SA will carry a flag which determines whether
packets through it should go through netfilter.

This flag would only affect transport mode SAs of course.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [NF+IPsec 4/6]: Make IPsec input processing symetrical to output

[Patrick McHardy]

Sorry for the huge delay Herbert, I really appreciate your input,
but I'm stuck in other work.

Herbert Xu wrote:
> I can see the concern of processing pure transport mode packets in IPv6
> twice in the stack because IPsec in IPv6 is used for a totally different
> purpose compared to IPv4.
> 
> So how about this? We let the SA tell us whether they want to go through
> netfilter again.  So each SA will carry a flag which determines whether
> packets through it should go through netfilter.
> 
> This flag would only affect transport mode SAs of course.

That would be one possibility. But I'm not a big fan of per-state flags
that affect packet flow, so I think I'd prefer to just ignore this
case. I don't think not handling inner transport mode SAs would be a
big loss, so how about we just skip inner transport mode SAs completely
on output and keep the input code as it is?

Regards
Patrick

Re: [NF+IPsec 4/6]: Make IPsec input processing symetrical to output

[Herbert Xu]

On Wed, Oct 26, 2005 at 01:09:12AM +0200, Patrick McHardy wrote:
>
> > So how about this? We let the SA tell us whether they want to go through
> > netfilter again.  So each SA will carry a flag which determines whether
> > packets through it should go through netfilter.
> > 
> > This flag would only affect transport mode SAs of course.
> 
> That would be one possibility. But I'm not a big fan of per-state flags
> that affect packet flow, so I think I'd prefer to just ignore this
> case. I don't think not handling inner transport mode SAs would be a
> big loss, so how about we just skip inner transport mode SAs completely
> on output and keep the input code as it is?

Actually I was thinking of transport mode SAs with no accompanying
tunnel mode SAs.  Did you have another way of dealing with them?

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [NF+IPsec 4/6]: Make IPsec input processing symetrical to output

[Patrick McHardy]

Herbert Xu wrote:
> On Wed, Oct 26, 2005 at 01:09:12AM +0200, Patrick McHardy wrote:
> 
>>>So how about this? We let the SA tell us whether they want to go through
>>>netfilter again.  So each SA will carry a flag which determines whether
>>>packets through it should go through netfilter.
>>>
>>>This flag would only affect transport mode SAs of course.
>>
>>That would be one possibility. But I'm not a big fan of per-state flags
>>that affect packet flow, so I think I'd prefer to just ignore this
>>case. I don't think not handling inner transport mode SAs would be a
>>big loss, so how about we just skip inner transport mode SAs completely
>>on output and keep the input code as it is?
> 
> 
> Actually I was thinking of transport mode SAs with no accompanying
> tunnel mode SAs.  Did you have another way of dealing with them?

No. I thought of this as a special case of inner transport mode SAs
(without any further SAs) which would be unhandled. I've never used
pure transport mode SAs except for testing, and I've never seen any
other users of this. Do you think it is important to handle?

Re: [NF+IPsec 4/6]: Make IPsec input processing symetrical to output

[Herbert Xu]

On Wed, Oct 26, 2005 at 01:14:31AM +0200, Patrick McHardy wrote:
> No. I thought of this as a special case of inner transport mode SAs
> (without any further SAs) which would be unhandled. I've never used
> pure transport mode SAs except for testing, and I've never seen any
> other users of this. Do you think it is important to handle?

Well the scenario is IPv4 transport mode ESP applied outside normal
IPIP tunnel devices.

Actually, this could work if we make sure that the user-space KMs
set the SA selectors properly in this case.

I presume that you will be changing the output path so that LOCAL_OUT
does not see the plain-text packet.  Otherwise it'll be asymmetric with
repsect to the inbound side which does not see plain-text packets for
transport mode SAs.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [NF+IPsec 4/6]: Make IPsec input processing symetrical to output

[James Morris]

On Wed, 26 Oct 2005, Patrick McHardy wrote:

> No. I thought of this as a special case of inner transport mode SAs
> (without any further SAs) which would be unhandled. I've never used
> pure transport mode SAs except for testing, and I've never seen any
> other users of this. Do you think it is important to handle?

Pure transport mode SAs are likely to be common when using IPSec labeling 
(the stuff Trent Jaeger has been working on).


- James
-- 
James Morris
<jmorris@namei.org>

Re: [NF+IPsec 4/6]: Make IPsec input processing symetrical to output

[Ingo Oeser]

Patrick McHardy wrote:
> Herbert Xu wrote:
> > Actually I was thinking of transport mode SAs with no accompanying
> > tunnel mode SAs.  Did you have another way of dealing with them?
>
> No. I thought of this as a special case of inner transport mode SAs
> (without any further SAs) which would be unhandled. I've never used
> pure transport mode SAs except for testing, and I've never seen any
> other users of this. Do you think it is important to handle?

These become important in untrusted LANs. These days, the LAN is often 
not safe enough for confidental data, so you need end to end encryption,
for which tunnel mode is a bitch to setup (due to the additional routing
required ).

Regards

Ingo Oeser

Re: [NF+IPsec 4/6]: Make IPsec input processing symetrical to output

[Stephen Frost]

[-- Attachment #1: Type: text/plain, Size: 1063 bytes --]

* Patrick McHardy (kaber@trash.net) wrote:
> Herbert Xu wrote:
> > Actually I was thinking of transport mode SAs with no accompanying
> > tunnel mode SAs.  Did you have another way of dealing with them?
> 
> No. I thought of this as a special case of inner transport mode SAs
> (without any further SAs) which would be unhandled. I've never used
> pure transport mode SAs except for testing, and I've never seen any
> other users of this. Do you think it is important to handle?

I've used pure transport mode SAs in a couple of cases and I do feel
they're important to support, and would really like to be able to use
firewall rules with them too..  In fact, I'd think it'd be more
important with transport-mode because it's more likely you'll want to
set up a pretty open use-ipsec-whenever-possible-with-remote-hosts mode,
unlike with tunnel mode where it's more likely you'll set up specific
policies which only accept tunnels which go to certain ports on certain
IPs, etc.  At least, I've set that up as well... :)

	Thanks,

		Stephen

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [NF+IPsec 4/6]: Make IPsec input processing symetrical to output

[Herbert Xu]

On Mon, Oct 17, 2005 at 09:49:19AM +0900, YOSHIFUJI Hideaki / ?$B5HF#1QL@ wrote:
>
> Stack should process the packet just once if it is of transport mode.
> (It is okay to process one twice if it is of tunnel mode.)

Thinking about this again, I'm not sure that I agree.

OK, I don't actually care whether we process it once or twice for
pure transport mode, but I think that it is absolutely essential
that LOCAL_IN/LOCAL_OUT see the decapsulated packet instead of
the encrypted version.

Whether it's IPv4 or IPv6, transport mode shouldn't make you lose the
ability to use netfilter to filter out packets.

Now if we can achieve this by sending the packet through netfilter
only once then I'm all for it.

But if for whatever reason we can't do that, then I'd rather have it
go through twice than not see the decapsulated packet at all which is
the case at the moment.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [NF+IPsec 4/6]: Make IPsec input processing symetrical to output

[Patrick McHardy]

Herbert Xu wrote:
> On Wed, Oct 26, 2005 at 01:14:31AM +0200, Patrick McHardy wrote:
> 
>>No. I thought of this as a special case of inner transport mode SAs
>>(without any further SAs) which would be unhandled. I've never used
>>pure transport mode SAs except for testing, and I've never seen any
>>other users of this. Do you think it is important to handle?
> 
> 
> Well the scenario is IPv4 transport mode ESP applied outside normal
> IPIP tunnel devices.
> 
> Actually, this could work if we make sure that the user-space KMs
> set the SA selectors properly in this case.
> 
> I presume that you will be changing the output path so that LOCAL_OUT
> does not see the plain-text packet.  Otherwise it'll be asymmetric with
> repsect to the inbound side which does not see plain-text packets for
> transport mode SAs.

Yes, that was the idea. But since people seem to consider this an
important case to handle I'm going to try the per-SA flag you
proposed. I'll send new patches in the next days.

Re: [NF+IPsec 4/6]: Make IPsec input processing symetrical to output

[YOSHIFUJI Hideaki / 吉藤英明]

In article <20051027121545.GA5530@gondor.apana.org.au> (at Thu, 27 Oct 2005 22:15:45 +1000), Herbert Xu <herbert@gondor.apana.org.au> says:

> On Mon, Oct 17, 2005 at 09:49:19AM +0900, YOSHIFUJI Hideaki / ?$B5HF#1QL@ wrote:
> >
> > Stack should process the packet just once if it is of transport mode.
> > (It is okay to process one twice if it is of tunnel mode.)
> 
> Thinking about this again, I'm not sure that I agree.
> 
> OK, I don't actually care whether we process it once or twice for
> pure transport mode, but I think that it is absolutely essential
> that LOCAL_IN/LOCAL_OUT see the decapsulated packet instead of
> the encrypted version.

Well, I really care.
I strongly believe that we SHOULD NOT mix encrypted
packets and plain text packets at the same hook.
e.g. LOCAL_IN is NOT for decrypted plain text packets,
but for the original encrypted ones.
I believe that we should have another set of hooks
after decryption (other than LOCAL_IN) if we want to
look inside the encrypted packets.

--yoshfuji

Re: [NF+IPsec 4/6]: Make IPsec input processing symetrical to output

[Patrick McHardy]

YOSHIFUJI Hideaki / ^[$B5HF#1QL@^[ wrote:
> In article <20051027121545.GA5530@gondor.apana.org.au> (at Thu, 27 Oct 2005 22:15:45 +1000), Herbert Xu <herbert@gondor.apana.org.au> says:
> 
> 
>>On Mon, Oct 17, 2005 at 09:49:19AM +0900, YOSHIFUJI Hideaki / ?$B5HF#1QL@ wrote:
>>
>>>Stack should process the packet just once if it is of transport mode.
>>>(It is okay to process one twice if it is of tunnel mode.)
>>
>>Thinking about this again, I'm not sure that I agree.
>>
>>OK, I don't actually care whether we process it once or twice for
>>pure transport mode, but I think that it is absolutely essential
>>that LOCAL_IN/LOCAL_OUT see the decapsulated packet instead of
>>the encrypted version.
> 
> 
> Well, I really care.
> I strongly believe that we SHOULD NOT mix encrypted
> packets and plain text packets at the same hook.
> e.g. LOCAL_IN is NOT for decrypted plain text packets,
> but for the original encrypted ones.

It is in tunnel mode. LOCAL_IN is for all packets that are
locally delivered, independant of encryption. Using new
hooks for decrypted transport mode packets is inconsistent
IMO.

> I believe that we should have another set of hooks
> after decryption (other than LOCAL_IN) if we want to
> look inside the encrypted packets.

This isn't possible without adding completely new tables since
we can't add new chains to existing tables without breaking
userspace compatiblity. If we want existing functionality to
continue working we would need an "xfrm" version of the raw,
mangle, nat and filter table and need to change conntrack (and
possibly NAT) to use the new hooks. Rerouting after NAT (with IPv4)
won't work if we don't pass the packet through the stack again.
I think adding new hooks has too many drawbacks compared to simply
sending the decrypted packet through the stack again.

Re: [NF+IPsec 4/6]: Make IPsec input processing symetrical to output

[Patrick McHardy]

Patrick McHardy wrote:
> Herbert Xu wrote:
> 
>>I presume that you will be changing the output path so that LOCAL_OUT
>>does not see the plain-text packet.  Otherwise it'll be asymmetric with
>>repsect to the inbound side which does not see plain-text packets for
>>transport mode SAs.
> 
> Yes, that was the idea. But since people seem to consider this an
> important case to handle I'm going to try the per-SA flag you
> proposed. I'll send new patches in the next days.

Unfortunately hiding the plain-text packets on output when transport
mode SAs are used and the flag is not set adds a new inconsistency
with NAT. In my last patchsets NAT was handled by redoing the policy
lookup when a packet was NATed at LOCAL_OUT or POST_ROUTING and wasn't
already transformed. If the new lookup yielded a policy
ip_dst_output/__ip_dst_output was called again. The hooks were always
called in the normal order. With a per-SA flag however we don't know
if the packet should be hidden before the second lookup is done, so
with NAT a packet that would usually be hidden might be visible
on LOCAL_OUT and POST_ROUTING, or just LOCAL_OUT. This also affects
ip_queue.

So far the by far cleanest solution from a netfilter point of view
was to ignore transport mode unless its the innermost transform on
output and to always send the decapsulated packets through the stack
again on input. Since Yoshifuji disagrees with this approach we seem
to be deadlocked ..

Re: [NF+IPsec 4/6]: Make IPsec input processing symetrical to output

[Yasuyuki KOZAKAI]

Hi, Patrick,

From: Patrick McHardy <kaber@trash.net>
Date: Mon, 31 Oct 2005 00:15:52 +0100

> Patrick McHardy wrote:
> > Herbert Xu wrote:
> > 
> >>I presume that you will be changing the output path so that LOCAL_OUT
> >>does not see the plain-text packet.  Otherwise it'll be asymmetric with
> >>repsect to the inbound side which does not see plain-text packets for
> >>transport mode SAs.

I support this way.

> > Yes, that was the idea. But since people seem to consider this an
> > important case to handle I'm going to try the per-SA flag you
> > proposed. I'll send new patches in the next days.

I think pure transport mode is important, too. For example, for users of
L2TP over IPsec.

But I'm not sure that how many people wants to use netfilter hook together.
At least, I don't need that. Because I can use IPsec policy instead of
iptables rule and that's enough for me.

I also think that it's the work for IPsec policy check to decide to
accept or drop decrypted packets on input path of transport mode, that is
not netfilter work.

On the other hand, for the users who want to use local NAT and IPsec
transport mode together, we might have to add netfilter hook to input
path. But I'm not sure how many such users are. If nobody want, hooks
we need are only LOCAL_OUT and POST_ROUTING on output path per tunnel.

> Unfortunately hiding the plain-text packets on output when transport
> mode SAs are used and the flag is not set adds a new inconsistency
> with NAT. In my last patchsets NAT was handled by redoing the policy
> lookup when a packet was NATed at LOCAL_OUT or POST_ROUTING and wasn't
> already transformed. If the new lookup yielded a policy
> ip_dst_output/__ip_dst_output was called again. The hooks were always
> called in the normal order. With a per-SA flag however we don't know
> if the packet should be hidden before the second lookup is done, so
> with NAT a packet that would usually be hidden might be visible
> on LOCAL_OUT and POST_ROUTING, or just LOCAL_OUT. This also affects
> ip_queue.

Sorry, maybe I don't understand. per-SA flag means that packets go through
netfilter hook before handling of its SA on output path ? Why it isn't
'after' ?

> So far the by far cleanest solution from a netfilter point of view
> was to ignore transport mode unless its the innermost transform on
> output and to always send the decapsulated packets through the stack
> again on input. Since Yoshifuji disagrees with this approach we seem
> to be deadlocked ..
> 
> -
> 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

--- Yasuyuki Kozakai

Re: [NF+IPsec 4/6]: Make IPsec input processing symetrical to output

[Patrick McHardy]

Yasuyuki KOZAKAI wrote:
>>Patrick McHardy wrote:
>>
>>>Herbert Xu wrote:
>>>
>>>>I presume that you will be changing the output path so that LOCAL_OUT
>>>>does not see the plain-text packet.  Otherwise it'll be asymmetric with
>>>>repsect to the inbound side which does not see plain-text packets for
>>>>transport mode SAs.
> 
> I support this way.

Hiding the packets from the hooks dependant on the xfrm_state is always
a problem with NAT, because when we do a second xfrm lookup after NAT
it is already to late to hide the packets.

>>>Yes, that was the idea. But since people seem to consider this an
>>>important case to handle I'm going to try the per-SA flag you
>>>proposed. I'll send new patches in the next days.
> 
> I think pure transport mode is important, too. For example, for users of
> L2TP over IPsec.
> 
> But I'm not sure that how many people wants to use netfilter hook together.
> At least, I don't need that. Because I can use IPsec policy instead of
> iptables rule and that's enough for me.
> 
> I also think that it's the work for IPsec policy check to decide to
> accept or drop decrypted packets on input path of transport mode, that is
> not netfilter work.

I agree that its the job of policy checks, but its often simpler to
add a policy which includes just IP addresses and use iptables for
filtering. Connection state, TCP flags, ..., can't be handled using
policy checks, and netfilter might also be used for marking, TCP MSS
rewriting, proxy redirection, ...

> On the other hand, for the users who want to use local NAT and IPsec
> transport mode together, we might have to add netfilter hook to input
> path. But I'm not sure how many such users are. If nobody want, hooks
> we need are only LOCAL_OUT and POST_ROUTING on output path per tunnel.

Yes, we could add new hooks and duplicate the relevant code from the
IP input path. But I think just sending the packets through the stack
again is cleaner.

>>Unfortunately hiding the plain-text packets on output when transport
>>mode SAs are used and the flag is not set adds a new inconsistency
>>with NAT. In my last patchsets NAT was handled by redoing the policy
>>lookup when a packet was NATed at LOCAL_OUT or POST_ROUTING and wasn't
>>already transformed. If the new lookup yielded a policy
>>ip_dst_output/__ip_dst_output was called again. The hooks were always
>>called in the normal order. With a per-SA flag however we don't know
>>if the packet should be hidden before the second lookup is done, so
>>with NAT a packet that would usually be hidden might be visible
>>on LOCAL_OUT and POST_ROUTING, or just LOCAL_OUT. This also affects
>>ip_queue.
> 
> Sorry, maybe I don't understand. per-SA flag means that packets go through
> netfilter hook before handling of its SA on output path ? Why it isn't
> 'after' ?

The idea was to usually hide transport mode packets on the output path
from netfilter hooks, unless a special flag in the xfrm_state is set.
On input the flag would make a packet decapsulated from a transport mode
SA through the stack again. But it has the same problem wrt. NAT I
described above as hiding unconditionally.

Re: [NF+IPsec 4/6]: Make IPsec input processing symetrical to output

[Stephen Frost]

[-- Attachment #1: Type: text/plain, Size: 2105 bytes --]

* Yasuyuki KOZAKAI (yasuyuki.kozakai@toshiba.co.jp) wrote:
> I think pure transport mode is important, too. For example, for users of
> L2TP over IPsec.
> 
> But I'm not sure that how many people wants to use netfilter hook together.
> At least, I don't need that. Because I can use IPsec policy instead of
> iptables rule and that's enough for me.
> 
> I also think that it's the work for IPsec policy check to decide to
> accept or drop decrypted packets on input path of transport mode, that is
> not netfilter work.

Trying to compare IPsec policy to netfilter is just plain silly.  IPsec
policy is *not* equivilant to a firewall system like netfilter.  Not
even close.  Not offering the ability to firewall transport-mode IPsec 
packets shouldn't even be an option. :/  With 15 servers which all talk
transport-mode IPsec to each other I'd still want to be able to do
firewalling to hopefully reduce the impact of one of the servers being
compromised.

> On the other hand, for the users who want to use local NAT and IPsec
> transport mode together, we might have to add netfilter hook to input
> path. But I'm not sure how many such users are. If nobody want, hooks
> we need are only LOCAL_OUT and POST_ROUTING on output path per tunnel.

I had wanted to do this, in order to hide the network configuration of
the machines behind the gateway but ended up not being able to. :/  My
situation is perhaps not very common but I think it will become more
common in the future: I've got a VPN setup with multiple different
people who I don't completely trust and who don't entirely trust me or
the other people.  This situation can exist for tunnel mode and
transport mode users.  As use-ipsec-when-available transport mode
increases in use this need seems likely to grow.

Consider a corporate network where the whole thing is set up to do
transport-mode ipsec.  Chances are the guys who run the corporate
servers are still going to want to be able to run firewalls on their
servers to cover things in case a given desktop is compromised.

	Thanks,

		Stephen

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [NF+IPsec 4/6]: Make IPsec input processing symetrical to output

[Herbert Xu]

On Thu, Oct 27, 2005 at 11:57:32PM +0900, YOSHIFUJI Hideaki / ?$B5HF#1QL@ wrote:
> 
> Well, I really care.
> I strongly believe that we SHOULD NOT mix encrypted
> packets and plain text packets at the same hook.
> e.g. LOCAL_IN is NOT for decrypted plain text packets,
> but for the original encrypted ones.

OK.  Would it be workable for you if LOCAL_IN only saw the decrypted
packets without ever seeing the encrypted ones?

I'd like to know where the boundaries are so we can find a compromise
that works for everyone.

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [NF+IPsec 4/6]: Make IPsec input processing symetrical to output

[Patrick McHardy]

Herbert Xu wrote:
> On Thu, Oct 27, 2005 at 11:57:32PM +0900, YOSHIFUJI Hideaki / ?$B5HF#1QL@ wrote:
> 
>>Well, I really care.
>>I strongly believe that we SHOULD NOT mix encrypted
>>packets and plain text packets at the same hook.
>>e.g. LOCAL_IN is NOT for decrypted plain text packets,
>>but for the original encrypted ones.
> 
> 
> OK.  Would it be workable for you if LOCAL_IN only saw the decrypted
> packets without ever seeing the encrypted ones?

How exactly would that work? I guess we couldn't do NAT with
the encrypted packet anymore?

> I'd like to know where the boundaries are so we can find a compromise
> that works for everyone.

I would prefer something similar to the second set of patches.
Instead of calling netif_rx we could use NF_HOOK and simulate
the relevant parts of the input path for IPv4 and NAT. This
would assure that statistics are still correct and tcpdump is
not affected, which were Yoshifuji's biggest concerns if I
understood correctly.

Re: [NF+IPsec 4/6]: Make IPsec input processing symetrical to output

[YOSHIFUJI Hideaki / 吉藤英明]

In article <20051105063030.GA32385@gondor.apana.org.au> (at Sat, 5 Nov 2005 17:30:30 +1100), Herbert Xu <herbert@gondor.apana.org.au> says:

> On Thu, Oct 27, 2005 at 11:57:32PM +0900, YOSHIFUJI Hideaki / ?$B5HF#1QL@ wrote:
> > 
> > Well, I really care.
> > I strongly believe that we SHOULD NOT mix encrypted
> > packets and plain text packets at the same hook.
> > e.g. LOCAL_IN is NOT for decrypted plain text packets,
> > but for the original encrypted ones.
> 
> OK.  Would it be workable for you if LOCAL_IN only saw the decrypted
> packets without ever seeing the encrypted ones?

Miyazawa-san, Kozakai-san and I have discussed on this issue.
The answer is "maybe," but from the point of view of the original context,
it'd be better to let it see encrypted packet.
Miyazawa-san, Kozakai-san, please comment further details.
(I'm about to fly...)

--yoshfuji @ NRT(->YVR)

Re: [NF+IPsec 4/6]: Make IPsec input processing symetrical to output

[Herbert Xu]

On Sat, Nov 05, 2005 at 08:55:44AM +0100, Patrick McHardy wrote:
>
> >OK.  Would it be workable for you if LOCAL_IN only saw the decrypted
> >packets without ever seeing the encrypted ones?
> 
> How exactly would that work? I guess we couldn't do NAT with
> the encrypted packet anymore?

I'm presuming that Yoshifuji-san has no objections to applying the
NAT-related hooks twice on IPsec since IPv6 does/will not have NAT.

Given that assumption, we should be able to separate the existing
LOCAL_IN into a read-only (filtering) part and a read-write part.
The latter would be applied unconditionally while the former can
be skipped.

> I would prefer something similar to the second set of patches.
> Instead of calling netif_rx we could use NF_HOOK and simulate
> the relevant parts of the input path for IPv4 and NAT. This
> would assure that statistics are still correct and tcpdump is
> not affected, which were Yoshifuji's biggest concerns if I
> understood correctly.

I don't think netif_rx is the problem here.  The question is
how and where do we place the netfilter hooks.  Even without
netif_rx the same problem is going to be there.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [NF+IPsec 4/6]: Make IPsec input processing symetrical to output

[Patrick McHardy]

Herbert Xu wrote:
> On Sat, Nov 05, 2005 at 08:55:44AM +0100, Patrick McHardy wrote:
> 
>>>OK.  Would it be workable for you if LOCAL_IN only saw the decrypted
>>>packets without ever seeing the encrypted ones?
>>
>>How exactly would that work? I guess we couldn't do NAT with
>>the encrypted packet anymore?
> 
> I'm presuming that Yoshifuji-san has no objections to applying the
> NAT-related hooks twice on IPsec since IPv6 does/will not have NAT.
> 
> Given that assumption, we should be able to separate the existing
> LOCAL_IN into a read-only (filtering) part and a read-write part.
> The latter would be applied unconditionally while the former can
> be skipped.

So far I don't see why we shouldn't just the LOCAL_IN hook as it
is and also haven't heard any reason against it. I'm fine with
not using netif_rx, but I don't see why we should complicate
things by introducing special semantics for decapsulated transport
mode packets.

>>I would prefer something similar to the second set of patches.
>>Instead of calling netif_rx we could use NF_HOOK and simulate
>>the relevant parts of the input path for IPv4 and NAT. This
>>would assure that statistics are still correct and tcpdump is
>>not affected, which were Yoshifuji's biggest concerns if I
>>understood correctly.
> 
> I don't think netif_rx is the problem here.  The question is
> how and where do we place the netfilter hooks.  Even without
> netif_rx the same problem is going to be there.

IMO the view for netfilter should be as if we called netif_rx,
so on the input path decapsulated packets from the innermost
transport mode SA should go through PRE_ROUTING->LOCAL_IN
or possibly FORWARD in case of NAT.

Re: [NF+IPsec 4/6]: Make IPsec input processing symetrical to output

[Herbert Xu]

On Sat, Nov 05, 2005 at 09:58:24AM +0100, Patrick McHardy wrote:
> 
> So far I don't see why we shouldn't just the LOCAL_IN hook as it
> is and also haven't heard any reason against it. I'm fine with
> not using netif_rx, but I don't see why we should complicate
> things by introducing special semantics for decapsulated transport
> mode packets.

The fewer changes we have to make the happier I am :)

> IMO the view for netfilter should be as if we called netif_rx,
> so on the input path decapsulated packets from the innermost
> transport mode SA should go through PRE_ROUTING->LOCAL_IN
> or possibly FORWARD in case of NAT.

You mean we simply skip the pre-decapsulation LOCAL_IN step
and the post-encapsulation LOCAL_OUT step? That sounds great
to me.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [NF+IPsec 4/6]: Make IPsec input processing symetrical to output

[Patrick McHardy]

Herbert Xu wrote:
> On Sat, Nov 05, 2005 at 09:58:24AM +0100, Patrick McHardy wrote:
> 
>>IMO the view for netfilter should be as if we called netif_rx,
>>so on the input path decapsulated packets from the innermost
>>transport mode SA should go through PRE_ROUTING->LOCAL_IN
>>or possibly FORWARD in case of NAT.
> 
> You mean we simply skip the pre-decapsulation LOCAL_IN step
> and the post-encapsulation LOCAL_OUT step? That sounds great
> to me.

No, that won't be possible if we have more than one SA and
would also make DNAT in LOCAL_OUT on the encapsulated packet
impossible.

What I propose is to keep tunnel mode handling as it is, so
for each tunnel mode SA we hit PRE_ROUTING and LOCAL_IN in
the normal packet path. If the final SA is a transport mode
SA, we don't call netif_rx as in my first patchset, but pass
the packet through a new PRE_ROUTING hook in xfrm{4,6}_input
and LOCAL_IN afterwards. The packet won't be processed a second
time by the stack, just the netfilter hooks will be called.
NAT be will be handled manually for IPv4 by doing a new route
lookup and calling dst_input if NAT took place.

Re: [NF+IPsec 4/6]: Make IPsec input processing symetrical to output

[Herbert Xu]

On Sat, Nov 05, 2005 at 10:19:51AM +0100, Patrick McHardy wrote:
> 
> What I propose is to keep tunnel mode handling as it is, so
> for each tunnel mode SA we hit PRE_ROUTING and LOCAL_IN in
> the normal packet path. If the final SA is a transport mode
> SA, we don't call netif_rx as in my first patchset, but pass
> the packet through a new PRE_ROUTING hook in xfrm{4,6}_input
> and LOCAL_IN afterwards. The packet won't be processed a second
> time by the stack, just the netfilter hooks will be called.
> NAT be will be handled manually for IPv4 by doing a new route
> lookup and calling dst_input if NAT took place.

In other words LOCAL_IN will still see the packet twice for
pure transport mode packets? That's going to be a problem for
me for the reasons that I outlined earlier:

<20051011131838.GA4934@gondor.apana.org.au>

Also, I thought Yoshifuji-san's objection is not just about
transport mode packets passing through netif_rx twice, but
passing through netfilter twice as well?

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [NF+IPsec 4/6]: Make IPsec input processing symetrical to output

[Patrick McHardy]

Herbert Xu wrote:
> On Sat, Nov 05, 2005 at 10:19:51AM +0100, Patrick McHardy wrote:
> 
>>What I propose is to keep tunnel mode handling as it is, so
>>for each tunnel mode SA we hit PRE_ROUTING and LOCAL_IN in
>>the normal packet path. If the final SA is a transport mode
>>SA, we don't call netif_rx as in my first patchset, but pass
>>the packet through a new PRE_ROUTING hook in xfrm{4,6}_input
>>and LOCAL_IN afterwards. The packet won't be processed a second
>>time by the stack, just the netfilter hooks will be called.
>>NAT be will be handled manually for IPv4 by doing a new route
>>lookup and calling dst_input if NAT took place.
> 
> 
> In other words LOCAL_IN will still see the packet twice for
> pure transport mode packets? That's going to be a problem for
> me for the reasons that I outlined earlier:
>
> <20051011131838.GA4934@gondor.apana.org.au>

Well, once encapsulated and once decapsulated.

What I propose is actually exactly what you suggested in that mail:

> Would it be workable to try something like this? We invoke netfilter
> after each tunnel mode transform as we do now.  In addition to that,
> we invoke netfilter at the very end of IPsec processing, that is,
> just before the point where the original xfrm*_rcv_encap would have
> returned.

In my last patchset I did it by calling netif_rx at that point,
now I want to add new hooks.

> Also, I thought Yoshifuji-san's objection is not just about
> transport mode packets passing through netif_rx twice, but
> passing through netfilter twice as well?

I think so, but he didn't mention a reason why he objects to it.
I also don't think it can be done otherwise while still keeping
netfilter "just working" for all cases, which IMO is highly
desirable.

Re: [NF+IPsec 4/6]: Make IPsec input processing symetrical to output

[Herbert Xu]

On Sat, Nov 05, 2005 at 10:55:57AM +0100, Patrick McHardy wrote:
>
> > <20051011131838.GA4934@gondor.apana.org.au>
> 
> Well, once encapsulated and once decapsulated.
> 
> What I propose is actually exactly what you suggested in that mail:

You got me there :) I was confused.  Yes this is OK with me.
 
> In my last patchset I did it by calling netif_rx at that point,
> now I want to add new hooks.

The only problem I can see is that at some point we're probably
going to add an AF_PACKET hook there as well for the pure transport
mode packet so that people can diagnose their transport mode IPsec
problems.

However, I reckon that's still miles ahead of passing the packet
back through netif_rx when we already know that it's still the
same address family as what we started out with.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [NF+IPsec 4/6]: Make IPsec input processing symetrical to output

[Patrick McHardy]

Herbert Xu wrote:
> On Sat, Nov 05, 2005 at 10:55:57AM +0100, Patrick McHardy wrote:
> 
>>In my last patchset I did it by calling netif_rx at that point,
>>now I want to add new hooks.
> 
> The only problem I can see is that at some point we're probably
> going to add an AF_PACKET hook there as well for the pure transport
> mode packet so that people can diagnose their transport mode IPsec
> problems.

Yes, that would be useful.

> However, I reckon that's still miles ahead of passing the packet
> back through netif_rx when we already know that it's still the
> same address family as what we started out with.

Great. I'm moving to a new appartment right now and will be offline
until monday. I'll try to get some patches ready until then.

Regards
Patrick

Re: [NF+IPsec 4/6]: Make IPsec input processing symetrical to output

[Yasuyuki KOZAKAI]

Hi, Patrick,

From: Patrick McHardy <kaber@trash.net>
Date: Sat, 05 Nov 2005 10:55:57 +0100

> > Also, I thought Yoshifuji-san's objection is not just about
> > transport mode packets passing through netif_rx twice, but
> > passing through netfilter twice as well?
> 
> I think so, but he didn't mention a reason why he objects to it.
> I also don't think it can be done otherwise while still keeping
> netfilter "just working" for all cases, which IMO is highly
> desirable.

I try to comment based on discussion with Yoshifuji-san and Miyazawa-san.

We think it's confusing for user to mix decrypted packets and pre-decrypted
ones to same hook. For example, if user want to accept packets encrypted by
transport mode ESP and drop others, he will do "iptables -A INPUT -p esp -j
ACCEPT" and "iptables -P INPUT DROP". But decrypted packets will be dropped
because of the 2nd command. Of cause the match module 'policy' will be helpful
in such case, but it's simple if he can different name of hook with INPUT.

And, in logical, the hook for decrypted packet and the one for pre-decrypted
packet is different like the current LOCAL_IN and LOCAL_OUT. Their place and
the packets they can see, are different.

This can be said about output path. The hook for encrypted packet and the one
for pre-encrypted packet is different.

In the current, LOCAL_OUT see pre-encrypted packet. I've been assuming
that LOCAL_OUT see the packets just before sending them from network device.
This is the reason why I said "I support the way" - which means LOCAL_OUT
doesn't see pre-encrypted packet.

Meanwhile the hook to see pre-encrypted packet is necessary for NAT
indeed as you pointed out. Then our suggestion is adding new hook
with new name, and distinguishing cleary between the usage of new and
current hook.

BTW, tunnel mode is special case. We can avoid confusing by operation
and so on. For example, using different address for inner and outer header.
We agree to call netif_rx() twice for tunneled packets, as ever.

Regards,

-- Yasuyuki Kozakai

Re: [NF+IPsec 4/6]: Make IPsec input processing symetrical to output

[Patrick McHardy]

Yasuyuki KOZAKAI wrote:
> I try to comment based on discussion with Yoshifuji-san and Miyazawa-san.
> 
> We think it's confusing for user to mix decrypted packets and pre-decrypted
> ones to same hook. For example, if user want to accept packets encrypted by
> transport mode ESP and drop others, he will do "iptables -A INPUT -p esp -j
> ACCEPT" and "iptables -P INPUT DROP". But decrypted packets will be dropped
> because of the 2nd command. Of cause the match module 'policy' will be helpful
> in such case, but it's simple if he can different name of hook with INPUT.
> 
> And, in logical, the hook for decrypted packet and the one for pre-decrypted
> packet is different like the current LOCAL_IN and LOCAL_OUT. Their place and
> the packets they can see, are different.

I disagree. LOCAL_IN is for locally delivered packets, and both the
decrypted and the encrypted packet are delivered locally. This is
true for tunnel mode, handling transport mode the same way seem
consistent, not confusing to me. This is also the way klips has
done it for ages and users are used to this and are actually asking
for exactly this behaviour.

> This can be said about output path. The hook for encrypted packet and the one
> for pre-encrypted packet is different.

Well, this is not intended but one of the problems I want to fix.

> In the current, LOCAL_OUT see pre-encrypted packet. I've been assuming
> that LOCAL_OUT see the packets just before sending them from network device.
> This is the reason why I said "I support the way" - which means LOCAL_OUT
> doesn't see pre-encrypted packet.

Same holds here. LOCAL_OUT is for locally generated packets, and should
see both the plain text and the encrypted packet. This is entirely
consistent with other tunnels like IPIP or GRE.

> Meanwhile the hook to see pre-encrypted packet is necessary for NAT
> indeed as you pointed out. Then our suggestion is adding new hook
> with new name, and distinguishing cleary between the usage of new and
> current hook.

We can't do that because introducing new hooks to the tables breaks
userspace compatibility.

Regards
Patrick