* [PATCH] IPsec NAT-T issue @ 2016-06-12 23:48 Blair Steven 2016-06-12 23:48 ` [PATCH] esp: correct offset for ESN when using NAT-T Blair Steven 2016-06-13 10:20 ` [PATCH] IPsec NAT-T issue Steffen Klassert 0 siblings, 2 replies; 12+ messages in thread From: Blair Steven @ 2016-06-12 23:48 UTC (permalink / raw) To: netdev; +Cc: Blair Steven During testing we have discovered an issue with IPsec NAT-T where the SPI is over writing the source and dest ports of the UDP header. I'm not super familiar with this code, but I've found a solution that seems to work in my setup. I'd like some feedback on this please, if it's the right thing to be doing here, or if it should be done elsewhere. Thanks very much Blair Steven (1): esp: correct offset for ESN when using NAT-T net/ipv4/esp4.c | 2 ++ 1 file changed, 2 insertions(+) -- 2.8.3 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] esp: correct offset for ESN when using NAT-T 2016-06-12 23:48 [PATCH] IPsec NAT-T issue Blair Steven @ 2016-06-12 23:48 ` Blair Steven 2016-06-14 21:12 ` David Miller 2016-06-13 10:20 ` [PATCH] IPsec NAT-T issue Steffen Klassert 1 sibling, 1 reply; 12+ messages in thread From: Blair Steven @ 2016-06-12 23:48 UTC (permalink / raw) To: netdev; +Cc: Blair Steven The offset for calculating ESN was not taking into account the new UDP header created for NAT-T. --- net/ipv4/esp4.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/net/ipv4/esp4.c b/net/ipv4/esp4.c index 4779374..c84d1fc 100644 --- a/net/ipv4/esp4.c +++ b/net/ipv4/esp4.c @@ -223,6 +223,8 @@ static int esp_output(struct xfrm_state *x, struct sk_buff *skb) uh->len = htons(skb->len - skb_transport_offset(skb)); uh->check = 0; + skb_set_transport_header(skb, skb_transport_offset(skb) + sizeof(struct udphdr)); + switch (encap_type) { default: case UDP_ENCAP_ESPINUDP: -- 2.8.3 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] esp: correct offset for ESN when using NAT-T 2016-06-12 23:48 ` [PATCH] esp: correct offset for ESN when using NAT-T Blair Steven @ 2016-06-14 21:12 ` David Miller 0 siblings, 0 replies; 12+ messages in thread From: David Miller @ 2016-06-14 21:12 UTC (permalink / raw) To: blair.steven; +Cc: netdev From: Blair Steven <blair.steven@alliedtelesis.co.nz> Date: Mon, 13 Jun 2016 11:48:14 +1200 > The offset for calculating ESN was not taking into account the new UDP > header created for NAT-T. All submissions must include a proper signoff. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] IPsec NAT-T issue 2016-06-12 23:48 [PATCH] IPsec NAT-T issue Blair Steven 2016-06-12 23:48 ` [PATCH] esp: correct offset for ESN when using NAT-T Blair Steven @ 2016-06-13 10:20 ` Steffen Klassert 2016-06-15 0:44 ` Blair Steven 1 sibling, 1 reply; 12+ messages in thread From: Steffen Klassert @ 2016-06-13 10:20 UTC (permalink / raw) To: Blair Steven; +Cc: netdev On Mon, Jun 13, 2016 at 11:48:13AM +1200, Blair Steven wrote: > > During testing we have discovered an issue with IPsec NAT-T where the SPI > is over writing the source and dest ports of the UDP header. The headers should be restored after the crypto operation in esp_restore_header(). Does this not happen in your case? What kind of problem do you experience? ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] IPsec NAT-T issue 2016-06-13 10:20 ` [PATCH] IPsec NAT-T issue Steffen Klassert @ 2016-06-15 0:44 ` Blair Steven 2016-06-17 10:24 ` Steffen Klassert 0 siblings, 1 reply; 12+ messages in thread From: Blair Steven @ 2016-06-15 0:44 UTC (permalink / raw) To: Steffen Klassert; +Cc: netdev The restoration is happening - but being actioned on the wrong location. The destination IP address is being saved and restored, and the SPI being written directly after the destination IP address. From my understanding though, the ESN shuffling should have saved and restored the UDP source / dest ports + SPI. -Blair On 06/13/2016 10:20 PM, Steffen Klassert wrote: > On Mon, Jun 13, 2016 at 11:48:13AM +1200, Blair Steven wrote: >> During testing we have discovered an issue with IPsec NAT-T where the SPI >> is over writing the source and dest ports of the UDP header. > The headers should be restored after the crypto operation in > esp_restore_header(). Does this not happen in your case? What > kind of problem do you experience? > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] IPsec NAT-T issue 2016-06-15 0:44 ` Blair Steven @ 2016-06-17 10:24 ` Steffen Klassert 2016-06-18 5:03 ` esp: Fix ESN generation under UDP encapsulation Herbert Xu 0 siblings, 1 reply; 12+ messages in thread From: Steffen Klassert @ 2016-06-17 10:24 UTC (permalink / raw) To: Blair Steven; +Cc: netdev, Herbert Xu On Wed, Jun 15, 2016 at 12:44:54AM +0000, Blair Steven wrote: > The restoration is happening - but being actioned on the wrong location. > > The destination IP address is being saved and restored, and the SPI > being written directly after the destination IP address. From my > understanding though, the ESN shuffling should have saved and restored > the UDP source / dest ports + SPI. Yes, looks like we copy with a wrong offset if udp encapsulation is used, skb_transport_header() does not point to the esp header in this case. Ccing Herbert, he changed this part when switching to the new AEAD interface with commit 7021b2e1cddd ("esp4: Switch to new AEAD interface"). > > -Blair > > On 06/13/2016 10:20 PM, Steffen Klassert wrote: > > On Mon, Jun 13, 2016 at 11:48:13AM +1200, Blair Steven wrote: > >> During testing we have discovered an issue with IPsec NAT-T where the SPI > >> is over writing the source and dest ports of the UDP header. > > The headers should be restored after the crypto operation in > > esp_restore_header(). Does this not happen in your case? What > > kind of problem do you experience? > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* esp: Fix ESN generation under UDP encapsulation 2016-06-17 10:24 ` Steffen Klassert @ 2016-06-18 5:03 ` Herbert Xu 2016-06-20 10:59 ` Steffen Klassert 0 siblings, 1 reply; 12+ messages in thread From: Herbert Xu @ 2016-06-18 5:03 UTC (permalink / raw) To: Steffen Klassert; +Cc: Blair Steven, netdev On Fri, Jun 17, 2016 at 12:24:29PM +0200, Steffen Klassert wrote: > On Wed, Jun 15, 2016 at 12:44:54AM +0000, Blair Steven wrote: > > The restoration is happening - but being actioned on the wrong location. > > > > The destination IP address is being saved and restored, and the SPI > > being written directly after the destination IP address. From my > > understanding though, the ESN shuffling should have saved and restored > > the UDP source / dest ports + SPI. > > Yes, looks like we copy with a wrong offset if udp encapsulation > is used, skb_transport_header() does not point to the esp header > in this case. Ccing Herbert, he changed this part when switching > to the new AEAD interface with > commit 7021b2e1cddd ("esp4: Switch to new AEAD interface"). Thanks for catching this! I think rather than changing the transport header (which isn't quite right because UDP still is the transport protocol), we can just save the offset locally. Something like this: ---8<--- Blair Steven noticed that ESN in conjunction with UDP encapsulation is broken because we set the temporary ESP header to the wrong spot. This patch fixes this by first of all using the right spot, i.e., 4 bytes off the real ESP header, and then saving this information so that after encryption we can restore it properly. Fixes: 7021b2e1cddd ("esp4: Switch to new AEAD interface") Reported-by: Blair Steven <Blair.Steven@alliedtelesis.co.nz> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> diff --git a/net/ipv4/esp4.c b/net/ipv4/esp4.c index 4779374..d95631d 100644 --- a/net/ipv4/esp4.c +++ b/net/ipv4/esp4.c @@ -23,6 +23,11 @@ struct esp_skb_cb { void *tmp; }; +struct esp_output_extra { + __be32 seqhi; + u32 esphoff; +}; + #define ESP_SKB_CB(__skb) ((struct esp_skb_cb *)&((__skb)->cb[0])) static u32 esp4_get_mtu(struct xfrm_state *x, int mtu); @@ -35,11 +40,11 @@ static u32 esp4_get_mtu(struct xfrm_state *x, int mtu); * * TODO: Use spare space in skb for this where possible. */ -static void *esp_alloc_tmp(struct crypto_aead *aead, int nfrags, int seqhilen) +static void *esp_alloc_tmp(struct crypto_aead *aead, int nfrags, int extralen) { unsigned int len; - len = seqhilen; + len = extralen; len += crypto_aead_ivsize(aead); @@ -57,15 +62,16 @@ static void *esp_alloc_tmp(struct crypto_aead *aead, int nfrags, int seqhilen) return kmalloc(len, GFP_ATOMIC); } -static inline __be32 *esp_tmp_seqhi(void *tmp) +static inline void *esp_tmp_extra(void *tmp) { - return PTR_ALIGN((__be32 *)tmp, __alignof__(__be32)); + return PTR_ALIGN(tmp, __alignof__(struct esp_output_extra)); } -static inline u8 *esp_tmp_iv(struct crypto_aead *aead, void *tmp, int seqhilen) + +static inline u8 *esp_tmp_iv(struct crypto_aead *aead, void *tmp, int extralen) { return crypto_aead_ivsize(aead) ? - PTR_ALIGN((u8 *)tmp + seqhilen, - crypto_aead_alignmask(aead) + 1) : tmp + seqhilen; + PTR_ALIGN((u8 *)tmp + extralen, + crypto_aead_alignmask(aead) + 1) : tmp + extralen; } static inline struct aead_request *esp_tmp_req(struct crypto_aead *aead, u8 *iv) @@ -99,7 +105,7 @@ static void esp_restore_header(struct sk_buff *skb, unsigned int offset) { struct ip_esp_hdr *esph = (void *)(skb->data + offset); void *tmp = ESP_SKB_CB(skb)->tmp; - __be32 *seqhi = esp_tmp_seqhi(tmp); + __be32 *seqhi = esp_tmp_extra(tmp); esph->seq_no = esph->spi; esph->spi = *seqhi; @@ -107,7 +113,11 @@ static void esp_restore_header(struct sk_buff *skb, unsigned int offset) static void esp_output_restore_header(struct sk_buff *skb) { - esp_restore_header(skb, skb_transport_offset(skb) - sizeof(__be32)); + void *tmp = ESP_SKB_CB(skb)->tmp; + struct esp_output_extra *extra = esp_tmp_extra(tmp); + + esp_restore_header(skb, skb_transport_offset(skb) + extra->esphoff - + sizeof(__be32)); } static void esp_output_done_esn(struct crypto_async_request *base, int err) @@ -121,6 +131,7 @@ static void esp_output_done_esn(struct crypto_async_request *base, int err) static int esp_output(struct xfrm_state *x, struct sk_buff *skb) { int err; + struct esp_output_extra *extra; struct ip_esp_hdr *esph; struct crypto_aead *aead; struct aead_request *req; @@ -137,8 +148,7 @@ static int esp_output(struct xfrm_state *x, struct sk_buff *skb) int tfclen; int nfrags; int assoclen; - int seqhilen; - __be32 *seqhi; + int extralen; __be64 seqno; /* skb is pure payload to encrypt */ @@ -166,21 +176,21 @@ static int esp_output(struct xfrm_state *x, struct sk_buff *skb) nfrags = err; assoclen = sizeof(*esph); - seqhilen = 0; + extralen = 0; if (x->props.flags & XFRM_STATE_ESN) { - seqhilen += sizeof(__be32); - assoclen += seqhilen; + extralen += sizeof(*extra); + assoclen += sizeof(__be32); } - tmp = esp_alloc_tmp(aead, nfrags, seqhilen); + tmp = esp_alloc_tmp(aead, nfrags, extralen); if (!tmp) { err = -ENOMEM; goto error; } - seqhi = esp_tmp_seqhi(tmp); - iv = esp_tmp_iv(aead, tmp, seqhilen); + extra = esp_tmp_extra(tmp); + iv = esp_tmp_iv(aead, tmp, extralen); req = esp_tmp_req(aead, iv); sg = esp_req_sg(aead, req); @@ -247,8 +257,10 @@ static int esp_output(struct xfrm_state *x, struct sk_buff *skb) * encryption. */ if ((x->props.flags & XFRM_STATE_ESN)) { - esph = (void *)(skb_transport_header(skb) - sizeof(__be32)); - *seqhi = esph->spi; + extra->esphoff = (unsigned char *)esph - + skb_transport_header(skb); + esph = (struct ip_esp_hdr *)((unsigned char *)esph - 4); + extra->seqhi = esph->spi; esph->seq_no = htonl(XFRM_SKB_CB(skb)->seq.output.hi); aead_request_set_callback(req, 0, esp_output_done_esn, skb); } @@ -445,7 +457,7 @@ static int esp_input(struct xfrm_state *x, struct sk_buff *skb) goto out; ESP_SKB_CB(skb)->tmp = tmp; - seqhi = esp_tmp_seqhi(tmp); + seqhi = esp_tmp_extra(tmp); iv = esp_tmp_iv(aead, tmp, seqhilen); req = esp_tmp_req(aead, iv); sg = esp_req_sg(aead, req); -- 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: esp: Fix ESN generation under UDP encapsulation 2016-06-18 5:03 ` esp: Fix ESN generation under UDP encapsulation Herbert Xu @ 2016-06-20 10:59 ` Steffen Klassert 2016-06-23 4:25 ` Blair Steven 0 siblings, 1 reply; 12+ messages in thread From: Steffen Klassert @ 2016-06-20 10:59 UTC (permalink / raw) To: Herbert Xu; +Cc: Blair Steven, netdev On Sat, Jun 18, 2016 at 01:03:36PM +0800, Herbert Xu wrote: > On Fri, Jun 17, 2016 at 12:24:29PM +0200, Steffen Klassert wrote: > > On Wed, Jun 15, 2016 at 12:44:54AM +0000, Blair Steven wrote: > > > The restoration is happening - but being actioned on the wrong location. > > > > > > The destination IP address is being saved and restored, and the SPI > > > being written directly after the destination IP address. From my > > > understanding though, the ESN shuffling should have saved and restored > > > the UDP source / dest ports + SPI. > > > > Yes, looks like we copy with a wrong offset if udp encapsulation > > is used, skb_transport_header() does not point to the esp header > > in this case. Ccing Herbert, he changed this part when switching > > to the new AEAD interface with > > commit 7021b2e1cddd ("esp4: Switch to new AEAD interface"). > > Thanks for catching this! > > I think rather than changing the transport header (which isn't > quite right because UDP still is the transport protocol), we can > just save the offset locally. Something like this: > > ---8<--- > Blair Steven noticed that ESN in conjunction with UDP encapsulation > is broken because we set the temporary ESP header to the wrong spot. > > This patch fixes this by first of all using the right spot, i.e., > 4 bytes off the real ESP header, and then saving this information > so that after encryption we can restore it properly. > > Fixes: 7021b2e1cddd ("esp4: Switch to new AEAD interface") > Reported-by: Blair Steven <Blair.Steven@alliedtelesis.co.nz> > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> Looks good. Blair could you please test this? Thanks! ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: esp: Fix ESN generation under UDP encapsulation 2016-06-20 10:59 ` Steffen Klassert @ 2016-06-23 4:25 ` Blair Steven 2016-06-23 10:40 ` Steffen Klassert 0 siblings, 1 reply; 12+ messages in thread From: Blair Steven @ 2016-06-23 4:25 UTC (permalink / raw) To: Steffen Klassert, Herbert Xu; +Cc: netdev This change tests okay in my setup. Thanks very much -Blair On 06/20/2016 10:59 PM, Steffen Klassert wrote: > On Sat, Jun 18, 2016 at 01:03:36PM +0800, Herbert Xu wrote: >> On Fri, Jun 17, 2016 at 12:24:29PM +0200, Steffen Klassert wrote: >>> On Wed, Jun 15, 2016 at 12:44:54AM +0000, Blair Steven wrote: >>>> The restoration is happening - but being actioned on the wrong location. >>>> >>>> The destination IP address is being saved and restored, and the SPI >>>> being written directly after the destination IP address. From my >>>> understanding though, the ESN shuffling should have saved and restored >>>> the UDP source / dest ports + SPI. >>> Yes, looks like we copy with a wrong offset if udp encapsulation >>> is used, skb_transport_header() does not point to the esp header >>> in this case. Ccing Herbert, he changed this part when switching >>> to the new AEAD interface with >>> commit 7021b2e1cddd ("esp4: Switch to new AEAD interface"). >> Thanks for catching this! >> >> I think rather than changing the transport header (which isn't >> quite right because UDP still is the transport protocol), we can >> just save the offset locally. Something like this: >> >> ---8<--- >> Blair Steven noticed that ESN in conjunction with UDP encapsulation >> is broken because we set the temporary ESP header to the wrong spot. >> >> This patch fixes this by first of all using the right spot, i.e., >> 4 bytes off the real ESP header, and then saving this information >> so that after encryption we can restore it properly. >> >> Fixes: 7021b2e1cddd ("esp4: Switch to new AEAD interface") >> Reported-by: Blair Steven <Blair.Steven@alliedtelesis.co.nz> >> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au> > Looks good. > Blair could you please test this? > > Thanks! ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: esp: Fix ESN generation under UDP encapsulation 2016-06-23 4:25 ` Blair Steven @ 2016-06-23 10:40 ` Steffen Klassert 2016-06-23 15:52 ` David Miller 0 siblings, 1 reply; 12+ messages in thread From: Steffen Klassert @ 2016-06-23 10:40 UTC (permalink / raw) To: Blair Steven, David Miller; +Cc: Herbert Xu, netdev On Thu, Jun 23, 2016 at 04:25:21AM +0000, Blair Steven wrote: > This change tests okay in my setup. > > Thanks very much > -Blair David, can you please take this patch directly in the net tree? This is a candidate for stable. Acked-by: Steffen Klassert <steffen.klassert@secunet.com> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: esp: Fix ESN generation under UDP encapsulation 2016-06-23 10:40 ` Steffen Klassert @ 2016-06-23 15:52 ` David Miller 2016-06-24 1:52 ` Herbert Xu 0 siblings, 1 reply; 12+ messages in thread From: David Miller @ 2016-06-23 15:52 UTC (permalink / raw) To: steffen.klassert; +Cc: Blair.Steven, herbert, netdev From: Steffen Klassert <steffen.klassert@secunet.com> Date: Thu, 23 Jun 2016 12:40:07 +0200 > On Thu, Jun 23, 2016 at 04:25:21AM +0000, Blair Steven wrote: >> This change tests okay in my setup. >> >> Thanks very much >> -Blair > > David, can you please take this patch directly in the net tree? > This is a candidate for stable. > > Acked-by: Steffen Klassert <steffen.klassert@secunet.com> Applied, thanks everyone. Does the ipv6 side need the same fix? ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: esp: Fix ESN generation under UDP encapsulation 2016-06-23 15:52 ` David Miller @ 2016-06-24 1:52 ` Herbert Xu 0 siblings, 0 replies; 12+ messages in thread From: Herbert Xu @ 2016-06-24 1:52 UTC (permalink / raw) To: David Miller; +Cc: steffen.klassert, Blair.Steven, netdev On Thu, Jun 23, 2016 at 11:52:45AM -0400, David Miller wrote: > > Does the ipv6 side need the same fix? Last I checked IPv6 didn't do IPsec UDP-encapsulation so we're safe for now. 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2016-06-24 1:53 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-06-12 23:48 [PATCH] IPsec NAT-T issue Blair Steven 2016-06-12 23:48 ` [PATCH] esp: correct offset for ESN when using NAT-T Blair Steven 2016-06-14 21:12 ` David Miller 2016-06-13 10:20 ` [PATCH] IPsec NAT-T issue Steffen Klassert 2016-06-15 0:44 ` Blair Steven 2016-06-17 10:24 ` Steffen Klassert 2016-06-18 5:03 ` esp: Fix ESN generation under UDP encapsulation Herbert Xu 2016-06-20 10:59 ` Steffen Klassert 2016-06-23 4:25 ` Blair Steven 2016-06-23 10:40 ` Steffen Klassert 2016-06-23 15:52 ` David Miller 2016-06-24 1:52 ` Herbert Xu
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.