[PATCH] IPsec NAT-T issue

[PATCH] IPsec NAT-T issue

[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

[PATCH] esp: correct offset for ESN when using NAT-T

[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

Re: [PATCH] IPsec NAT-T issue

[Steffen Klassert]

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?

Re: [PATCH] esp: correct offset for ESN when using NAT-T

[David Miller]

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.

Re: [PATCH] IPsec NAT-T issue

[Blair Steven]

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

Re: [PATCH] IPsec NAT-T issue

[Steffen Klassert]

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

esp: Fix ESN generation under UDP encapsulation

[Herbert Xu]

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

Re: esp: Fix ESN generation under UDP encapsulation

[Steffen Klassert]

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!

Re: esp: Fix ESN generation under UDP encapsulation

[Blair Steven]

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!

Re: esp: Fix ESN generation under UDP encapsulation

[Steffen Klassert]

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>

Re: esp: Fix ESN generation under UDP encapsulation

[David Miller]

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?

Re: esp: Fix ESN generation under UDP encapsulation

[Herbert Xu]

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