| Message ID | 1228396731.1643.57.camel@martin |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Martin Willi <martin@strongswan.org> Date: Thu, 04 Dec 2008 14:18:50 +0100 > An IPsec node speaking IKEv2 MUST accept incoming UDP encapsulated > ESP packets, even if no NAT situation is detected. This is important > if MOBIKE is in use. Some implementation keep the encapsulation > mode if they move out of a NAT situation. Applied, please provide a proper Signed-off-by line next time. -- 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
Martin Willi <martin@strongswan.org> wrote: > An IPsec node speaking IKEv2 MUST accept incoming UDP encapsulated > ESP packets, even if no NAT situation is detected. This is important > if MOBIKE is in use. Some implementation keep the encapsulation > mode if they move out of a NAT situation. > > diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c > index b4a1317..65bcf09 100644 > --- a/net/xfrm/xfrm_input.c > +++ b/net/xfrm/xfrm_input.c > @@ -167,11 +167,6 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type) > goto drop_unlock; > } > > - if ((x->encap ? x->encap->encap_type : 0) != encap_type) { > - XFRM_INC_STATS(net, LINUX_MIB_XFRMINSTATEMISMATCH); > - goto drop_unlock; > - } This can't work as ESP relies on this check. Now that it's gone ESP may touch a UDP header which doesn't exist. What exactly are you trying to achieve? Thanks,
From: Herbert Xu <herbert@gondor.apana.org.au> Date: Thu, 18 Dec 2008 14:47:26 +1100 > Martin Willi <martin@strongswan.org> wrote: > > An IPsec node speaking IKEv2 MUST accept incoming UDP encapsulated > > ESP packets, even if no NAT situation is detected. This is important > > if MOBIKE is in use. Some implementation keep the encapsulation > > mode if they move out of a NAT situation. > > > > diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c > > index b4a1317..65bcf09 100644 > > --- a/net/xfrm/xfrm_input.c > > +++ b/net/xfrm/xfrm_input.c > > @@ -167,11 +167,6 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type) > > goto drop_unlock; > > } > > > > - if ((x->encap ? x->encap->encap_type : 0) != encap_type) { > > - XFRM_INC_STATS(net, LINUX_MIB_XFRMINSTATEMISMATCH); > > - goto drop_unlock; > > - } > > This can't work as ESP relies on this check. Now that it's gone > ESP may touch a UDP header which doesn't exist. > > What exactly are you trying to achieve? IKEv2 daemon needs to be agnostic about this situation. So we have to accept everything, regardless of configured encapsulation type. At least that is my impression after reading the MOBIKE documents. -- 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
On Wed, Dec 17, 2008 at 07:49:42PM -0800, David Miller wrote: > > IKEv2 daemon needs to be agnostic about this situation. So we have to > accept everything, regardless of configured encapsulation type. > > At least that is my impression after reading the MOBIKE documents. The NAT-T encapsulation is an SA-specific attribute, so it's decided when the SA is negotiated. There is a mechanism for detecting changes in the peer's address and updating the SA. However, I can't see why you would ever need to switch between having UDP encapsulation and not having it, without negotiating it with the peer. That is, the UDP encapsulation works whether you have NAT or not. Do you have a specific quote that I can check against? Thanks,
On Thu, Dec 18, 2008 at 02:57:58PM +1100, Herbert Xu wrote: > > However, I can't see why you would ever need to switch between > having UDP encapsulation and not having it, without negotiating > it with the peer. That is, the UDP encapsulation works whether > you have NAT or not. > > Do you have a specific quote that I can check against? A quick google failed to reveal any specific requirements apart from the need to move in and out of NAT environments. That isn't actually an issue because when your addresses change you have to renegotiate with the other side to ensure that this isn't some kind of an attack. Afterwards you have to recreate the SAs at which point you can easily set the encapsulation to whatever it should be. The only time when you need this patch is if the other side unilaterally switched from NAT-T to no NAT-T, or vice versa, which does not sound like a sane thing to do. Cheers,
From: Herbert Xu <herbert@gondor.apana.org.au> Date: Thu, 18 Dec 2008 15:14:19 +1100 > A quick google failed to reveal any specific requirements apart > from the need to move in and out of NAT environments. > > That isn't actually an issue because when your addresses change > you have to renegotiate with the other side to ensure that this > isn't some kind of an attack. Afterwards you have to recreate > the SAs at which point you can easily set the encapsulation to > whatever it should be. > > The only time when you need this patch is if the other side > unilaterally switched from NAT-T to no NAT-T, or vice versa, > which does not sound like a sane thing to do. My interpretation of the situation is that when you change (address or NAT-T) you still have to perform the renegotiation over the old SA. Or something like that. -- 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
On Wed, Dec 17, 2008 at 08:17:55PM -0800, David Miller wrote: > > My interpretation of the situation is that when you change (address or > NAT-T) you still have to perform the renegotiation over the old SA. > > Or something like that. Do you have a pointer to that? For each connection you have a pair of SAs, they're keyed by (dst, spi, proto) So if one party's address changes then you immediately lose one direction, which means the SAs are now inoperable as a bi-directional channel. In any case, AFAIK negotiations are conducted outside of SAs, on port 500 or 4500, so this shouldn't matter. Cheers,
Hi Herbert, > This can't work as ESP relies on this check. Now that it's gone > ESP may touch a UDP header which doesn't exist. Hm, this worked perfectly fine in my tests... > ... when your addresses change > you have to renegotiate with the other side to ensure that this > isn't some kind of an attack. Afterwards you have to recreate > the SAs at which point you can easily set the encapsulation to > whatever it should be. Such address changes are recorded in the IKEv2 daemon and addresses are updated through MOBIKE (RFC4555). Each peer updates its SA using the new address. > The only time when you need this patch is if the other side > unilaterally switched from NAT-T to no NAT-T, or vice versa, > which does not sound like a sane thing to do. This is a perfectly valid use case. MOBIKE allows you roam your SAs between different networks, NATed or not. In our implementation, we switch UDP encapsulation strictly on and off depending on the new NAT situation. However, other implementations don't [1]. It is a requirement for MOBIKE enabled peers to accept UDP encapsulated packets in any case (see discussion [2]), and it is currently discussed to add this requirement to the revised IKEv2 core standard [3]: > o Implementations MUST process received UDP-encapsulated ESP packets > even when no NAT was detected. The fact that there is not NAT between peers is no guarantee that the peer will not use UDP encapsulation. I don't see any reason to drop ESP packets because of the used UDP encapsulation mode. What's the point of doing so? Martin [1]https://lists.strongswan.org/pipermail/users/2008-December/002936.html [2]http://www.machshav.com/pipermail/mobike/2006-September/001491.html [3]http://tools.ietf.org/html/draft-ietf-ipsecme-ikev2bis-01#section-2.23 -- 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
Martin Willi <martin@strongswan.org> wrote: > >> This can't work as ESP relies on this check. Now that it's gone >> ESP may touch a UDP header which doesn't exist. > > Hm, this worked perfectly fine in my tests... Well if you setup an SA with encap != NULL, then send it an unencapsulated ESP packet it'll try to access the UDP header (to check for address/port changes) which isn't there. >> ... when your addresses change >> you have to renegotiate with the other side to ensure that this >> isn't some kind of an attack. Afterwards you have to recreate >> the SAs at which point you can easily set the encapsulation to >> whatever it should be. > > Such address changes are recorded in the IKEv2 daemon and addresses are > updated through MOBIKE (RFC4555). Each peer updates its SA using the new > address. I know. The point is when such an address change occurs you need to recreate the kernel SAs anyway, in which case you can reset the encapsulation status to what it should be. > This is a perfectly valid use case. MOBIKE allows you roam your SAs > between different networks, NATed or not. In our implementation, we > switch UDP encapsulation strictly on and off depending on the new NAT > situation. However, other implementations don't [1]. You didn't understand me correctly. Of course the peer is allowed to change addresses. The point is that after such an address change occurs, we must replace the existing SAs with a new set of SAs (otherwise the communication breaks down), at which point you can set the NAT-T encapsulation status to what it should be. > It is a requirement for MOBIKE enabled peers to accept UDP encapsulated > packets in any case (see discussion [2]), and it is currently discussed > to add this requirement to the revised IKEv2 core standard [3]: I have absolutely no qualms about that. But this has nothing to do with the kernel. What they were discussing in [2] is that the IKE manager must be willing to negotiate SAs with/without UDP encapsulation if NAT is not detected. The kernel only gets involved once the negotiation is complete at which point you have agreed with the other side whether UDP encapsulation is to be used. Once this negotiation is complete, neither the other side nor us can unilaterally change the UDP encapsulation status without performing a new negotiation. So there is absolutely no need to handle this in the kernel. >> o Implementations MUST process received UDP-encapsulated ESP packets >> even when no NAT was detected. > > The fact that there is not NAT between peers is no guarantee that the > peer will not use UDP encapsulation. Which is fine. The kernel is perfectly able to perform ESP-over-UDP without your patch. What it doesn't do is automatically switch from UDP encapsulation to without UDP encapsulation, or vice versa without key manager instructions. > I don't see any reason to drop ESP packets because of the used UDP > encapsulation mode. What's the point of doing so? For a start, you've introduced a bug in the ESP code as it may try access a non-existant UDP header and then interpret that as an address change which will generate bogus events to your daemon. This will happen on every packet if you move from with encapsulation to without encapsulation. More importantly, you've failed to demonstrate why you need this in the first place. None of the URLs you've quoted tells us that the kernel needs to handle an SA switching between with encap and without encap without the key manager telling it to do so. > [1]https://lists.strongswan.org/pipermail/users/2008-December/002936.html Right that's what led to this patch. Reading this it seems that there is either a bug in strongswan or in its peer. IKE or IKEv2 is supposed to negotiate UDP encapsulation explicitly. So either we did negotiate to use UDP encapsulation, and Strongswan ignored it (it's buggy), or we didn't and the peer still chose to use it (the peer is buggy). In either case it's a serious bug in the key manager. I don't see why we should work around such brokenness in the kernel. Thanks,
> IKE or IKEv2 is supposed to negotiate UDP encapsulation > explicitly. So either we did negotiate to use UDP encapsulation, > and Strongswan ignored it (it's buggy), or we didn't and the peer > still chose to use it (the peer is buggy). This is just not true. UDP encapsulation is never negotiated explicitly in IKEv2. It is enabled if the peer thinks it is might help, for example if it detects a NAT situation. There is no way to say "use UDP encapsulation". > More importantly, you've failed to demonstrate why you need this > in the first place. None of the URLs you've quoted tells us that > the kernel needs to handle an SA switching between with encap and > without encap without the key manager telling it to do so. The local key manager does not know whether the peer enables UDP encapsulation, it can't. Most likely it will in NAT situations, but it might do so even if there is no NAT detected. And this is not a bug, it is allowed to do so by the protocol. > In either case it's a serious bug in the key manager. I don't see > why we should work around such brokenness in the kernel. >From the key manager perspective, I can enable or disable UDP encapsulation, fine. I decide locally what I'll use for outgoing packets. But how should I know what the peer uses? I can't, it isn't negotiated. It is, by the standard, perfectly valid to send UDP encapsulated packets if the peer wants to do so. And there is no need to communicate this to the key manager, there is actually no such mechanism in IKEv2. Therefore I need the kernel to accept packet, encapsulated or not. > For a start, you've introduced a bug in the ESP code as it may try > access a non-existant UDP header and then interpret that as an > address change which will generate bogus events to your daemon. I agree, I've missed that (because "my" daemon uses Netlink and the km_new_mapping event was not implemented until recently). But this is no valid reason to drop that approach in general, it is a side effect introduced by my specific patch. This can be fixed. Martin -- 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
On Thu, Dec 18, 2008 at 01:36:56PM +0100, Martin Willi wrote: > > >From the key manager perspective, I can enable or disable UDP > encapsulation, fine. I decide locally what I'll use for outgoing > packets. But how should I know what the peer uses? I can't, it isn't > negotiated. It is, by the standard, perfectly valid to send UDP > encapsulated packets if the peer wants to do so. And there is no need to > communicate this to the key manager, there is actually no such mechanism > in IKEv2. Therefore I need the kernel to accept packet, encapsulated or > not. Even if the kernel did accept such packets, there is no guarantee that your return traffic will make it back to the other side because stateful firewalls may be present. Responding with unencapsulated ESP traffic when the peer is sending you UDP-encapsulated traffic is just not going to fly. BTW I think the IKEv2 draft has stuffed it up on this one (though luckily it hasn't made it to RFC yet). I'll open a report on it. Cheers,
On Thu, Dec 18, 2008 at 10:04:53PM +1100, Herbert Xu wrote: > > > [1]https://lists.strongswan.org/pipermail/users/2008-December/002936.html > > Right that's what led to this patch. > > Reading this it seems that there is either a bug in strongswan or > in its peer. IKE or IKEv2 is supposed to negotiate UDP encapsulation > explicitly. So either we did negotiate to use UDP encapsulation, > and Strongswan ignored it (it's buggy), or we didn't and the peer > still chose to use it (the peer is buggy). You didn't address this part. What KM/OS was the peer running? How was it configured? Cheers,
From: Herbert Xu <herbert@gondor.apana.org.au> Date: Fri, 19 Dec 2008 07:54:06 +1100 > On Thu, Dec 18, 2008 at 01:36:56PM +0100, Martin Willi wrote: > > > > >From the key manager perspective, I can enable or disable UDP > > encapsulation, fine. I decide locally what I'll use for outgoing > > packets. But how should I know what the peer uses? I can't, it isn't > > negotiated. It is, by the standard, perfectly valid to send UDP > > encapsulated packets if the peer wants to do so. And there is no need to > > communicate this to the key manager, there is actually no such mechanism > > in IKEv2. Therefore I need the kernel to accept packet, encapsulated or > > not. > > Even if the kernel did accept such packets, there is no guarantee > that your return traffic will make it back to the other side because > stateful firewalls may be present. > > Responding with unencapsulated ESP traffic when the peer is sending > you UDP-encapsulated traffic is just not going to fly. > > BTW I think the IKEv2 draft has stuffed it up on this one (though > luckily it hasn't made it to RFC yet). I'll open a report on it. I'm going to revert the change from net-next-2.6 from now becasue: 1) The general scheme's validity is still suspect and under discussion 2) The change has a known bug (the UDP header access issue) 3) Martin can apply the change locally to do testing until we work this stuff out. Thanks guys. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c index b4a1317..65bcf09 100644 --- a/net/xfrm/xfrm_input.c +++ b/net/xfrm/xfrm_input.c @@ -167,11 +167,6 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type) goto drop_unlock; } - if ((x->encap ? x->encap->encap_type : 0) != encap_type) { - XFRM_INC_STATS(net, LINUX_MIB_XFRMINSTATEMISMATCH); - goto drop_unlock; - } - if (x->props.replay_window && xfrm_replay_check(x, skb, seq)) { XFRM_INC_STATS(net, LINUX_MIB_XFRMINSTATESEQERROR); goto drop_unlock;