[IPCOMP6] Exclude IPCOMP header from props.header_len

[IPCOMP6] Exclude IPCOMP header from props.header_len

[Herbert Xu]

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

Hi:

Now that the IPv4 encap stuff is out of the way, I'll be sending you
the IPv6 versions.

Here is the one to remove the unnecessary extra space reserved for
IPCOMP.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

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

[-- Attachment #2: p --]
[-- Type: text/plain, Size: 1395 bytes --]

===== net/ipv6/ipcomp6.c 1.11 vs edited =====
--- 1.11/net/ipv6/ipcomp6.c	2004-06-18 16:20:58 +10:00
+++ edited/net/ipv6/ipcomp6.c	2004-07-10 13:26:45 +10:00
@@ -123,7 +123,7 @@
 	int err;
 	struct dst_entry *dst = (*pskb)->dst;
 	struct xfrm_state *x = dst->xfrm;
-	struct ipv6hdr *tmp_iph = NULL, *iph, *top_iph;
+	struct ipv6hdr *iph, *top_iph;
 	int hdr_len = 0;
 	struct ipv6_comp_hdr *ipch;
 	struct ipcomp_data *ipcd = x->data;
@@ -193,19 +193,11 @@
 	if ((dlen + sizeof(struct ipv6_comp_hdr)) >= plen) {
 		goto out_ok;
 	}
-	memcpy(start, scratch, dlen);
-	pskb_trim(*pskb, hdr_len+dlen);
+	memcpy(start + sizeof(struct ip_comp_hdr), scratch, dlen);
+	pskb_trim(*pskb, hdr_len + dlen + sizeof(struct ip_comp_hdr));
 
 	/* insert ipcomp header and replace datagram */
-	tmp_iph = kmalloc(hdr_len, GFP_ATOMIC);
-	if (!tmp_iph) {
-		err = -ENOMEM;
-		goto error;
-	}
-	memcpy(tmp_iph, (*pskb)->nh.raw, hdr_len);
-	top_iph = (struct ipv6hdr*)skb_push(*pskb, sizeof(struct ipv6_comp_hdr));
-	memcpy(top_iph, tmp_iph, hdr_len);
-	kfree(tmp_iph);
+	top_iph = (*pskb)->nh.ipv6h;
 
 	if (x->props.mode && (x->props.flags & XFRM_STATE_NOECN))
 		IP6_ECN_clear(top_iph);
@@ -358,7 +350,7 @@
 		goto error;
 
 	memset(ipcd, 0, sizeof(*ipcd));
-	x->props.header_len = sizeof(struct ipv6_comp_hdr);
+	x->props.header_len = 0;
 	if (x->props.mode)
 		x->props.header_len += sizeof(struct ipv6hdr);
 	

Re: [IPCOMP6] Exclude IPCOMP header from props.header_len

[YOSHIFUJI Hideaki / 吉藤英明]

In article <20040710033209.GA14316@gondor.apana.org.au> (at Sat, 10 Jul 2004 13:32:09 +1000), Herbert Xu <herbert@gondor.apana.org.au> says:

> Now that the IPv4 encap stuff is out of the way, I'll be sending you
> the IPv6 versions.

Looks good.

--yoshfuji

IPv6 and encapsulation headers

[Herbert Xu]

On Sat, Jul 10, 2004 at 01:36:41PM +0900, YOSHIFUJI Hideaki / ?$B5HF#1QL@ wrote:
> 
> Looks good.

Thanks for reviewing it.

I've got a couple of questions that you might be able to help me with.

It appears that the value of hdr_len in ah6 for transport mode is broken.
It's setting hdr_len to be skb->h.raw - skb->nh.raw.  When AH is being
applied outside an ESP tunnel, skb->h.raw will be pointing somewhere
inside the tunnel.  The end result is that leading bytes of the payload
inside the tunnel gets moved before the AH header.

So should it be changed to ip6_find_1stfragopt() as is the case with
esp6 and ipcomp6?

A second problem is that ip6_find_1stfragopt() seems to be the wrong
thing to do for ah6/esp6/ipcomp6.  RFC 2402/2406/3173 all say that
fragment headers should be placed before the encapsulation header.
So should it be changed accordingly?

Thanks again,
-- 
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: IPv6 and encapsulation headers

[Kazunori Miyazawa]

Hello Herbert,

> On Sat, Jul 10, 2004 at 01:36:41PM +0900, YOSHIFUJI Hideaki / ?$B5HF#1QL@ 
wrote:
> > Looks good.
>
> Thanks for reviewing it.
>
> I've got a couple of questions that you might be able to help me with.
>
> It appears that the value of hdr_len in ah6 for transport mode is broken.
> It's setting hdr_len to be skb->h.raw - skb->nh.raw.  When AH is being
> applied outside an ESP tunnel, skb->h.raw will be pointing somewhere
> inside the tunnel.  The end result is that leading bytes of the payload
> inside the tunnel gets moved before the AH header.
>

right, esp6 tunnel doesn't care about skb->h.raw. we need to fix it.

> So should it be changed to ip6_find_1stfragopt() as is the case with
> esp6 and ipcomp6?

Do we need to skip esp or ipcomp payload?
I thinks those are similar with transport layer protocol in outer esp process.
Did I misunderstand your question?

> A second problem is that ip6_find_1stfragopt() seems to be the wrong
> thing to do for ah6/esp6/ipcomp6.  RFC 2402/2406/3173 all say that
> fragment headers should be placed before the encapsulation header.
> So should it be changed accordingly?
>


Because fragmentation takes place after IPsec processing,
do we need to make ip6_find_1stfragopt care fragment header?
I think there is no fragment header in skb at that point.

--Kazunori Miyazawa

Re: IPv6 and encapsulation headers

[Herbert Xu]

On Mon, Jul 12, 2004 at 05:32:52PM +0900, Kazunori Miyazawa wrote:
> 
> right, esp6 tunnel doesn't care about skb->h.raw. we need to fix it.

The same needs to be done to the other tunnels as well.  But please
consider the issue in the next paragraph first before doing this.

> > So should it be changed to ip6_find_1stfragopt() as is the case with
> > esp6 and ipcomp6?
> 
> Do we need to skip esp or ipcomp payload?
> I thinks those are similar with transport layer protocol in outer esp process.
> Did I misunderstand your question?

I don't know because I didn't understand your question :)

