* [AH6] Disallow mutable bits after AH header
@ 2004-07-23 13:53 Herbert Xu
2004-07-23 20:37 ` David S. Miller
0 siblings, 1 reply; 4+ messages in thread
From: Herbert Xu @ 2004-07-23 13:53 UTC (permalink / raw)
To: Kazunori Miyazawa, David S. Miller, netdev
[-- Attachment #1: Type: text/plain, Size: 756 bytes --]
Hi:
As we discussed before, mutable headers should not be allowed after
the AH header. In fact, this appears to be the intention of RFC 2402.
It is further clarified in section 3.1.1 of
http://www.ietf.org/internet-drafts/draft-ietf-ipsec-rfc2402bis-07.txt
This allows us to simplify the code in ah6.c. As a result, this also
fixes the following issues:
* Dependence on skb->h in ah6_output().
* Bogus clearing of auth_data of 2nd AH header in ipv6_clear_mutable_options().
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: 5718 bytes --]
===== net/ipv6/ah6.c 1.32 vs edited =====
--- 1.32/net/ipv6/ah6.c 2004-06-18 16:20:58 +10:00
+++ edited/net/ipv6/ah6.c 2004-07-23 23:44:50 +10:00
@@ -74,34 +74,29 @@
return 0;
}
-static int ipv6_clear_mutable_options(struct sk_buff *skb, u16 *nh_offset, int dir)
+static int ipv6_clear_mutable_options(struct sk_buff *skb,
+ unsigned int hdr_len)
{
u16 offset = sizeof(struct ipv6hdr);
struct ipv6_opt_hdr *exthdr = (struct ipv6_opt_hdr*)(skb->nh.raw + offset);
- unsigned int packet_len = skb->tail - skb->nh.raw;
u8 nexthdr = skb->nh.ipv6h->nexthdr;
- u8 nextnexthdr = 0;
- *nh_offset = ((unsigned char *)&skb->nh.ipv6h->nexthdr) - skb->nh.raw;
-
- while (offset + 1 <= packet_len) {
+ while (offset + 1 <= hdr_len) {
switch (nexthdr) {
case NEXTHDR_HOP:
- *nh_offset = offset;
offset += ipv6_optlen(exthdr);
if (!zero_out_mutable_opts(exthdr)) {
LIMIT_NETDEBUG(
printk(KERN_WARNING "overrun hopopts\n"));
- return 0;
+ return -EINVAL;
}
nexthdr = exthdr->nexthdr;
exthdr = (struct ipv6_opt_hdr*)(skb->nh.raw + offset);
break;
case NEXTHDR_ROUTING:
- *nh_offset = offset;
offset += ipv6_optlen(exthdr);
((struct ipv6_rt_hdr*)exthdr)->segments_left = 0;
nexthdr = exthdr->nexthdr;
@@ -109,39 +104,22 @@
break;
case NEXTHDR_DEST:
- *nh_offset = offset;
offset += ipv6_optlen(exthdr);
if (!zero_out_mutable_opts(exthdr)) {
LIMIT_NETDEBUG(
printk(KERN_WARNING "overrun destopt\n"));
- return 0;
+ return -EINVAL;
}
nexthdr = exthdr->nexthdr;
exthdr = (struct ipv6_opt_hdr*)(skb->nh.raw + offset);
break;
- case NEXTHDR_AUTH:
- if (dir == XFRM_POLICY_OUT) {
- memset(((struct ipv6_auth_hdr*)exthdr)->auth_data, 0,
- (((struct ipv6_auth_hdr*)exthdr)->hdrlen - 1) << 2);
- }
- if (exthdr->nexthdr == NEXTHDR_DEST) {
- offset += (((struct ipv6_auth_hdr*)exthdr)->hdrlen + 2) << 2;
- exthdr = (struct ipv6_opt_hdr*)(skb->nh.raw + offset);
- nextnexthdr = exthdr->nexthdr;
- if (!zero_out_mutable_opts(exthdr)) {
- LIMIT_NETDEBUG(
- printk(KERN_WARNING "overrun destopt\n"));
- return 0;
- }
- }
- return nexthdr;
default :
- return nexthdr;
+ return 0;
}
}
- return nexthdr;
+ return 0;
}
int ah6_output(struct sk_buff **pskb)
@@ -153,7 +131,6 @@
struct ipv6hdr *iph = NULL;
struct ip_auth_hdr *ah;
struct ah_data *ahp;
- u16 nh_offset = 0;
u8 nexthdr;
if ((*pskb)->ip_summed == CHECKSUM_HW) {
@@ -184,7 +161,11 @@
ah = (struct ip_auth_hdr*)((*pskb)->nh.ipv6h+1);
ah->nexthdr = IPPROTO_IPV6;
} else {
- hdr_len = (*pskb)->h.raw - (*pskb)->nh.raw;
+ u8 *prevhdr;
+
+ hdr_len = ip6_find_1stfragopt(*pskb, &prevhdr);
+ nexthdr = *prevhdr;
+ *prevhdr = IPPROTO_AH;
iph = kmalloc(hdr_len, GFP_ATOMIC);
if (!iph) {
err = -ENOMEM;
@@ -192,13 +173,12 @@
}
memcpy(iph, (*pskb)->data, hdr_len);
(*pskb)->nh.ipv6h = (struct ipv6hdr*)skb_push(*pskb, x->props.header_len);
+ iph->payload_len = htons((*pskb)->len - sizeof(struct ipv6hdr));
memcpy((*pskb)->nh.ipv6h, iph, hdr_len);
- nexthdr = ipv6_clear_mutable_options(*pskb, &nh_offset, XFRM_POLICY_OUT);
- if (nexthdr == 0)
+ err = ipv6_clear_mutable_options(*pskb, hdr_len);
+ if (err)
goto error_free_iph;
- (*pskb)->nh.raw[nh_offset] = IPPROTO_AH;
- (*pskb)->nh.ipv6h->payload_len = htons((*pskb)->len - sizeof(struct ipv6hdr));
ah = (struct ip_auth_hdr*)((*pskb)->nh.raw+hdr_len);
(*pskb)->h.raw = (unsigned char*) ah;
ah->nexthdr = nexthdr;
@@ -229,8 +209,6 @@
IP6_ECN_clear((*pskb)->nh.ipv6h);
} else {
memcpy((*pskb)->nh.ipv6h, iph, hdr_len);
- (*pskb)->nh.raw[nh_offset] = IPPROTO_AH;
- (*pskb)->nh.ipv6h->payload_len = htons((*pskb)->len - sizeof(struct ipv6hdr));
kfree (iph);
}
@@ -259,7 +237,6 @@
* Before process AH
* [IPv6][Ext1][Ext2][AH][Dest][Payload]
* |<-------------->| hdr_len
- * |<------------------------>| cleared_hlen
*
* To erase AH:
* Keeping copy of cleared headers. After AH processing,
@@ -276,7 +253,6 @@
unsigned char *tmp_hdr = NULL;
u16 hdr_len;
u16 ah_hlen;
- u16 cleared_hlen;
u16 nh_offset = 0;
u8 nexthdr = 0;
u8 *prevhdr;
@@ -291,17 +267,10 @@
goto out;
hdr_len = skb->data - skb->nh.raw;
- cleared_hlen = hdr_len;
ah = (struct ipv6_auth_hdr*)skb->data;
ahp = x->data;
nexthdr = ah->nexthdr;
ah_hlen = (ah->hdrlen + 2) << 2;
- cleared_hlen += ah_hlen;
-
- if (nexthdr == NEXTHDR_DEST) {
- struct ipv6_opt_hdr *dsthdr = (struct ipv6_opt_hdr*)(skb->data + ah_hlen);
- cleared_hlen += ipv6_optlen(dsthdr);
- }
if (ah_hlen != XFRM_ALIGN8(sizeof(struct ipv6_auth_hdr) + ahp->icv_full_len) &&
ah_hlen != XFRM_ALIGN8(sizeof(struct ipv6_auth_hdr) + ahp->icv_trunc_len))
@@ -310,11 +279,12 @@
if (!pskb_may_pull(skb, ah_hlen))
goto out;
- tmp_hdr = kmalloc(cleared_hlen, GFP_ATOMIC);
+ tmp_hdr = kmalloc(hdr_len, GFP_ATOMIC);
if (!tmp_hdr)
goto out;
- memcpy(tmp_hdr, skb->nh.raw, cleared_hlen);
- ipv6_clear_mutable_options(skb, &nh_offset, XFRM_POLICY_IN);
+ memcpy(tmp_hdr, skb->nh.raw, hdr_len);
+ if (ipv6_clear_mutable_options(skb, hdr_len))
+ goto out;
skb->nh.ipv6h->priority = 0;
skb->nh.ipv6h->flow_lbl[0] = 0;
skb->nh.ipv6h->flow_lbl[1] = 0;
@@ -338,11 +308,6 @@
skb->nh.raw = skb_pull(skb, ah_hlen);
memcpy(skb->nh.raw, tmp_hdr, hdr_len);
- if (nexthdr == NEXTHDR_DEST) {
- memcpy(skb->nh.raw + hdr_len,
- tmp_hdr + hdr_len + ah_hlen,
- cleared_hlen - hdr_len - ah_hlen);
- }
prevhdr = (u8*)(skb->nh.raw + nh_offset);
*prevhdr = nexthdr;
skb->nh.ipv6h->payload_len = htons(skb->len - sizeof(struct ipv6hdr));
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [AH6] Disallow mutable bits after AH header
2004-07-23 13:53 [AH6] Disallow mutable bits after AH header Herbert Xu
@ 2004-07-23 20:37 ` David S. Miller
2004-07-28 11:46 ` Herbert Xu
0 siblings, 1 reply; 4+ messages in thread
From: David S. Miller @ 2004-07-23 20:37 UTC (permalink / raw)
To: Herbert Xu; +Cc: kazunori, netdev
On Fri, 23 Jul 2004 23:53:21 +1000
Herbert Xu <herbert@gondor.apana.org.au> wrote:
> As we discussed before, mutable headers should not be allowed after
> the AH header. In fact, this appears to be the intention of RFC 2402.
> It is further clarified in section 3.1.1 of
>
> http://www.ietf.org/internet-drafts/draft-ietf-ipsec-rfc2402bis-07.txt
>
> This allows us to simplify the code in ah6.c. As a result, this also
> fixes the following issues:
>
> * Dependence on skb->h in ah6_output().
> * Bogus clearing of auth_data of 2nd AH header in ipv6_clear_mutable_options().
Applied, thanks Herbert.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [AH6] Disallow mutable bits after AH header
2004-07-23 20:37 ` David S. Miller
@ 2004-07-28 11:46 ` Herbert Xu
2004-07-28 16:11 ` David S. Miller
0 siblings, 1 reply; 4+ messages in thread
From: Herbert Xu @ 2004-07-28 11:46 UTC (permalink / raw)
To: David S. Miller; +Cc: kazunori, netdev
[-- Attachment #1: Type: text/plain, Size: 1196 bytes --]
On Fri, Jul 23, 2004 at 01:37:37PM -0700, David S. Miller wrote:
> On Fri, 23 Jul 2004 23:53:21 +1000
> Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> > As we discussed before, mutable headers should not be allowed after
> > the AH header. In fact, this appears to be the intention of RFC 2402.
> > It is further clarified in section 3.1.1 of
>
> Applied, thanks Herbert.
Unfortunately I broke ah6_input() in that patch. Thanks to Miyazawa-san
for notifying me of the problem.
In that patch I removed the nh_offset parameter to ipv6_clear_mutable_options.
That broke ah6_input() because it relies on that variable to set the nexthdr.
The following patch fixes this by moving this work out to the caller
xfrm6_rcv() where the information is already available. It also removes
an unnecessary call to ip6_find_1stfragopt() in xfrm6_rcv() since nhoffp
already points to the nexthdr preceding the current header.
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: r --]
[-- Type: text/plain, Size: 2630 bytes --]
===== net/ipv6/ah6.c 1.36 vs edited =====
--- 1.36/net/ipv6/ah6.c 2004-07-25 20:33:46 +10:00
+++ edited/net/ipv6/ah6.c 2004-07-28 19:36:35 +10:00
@@ -252,9 +252,7 @@
unsigned char *tmp_hdr = NULL;
u16 hdr_len;
u16 ah_hlen;
- u16 nh_offset = 0;
- u8 nexthdr = 0;
- u8 *prevhdr;
+ int nexthdr;
if (!pskb_may_pull(skb, sizeof(struct ip_auth_hdr)))
goto out;
@@ -307,8 +305,6 @@
skb->nh.raw = skb_pull(skb, ah_hlen);
memcpy(skb->nh.raw, tmp_hdr, hdr_len);
- prevhdr = (u8*)(skb->nh.raw + nh_offset);
- *prevhdr = nexthdr;
skb->nh.ipv6h->payload_len = htons(skb->len - sizeof(struct ipv6hdr));
skb_pull(skb, hdr_len);
skb->h.raw = skb->data;
===== net/ipv6/esp6.c 1.31 vs edited =====
--- 1.31/net/ipv6/esp6.c 2004-07-25 20:33:46 +10:00
+++ edited/net/ipv6/esp6.c 2004-07-28 19:35:13 +10:00
@@ -197,7 +197,6 @@
u8 nexthdr[2];
struct scatterlist *sg = &esp->sgbuf[0];
u8 padlen;
- u8 *prevhdr;
if (unlikely(nfrags > ESP_NUM_FAST_SG)) {
sg = kmalloc(sizeof(struct scatterlist)*nfrags, GFP_ATOMIC);
@@ -228,8 +227,7 @@
skb->nh.raw += sizeof(struct ipv6_esp_hdr) + esp->conf.ivlen;
memcpy(skb->nh.raw, tmp_hdr, hdr_len);
skb->nh.ipv6h->payload_len = htons(skb->len - sizeof(struct ipv6hdr));
- ip6_find_1stfragopt(skb, &prevhdr);
- ret = *prevhdr = nexthdr[1];
+ ret = nexthdr[1];
}
out:
===== net/ipv6/ipcomp6.c 1.16 vs edited =====
--- 1.16/net/ipv6/ipcomp6.c 2004-07-25 20:33:46 +10:00
+++ edited/net/ipv6/ipcomp6.c 2004-07-28 19:37:00 +10:00
@@ -49,7 +49,6 @@
{
int err = 0;
u8 nexthdr = 0;
- u8 *prevhdr;
int hdr_len = skb->h.raw - skb->nh.raw;
unsigned char *tmp_hdr = NULL;
struct ipv6hdr *iph;
@@ -106,8 +105,6 @@
iph = skb->nh.ipv6h;
iph->payload_len = htons(skb->len);
- ip6_find_1stfragopt(skb, &prevhdr);
- *prevhdr = nexthdr;
out:
if (tmp_hdr)
kfree(tmp_hdr);
===== net/ipv6/xfrm6_input.c 1.15 vs edited =====
--- 1.15/net/ipv6/xfrm6_input.c 2004-02-14 18:06:45 +11:00
+++ edited/net/ipv6/xfrm6_input.c 2004-07-28 19:42:26 +10:00
@@ -34,12 +34,11 @@
struct xfrm_state *x;
int xfrm_nr = 0;
int decaps = 0;
- int nexthdr = 0;
- u8 *prevhdr = NULL;
+ int nexthdr;
+ unsigned int nhoff;
- ip6_find_1stfragopt(skb, &prevhdr);
- nexthdr = *prevhdr;
- *nhoffp = prevhdr - skb->nh.raw;
+ nhoff = *nhoffp;
+ nexthdr = skb->nh.raw[nhoff];
if ((err = xfrm_parse_spi(skb, nexthdr, &spi, &seq)) != 0)
goto drop;
@@ -66,6 +65,8 @@
nexthdr = x->type->input(x, &(xfrm_vec[xfrm_nr].decap), skb);
if (nexthdr <= 0)
goto drop_unlock;
+
+ skb->nh.raw[nhoff] = nexthdr;
if (x->props.replay_window)
xfrm_replay_advance(x, seq);
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [AH6] Disallow mutable bits after AH header
2004-07-28 11:46 ` Herbert Xu
@ 2004-07-28 16:11 ` David S. Miller
0 siblings, 0 replies; 4+ messages in thread
From: David S. Miller @ 2004-07-28 16:11 UTC (permalink / raw)
To: Herbert Xu; +Cc: kazunori, netdev
On Wed, 28 Jul 2004 21:46:32 +1000
Herbert Xu <herbert@gondor.apana.org.au> wrote:
> In that patch I removed the nh_offset parameter to ipv6_clear_mutable_options.
> That broke ah6_input() because it relies on that variable to set the nexthdr.
>
> The following patch fixes this by moving this work out to the caller
> xfrm6_rcv() where the information is already available. It also removes
> an unnecessary call to ip6_find_1stfragopt() in xfrm6_rcv() since nhoffp
> already points to the nexthdr preceding the current header.
Applied, thanks.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2004-07-28 16:11 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2004-07-23 13:53 [AH6] Disallow mutable bits after AH header Herbert Xu
2004-07-23 20:37 ` David S. Miller
2004-07-28 11:46 ` Herbert Xu
2004-07-28 16:11 ` David S. Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).