: Make IPsec input processing symetrical to output

[NETFILTER]: Make IPsec input processing symetrical to output

net/ipv6/xfrm6_input.c +7 -9

@@ -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);
 	}

In article <[email protected]> (at Mon, 17 Oct 2005 02:22:32 +0200), Patrick McHardy <[email protected]> says:

Quoted 1 line

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

Quoted 32 lines

net/ipv6/xfrm6_input.c +14 -9

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

Quoted 2 lines

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.

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.

Quoted 5 lines

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.

Quoted 1 line

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?

Quoted 11 lines · Patrick McHardy

Quoted 5 lines

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?

Quoted 4 lines · Patrick McHardy

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.

Quoted 4 lines · Herbert Xu

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.

Quoted 6 lines · Patrick McHardy

Quoted 4 lines · Herbert Xu

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.

Quoted 3 lines

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

-
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

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.

Quoted 8 lines

Quoted 6 lines · Patrick McHardy

Quoted 4 lines · Herbert Xu

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.

Quoted 3 lines

Quoted 3 lines

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.

Quoted 11 lines

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

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?

Quoted 2 lines · Herbert Xu

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?

Quoted 2 lines · Herbert Xu

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?

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

Quoted 3 lines

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.

In article <[email protected]> (at Thu, 27 Oct 2005 22:15:45 +1000), Herbert Xu <[email protected]> says:

Quoted 11 lines · On Mon, Oct 17, 2005 at 09:49:19AM +0900, YOSHIFUJI Hideaki / ?$B5HF#1QL@ wrote:

Quoted 3 lines

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.

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.

Quoted 5 lines · 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.

Quoted 3 lines

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

Quoted 5 lines · Patrick McHardy

Quoted 2 lines

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.

Quoted 6 lines

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.

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.

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.

Quoted 4 lines · Patrick McHardy

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.

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.

Quoted 9 lines · Patrick McHardy

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:

<[email protected]>

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.

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?

Quoted 2 lines

<[email protected]>

Well, once encapsulated and once decapsulated.

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

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

Quoted 2 lines · Patrick McHardy

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.

Quoted 3 lines

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.

Quoted 6 lines

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?