Let me state a few things and please tell me whether you agree or
disagree:

1. AH's position should be determined by the bundle.  So if the
bundle says AH+ESP then AH goes on the outside, if the bundle says
ESP+AH or just AH then AH goes on the inside.

2. If AH is the inner-most xfrm then it should be applied before
the second destination options header.

It seems to me that skb->h is not actually set to the spot pointed
to ip6_find_1stfragopt() by anything apart from the xfrm output
functions.

Therefore if AH is the inner-most xfrm, then skb->h will also point
to the wrong spot.  It would appear to be safest to call
ip6_find_1stfragopt() in AH instead of relying on the value of skb->h.

Regardless of whether we use skb->h or ip6_find_1stfragopt() though,
ah6/esp6/ipcomp6 should all use the same logic to find their spot for
encapsulation.  The reason is that the specification in 2402/2406/3173
is identical so we shouldn't have special-case code in AH.
 
> Because fragmentation takes place after IPsec processing,
> do we need to make ip6_find_1stfragopt care fragment header?
> I think there is no fragment header in skb at that point.

Good point.

Hmm, what about address spoofing? Is there code in IPv6 to prevent
another machine from relaying a packet through us with our source
address?

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: IPv6 and encapsulation headers

[Kazunori Miyazawa]

2004/07/12(月) 19:47、Herbert Xu  wrote:
> On Mon, Jul 12, 2004 at 05:32:52PM +0900, Kazunori Miyazawa wrote:
> > right, esp6 tunnel doesn't care about skb->h.raw. we need to fix it.
>
> The same needs to be done to the other tunnels as well.  But please
> consider the issue in the next paragraph first before doing this.
>
> > > So should it be changed to ip6_find_1stfragopt() as is the case with
> > > esp6 and ipcomp6?
> >
> > Do we need to skip esp or ipcomp payload?
> > I thinks those are similar with transport layer protocol in outer esp
> > process. Did I misunderstand your question?
>
> I don't know because I didn't understand your question :)
>
> Let me state a few things and please tell me whether you agree or
> disagree:
>
> 1. AH's position should be determined by the bundle.  So if the
> bundle says AH+ESP then AH goes on the outside, if the bundle says
> ESP+AH or just AH then AH goes on the inside.
>
agree. 

> 2. If AH is the inner-most xfrm then it should be applied before
> the second destination options header.
Yes.

>
> It seems to me that skb->h is not actually set to the spot pointed
> to ip6_find_1stfragopt() by anything apart from the xfrm output
> functions.
>
> Therefore if AH is the inner-most xfrm, then skb->h will also point
> to the wrong spot.  It would appear to be safest to call
> ip6_find_1stfragopt() in AH instead of relying on the value of skb->h.
>
I agree with you. It should uses ip6_find_1stfragopt.
However please consider zero_out_mutable_opts in ah6.c clears second 
destination options. We need to get the copy length by other way.
Because ip6_find_1stfragopt returns the insert point of IPsec.

> Regardless of whether we use skb->h or ip6_find_1stfragopt() though,
> ah6/esp6/ipcomp6 should all use the same logic to find their spot for
> encapsulation.  The reason is that the specification in 2402/2406/3173
> is identical so we shouldn't have special-case code in AH.
>
> > Because fragmentation takes place after IPsec processing,
> > do we need to make ip6_find_1stfragopt care fragment header?
> > I think there is no fragment header in skb at that point.
>
> Good point.
>
> Hmm, what about address spoofing? Is there code in IPv6 to prevent
> another machine from relaying a packet through us with our source
> address?
>
Does it concern with IPsec or fragmentation?
It might be netfiler stuff. 

Thank you,

--Kazunori Miyazawa

Re: IPv6 and encapsulation headers

[Herbert Xu]

On Tue, Jul 13, 2004 at 10:42:41AM +0900, Kazunori Miyazawa wrote:
>
> I agree with you. It should uses ip6_find_1stfragopt.
> However please consider zero_out_mutable_opts in ah6.c clears second 
> destination options. We need to get the copy length by other way.
> Because ip6_find_1stfragopt returns the insert point of IPsec.

Are there situations where it is desirable to have mutable options
in the second destination header? Isn't the idea of the second
destination header so that it is processed only by the final
destination? I would've thought that it only made sense to have
immutable options there.

In fact if ESP were present then it guarantees the second dest
header to be immutable.  So perhaps we should simply disallow
users from putting mutable options in the second destination
header.  Or we can document it as undefined and let the user
handle the consequences (cf HDRINCL with raw sockets + IPsec).

BTW, the current code in ipv6_clear_mutable_options() that deals
with NEXTHDR_AUTH is buggy.  It assumes that there are at most two
AH headers.

> > Hmm, what about address spoofing? Is there code in IPv6 to prevent
> > another machine from relaying a packet through us with our source
> > address?
>
> Does it concern with IPsec or fragmentation?
> It might be netfiler stuff. 

Well it means that you can't make certain assumptions in the
xfrm code.  For example, you cannot rely on the ordering of
extension headers since the original sender may have applied
a different ordering mechanism.

But I don't think it poses any security risks to the current
xfrm6 code so we can just leave that as undefined behaviour.

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: [IPCOMP6] Exclude IPCOMP header from props.header_len

[David S. Miller]

On Sat, 10 Jul 2004 13:36:41 +0900 (JST)
YOSHIFUJI Hideaki / ^[$B5HF#1QL@^[(B <yoshfuji@linux-ipv6.org> wrote:

> In article <20040710033209.GA14316@gondor.apana.org.au> (at Sat, 10 Jul 2004 13:32:09 +1000), Herbert Xu <herbert@gondor.apana.org.au> says:
> 
> > Now that the IPv4 encap stuff is out of the way, I'll be sending you
> > the IPv6 versions.
> 
> Looks good.

To me too, patch applied.