Skip to contentSkip to conversation

netfilter6: Skip extension headers when matching icmp6-type

PATCH In review Olaf Kirch 5 participants 9 messages Sep 30, 2004 – Oct 1, 2004

Subsystems (3)
Networking Network drivers netfilter
lore
Share & tools
lore

Olaf Kirch

author
#0 lore raw

PATCH 1/1 netfilter6: Skip extension headers when matching icmp6-type

This patch fixes a bug in the ip6_tables code when matching ICMP type and code within ICMPv6 packets. The icmpv6 packet matcher expects the nexthdr to be ICMPv6 and does not deal with hop-by-hop headers etc.

Signed-off-by Olaf Kirch <[email protected]>
net/ipv6/netfilter/ip6_tables.c +17 -5
Index: linux-2.6.8.nf/net/ipv6/netfilter/ip6_tables.c
===================================================================
@@ -1751,10 +1751,23 @@
 	   u_int16_t datalen,
 	   int *hotdrop)
 {
-	const struct icmp6hdr *icmp = hdr;
+	struct icmp6hdr icmph;
 	const struct ip6t_icmp *icmpinfo = matchinfo;
+	int hdroff;
+	u8 nexthdr = skb->nh.ipv6h->nexthdr;
 
-	if (offset == 0 && datalen < 2) {
+	/* Must not be a fragment. */
+	if (offset)
+		return 0;
+
+	hdroff = (u8*)(skb->nh.ipv6h+1) - skb->data;
+	hdroff = ipv6_skip_exthdr(skb, hdroff, &nexthdr, skb->len - hdroff);
+	if (hdroff < 0 || hdroff > skb->len || nexthdr != IPPROTO_ICMPV6) {
+		*hotdrop = 1;
+		return 0;
+	}
+
+	if (skb_copy_bits(skb, hdroff, &icmph, sizeof(icmph)) < 0) {
 		/* We've been asked to examine this packet, and we
 		   can't.  Hence, no choice but to drop. */
 		duprintf("Dropping evil ICMP tinygram.\n");

@@ -1763,11 +1776,10 @@
 	}
 
 	/* Must not be a fragment. */
-	return !offset
-		&& icmp6_type_code_match(icmpinfo->type,
+	return icmp6_type_code_match(icmpinfo->type,
 					icmpinfo->code[0],
 					icmpinfo->code[1],
-					icmp->icmp6_type, icmp->icmp6_code,
+					icmph.icmp6_type, icmph.icmp6_code,
 					!!(icmpinfo->invflags&IP6T_ICMP_INV));
 }
Signature · 3 lines
Olaf Kirch     | Things that make Monday morning interesting, #1:
[email protected]   |        "I want to use NFS over AX25, can you help me?"
---------------+
#1 lore raw

In article <[email protected]> (at Thu, 30 Sep 2004 14:16:20 +0200), Olaf Kirch <[email protected]> says:

net/ipv6/netfilter/ip6_tables.c line 1773

Quoted 5 lines
net/ipv6/netfilter/ip6_tables.c +2 -0
+
+	if (skb_copy_bits(skb, hdroff, &icmph, sizeof(icmph)) < 0) {
 		/* We've been asked to examine this packet, and we
 		   can't.  Hence, no choice but to drop. */
 		duprintf("Dropping evil ICMP tinygram.\n");

Please use skb_header_pointer(). Thank you.

Signature · 2 lines
Hideaki YOSHIFUJI @ USAGI Project <[email protected]>
GPG FP: 9022 65EB 1ECF 3AD1 0BDF  80D8 4807 F894 E062 0EEA
#2 lore raw
In reply to Olaf Kirch

Thanks. and maybe current kernel has same problem in ip6t_multiport.c, too. But I already sent a patch which fixes this problem to this ml.

See

https://lists.netfilter.org/pipermail/netfilter-devel/2004-September/016783.html

and

https://lists.netfilter.org/pipermail/netfilter-devel/2004-September/016851.html

Regards,

-----------------------------------------------------------------
Yasuyuki KOZAKAI @ USAGI Project <[email protected]>

From: Olaf Kirch <[email protected]>
Date: Thu, 30 Sep 2004 14:16:20 +0200

Quoted 57 lines

This patch fixes a bug in the ip6_tables code when matching ICMP type and code within ICMPv6 packets. The icmpv6 packet matcher expects the nexthdr to be ICMPv6 and does not deal with hop-by-hop headers etc.

Signed-off-by Olaf Kirch <[email protected]>

Index: linux-2.6.8.nf/net/ipv6/netfilter/ip6_tables.c
===================================================================

net/ipv6/netfilter/ip6_tables.c +17 -5
@@ -1751,10 +1751,23 @@
 	   u_int16_t datalen,
 	   int *hotdrop)
 {
-	const struct icmp6hdr *icmp = hdr;
+	struct icmp6hdr icmph;
 	const struct ip6t_icmp *icmpinfo = matchinfo;
+	int hdroff;
+	u8 nexthdr = skb->nh.ipv6h->nexthdr;
 
-	if (offset == 0 && datalen < 2) {
+	/* Must not be a fragment. */
+	if (offset)
+		return 0;
+
+	hdroff = (u8*)(skb->nh.ipv6h+1) - skb->data;
+	hdroff = ipv6_skip_exthdr(skb, hdroff, &nexthdr, skb->len - hdroff);
+	if (hdroff < 0 || hdroff > skb->len || nexthdr != IPPROTO_ICMPV6) {
+		*hotdrop = 1;
+		return 0;
+	}
+
+	if (skb_copy_bits(skb, hdroff, &icmph, sizeof(icmph)) < 0) {
 		/* We've been asked to examine this packet, and we
 		   can't.  Hence, no choice but to drop. */
 		duprintf("Dropping evil ICMP tinygram.\n");

@@ -1763,11 +1776,10 @@
 	}
 
 	/* Must not be a fragment. */
-	return !offset
-		&& icmp6_type_code_match(icmpinfo->type,
+	return icmp6_type_code_match(icmpinfo->type,
 					icmpinfo->code[0],
 					icmpinfo->code[1],
-					icmp->icmp6_type, icmp->icmp6_code,
+					icmph.icmp6_type, icmph.icmp6_code,
 					!!(icmpinfo->invflags&IP6T_ICMP_INV));
 }
Signature · 3 lines
Olaf Kirch     | Things that make Monday morning interesting, #1:
[email protected]   |        "I want to use NFS over AX25, can you help me?"
---------------+
#3 lore raw

From: Yasuyuki Kozakai <[email protected]>
Date: Thu, 30 Sep 2004 21:39:09 +0900 (JST)

Quoted 2 lines

Thanks. and maybe current kernel has same problem in ip6t_multiport.c, too. But I already sent a patch which fixes this problem to this ml.

Sorry, I sent it to only netfilter-devel ml because I wanted someone to test it more before sending it to netdev ml.

-----------------------------------------------------------------
Yasuyuki KOZAKAI @ USAGI Project <[email protected]>

#4 lore raw
Quoted 5 lines · Yasuyuki Kozakai

From: Yasuyuki Kozakai <[email protected]>
Date: Thu, 30 Sep 2004 21:39:09 +0900 (JST)

Quoted 9 lines
Quoted 4 lines

Thanks. and maybe current kernel has same problem in ip6t_multiport.c, too. But I already sent a patch which fixes this problem to this ml.

Sorry, I sent it to only netfilter-devel ml because I wanted someone to test it more before sending it to netdev ml.

I've reviewed the patch, I think we can push it soon. Did you do any changes besides the u_int8_t fix since the last patch you sent ?

Regards
Patrick

#5 lore raw

Hi,

From: Patrick McHardy <[email protected]>
Date: Thu, 30 Sep 2004 16:48:41 +0200

Quoted 8 lines
Quoted 4 lines

Sorry, I sent it to only netfilter-devel ml because I wanted someone to test it more before sending it to netdev ml.

I've reviewed the patch, I think we can push it soon. Did you do any changes besides the u_int8_t fix since the last patch you sent ?

No.

Regards,

-----------------------------------------------------------------
Yasuyuki KOZAKAI @ USAGI Project <[email protected]>

#6 lore raw

Hi,

From: Yasuyuki Kozakai <[email protected]>
Date: Fri, 01 Oct 2004 09:19:46 +0900 (JST)

Quoted 11 lines

From: Patrick McHardy <[email protected]>
Date: Thu, 30 Sep 2004 16:48:41 +0200

Quoted 8 lines
Quoted 4 lines

Sorry, I sent it to only netfilter-devel ml because I wanted someone to test it more before sending it to netdev ml.

I've reviewed the patch, I think we can push it soon. Did you do any changes besides the u_int8_t fix since the last patch you sent ?

You mean that "I" should send the fixed patch to netdev ? OK, David, here you are.

# Sometimes I confuse I should send patches to netdev, or send them to

# netfilter-devel and core team of netfilter review/send it to netdev.

<Summary>
This patch is the preparation before removing skb_linearize() from ip6_tables.c. The codes parsing headers are changed to use skb_header_pointer().

To do this, I also changed the arguments of match functions. The match functions get the offset to layer 4 protocol header instead of the pointer to it. The offset is calculated at ip6_packet_match().

In the result, this patch fixes the bug which assumes layer 4 protocol header is next to IPv6 header.

Moreover, the arguments order of target functions are changed likely IPv4 to occur compiling error when a user try this patch and old target modules.

This patch doesn't remove skb_linearize() yet. It will be done after all match/target functions are changed to use skb_header_pointer().

Signed-off-by Yasuyuki KOZAKAI <[email protected]>

Regards,
-----------------------------------------------------------------
Yasuyuki KOZAKAI @ USAGI Project <[email protected]>

include/linux/netfilter_ipv6/ip6_tables.h +8 -4
diff -Nur linux-2.6.9-rc3/include/linux/netfilter_ipv6/ip6_tables.h linux-2.6.9-rc3-nolinearize/include/linux/netfilter_ipv6/ip6_tables.h
@@ -355,13 +355,15 @@
 
 	/* Return true or false: return FALSE and set *hotdrop = 1 to
            force immediate packet drop. */
+	/* Arguments changed since 2.6.9, as this must now handle
+	   non-linear skb, using skb_header_pointer and
+	   skb_ip_make_writable. */
 	int (*match)(const struct sk_buff *skb,
 		     const struct net_device *in,
 		     const struct net_device *out,
 		     const void *matchinfo,
 		     int offset,
-		     const void *hdr,
-		     u_int16_t datalen,
+		     unsigned int protoff,
 		     int *hotdrop);
 
 	/* Called when user tries to insert an entry of this type. */

@@ -386,11 +388,13 @@
 
 	const char name[IP6T_FUNCTION_MAXNAMELEN];
 
-	/* Returns verdict. */
+	/* Returns verdict. Argument order changed since 2.6.9, as this
+	   must now handle non-linear skbs, using skb_copy_bits and
+	   skb_ip_make_writable. */
 	unsigned int (*target)(struct sk_buff **pskb,
-			       unsigned int hooknum,
 			       const struct net_device *in,
 			       const struct net_device *out,
+			       unsigned int hooknum,
 			       const void *targinfo,
 			       void *userdata);
net/ipv6/netfilter/ip6_tables.c +122 -132

416 lines not on this page · raw message

net/ipv6/netfilter/ip6t_LOG.c +1 -1
diff -Nur linux-2.6.9-rc3/net/ipv6/netfilter/ip6t_LOG.c linux-2.6.9-rc3-nolinearize/net/ipv6/netfilter/ip6t_LOG.c
@@ -335,9 +335,9 @@
 
 static unsigned int
 ip6t_log_target(struct sk_buff **pskb,
-		unsigned int hooknum,
 		const struct net_device *in,
 		const struct net_device *out,
+		unsigned int hooknum,
 		const void *targinfo,
 		void *userinfo)
 {
net/ipv6/netfilter/ip6t_MARK.c +1 -1
diff -Nur linux-2.6.9-rc3/net/ipv6/netfilter/ip6t_MARK.c linux-2.6.9-rc3-nolinearize/net/ipv6/netfilter/ip6t_MARK.c
@@ -20,9 +20,9 @@
 
 static unsigned int
 target(struct sk_buff **pskb,
-       unsigned int hooknum,
        const struct net_device *in,
        const struct net_device *out,
+       unsigned int hooknum,
        const void *targinfo,
        void *userinfo)
 {
net/ipv6/netfilter/ip6t_ah.c +1 -2
diff -Nur linux-2.6.9-rc3/net/ipv6/netfilter/ip6t_ah.c linux-2.6.9-rc3-nolinearize/net/ipv6/netfilter/ip6t_ah.c
@@ -45,8 +45,7 @@
       const struct net_device *out,
       const void *matchinfo,
       int offset,
-      const void *protohdr,
-      u_int16_t datalen,
+      unsigned int protoff,
       int *hotdrop)
 {
        struct ip_auth_hdr *ah = NULL;
net/ipv6/netfilter/ip6t_dst.c +1 -2
diff -Nur linux-2.6.9-rc3/net/ipv6/netfilter/ip6t_dst.c linux-2.6.9-rc3-nolinearize/net/ipv6/netfilter/ip6t_dst.c
@@ -60,8 +60,7 @@
       const struct net_device *out,
       const void *matchinfo,
       int offset,
-      const void *protohdr,
-      u_int16_t datalen,
+      unsigned int protoff,
       int *hotdrop)
 {
        struct ipv6_opt_hdr *optsh = NULL;
net/ipv6/netfilter/ip6t_esp.c +1 -2
diff -Nur linux-2.6.9-rc3/net/ipv6/netfilter/ip6t_esp.c linux-2.6.9-rc3-nolinearize/net/ipv6/netfilter/ip6t_esp.c
@@ -45,8 +45,7 @@
       const struct net_device *out,
       const void *matchinfo,
       int offset,
-      const void *protohdr,
-      u_int16_t datalen,
+      unsigned int protoff,
       int *hotdrop)
 {
 	struct ip_esp_hdr *esp = NULL;
net/ipv6/netfilter/ip6t_eui64.c +1 -2
diff -Nur linux-2.6.9-rc3/net/ipv6/netfilter/ip6t_eui64.c linux-2.6.9-rc3-nolinearize/net/ipv6/netfilter/ip6t_eui64.c
@@ -24,8 +24,7 @@
       const struct net_device *out,
       const void *matchinfo,
       int offset,
-      const void *hdr,
-      u_int16_t datalen,
+      unsigned int protoff,
       int *hotdrop)
 {
net/ipv6/netfilter/ip6t_frag.c +1 -2
diff -Nur linux-2.6.9-rc3/net/ipv6/netfilter/ip6t_frag.c linux-2.6.9-rc3-nolinearize/net/ipv6/netfilter/ip6t_frag.c
@@ -70,8 +70,7 @@
       const struct net_device *out,
       const void *matchinfo,
       int offset,
-      const void *protohdr,
-      u_int16_t datalen,
+      unsigned int protoff,
       int *hotdrop)
 {
        struct fraghdr *frag = NULL;
net/ipv6/netfilter/ip6t_hbh.c +1 -2
diff -Nur linux-2.6.9-rc3/net/ipv6/netfilter/ip6t_hbh.c linux-2.6.9-rc3-nolinearize/net/ipv6/netfilter/ip6t_hbh.c
@@ -59,8 +59,7 @@
       const struct net_device *out,
       const void *matchinfo,
       int offset,
-      const void *protohdr,
-      u_int16_t datalen,
+      unsigned int protoff,
       int *hotdrop)
 {
        struct ipv6_opt_hdr *optsh = NULL;
net/ipv6/netfilter/ip6t_hl.c +1 -1
diff -Nur linux-2.6.9-rc3/net/ipv6/netfilter/ip6t_hl.c linux-2.6.9-rc3-nolinearize/net/ipv6/netfilter/ip6t_hl.c
@@ -20,7 +20,7 @@
 
 static int match(const struct sk_buff *skb, const struct net_device *in,
 		 const struct net_device *out, const void *matchinfo,
-		 int offset, const void *hdr, u_int16_t datalen,
+		 int offset, unsigned int protoff,
 		 int *hotdrop)
 {
 	const struct ip6t_hl_info *info = matchinfo;
net/ipv6/netfilter/ip6t_ipv6header.c +1 -2
diff -Nur linux-2.6.9-rc3/net/ipv6/netfilter/ip6t_ipv6header.c linux-2.6.9-rc3-nolinearize/net/ipv6/netfilter/ip6t_ipv6header.c
@@ -31,8 +31,7 @@
 		 const struct net_device *out,
 		 const void *matchinfo,
 		 int offset,
-		 const void *protohdr,
-		 u_int16_t datalen,
+		 unsigned int protoff,
 		 int *hotdrop)
 {
 	const struct ip6t_ipv6header_info *info = matchinfo;
net/ipv6/netfilter/ip6t_length.c +1 -2
diff -Nur linux-2.6.9-rc3/net/ipv6/netfilter/ip6t_length.c linux-2.6.9-rc3-nolinearize/net/ipv6/netfilter/ip6t_length.c
@@ -23,8 +23,7 @@
       const struct net_device *out,
       const void *matchinfo,
       int offset,
-      const void *hdr,
-      u_int16_t datalen,
+      unsigned int protoff,
       int *hotdrop)
 {
 	const struct ip6t_length_info *info = matchinfo;
net/ipv6/netfilter/ip6t_limit.c +1 -2
diff -Nur linux-2.6.9-rc3/net/ipv6/netfilter/ip6t_limit.c linux-2.6.9-rc3-nolinearize/net/ipv6/netfilter/ip6t_limit.c
@@ -57,8 +57,7 @@
 		const struct net_device *out,
 		const void *matchinfo,
 		int offset,
-		const void *hdr,
-		u_int16_t datalen,
+		unsigned int protoff,
 		int *hotdrop)
 {
 	struct ip6t_rateinfo *r = ((struct ip6t_rateinfo *)matchinfo)->master;
net/ipv6/netfilter/ip6t_mac.c +1 -2
diff -Nur linux-2.6.9-rc3/net/ipv6/netfilter/ip6t_mac.c linux-2.6.9-rc3-nolinearize/net/ipv6/netfilter/ip6t_mac.c
@@ -25,8 +25,7 @@
       const struct net_device *out,
       const void *matchinfo,
       int offset,
-      const void *hdr,
-      u_int16_t datalen,
+      unsigned int protoff,
       int *hotdrop)
 {
     const struct ip6t_mac_info *info = matchinfo;
net/ipv6/netfilter/ip6t_mark.c +1 -2
diff -Nur linux-2.6.9-rc3/net/ipv6/netfilter/ip6t_mark.c linux-2.6.9-rc3-nolinearize/net/ipv6/netfilter/ip6t_mark.c
@@ -24,8 +24,7 @@
       const struct net_device *out,
       const void *matchinfo,
       int offset,
-      const void *hdr,
-      u_int16_t datalen,
+      unsigned int protoff,
       int *hotdrop)
 {
 	const struct ip6t_mark_info *info = matchinfo;
net/ipv6/netfilter/ip6t_multiport.c +3 -4
diff -Nur linux-2.6.9-rc3/net/ipv6/netfilter/ip6t_multiport.c linux-2.6.9-rc3-nolinearize/net/ipv6/netfilter/ip6t_multiport.c
@@ -53,15 +53,14 @@
       const struct net_device *out,
       const void *matchinfo,
       int offset,
-      const void *hdr,
-      u_int16_t datalen,
+      unsigned int protoff,
       int *hotdrop)
 {
-	const struct udphdr *udp = hdr;
+	const struct udphdr *udp = (const struct udphdr *)(skb->data + protoff);
 	const struct ip6t_multiport *multiinfo = matchinfo;
 
 	/* Must be big enough to read ports. */
-	if (offset == 0 && datalen < sizeof(struct udphdr)) {
+	if (offset == 0 && skb->len - protoff < sizeof(struct udphdr)) {
 		/* We've been asked to examine this packet, and we
 		   can't.  Hence, no choice but to drop. */
 			duprintf("ip6t_multiport:"
net/ipv6/netfilter/ip6t_owner.c +1 -2
diff -Nur linux-2.6.9-rc3/net/ipv6/netfilter/ip6t_owner.c linux-2.6.9-rc3-nolinearize/net/ipv6/netfilter/ip6t_owner.c
@@ -92,8 +92,7 @@
       const struct net_device *out,
       const void *matchinfo,
       int offset,
-      const void *hdr,
-      u_int16_t datalen,
+      unsigned int protoff,
       int *hotdrop)
 {
 	const struct ip6t_owner_info *info = matchinfo;
net/ipv6/netfilter/ip6t_rt.c +1 -2
diff -Nur linux-2.6.9-rc3/net/ipv6/netfilter/ip6t_rt.c linux-2.6.9-rc3-nolinearize/net/ipv6/netfilter/ip6t_rt.c
@@ -47,8 +47,7 @@
       const struct net_device *out,
       const void *matchinfo,
       int offset,
-      const void *protohdr,
-      u_int16_t datalen,
+      unsigned int protoff,
       int *hotdrop)
 {
        struct ipv6_rt_hdr *route = NULL;
#7 lore raw

When to submit to which list (was Re: [PATCH] netfilter6: Skip extension headers when matching icmp6-type)

Quoted 2 lines · Yasuyuki Kozakai

# Sometimes I confuse I should send patches to netdev, or send them to

# netfilter-devel and core team of netfilter review/send it to netdev.

I don't know what Patrick told you, but I think as a general rule of thumb, all netfilter-related patches should go to netfilter-devel first. Patrick or I will then push them upstream to DaveM (most times with Cc to netdev).

For urgent/critical bugfixes (that are not too complex), I am ok if netfilter-devel is bypassed and you submit it to netdev/davem immediately.

Pragmatically speaking, Patrick or me will read your emails on either list - but a number of other netfilter developers is not following netdev, so you deprive them of the chance to give comments before it is submitted ;)

Thanks!

Signature · 5 lines
- Harald Welte <[email protected]>             http://www.netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie
#8 lore raw

Re: When to submit to which list

OK, I'll send patches to netfilter-devel as ever.

Thanks,

-----------------------------------------------------------------
Yasuyuki KOZAKAI @ USAGI Project <[email protected]>

From: Harald Welte <[email protected]>
Date: Fri, 1 Oct 2004 11:02:21 +0200

Quoted 26 lines · Yasuyuki Kozakai
Quoted 2 lines

# Sometimes I confuse I should send patches to netdev, or send them to

# netfilter-devel and core team of netfilter review/send it to netdev.

I don't know what Patrick told you, but I think as a general rule of thumb, all netfilter-related patches should go to netfilter-devel first. Patrick or I will then push them upstream to DaveM (most times with Cc to netdev).

For urgent/critical bugfixes (that are not too complex), I am ok if netfilter-devel is bypassed and you submit it to netdev/davem immediately.

Pragmatically speaking, Patrick or me will read your emails on either list - but a number of other netfilter developers is not following netdev, so you deprive them of the chance to give comments before it is submitted ;)

Thanks!

Signature · 5 lines
- Harald Welte <[email protected]>             http://www.netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie