[PATCH] modification in current protocol helper API to handle error/unclean packets

[PATCH] modification in current protocol helper API to handle error/unclean packets

[Pablo Neira]

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

Hi Patrick and everyone,

I added a new function to the current protocol helper API. It is called 
before resolve_normal_ct, this could let us perform actions before 
starting a new session/conntrack.

These actions could be:
- check for malformed/unclean packets.
- handle special packets like in ICMP.

This way, Jozsef could call unclean() here in his tcp-window-tracking 
patch and we won't need to add protocol specific function to the core of 
the conntrack system like icmp_error_track to handle special ICMP 
messages. We could also check for unclean udp and icmp packets.

This function returns 0 if no error was found or an action (NF_*) if an 
error/special action needs to be done to handle a message.

Three issues to consider:

- I copied and pasted icmp_error_track to ip_fw_compat.c since this 
function is used there, I know that this implies replicating code but 
since that backward compability will be drop someday, I consider that 
it's not so serious.
- I marked ip_ct_find_proto, ip_conntrack_find_get, invert_tuple and 
get_tuple as extern in ip_conntrack_protocol.h for icmp. I think that we 
could need them later to track protocols like AH/ESP which are 
encapsulated, in that case we could call ip_ct_find_proto and the the 
protocol helper functions for the protocol encapsulated. If this is the 
correct way to do this, we will also need to review again the API if i'm 
not wrong (add something like an offset to the header of the 
encapsulated protocol).
- oh! I added udp checksum checking, well, I just stole that from 
Joszef's tcp tracking code and rename things... I also read that email 
that Martin sent to report the bug with checksum checking. So if you 
consider that this patch is ok, I'll implement correctly checking for 
malformed packets for udp and icmp :-) (in that case I will try again to 
steal as much Joszef's code as possible :-))

Hope that I didn't break/miss anything, if so, please let me know.

best regards,
Pablo

[-- Attachment #2: protocol_helper.patch --]
[-- Type: text/x-patch, Size: 14156 bytes --]

diff -u -r1.1.1.1 ip_conntrack_core.c
--- a/net/ipv4/netfilter/ip_conntrack_core.c	11 May 2004 13:07:08 -0000	1.1.1.1
+++ b/net/ipv4/netfilter/ip_conntrack_core.c	13 Jun 2004 22:47:49 -0000
@@ -146,7 +146,7 @@
 	return protocol->pkt_to_tuple(skb, dataoff, tuple);
 }
 
-static int
+int
 invert_tuple(struct ip_conntrack_tuple *inverse,
 	     const struct ip_conntrack_tuple *orig,
 	     const struct ip_conntrack_protocol *protocol)
@@ -495,83 +495,6 @@
 	return h != NULL;
 }
 
-/* Returns conntrack if it dealt with ICMP, and filled in skb fields */
-struct ip_conntrack *
-icmp_error_track(struct sk_buff *skb,
-		 enum ip_conntrack_info *ctinfo,
-		 unsigned int hooknum)
-{
-	struct ip_conntrack_tuple innertuple, origtuple;
-	struct {
-		struct icmphdr icmp;
-		struct iphdr ip;
-	} inside;
-	struct ip_conntrack_protocol *innerproto;
-	struct ip_conntrack_tuple_hash *h;
-	int dataoff;
-
-	IP_NF_ASSERT(skb->nfct == NULL);
-
-	/* Not enough header? */
-	if (skb_copy_bits(skb, skb->nh.iph->ihl*4, &inside, sizeof(inside))!=0)
-		return NULL;
-
-	if (inside.icmp.type != ICMP_DEST_UNREACH
-	    && inside.icmp.type != ICMP_SOURCE_QUENCH
-	    && inside.icmp.type != ICMP_TIME_EXCEEDED
-	    && inside.icmp.type != ICMP_PARAMETERPROB
-	    && inside.icmp.type != ICMP_REDIRECT)
-		return NULL;
-
-	/* Ignore ICMP's containing fragments (shouldn't happen) */
-	if (inside.ip.frag_off & htons(IP_OFFSET)) {
-		DEBUGP("icmp_error_track: fragment of proto %u\n",
-		       inside.ip.protocol);
-		return NULL;
-	}
-
-	innerproto = ip_ct_find_proto(inside.ip.protocol);
-	dataoff = skb->nh.iph->ihl*4 + sizeof(inside.icmp) + inside.ip.ihl*4;
-	/* Are they talking about one of our connections? */
-	if (!get_tuple(&inside.ip, skb, dataoff, &origtuple, innerproto)) {
-		DEBUGP("icmp_error: ! get_tuple p=%u", inside.ip.protocol);
-		return NULL;
-	}
-
-	/* Ordinarily, we'd expect the inverted tupleproto, but it's
-	   been preserved inside the ICMP. */
-	if (!invert_tuple(&innertuple, &origtuple, innerproto)) {
-		DEBUGP("icmp_error_track: Can't invert tuple\n");
-		return NULL;
-	}
-
-	*ctinfo = IP_CT_RELATED;
-
-	h = ip_conntrack_find_get(&innertuple, NULL);
-	if (!h) {
-		/* Locally generated ICMPs will match inverted if they
-		   haven't been SNAT'ed yet */
-		/* FIXME: NAT code has to handle half-done double NAT --RR */
-		if (hooknum == NF_IP_LOCAL_OUT)
-			h = ip_conntrack_find_get(&origtuple, NULL);
-
-		if (!h) {
-			DEBUGP("icmp_error_track: no match\n");
-			return NULL;
-		}
-		/* Reverse direction from that found */
-		if (DIRECTION(h) != IP_CT_DIR_REPLY)
-			*ctinfo += IP_CT_IS_REPLY;
-	} else {
-		if (DIRECTION(h) == IP_CT_DIR_REPLY)
-			*ctinfo += IP_CT_IS_REPLY;
-	}
-
-	/* Update skb to refer to this connection */
-	skb->nfct = &h->ctrack->infos[*ctinfo];
-	return h->ctrack;
-}
-
 /* There's a small race here where we may free a just-assured
    connection.  Too bad: we're in trouble anyway. */
 static inline int unreplied(const struct ip_conntrack_tuple_hash *i)
@@ -828,10 +751,9 @@
 
 	proto = ip_ct_find_proto((*pskb)->nh.iph->protocol);
 
-	/* It may be an icmp error... */
-	if ((*pskb)->nh.iph->protocol == IPPROTO_ICMP 
-	    && icmp_error_track(*pskb, &ctinfo, hooknum))
-		return NF_ACCEPT;
+	/* It may be an special packet, error, unclean... */
+	if ((ret = proto->error(*pskb, &ctinfo, hooknum)))
+		return ret;
 
 	if (!(ct = resolve_normal_ct(*pskb, proto,&set_reply,hooknum,&ctinfo)))
 		/* Not valid part of a connection */
diff -u -r1.1.1.1 ip_conntrack_proto_generic.c
--- a/net/ipv4/netfilter/ip_conntrack_proto_generic.c	11 May 2004 13:07:08 -0000	1.1.1.1
+++ b/net/ipv4/netfilter/ip_conntrack_proto_generic.c	13 Jun 2004 22:54:47 -0000
@@ -62,8 +62,15 @@
 	return 1;
 }
 
+static int
+error(struct sk_buff *skb, enum ip_conntrack_info *ctinfo, 
+      unsigned int hooknum)
+{
+	return 0;
+}
+
 struct ip_conntrack_protocol ip_conntrack_generic_protocol
 = { { NULL, NULL }, 0, "unknown",
     generic_pkt_to_tuple, generic_invert_tuple, generic_print_tuple,
-    generic_print_conntrack, packet, new, NULL, NULL, NULL };
+    generic_print_conntrack, packet, new, NULL, NULL, error, NULL };
 
diff -u -r1.1.1.1 ip_conntrack_proto_icmp.c
--- a/net/ipv4/netfilter/ip_conntrack_proto_icmp.c	11 May 2004 13:07:08 -0000	1.1.1.1
+++ b/net/ipv4/netfilter/ip_conntrack_proto_icmp.c	13 Jun 2004 22:51:49 -0000
@@ -12,6 +12,8 @@
 #include <linux/netfilter.h>
 #include <linux/in.h>
 #include <linux/icmp.h>
+#include <net/ip.h>
+#include <linux/netfilter_ipv4/ip_conntrack.h>
 #include <linux/netfilter_ipv4/ip_conntrack_protocol.h>
 
 unsigned long ip_ct_icmp_timeout = 30*HZ;
@@ -122,7 +124,83 @@
 	return 1;
 }
 
+static int
+icmp_error(struct sk_buff *skb,
+		 enum ip_conntrack_info *ctinfo,
+		 unsigned int hooknum)
+{
+	struct ip_conntrack_tuple innertuple, origtuple;
+	struct {
+		struct icmphdr icmp;
+		struct iphdr ip;
+	} inside;
+	struct ip_conntrack_protocol *innerproto;
+	struct ip_conntrack_tuple_hash *h;
+	int dataoff;
+
+	IP_NF_ASSERT(skb->nfct == NULL);
+
+	/* Not enough header? */
+	if (skb_copy_bits(skb, skb->nh.iph->ihl*4, &inside, sizeof(inside))!=0)
+		return NF_ACCEPT;
+
+	if (inside.icmp.type != ICMP_DEST_UNREACH
+	    && inside.icmp.type != ICMP_SOURCE_QUENCH
+	    && inside.icmp.type != ICMP_TIME_EXCEEDED
+	    && inside.icmp.type != ICMP_PARAMETERPROB
+	    && inside.icmp.type != ICMP_REDIRECT)
+		return 0;
+
+	/* Ignore ICMP's containing fragments (shouldn't happen) */
+	if (inside.ip.frag_off & htons(IP_OFFSET)) {
+		DEBUGP("icmp_error_track: fragment of proto %u\n",
+		       inside.ip.protocol);
+		return NF_ACCEPT;
+	}
+
+	innerproto = ip_ct_find_proto(inside.ip.protocol);
+	dataoff = skb->nh.iph->ihl*4 + sizeof(inside.icmp) + inside.ip.ihl*4;
+	/* Are they talking about one of our connections? */
+	if (!get_tuple(&inside.ip, skb, dataoff, &origtuple, innerproto)) {
+		DEBUGP("icmp_error: ! get_tuple p=%u", inside.ip.protocol);
+		return NF_ACCEPT;
+	}
+
+	/* Ordinarily, we'd expect the inverted tupleproto, but it's
+	   been preserved inside the ICMP. */
+	if (!invert_tuple(&innertuple, &origtuple, innerproto)) {
+		DEBUGP("icmp_error_track: Can't invert tuple\n");
+		return NF_ACCEPT;
+	}
+
+	*ctinfo = IP_CT_RELATED;
+
+	h = ip_conntrack_find_get(&innertuple, NULL);
+	if (!h) {
+		/* Locally generated ICMPs will match inverted if they
+		   haven't been SNAT'ed yet */
+		/* FIXME: NAT code has to handle half-done double NAT --RR */
+		if (hooknum == NF_IP_LOCAL_OUT)
+			h = ip_conntrack_find_get(&origtuple, NULL);
+
+		if (!h) {
+			DEBUGP("icmp_error_track: no match\n");
+			return 0;
+		}
+		/* Reverse direction from that found */
+		if (DIRECTION(h) != IP_CT_DIR_REPLY)
+			*ctinfo += IP_CT_IS_REPLY;
+	} else {
+		if (DIRECTION(h) == IP_CT_DIR_REPLY)
+			*ctinfo += IP_CT_IS_REPLY;
+	}
+
+	/* Update skb to refer to this connection */
+	skb->nfct = &h->ctrack->infos[*ctinfo];
+	return NF_ACCEPT;
+}
+
 struct ip_conntrack_protocol ip_conntrack_protocol_icmp
 = { { NULL, NULL }, IPPROTO_ICMP, "icmp",
     icmp_pkt_to_tuple, icmp_invert_tuple, icmp_print_tuple,
-    icmp_print_conntrack, icmp_packet, icmp_new, NULL, NULL, NULL };
+    icmp_print_conntrack, icmp_packet, icmp_new, NULL, NULL, icmp_error, NULL};
diff -u -r1.1.1.1 ip_conntrack_proto_tcp.c
--- a/net/ipv4/netfilter/ip_conntrack_proto_tcp.c	11 May 2004 13:07:08 -0000	1.1.1.1
+++ b/net/ipv4/netfilter/ip_conntrack_proto_tcp.c	13 Jun 2004 22:53:23 -0000
@@ -266,7 +266,15 @@
 	return between(exp->seq, ntohl(tcph.seq), ntohl(tcph.seq) + datalen);
 }
 
+static int
+tcp_error(struct sk_buff *skb,
+	   enum ip_conntrack_info *ctinfo,
+	   unsigned int hooknum)
+{
+	return 0;
+}
+
 struct ip_conntrack_protocol ip_conntrack_protocol_tcp
 = { { NULL, NULL }, IPPROTO_TCP, "tcp",
     tcp_pkt_to_tuple, tcp_invert_tuple, tcp_print_tuple, tcp_print_conntrack,
-    tcp_packet, tcp_new, NULL, tcp_exp_matches_pkt, NULL };
+    tcp_packet, tcp_new, NULL, tcp_exp_matches_pkt, tcp_error, NULL };
diff -u -r1.1.1.1 ip_conntrack_proto_udp.c
--- a/net/ipv4/netfilter/ip_conntrack_proto_udp.c	11 May 2004 13:07:08 -0000	1.1.1.1
+++ b/net/ipv4/netfilter/ip_conntrack_proto_udp.c	13 Jun 2004 23:17:51 -0000
@@ -12,8 +12,16 @@
 #include <linux/netfilter.h>
 #include <linux/in.h>
 #include <linux/udp.h>
+#include <asm/checksum.h>
 #include <linux/netfilter_ipv4/ip_conntrack_protocol.h>
 
+#if 0
+#define NET_RATELIMIT(foo)     (foo)
+#else
+#define NET_RATELIMIT(foo)     ((foo) && net_ratelimit())
+#endif
+
+int ip_ct_udp_log_invalid = 1;
 unsigned long ip_ct_udp_timeout = 30*HZ;
 unsigned long ip_ct_udp_timeout_stream = 180*HZ;
 
@@ -80,7 +88,31 @@
 	return 1;
 }
 
+static int
+udp_error(struct sk_buff *skb, enum ip_conntrack_info *ctinfo,
+	  unsigned int hooknum)
+{
+	struct iphdr *iph = skb->nh.iph;
+	unsigned int udplen = skb->len - iph->ihl * 4;
+
+	/* Checksum invalid?  Ignore. */
+	/* FIXME: Source route IP option packets --RR */
+	if (csum_tcpudp_magic(iph->saddr, iph->daddr,
+			      udplen, IPPROTO_UDP,
+			      skb->ip_summed == CHECKSUM_HW 
+			      	? skb->csum
+			      	: skb_checksum(skb, iph->ihl*4, udplen, 0))) {
+		if (NET_RATELIMIT(ip_ct_udp_log_invalid))
+			nf_log_packet(PF_INET, 0, skb, NULL, NULL, 
+				  "ip_conntrack_udp: INVALID: "
+				  "bad UDP checksum ");
+		return NF_ACCEPT;
+	}
+
+	return 0;
+}
+
 struct ip_conntrack_protocol ip_conntrack_protocol_udp
 = { { NULL, NULL }, IPPROTO_UDP, "udp",
     udp_pkt_to_tuple, udp_invert_tuple, udp_print_tuple, udp_print_conntrack,
-    udp_packet, udp_new, NULL, NULL, NULL };
+    udp_packet, udp_new, NULL, NULL, udp_error, NULL };
diff -u -r1.1.1.1 ip_fw_compat_masq.c
--- a/net/ipv4/netfilter/ip_fw_compat_masq.c	11 May 2004 13:07:08 -0000	1.1.1.1
+++ b/net/ipv4/netfilter/ip_fw_compat_masq.c	13 Jun 2004 23:25:21 -0000
@@ -24,6 +24,7 @@
 #include <linux/proc_fs.h>
 #include <linux/module.h>
 #include <net/route.h>
+#include <net/ip.h>
 
 #define ASSERT_READ_LOCK(x) MUST_BE_READ_LOCKED(&ip_conntrack_lock)
 #define ASSERT_WRITE_LOCK(x) MUST_BE_WRITE_LOCKED(&ip_conntrack_lock)
@@ -41,6 +42,83 @@
 #define DEBUGP(format, args...)
 #endif
 
+/* Returns conntrack if it dealt with ICMP, and filled in skb fields */
+struct ip_conntrack *
+icmp_error_track(struct sk_buff *skb,
+		 enum ip_conntrack_info *ctinfo,
+		 unsigned int hooknum)
+{
+	struct ip_conntrack_tuple innertuple, origtuple;
+	struct {
+		struct icmphdr icmp;
+		struct iphdr ip;
+	} inside;
+	struct ip_conntrack_protocol *innerproto;
+	struct ip_conntrack_tuple_hash *h;
+	int dataoff;
+
+	IP_NF_ASSERT(skb->nfct == NULL);
+
+	/* Not enough header? */
+	if (skb_copy_bits(skb, skb->nh.iph->ihl*4, &inside, sizeof(inside))!=0)
+		return NULL;
+
+	if (inside.icmp.type != ICMP_DEST_UNREACH
+	    && inside.icmp.type != ICMP_SOURCE_QUENCH
+	    && inside.icmp.type != ICMP_TIME_EXCEEDED
+	    && inside.icmp.type != ICMP_PARAMETERPROB
+	    && inside.icmp.type != ICMP_REDIRECT)
+		return NULL;
+
+	/* Ignore ICMP's containing fragments (shouldn't happen) */
+	if (inside.ip.frag_off & htons(IP_OFFSET)) {
+		DEBUGP("icmp_error_track: fragment of proto %u\n",
+		       inside.ip.protocol);
+		return NULL;
+	}
+
+	innerproto = ip_ct_find_proto(inside.ip.protocol);
+	dataoff = skb->nh.iph->ihl*4 + sizeof(inside.icmp) + inside.ip.ihl*4;
+	/* Are they talking about one of our connections? */
+	if (!get_tuple(&inside.ip, skb, dataoff, &origtuple, innerproto)) {
+		DEBUGP("icmp_error: ! get_tuple p=%u", inside.ip.protocol);
+		return NULL;
+	}
+
+	/* Ordinarily, we'd expect the inverted tupleproto, but it's
+	   been preserved inside the ICMP. */
+	if (!invert_tuple(&innertuple, &origtuple, innerproto)) {
+		DEBUGP("icmp_error_track: Can't invert tuple\n");
+		return NULL;
+	}
+
+	*ctinfo = IP_CT_RELATED;
+
+	h = ip_conntrack_find_get(&innertuple, NULL);
+	if (!h) {
+		/* Locally generated ICMPs will match inverted if they
+		   haven't been SNAT'ed yet */
+		/* FIXME: NAT code has to handle half-done double NAT --RR */
+		if (hooknum == NF_IP_LOCAL_OUT)
+			h = ip_conntrack_find_get(&origtuple, NULL);
+
+		if (!h) {
+			DEBUGP("icmp_error_track: no match\n");
+			return NULL;
+		}
+		/* Reverse direction from that found */
+		if (DIRECTION(h) != IP_CT_DIR_REPLY)
+			*ctinfo += IP_CT_IS_REPLY;
+	} else {
+		if (DIRECTION(h) == IP_CT_DIR_REPLY)
+			*ctinfo += IP_CT_IS_REPLY;
+	}
+
+	/* Update skb to refer to this connection */
+	skb->nfct = &h->ctrack->infos[*ctinfo];
+	return h->ctrack;
+}
+
 unsigned int
 do_masquerade(struct sk_buff **pskb, const struct net_device *dev)
 {
diff -u -r1.1.1.1 ip_conntrack.h
--- a/include/linux/netfilter_ipv4/ip_conntrack.h	11 May 2004 13:35:40 -0000	1.1.1.1
+++ b/include/linux/netfilter_ipv4/ip_conntrack.h	13 Jun 2004 09:23:03 -0000
@@ -264,6 +264,26 @@
 ip_ct_selective_cleanup(int (*kill)(const struct ip_conntrack *i, void *data),
 			void *data);
 
+extern struct ip_conntrack_protocol *
+ip_ct_find_proto(u_int8_t protocol);
+
+extern int
+get_tuple(const struct iphdr *iph,
+	  const struct sk_buff *skb,
+	  unsigned int dataoff,
+	  struct ip_conntrack_tuple *tuple,
+	  const struct ip_conntrack_protocol *protocol);
+
+extern int
+invert_tuple(struct ip_conntrack_tuple *inverse,
+	     const struct ip_conntrack_tuple *orig,
+	     const struct ip_conntrack_protocol *protocol);
+
+/* Find a connection corresponding to a tuple. */
+extern struct ip_conntrack_tuple_hash *
+ip_conntrack_find_get(const struct ip_conntrack_tuple *tuple,
+		      const struct ip_conntrack *ignored_conntrack);
+
 /* It's confirmed if it is, or has been in the hash table. */
 static inline int is_confirmed(struct ip_conntrack *ct)
 {
diff -u -r1.1.1.1 ip_conntrack_protocol.h
--- a/include/linux/netfilter_ipv4/ip_conntrack_protocol.h	11 May 2004 13:35:39 -0000	1.1.1.1
+++ b/include/linux/netfilter_ipv4/ip_conntrack_protocol.h	13 Jun 2004 22:54:02 -0000
@@ -50,6 +50,9 @@
 	int (*exp_matches_pkt)(struct ip_conntrack_expect *exp,
 			       const struct sk_buff *skb);
 
+	int (*error)(struct sk_buff *skb, enum ip_conntrack_info *ctinfo,
+		     unsigned int hooknum);
+
 	/* Module (if any) which this is connected to. */
 	struct module *me;
 };

Re: [PATCH] modification in current protocol helper API to handle error/unclean packets

[Patrick McHardy]

Hi Pablo,

Pablo Neira wrote:
> Hi Patrick and everyone,
> 
> I added a new function to the current protocol helper API. It is called 
> before resolve_normal_ct, this could let us perform actions before 
> starting a new session/conntrack.
> 
> These actions could be:
> - check for malformed/unclean packets.
> - handle special packets like in ICMP.
> 
> This way, Jozsef could call unclean() here in his tcp-window-tracking 
> patch and we won't need to add protocol specific function to the core of 
> the conntrack system like icmp_error_track to handle special ICMP 
> messages. We could also check for unclean udp and icmp packets.

Nice, I like that ;)

> This function returns 0 if no error was found or an action (NF_*) if an 
> error/special action needs to be done to handle a message.
> 
> Three issues to consider:
> 
> - I copied and pasted icmp_error_track to ip_fw_compat.c since this 
> function is used there, I know that this implies replicating code but 
> since that backward compability will be drop someday, I consider that 
> it's not so serious.

Couldn't you use ip_ct_find_proto(IPPROTO_ICMP)->error() instead ?

> - I marked ip_ct_find_proto, ip_conntrack_find_get, invert_tuple and 
> get_tuple as extern in ip_conntrack_protocol.h for icmp. I think that we 
> could need them later to track protocols like AH/ESP which are 
> encapsulated, in that case we could call ip_ct_find_proto and the the 
> protocol helper functions for the protocol encapsulated. If this is the 
> correct way to do this, we will also need to review again the API if i'm 
> not wrong (add something like an offset to the header of the 
> encapsulated protocol).

invert_tuple and find_tuple probably should be prefixed with ip_ct_
then.

> - oh! I added udp checksum checking, well, I just stole that from 
> Joszef's tcp tracking code and rename things... I also read that email 
> that Martin sent to report the bug with checksum checking. So if you 
> consider that this patch is ok, I'll implement correctly checking for 
> malformed packets for udp and icmp :-) (in that case I will try again to 
> steal as much Joszef's code as possible :-))

This should be an extra patch.

> 
> Hope that I didn't break/miss anything, if so, please let me know.

Some more comments below.

Regards
Patrick

> 
> best regards,
> Pablo
> 
> 
> ------------------------------------------------------------------------
> 
> diff -u -r1.1.1.1 ip_conntrack_core.c
> --- a/net/ipv4/netfilter/ip_conntrack_core.c	11 May 2004 13:07:08 -0000	1.1.1.1
> +++ b/net/ipv4/netfilter/ip_conntrack_core.c	13 Jun 2004 22:47:49 -0000
> @@ -146,7 +146,7 @@
>  	return protocol->pkt_to_tuple(skb, dataoff, tuple);
>  }
>  
> -static int
> +int
>  invert_tuple(struct ip_conntrack_tuple *inverse,
>  	     const struct ip_conntrack_tuple *orig,
>  	     const struct ip_conntrack_protocol *protocol)
> @@ -495,83 +495,6 @@
>  	return h != NULL;
>  }
>  
> -/* Returns conntrack if it dealt with ICMP, and filled in skb fields */
> -struct ip_conntrack *
> -icmp_error_track(struct sk_buff *skb,
> -		 enum ip_conntrack_info *ctinfo,
> -		 unsigned int hooknum)
> -{
> -	struct ip_conntrack_tuple innertuple, origtuple;
> -	struct {
> -		struct icmphdr icmp;
> -		struct iphdr ip;
> -	} inside;
> -	struct ip_conntrack_protocol *innerproto;
> -	struct ip_conntrack_tuple_hash *h;
> -	int dataoff;
> -
> -	IP_NF_ASSERT(skb->nfct == NULL);
> -
> -	/* Not enough header? */
> -	if (skb_copy_bits(skb, skb->nh.iph->ihl*4, &inside, sizeof(inside))!=0)
> -		return NULL;
> -
> -	if (inside.icmp.type != ICMP_DEST_UNREACH
> -	    && inside.icmp.type != ICMP_SOURCE_QUENCH
> -	    && inside.icmp.type != ICMP_TIME_EXCEEDED
> -	    && inside.icmp.type != ICMP_PARAMETERPROB
> -	    && inside.icmp.type != ICMP_REDIRECT)
> -		return NULL;
> -
> -	/* Ignore ICMP's containing fragments (shouldn't happen) */
> -	if (inside.ip.frag_off & htons(IP_OFFSET)) {
> -		DEBUGP("icmp_error_track: fragment of proto %u\n",
> -		       inside.ip.protocol);
> -		return NULL;
> -	}
> -
> -	innerproto = ip_ct_find_proto(inside.ip.protocol);
> -	dataoff = skb->nh.iph->ihl*4 + sizeof(inside.icmp) + inside.ip.ihl*4;
> -	/* Are they talking about one of our connections? */
> -	if (!get_tuple(&inside.ip, skb, dataoff, &origtuple, innerproto)) {
> -		DEBUGP("icmp_error: ! get_tuple p=%u", inside.ip.protocol);
> -		return NULL;
> -	}
> -
> -	/* Ordinarily, we'd expect the inverted tupleproto, but it's
> -	   been preserved inside the ICMP. */
> -	if (!invert_tuple(&innertuple, &origtuple, innerproto)) {
> -		DEBUGP("icmp_error_track: Can't invert tuple\n");
> -		return NULL;
> -	}
> -
> -	*ctinfo = IP_CT_RELATED;
> -
> -	h = ip_conntrack_find_get(&innertuple, NULL);
> -	if (!h) {
> -		/* Locally generated ICMPs will match inverted if they
> -		   haven't been SNAT'ed yet */
> -		/* FIXME: NAT code has to handle half-done double NAT --RR */
> -		if (hooknum == NF_IP_LOCAL_OUT)
> -			h = ip_conntrack_find_get(&origtuple, NULL);
> -
> -		if (!h) {
> -			DEBUGP("icmp_error_track: no match\n");
> -			return NULL;
> -		}
> -		/* Reverse direction from that found */
> -		if (DIRECTION(h) != IP_CT_DIR_REPLY)
> -			*ctinfo += IP_CT_IS_REPLY;
> -	} else {
> -		if (DIRECTION(h) == IP_CT_DIR_REPLY)
> -			*ctinfo += IP_CT_IS_REPLY;
> -	}
> -
> -	/* Update skb to refer to this connection */
> -	skb->nfct = &h->ctrack->infos[*ctinfo];
> -	return h->ctrack;
> -}
> -
>  /* There's a small race here where we may free a just-assured
>     connection.  Too bad: we're in trouble anyway. */
>  static inline int unreplied(const struct ip_conntrack_tuple_hash *i)
> @@ -828,10 +751,9 @@
>  
>  	proto = ip_ct_find_proto((*pskb)->nh.iph->protocol);
>  
> -	/* It may be an icmp error... */
> -	if ((*pskb)->nh.iph->protocol == IPPROTO_ICMP 
> -	    && icmp_error_track(*pskb, &ctinfo, hooknum))
> -		return NF_ACCEPT;
> +	/* It may be an special packet, error, unclean... */
> +	if ((ret = proto->error(*pskb, &ctinfo, hooknum)))

should check for != NF_ACCEPT here

> +		return ret;
>  
>  	if (!(ct = resolve_normal_ct(*pskb, proto,&set_reply,hooknum,&ctinfo)))
>  		/* Not valid part of a connection */
> diff -u -r1.1.1.1 ip_conntrack_proto_generic.c
> --- a/net/ipv4/netfilter/ip_conntrack_proto_generic.c	11 May 2004 13:07:08 -0000	1.1.1.1
> +++ b/net/ipv4/netfilter/ip_conntrack_proto_generic.c	13 Jun 2004 22:54:47 -0000
> @@ -62,8 +62,15 @@
>  	return 1;
>  }
>  
> +static int
> +error(struct sk_buff *skb, enum ip_conntrack_info *ctinfo, 
> +      unsigned int hooknum)
> +{
> +	return 0;
> +}
> +
>  struct ip_conntrack_protocol ip_conntrack_generic_protocol
>  = { { NULL, NULL }, 0, "unknown",
>      generic_pkt_to_tuple, generic_invert_tuple, generic_print_tuple,
> -    generic_print_conntrack, packet, new, NULL, NULL, NULL };
> +    generic_print_conntrack, packet, new, NULL, NULL, error, NULL };
>  
> diff -u -r1.1.1.1 ip_conntrack_proto_icmp.c
> --- a/net/ipv4/netfilter/ip_conntrack_proto_icmp.c	11 May 2004 13:07:08 -0000	1.1.1.1
> +++ b/net/ipv4/netfilter/ip_conntrack_proto_icmp.c	13 Jun 2004 22:51:49 -0000
> @@ -12,6 +12,8 @@
>  #include <linux/netfilter.h>
>  #include <linux/in.h>
>  #include <linux/icmp.h>
> +#include <net/ip.h>
> +#include <linux/netfilter_ipv4/ip_conntrack.h>
>  #include <linux/netfilter_ipv4/ip_conntrack_protocol.h>
>  
>  unsigned long ip_ct_icmp_timeout = 30*HZ;
> @@ -122,7 +124,83 @@
>  	return 1;
>  }
>  
> +static int
> +icmp_error(struct sk_buff *skb,
> +		 enum ip_conntrack_info *ctinfo,
> +		 unsigned int hooknum)
> +{
> +	struct ip_conntrack_tuple innertuple, origtuple;
> +	struct {
> +		struct icmphdr icmp;
> +		struct iphdr ip;
> +	} inside;
> +	struct ip_conntrack_protocol *innerproto;
> +	struct ip_conntrack_tuple_hash *h;
> +	int dataoff;
> +
> +	IP_NF_ASSERT(skb->nfct == NULL);
> +
> +	/* Not enough header? */
> +	if (skb_copy_bits(skb, skb->nh.iph->ihl*4, &inside, sizeof(inside))!=0)
> +		return NF_ACCEPT;
> +
> +	if (inside.icmp.type != ICMP_DEST_UNREACH
> +	    && inside.icmp.type != ICMP_SOURCE_QUENCH
> +	    && inside.icmp.type != ICMP_TIME_EXCEEDED
> +	    && inside.icmp.type != ICMP_PARAMETERPROB
> +	    && inside.icmp.type != ICMP_REDIRECT)
> +		return 0;

while you're at it, change this to NF_ACCEPT please ;)

> +
> +	/* Ignore ICMP's containing fragments (shouldn't happen) */
> +	if (inside.ip.frag_off & htons(IP_OFFSET)) {
> +		DEBUGP("icmp_error_track: fragment of proto %u\n",
> +		       inside.ip.protocol);
> +		return NF_ACCEPT;
> +	}
> +
> +	innerproto = ip_ct_find_proto(inside.ip.protocol);
> +	dataoff = skb->nh.iph->ihl*4 + sizeof(inside.icmp) + inside.ip.ihl*4;
> +	/* Are they talking about one of our connections? */
> +	if (!get_tuple(&inside.ip, skb, dataoff, &origtuple, innerproto)) {
> +		DEBUGP("icmp_error: ! get_tuple p=%u", inside.ip.protocol);
> +		return NF_ACCEPT;
> +	}
> +
> +	/* Ordinarily, we'd expect the inverted tupleproto, but it's
> +	   been preserved inside the ICMP. */
> +	if (!invert_tuple(&innertuple, &origtuple, innerproto)) {
> +		DEBUGP("icmp_error_track: Can't invert tuple\n");
> +		return NF_ACCEPT;
> +	}
> +
> +	*ctinfo = IP_CT_RELATED;
> +
> +	h = ip_conntrack_find_get(&innertuple, NULL);
> +	if (!h) {
> +		/* Locally generated ICMPs will match inverted if they
> +		   haven't been SNAT'ed yet */
> +		/* FIXME: NAT code has to handle half-done double NAT --RR */
> +		if (hooknum == NF_IP_LOCAL_OUT)
> +			h = ip_conntrack_find_get(&origtuple, NULL);
> +
> +		if (!h) {
> +			DEBUGP("icmp_error_track: no match\n");
> +			return 0;

this too

> +		}
> +		/* Reverse direction from that found */
> +		if (DIRECTION(h) != IP_CT_DIR_REPLY)
> +			*ctinfo += IP_CT_IS_REPLY;
> +	} else {
> +		if (DIRECTION(h) == IP_CT_DIR_REPLY)
> +			*ctinfo += IP_CT_IS_REPLY;
> +	}
> +
> +	/* Update skb to refer to this connection */
> +	skb->nfct = &h->ctrack->infos[*ctinfo];
> +	return NF_ACCEPT;
> +}
> +
>  struct ip_conntrack_protocol ip_conntrack_protocol_icmp
>  = { { NULL, NULL }, IPPROTO_ICMP, "icmp",
>      icmp_pkt_to_tuple, icmp_invert_tuple, icmp_print_tuple,
> -    icmp_print_conntrack, icmp_packet, icmp_new, NULL, NULL, NULL };
> +    icmp_print_conntrack, icmp_packet, icmp_new, NULL, NULL, icmp_error, NULL};
> diff -u -r1.1.1.1 ip_conntrack_proto_tcp.c
> --- a/net/ipv4/netfilter/ip_conntrack_proto_tcp.c	11 May 2004 13:07:08 -0000	1.1.1.1
> +++ b/net/ipv4/netfilter/ip_conntrack_proto_tcp.c	13 Jun 2004 22:53:23 -0000
> @@ -266,7 +266,15 @@
>  	return between(exp->seq, ntohl(tcph.seq), ntohl(tcph.seq) + datalen);
>  }
>  
> +static int
> +tcp_error(struct sk_buff *skb,
> +	   enum ip_conntrack_info *ctinfo,
> +	   unsigned int hooknum)

please don't use newlines unless neccessarry .. looks like it would fit
in one line

> +{
> +	return 0;
> +}
> +
>  struct ip_conntrack_protocol ip_conntrack_protocol_tcp
>  = { { NULL, NULL }, IPPROTO_TCP, "tcp",
>      tcp_pkt_to_tuple, tcp_invert_tuple, tcp_print_tuple, tcp_print_conntrack,
> -    tcp_packet, tcp_new, NULL, tcp_exp_matches_pkt, NULL };
> +    tcp_packet, tcp_new, NULL, tcp_exp_matches_pkt, tcp_error, NULL };
> diff -u -r1.1.1.1 ip_conntrack_proto_udp.c
> --- a/net/ipv4/netfilter/ip_conntrack_proto_udp.c	11 May 2004 13:07:08 -0000	1.1.1.1
> +++ b/net/ipv4/netfilter/ip_conntrack_proto_udp.c	13 Jun 2004 23:17:51 -0000
> @@ -12,8 +12,16 @@
>  #include <linux/netfilter.h>
>  #include <linux/in.h>
>  #include <linux/udp.h>
> +#include <asm/checksum.h>
>  #include <linux/netfilter_ipv4/ip_conntrack_protocol.h>
>  
> +#if 0
> +#define NET_RATELIMIT(foo)     (foo)
> +#else
> +#define NET_RATELIMIT(foo)     ((foo) && net_ratelimit())
> +#endif
> +
> +int ip_ct_udp_log_invalid = 1;
>  unsigned long ip_ct_udp_timeout = 30*HZ;
>  unsigned long ip_ct_udp_timeout_stream = 180*HZ;
>  
> @@ -80,7 +88,31 @@
>  	return 1;
>  }
>  
> +static int
> +udp_error(struct sk_buff *skb, enum ip_conntrack_info *ctinfo,
> +	  unsigned int hooknum)
> +{
> +	struct iphdr *iph = skb->nh.iph;
> +	unsigned int udplen = skb->len - iph->ihl * 4;
> +
> +	/* Checksum invalid?  Ignore. */
> +	/* FIXME: Source route IP option packets --RR */

UDP checksum may be zero (unset)

> +	if (csum_tcpudp_magic(iph->saddr, iph->daddr,
> +			      udplen, IPPROTO_UDP,
> +			      skb->ip_summed == CHECKSUM_HW 
> +			      	? skb->csum
> +			      	: skb_checksum(skb, iph->ihl*4, udplen, 0))) {
> +		if (NET_RATELIMIT(ip_ct_udp_log_invalid))
> +			nf_log_packet(PF_INET, 0, skb, NULL, NULL, 
> +				  "ip_conntrack_udp: INVALID: "
> +				  "bad UDP checksum ");
> +		return NF_ACCEPT;
> +	}
> +
> +	return 0;
> +}
> +
>  struct ip_conntrack_protocol ip_conntrack_protocol_udp
>  = { { NULL, NULL }, IPPROTO_UDP, "udp",
>      udp_pkt_to_tuple, udp_invert_tuple, udp_print_tuple, udp_print_conntrack,
> -    udp_packet, udp_new, NULL, NULL, NULL };
> +    udp_packet, udp_new, NULL, NULL, udp_error, NULL };
> diff -u -r1.1.1.1 ip_fw_compat_masq.c
> --- a/net/ipv4/netfilter/ip_fw_compat_masq.c	11 May 2004 13:07:08 -0000	1.1.1.1
> +++ b/net/ipv4/netfilter/ip_fw_compat_masq.c	13 Jun 2004 23:25:21 -0000
> @@ -24,6 +24,7 @@
>  #include <linux/proc_fs.h>
>  #include <linux/module.h>
>  #include <net/route.h>
> +#include <net/ip.h>
>  
>  #define ASSERT_READ_LOCK(x) MUST_BE_READ_LOCKED(&ip_conntrack_lock)
>  #define ASSERT_WRITE_LOCK(x) MUST_BE_WRITE_LOCKED(&ip_conntrack_lock)
> @@ -41,6 +42,83 @@
>  #define DEBUGP(format, args...)
>  #endif
>  
> +/* Returns conntrack if it dealt with ICMP, and filled in skb fields */
> +struct ip_conntrack *
> +icmp_error_track(struct sk_buff *skb,
> +		 enum ip_conntrack_info *ctinfo,
> +		 unsigned int hooknum)
> +{
> +	struct ip_conntrack_tuple innertuple, origtuple;
> +	struct {
> +		struct icmphdr icmp;
> +		struct iphdr ip;
> +	} inside;
> +	struct ip_conntrack_protocol *innerproto;
> +	struct ip_conntrack_tuple_hash *h;
> +	int dataoff;
> +
> +	IP_NF_ASSERT(skb->nfct == NULL);
> +
> +	/* Not enough header? */
> +	if (skb_copy_bits(skb, skb->nh.iph->ihl*4, &inside, sizeof(inside))!=0)
> +		return NULL;
> +
> +	if (inside.icmp.type != ICMP_DEST_UNREACH
> +	    && inside.icmp.type != ICMP_SOURCE_QUENCH
> +	    && inside.icmp.type != ICMP_TIME_EXCEEDED
> +	    && inside.icmp.type != ICMP_PARAMETERPROB
> +	    && inside.icmp.type != ICMP_REDIRECT)
> +		return NULL;
> +
> +	/* Ignore ICMP's containing fragments (shouldn't happen) */
> +	if (inside.ip.frag_off & htons(IP_OFFSET)) {
> +		DEBUGP("icmp_error_track: fragment of proto %u\n",
> +		       inside.ip.protocol);
> +		return NULL;
> +	}
> +
> +	innerproto = ip_ct_find_proto(inside.ip.protocol);
> +	dataoff = skb->nh.iph->ihl*4 + sizeof(inside.icmp) + inside.ip.ihl*4;
> +	/* Are they talking about one of our connections? */
> +	if (!get_tuple(&inside.ip, skb, dataoff, &origtuple, innerproto)) {
> +		DEBUGP("icmp_error: ! get_tuple p=%u", inside.ip.protocol);
> +		return NULL;
> +	}
> +
> +	/* Ordinarily, we'd expect the inverted tupleproto, but it's
> +	   been preserved inside the ICMP. */
> +	if (!invert_tuple(&innertuple, &origtuple, innerproto)) {
> +		DEBUGP("icmp_error_track: Can't invert tuple\n");
> +		return NULL;
> +	}
> +
> +	*ctinfo = IP_CT_RELATED;
> +
> +	h = ip_conntrack_find_get(&innertuple, NULL);
> +	if (!h) {
> +		/* Locally generated ICMPs will match inverted if they
> +		   haven't been SNAT'ed yet */
> +		/* FIXME: NAT code has to handle half-done double NAT --RR */
> +		if (hooknum == NF_IP_LOCAL_OUT)
> +			h = ip_conntrack_find_get(&origtuple, NULL);
> +
> +		if (!h) {
> +			DEBUGP("icmp_error_track: no match\n");
> +			return NULL;
> +		}
> +		/* Reverse direction from that found */
> +		if (DIRECTION(h) != IP_CT_DIR_REPLY)
> +			*ctinfo += IP_CT_IS_REPLY;
> +	} else {
> +		if (DIRECTION(h) == IP_CT_DIR_REPLY)
> +			*ctinfo += IP_CT_IS_REPLY;
> +	}
> +
> +	/* Update skb to refer to this connection */
> +	skb->nfct = &h->ctrack->infos[*ctinfo];
> +	return h->ctrack;
> +}
> +
>  unsigned int
>  do_masquerade(struct sk_buff **pskb, const struct net_device *dev)
>  {
> diff -u -r1.1.1.1 ip_conntrack.h
> --- a/include/linux/netfilter_ipv4/ip_conntrack.h	11 May 2004 13:35:40 -0000	1.1.1.1
> +++ b/include/linux/netfilter_ipv4/ip_conntrack.h	13 Jun 2004 09:23:03 -0000
> @@ -264,6 +264,26 @@
>  ip_ct_selective_cleanup(int (*kill)(const struct ip_conntrack *i, void *data),
>  			void *data);
>  
> +extern struct ip_conntrack_protocol *
> +ip_ct_find_proto(u_int8_t protocol);
> +
> +extern int
> +get_tuple(const struct iphdr *iph,
> +	  const struct sk_buff *skb,
> +	  unsigned int dataoff,
> +	  struct ip_conntrack_tuple *tuple,
> +	  const struct ip_conntrack_protocol *protocol);
> +
> +extern int
> +invert_tuple(struct ip_conntrack_tuple *inverse,
> +	     const struct ip_conntrack_tuple *orig,
> +	     const struct ip_conntrack_protocol *protocol);
> +
> +/* Find a connection corresponding to a tuple. */
> +extern struct ip_conntrack_tuple_hash *
> +ip_conntrack_find_get(const struct ip_conntrack_tuple *tuple,
> +		      const struct ip_conntrack *ignored_conntrack);
> +
>  /* It's confirmed if it is, or has been in the hash table. */
>  static inline int is_confirmed(struct ip_conntrack *ct)
>  {
> diff -u -r1.1.1.1 ip_conntrack_protocol.h
> --- a/include/linux/netfilter_ipv4/ip_conntrack_protocol.h	11 May 2004 13:35:39 -0000	1.1.1.1
> +++ b/include/linux/netfilter_ipv4/ip_conntrack_protocol.h	13 Jun 2004 22:54:02 -0000
> @@ -50,6 +50,9 @@
>  	int (*exp_matches_pkt)(struct ip_conntrack_expect *exp,
>  			       const struct sk_buff *skb);
>  
> +	int (*error)(struct sk_buff *skb, enum ip_conntrack_info *ctinfo,
> +		     unsigned int hooknum);
> +
>  	/* Module (if any) which this is connected to. */
>  	struct module *me;
>  };

Re: [PATCH] modification in current protocol helper API to handle error/unclean packets

[Patrick McHardy]

Patrick McHardy wrote:
> Pablo Neira wrote:
>
>> Hope that I didn't break/miss anything, if so, please let me know.
>  
> Some more comments below.

Please forget about my comments regarding NF_ACCEPT, I was under the
impression it was defined as 0.

> 
> Regards
> Patrick

Re: [PATCH] modification in current protocol helper API to handle error/unclean packets

[Jozsef Kadlecsik]

Hi Pablo,

On Mon, 14 Jun 2004, Pablo Neira wrote:

> I added a new function to the current protocol helper API. It is called
> before resolve_normal_ct, this could let us perform actions before
> starting a new session/conntrack.
>
> These actions could be:
> - check for malformed/unclean packets.
> - handle special packets like in ICMP.
>
> This way, Jozsef could call unclean() here in his tcp-window-tracking
> patch and we won't need to add protocol specific function to the core of
> the conntrack system like icmp_error_track to handle special ICMP
> messages. We could also check for unclean udp and icmp packets.

I like it, too! I think the tcp-window-tracking patch should be made
dependent of your patch, because it'd make the code cleaner (the quick fix
for the local checksum problem reported by Martin was to pass down the
hooknum as additional argument of the packet/new functions, but it's ugly
and changes the API as well.)

> This function returns 0 if no error was found or an action (NF_*) if an
> error/special action needs to be done to handle a message.

NF_DROP == 0, so it should simply return an NF_* value. Thus the condition

+       /* It may be an special packet, error, unclean... */
+       if ((ret = proto->error(*pskb, &ctinfo, hooknum)))
+               return ret;

should be

+       /* It may be an special packet, error, unclean... */
+       if ((ret = proto->error(*pskb, &ctinfo, hooknum)) != NF_ACCEPT)
+               return ret;

And as Patrick noted, UDP checksumming in IPv4 is not mandatory.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlec@sunserv.kfki.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : KFKI Research Institute for Particle and Nuclear Physics
          H-1525 Budapest 114, POB. 49, Hungary

Re: [PATCH] modification in current protocol helper API to handle error/unclean packets

[Pablo Neira]

Hi Patrick and Jozsef,

I'll reply you both in this email

Jozsef Kadlecsik wrote:

>I like it, too! I think the tcp-window-tracking patch should be made
>dependent of your patch
>  
>

happy that you both like it! :-)

>NF_DROP == 0, so it should simply return an NF_* value. Thus the condition
>
>+       /* It may be an special packet, error, unclean... */
>+       if ((ret = proto->error(*pskb, &ctinfo, hooknum)))
>+               return ret;
>
>should be
>
>+       /* It may be an special packet, error, unclean... */
>+       if ((ret = proto->error(*pskb, &ctinfo, hooknum)) != NF_ACCEPT)
>+               return ret;
>  
>

I'm still in doubt, if we found an unclean packet, we should return 
NF_ACCEPT, so we let the packet continues its travel. In that case, the 
skbuff  won't have a conntrack associated and the sysadmin could 
explicitely drop invalid packets with an iptables rule, am I right?

So I need a return value to say: "do nothing", that is, go ahead with 
conntrack session, and I can  use neither NF_ACCEPT to do that nor 0 
because == NF_DROP. Should I use -1 then?

>And as Patrick noted, UDP checksumming in IPv4 is not mandatory.
>

I missed that, I'll take it into account for an extra patch to check for 
udp/icmp unclean packets which will go on top of my patch.


Patrick McHardy wrote:

> Couldn't you use ip_ct_find_proto(IPPROTO_ICMP)->error() instead ?


true, I'll have a look at this

> invert_tuple and find_tuple probably should be prefixed with ip_ct_
> then.


so, I could rename those

>> - oh! I added udp checksum checking, well, I just stole that from 
>> Joszef's tcp tracking code and rename things...
>
>
> This should be an extra patch.


ok, so I'll do them like an extra patch on top of this.

regards,
Pablo

Re: [PATCH] modification in current protocol helper API to handle error/unclean packets

[Jozsef Kadlecsik]

On Mon, 14 Jun 2004, Pablo Neira wrote:

> >+       /* It may be an special packet, error, unclean... */
> >+       if ((ret = proto->error(*pskb, &ctinfo, hooknum)) != NF_ACCEPT)
> >+               return ret;
>
> I'm still in doubt, if we found an unclean packet, we should return
> NF_ACCEPT, so we let the packet continues its travel. In that case, the
> skbuff  won't have a conntrack associated and the sysadmin could
> explicitely drop invalid packets with an iptables rule, am I right?

As I sent my mail I noticed that I'm mistaken: following what I suggested
the goal to encapsulate the ICMP error message handling could not be done.

So the semantics could follow the one applied in the tcp-window-tracking
patch: stop the processing of the packet by returning negated error code;
otherwise continue:

+       /* It may be an special packet, error, unclean...
+        * inverse of the return code tells to the netfilter
+	 * core what to do with the packet. */
+       if ((ret = proto->error(*pskb, &ctinfo, hooknum)) <= 0)
+               return -ret;

That way we can draw any shortcut decision in the error function and
NF_ACCEPT means just to traverse further in conntrack.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlec@sunserv.kfki.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : KFKI Research Institute for Particle and Nuclear Physics
          H-1525 Budapest 114, POB. 49, Hungary

Re: [PATCH] modification in current protocol helper API to handle error/unclean packets

[Pablo Neira]

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

Hi,

Attached the patch to add the error function to the current protocol 
helper API. Some comments:

- Patrick, I had to copy and paste the icmp_error_track function to 
ip_fw_compat_masq.c because new error function returns an int and we 
need to return the conntrack associated to inner protocol embedded in 
the icmp message. I know that replicating code is not a good idea but 
that compatibility layer will disappear some day... any idea? I also 
rename some functions (invert_tuple and get_tuple) as you pointed out.
- Jozsef, I resolve to use your inverse return value but I must confess 
that I'm still thinking about another way to do that with no sucess, 
well this issue is not important. BTW, you also modified conntrack core 
in your tcp-window-tracking patch, do you think that I could add that 
part to my patch since I think that it's more related stuff and your 
patch will go on top of this?
- I'll work on that extra patch to check for malformed udp/icmp packets 
these days.

If any comment or any indentation problem, please let me know.

regards,
Pablo

[-- Attachment #2: protocol_helper-error.patch --]
[-- Type: text/x-patch, Size: 15405 bytes --]

diff -u -r1.1.1.1 ip_conntrack_core.c
--- a/net/ipv4/netfilter/ip_conntrack_core.c	11 May 2004 13:07:08 -0000	1.1.1.1
+++ b/net/ipv4/netfilter/ip_conntrack_core.c	16 Jun 2004 12:59:39 -0000
@@ -126,11 +126,11 @@
 }
 
 int
-get_tuple(const struct iphdr *iph,
-	  const struct sk_buff *skb,
-	  unsigned int dataoff,
-	  struct ip_conntrack_tuple *tuple,
-	  const struct ip_conntrack_protocol *protocol)
+ip_ct_get_tuple(const struct iphdr *iph,
+		const struct sk_buff *skb,
+		unsigned int dataoff,
+		struct ip_conntrack_tuple *tuple,
+		const struct ip_conntrack_protocol *protocol)
 {
 	/* Never happen */
 	if (iph->frag_off & htons(IP_OFFSET)) {
@@ -146,10 +146,10 @@
 	return protocol->pkt_to_tuple(skb, dataoff, tuple);
 }
 
-static int
-invert_tuple(struct ip_conntrack_tuple *inverse,
-	     const struct ip_conntrack_tuple *orig,
-	     const struct ip_conntrack_protocol *protocol)
+int
+ip_ct_invert_tuple(struct ip_conntrack_tuple *inverse,
+		   const struct ip_conntrack_tuple *orig,
+		   const struct ip_conntrack_protocol *protocol)
 {
 	inverse->src.ip = orig->dst.ip;
 	inverse->dst.ip = orig->src.ip;
@@ -495,83 +495,6 @@
 	return h != NULL;
 }
 
-/* Returns conntrack if it dealt with ICMP, and filled in skb fields */
-struct ip_conntrack *
-icmp_error_track(struct sk_buff *skb,
-		 enum ip_conntrack_info *ctinfo,
-		 unsigned int hooknum)
-{
-	struct ip_conntrack_tuple innertuple, origtuple;
-	struct {
-		struct icmphdr icmp;
-		struct iphdr ip;
-	} inside;
-	struct ip_conntrack_protocol *innerproto;
-	struct ip_conntrack_tuple_hash *h;
-	int dataoff;
-
-	IP_NF_ASSERT(skb->nfct == NULL);
-
-	/* Not enough header? */
-	if (skb_copy_bits(skb, skb->nh.iph->ihl*4, &inside, sizeof(inside))!=0)
-		return NULL;
-
-	if (inside.icmp.type != ICMP_DEST_UNREACH
-	    && inside.icmp.type != ICMP_SOURCE_QUENCH
-	    && inside.icmp.type != ICMP_TIME_EXCEEDED
-	    && inside.icmp.type != ICMP_PARAMETERPROB
-	    && inside.icmp.type != ICMP_REDIRECT)
-		return NULL;
-
-	/* Ignore ICMP's containing fragments (shouldn't happen) */
-	if (inside.ip.frag_off & htons(IP_OFFSET)) {
-		DEBUGP("icmp_error_track: fragment of proto %u\n",
-		       inside.ip.protocol);
-		return NULL;
-	}
-
-	innerproto = ip_ct_find_proto(inside.ip.protocol);
-	dataoff = skb->nh.iph->ihl*4 + sizeof(inside.icmp) + inside.ip.ihl*4;
-	/* Are they talking about one of our connections? */
-	if (!get_tuple(&inside.ip, skb, dataoff, &origtuple, innerproto)) {
-		DEBUGP("icmp_error: ! get_tuple p=%u", inside.ip.protocol);
-		return NULL;
-	}
-
-	/* Ordinarily, we'd expect the inverted tupleproto, but it's
-	   been preserved inside the ICMP. */
-	if (!invert_tuple(&innertuple, &origtuple, innerproto)) {
-		DEBUGP("icmp_error_track: Can't invert tuple\n");
-		return NULL;
-	}
-
-	*ctinfo = IP_CT_RELATED;
-
-	h = ip_conntrack_find_get(&innertuple, NULL);
-	if (!h) {
-		/* Locally generated ICMPs will match inverted if they
-		   haven't been SNAT'ed yet */
-		/* FIXME: NAT code has to handle half-done double NAT --RR */
-		if (hooknum == NF_IP_LOCAL_OUT)
-			h = ip_conntrack_find_get(&origtuple, NULL);
-
-		if (!h) {
-			DEBUGP("icmp_error_track: no match\n");
-			return NULL;
-		}
-		/* Reverse direction from that found */
-		if (DIRECTION(h) != IP_CT_DIR_REPLY)
-			*ctinfo += IP_CT_IS_REPLY;
-	} else {
-		if (DIRECTION(h) == IP_CT_DIR_REPLY)
-			*ctinfo += IP_CT_IS_REPLY;
-	}
-
-	/* Update skb to refer to this connection */
-	skb->nfct = &h->ctrack->infos[*ctinfo];
-	return h->ctrack;
-}
-
 /* There's a small race here where we may free a just-assured
    connection.  Too bad: we're in trouble anyway. */
 static inline int unreplied(const struct ip_conntrack_tuple_hash *i)
@@ -653,7 +576,7 @@
 		}
 	}
 
-	if (!invert_tuple(&repl_tuple, tuple, protocol)) {
+	if (!ip_ct_invert_tuple(&repl_tuple, tuple, protocol)) {
 		DEBUGP("Can't invert tuple.\n");
 		return NULL;
 	}
@@ -743,7 +666,8 @@
 
 	IP_NF_ASSERT((skb->nh.iph->frag_off & htons(IP_OFFSET)) == 0);
 
-	if (!get_tuple(skb->nh.iph, skb, skb->nh.iph->ihl*4, &tuple, proto))
+	if (!ip_ct_get_tuple(skb->nh.iph, skb, skb->nh.iph->ihl*4, 
+				&tuple,proto))
 		return NULL;
 
 	/* look for tuple match */
@@ -828,10 +752,11 @@
 
 	proto = ip_ct_find_proto((*pskb)->nh.iph->protocol);
 
-	/* It may be an icmp error... */
-	if ((*pskb)->nh.iph->protocol == IPPROTO_ICMP 
-	    && icmp_error_track(*pskb, &ctinfo, hooknum))
-		return NF_ACCEPT;
+	/* It may be an special packet, error, unclean...
+	 * inverse of the return code tells to the netfilter
+	 * core what to do with the packet. */
+	if ((ret = proto->error(*pskb, &ctinfo, hooknum)) <= 0)
+		return -ret;
 
 	if (!(ct = resolve_normal_ct(*pskb, proto,&set_reply,hooknum,&ctinfo)))
 		/* Not valid part of a connection */
@@ -869,7 +794,8 @@
 int invert_tuplepr(struct ip_conntrack_tuple *inverse,
 		   const struct ip_conntrack_tuple *orig)
 {
-	return invert_tuple(inverse, orig, ip_ct_find_proto(orig->dst.protonum));
+	return ip_ct_invert_tuple(inverse, orig, 
+				  ip_ct_find_proto(orig->dst.protonum));
 }
 
 static inline int resent_expect(const struct ip_conntrack_expect *i,
diff -u -r1.1.1.1 ip_conntrack_proto_generic.c
--- a/net/ipv4/netfilter/ip_conntrack_proto_generic.c	11 May 2004 13:07:08 -0000	1.1.1.1
+++ b/net/ipv4/netfilter/ip_conntrack_proto_generic.c	16 Jun 2004 12:58:02 -0000
@@ -62,8 +62,15 @@
 	return 1;
 }
 
+static int
+error(struct sk_buff *skb, enum ip_conntrack_info *ctinfo, 
+      unsigned int hooknum)
+{
+	return NF_ACCEPT;
+}
+
 struct ip_conntrack_protocol ip_conntrack_generic_protocol
 = { { NULL, NULL }, 0, "unknown",
     generic_pkt_to_tuple, generic_invert_tuple, generic_print_tuple,
-    generic_print_conntrack, packet, new, NULL, NULL, NULL };
+    generic_print_conntrack, packet, new, NULL, NULL, error, NULL };
 
diff -u -r1.1.1.1 ip_conntrack_proto_icmp.c
--- a/net/ipv4/netfilter/ip_conntrack_proto_icmp.c	11 May 2004 13:07:08 -0000	1.1.1.1
+++ b/net/ipv4/netfilter/ip_conntrack_proto_icmp.c	16 Jun 2004 18:06:57 -0000
@@ -12,6 +12,8 @@
 #include <linux/netfilter.h>
 #include <linux/in.h>
 #include <linux/icmp.h>
+#include <net/ip.h>
+#include <linux/netfilter_ipv4/ip_conntrack.h>
 #include <linux/netfilter_ipv4/ip_conntrack_protocol.h>
 
 unsigned long ip_ct_icmp_timeout = 30*HZ;
@@ -122,7 +124,85 @@
 	return 1;
 }
 
+/* backwards compatibility: If you modify this function you should also do 
+ * the same for icmp_error_track in ip_fw_compat_masq.c */
+static int
+icmp_error(struct sk_buff *skb,
+		 enum ip_conntrack_info *ctinfo,
+		 unsigned int hooknum)
+{
+	struct ip_conntrack_tuple innertuple, origtuple;
+	struct {
+		struct icmphdr icmp;
+		struct iphdr ip;
+	} inside;
+	struct ip_conntrack_protocol *innerproto;
+	struct ip_conntrack_tuple_hash *h;
+	int dataoff;
+
+	IP_NF_ASSERT(skb->nfct == NULL);
+
+	/* Not enough header? */
+	if (skb_copy_bits(skb, skb->nh.iph->ihl*4, &inside, sizeof(inside))!=0)
+		return NF_ACCEPT;
+
+	if (inside.icmp.type != ICMP_DEST_UNREACH
+	    && inside.icmp.type != ICMP_SOURCE_QUENCH
+	    && inside.icmp.type != ICMP_TIME_EXCEEDED
+	    && inside.icmp.type != ICMP_PARAMETERPROB
+	    && inside.icmp.type != ICMP_REDIRECT)
+		return NF_ACCEPT;
+
+	/* Ignore ICMP's containing fragments (shouldn't happen) */
+	if (inside.ip.frag_off & htons(IP_OFFSET)) {
+		DEBUGP("icmp_error_track: fragment of proto %u\n",
+		       inside.ip.protocol);
+		return NF_ACCEPT;
+	}
+
+	innerproto = ip_ct_find_proto(inside.ip.protocol);
+	dataoff = skb->nh.iph->ihl*4 + sizeof(inside.icmp) + inside.ip.ihl*4;
+	/* Are they talking about one of our connections? */
+	if (!ip_ct_get_tuple(&inside.ip, skb, dataoff, &origtuple, innerproto)) {
+		DEBUGP("icmp_error: ! get_tuple p=%u", inside.ip.protocol);
+		return NF_ACCEPT;
+	}
+
+	/* Ordinarily, we'd expect the inverted tupleproto, but it's
+	   been preserved inside the ICMP. */
+	if (!ip_ct_invert_tuple(&innertuple, &origtuple, innerproto)) {
+		DEBUGP("icmp_error_track: Can't invert tuple\n");
+		return NF_ACCEPT;
+	}
+
+	*ctinfo = IP_CT_RELATED;
+
+	h = ip_conntrack_find_get(&innertuple, NULL);
+	if (!h) {
+		/* Locally generated ICMPs will match inverted if they
+		   haven't been SNAT'ed yet */
+		/* FIXME: NAT code has to handle half-done double NAT --RR */
+		if (hooknum == NF_IP_LOCAL_OUT)
+			h = ip_conntrack_find_get(&origtuple, NULL);
+
+		if (!h) {
+			DEBUGP("icmp_error_track: no match\n");
+			return NF_ACCEPT;
+		}
+		/* Reverse direction from that found */
+		if (DIRECTION(h) != IP_CT_DIR_REPLY)
+			*ctinfo += IP_CT_IS_REPLY;
+	} else {
+		if (DIRECTION(h) == IP_CT_DIR_REPLY)
+			*ctinfo += IP_CT_IS_REPLY;
+	}
+
+	/* Update skb to refer to this connection */
+	skb->nfct = &h->ctrack->infos[*ctinfo];
+	return -NF_ACCEPT;
+}
+
 struct ip_conntrack_protocol ip_conntrack_protocol_icmp
 = { { NULL, NULL }, IPPROTO_ICMP, "icmp",
     icmp_pkt_to_tuple, icmp_invert_tuple, icmp_print_tuple,
-    icmp_print_conntrack, icmp_packet, icmp_new, NULL, NULL, NULL };
+    icmp_print_conntrack, icmp_packet, icmp_new, NULL, NULL, icmp_error, NULL};
diff -u -r1.1.1.1 ip_conntrack_proto_tcp.c
--- a/net/ipv4/netfilter/ip_conntrack_proto_tcp.c	11 May 2004 13:07:08 -0000	1.1.1.1
+++ b/net/ipv4/netfilter/ip_conntrack_proto_tcp.c	16 Jun 2004 12:57:50 -0000
@@ -266,7 +266,14 @@
 	return between(exp->seq, ntohl(tcph.seq), ntohl(tcph.seq) + datalen);
 }
 
+static int
+tcp_error(struct sk_buff *skb, enum ip_conntrack_info *ctinfo, 
+	   unsigned int hooknum)
+{
+	return NF_ACCEPT;
+}
+
 struct ip_conntrack_protocol ip_conntrack_protocol_tcp
 = { { NULL, NULL }, IPPROTO_TCP, "tcp",
     tcp_pkt_to_tuple, tcp_invert_tuple, tcp_print_tuple, tcp_print_conntrack,
-    tcp_packet, tcp_new, NULL, tcp_exp_matches_pkt, NULL };
+    tcp_packet, tcp_new, NULL, tcp_exp_matches_pkt, tcp_error, NULL };
diff -u -r1.1.1.1 ip_conntrack_proto_udp.c
--- a/net/ipv4/netfilter/ip_conntrack_proto_udp.c	11 May 2004 13:07:08 -0000	1.1.1.1
+++ b/net/ipv4/netfilter/ip_conntrack_proto_udp.c	16 Jun 2004 13:02:46 -0000
@@ -80,7 +80,14 @@
 	return 1;
 }
 
+static int
+udp_error(struct sk_buff *skb, enum ip_conntrack_info *ctinfo,
+	  unsigned int hooknum)
+{
+	return NF_ACCEPT;
+}
+
 struct ip_conntrack_protocol ip_conntrack_protocol_udp
 = { { NULL, NULL }, IPPROTO_UDP, "udp",
     udp_pkt_to_tuple, udp_invert_tuple, udp_print_tuple, udp_print_conntrack,
-    udp_packet, udp_new, NULL, NULL, NULL };
+    udp_packet, udp_new, NULL, NULL, udp_error, NULL };
diff -u -r1.1.1.1 ip_fw_compat_masq.c
--- a/net/ipv4/netfilter/ip_fw_compat_masq.c	11 May 2004 13:07:08 -0000	1.1.1.1
+++ b/net/ipv4/netfilter/ip_fw_compat_masq.c	16 Jun 2004 13:13:57 -0000
@@ -24,6 +24,7 @@
 #include <linux/proc_fs.h>
 #include <linux/module.h>
 #include <net/route.h>
+#include <net/ip.h>
 
 #define ASSERT_READ_LOCK(x) MUST_BE_READ_LOCKED(&ip_conntrack_lock)
 #define ASSERT_WRITE_LOCK(x) MUST_BE_WRITE_LOCKED(&ip_conntrack_lock)
@@ -41,6 +42,83 @@
 #define DEBUGP(format, args...)
 #endif
 
+/* Returns conntrack if it dealt with ICMP, and filled in skb fields */
+struct ip_conntrack *
+icmp_error_track(struct sk_buff *skb,
+		 enum ip_conntrack_info *ctinfo,
+		 unsigned int hooknum)
+{
+	struct ip_conntrack_tuple innertuple, origtuple;
+	struct {
+		struct icmphdr icmp;
+		struct iphdr ip;
+	} inside;
+	struct ip_conntrack_protocol *innerproto;
+	struct ip_conntrack_tuple_hash *h;
+	int dataoff;
+
+	IP_NF_ASSERT(skb->nfct == NULL);
+
+	/* Not enough header? */
+	if (skb_copy_bits(skb, skb->nh.iph->ihl*4, &inside, sizeof(inside))!=0)
+		return NULL;
+
+	if (inside.icmp.type != ICMP_DEST_UNREACH
+	    && inside.icmp.type != ICMP_SOURCE_QUENCH
+	    && inside.icmp.type != ICMP_TIME_EXCEEDED
+	    && inside.icmp.type != ICMP_PARAMETERPROB
+	    && inside.icmp.type != ICMP_REDIRECT)
+		return NULL;
+
+	/* Ignore ICMP's containing fragments (shouldn't happen) */
+	if (inside.ip.frag_off & htons(IP_OFFSET)) {
+		DEBUGP("icmp_error_track: fragment of proto %u\n",
+		       inside.ip.protocol);
+		return NULL;
+	}
+
+	innerproto = ip_ct_find_proto(inside.ip.protocol);
+	dataoff = skb->nh.iph->ihl*4 + sizeof(inside.icmp) + inside.ip.ihl*4;
+	/* Are they talking about one of our connections? */
+	if (!ip_ct_get_tuple(&inside.ip, skb, dataoff, &origtuple, innerproto)) {
+		DEBUGP("icmp_error: ! get_tuple p=%u", inside.ip.protocol);
+		return NULL;
+	}
+
+	/* Ordinarily, we'd expect the inverted tupleproto, but it's
+	   been preserved inside the ICMP. */
+	if (!ip_ct_invert_tuple(&innertuple, &origtuple, innerproto)) {
+		DEBUGP("icmp_error_track: Can't invert tuple\n");
+		return NULL;
+	}
+
+	*ctinfo = IP_CT_RELATED;
+
+	h = ip_conntrack_find_get(&innertuple, NULL);
+	if (!h) {
+		/* Locally generated ICMPs will match inverted if they
+		   haven't been SNAT'ed yet */
+		/* FIXME: NAT code has to handle half-done double NAT --RR */
+		if (hooknum == NF_IP_LOCAL_OUT)
+			h = ip_conntrack_find_get(&origtuple, NULL);
+
+		if (!h) {
+			DEBUGP("icmp_error_track: no match\n");
+			return NULL;
+		}
+		/* Reverse direction from that found */
+		if (DIRECTION(h) != IP_CT_DIR_REPLY)
+			*ctinfo += IP_CT_IS_REPLY;
+	} else {
+		if (DIRECTION(h) == IP_CT_DIR_REPLY)
+			*ctinfo += IP_CT_IS_REPLY;
+	}
+
+	/* Update skb to refer to this connection */
+	skb->nfct = &h->ctrack->infos[*ctinfo];
+	return h->ctrack;
+}
+
 unsigned int
 do_masquerade(struct sk_buff **pskb, const struct net_device *dev)
 {
@@ -164,7 +242,7 @@
 	case IPPROTO_UDP:
 		IP_NF_ASSERT(((*pskb)->nh.iph->frag_off & htons(IP_OFFSET)) == 0);
 
-		if (!get_tuple((*pskb)->nh.iph, *pskb, (*pskb)->nh.iph->ihl*4, &tuple, protocol)) {
+		if (!ip_ct_get_tuple((*pskb)->nh.iph, *pskb, (*pskb)->nh.iph->ihl*4, &tuple, protocol)) {
 			if (net_ratelimit())
 				printk("ip_fw_compat_masq: Can't get tuple\n");
 			return NF_ACCEPT;
diff -u -r1.1.1.1 ip_conntrack.h
--- a/include/linux/netfilter_ipv4/ip_conntrack.h	11 May 2004 13:35:40 -0000	1.1.1.1
+++ b/include/linux/netfilter_ipv4/ip_conntrack.h	16 Jun 2004 13:20:51 -0000
@@ -264,6 +264,26 @@
 ip_ct_selective_cleanup(int (*kill)(const struct ip_conntrack *i, void *data),
 			void *data);
 
+extern struct ip_conntrack_protocol *
+ip_ct_find_proto(u_int8_t protocol);
+
+extern int
+ip_ct_get_tuple(const struct iphdr *iph,
+		const struct sk_buff *skb,
+		unsigned int dataoff,
+		struct ip_conntrack_tuple *tuple,
+		const struct ip_conntrack_protocol *protocol);
+
+extern int
+ip_ct_invert_tuple(struct ip_conntrack_tuple *inverse,
+		   const struct ip_conntrack_tuple *orig,
+		   const struct ip_conntrack_protocol *protocol);
+
+/* Find a connection corresponding to a tuple. */
+extern struct ip_conntrack_tuple_hash *
+ip_conntrack_find_get(const struct ip_conntrack_tuple *tuple,
+		      const struct ip_conntrack *ignored_conntrack);
+
 /* It's confirmed if it is, or has been in the hash table. */
 static inline int is_confirmed(struct ip_conntrack *ct)
 {
diff -u -r1.1.1.1 ip_conntrack_protocol.h
--- a/include/linux/netfilter_ipv4/ip_conntrack_protocol.h	11 May 2004 13:35:39 -0000	1.1.1.1
+++ b/include/linux/netfilter_ipv4/ip_conntrack_protocol.h	15 Jun 2004 18:00:54 -0000
@@ -50,6 +50,9 @@
 	int (*exp_matches_pkt)(struct ip_conntrack_expect *exp,
 			       const struct sk_buff *skb);
 
+	int (*error)(struct sk_buff *skb, enum ip_conntrack_info *ctinfo,
+		     unsigned int hooknum);
+
 	/* Module (if any) which this is connected to. */
 	struct module *me;
 };

Re: [PATCH] modification in current protocol helper API to handle error/unclean packets

[Patrick McHardy]

Hi Pablo,

Pablo Neira wrote:
> Hi,
> 
> Attached the patch to add the error function to the current protocol 
> helper API. Some comments:
> 
> - Patrick, I had to copy and paste the icmp_error_track function to 
> ip_fw_compat_masq.c because new error function returns an int and we 
> need to return the conntrack associated to inner protocol embedded in 
> the icmp message. I know that replicating code is not a good idea but 
> that compatibility layer will disappear some day... any idea? I also 
> rename some functions (invert_tuple and get_tuple) as you pointed out.

What about this:

===== net/ipv4/netfilter/ip_fw_compat_masq.c 1.14 vs edited =====
--- 1.14/net/ipv4/netfilter/ip_fw_compat_masq.c	2004-03-23 02:17:04 +01:00
+++ edited/net/ipv4/netfilter/ip_fw_compat_masq.c	2004-06-17 14:33:49 +02:00
@@ -143,7 +143,9 @@
  	switch ((*pskb)->nh.iph->protocol) {
  	case IPPROTO_ICMP:
  		/* ICMP errors. */
-		ct = icmp_error_track(*pskb, &ctinfo, NF_IP_PRE_ROUTING);
+		ip_ct_find_proto(IPPROTO_ICMP)->error(*pskb, &ctinfo,
+		                                      NF_IP_PRE_ROUTING);
+		ct = (struct ip_conntrack *)skb->nfct->master;
  		if (ct) {
  			/* We only do SNAT in the compatibility layer.
  			   So we can manipulate ICMP errors from



> - Jozsef, I resolve to use your inverse return value but I must confess 
> that I'm still thinking about another way to do that with no sucess, 
> well this issue is not important. BTW, you also modified conntrack core 
> in your tcp-window-tracking patch, do you think that I could add that 
> part to my patch since I think that it's more related stuff and your 
> patch will go on top of this?
> - I'll work on that extra patch to check for malformed udp/icmp packets 
> these days.
> 
> If any comment or any indentation problem, please let me know.

You could make transition for existing protocols easier by makeing the
error function optional:

if (proto->error != NULL && ret = proto->error(....))

Other than that, it looks fine to me.

Regards
Patrick

> 
> regards,
> Pablo
> 
> 
> ------------------------------------------------------------------------
> 
> diff -u -r1.1.1.1 ip_conntrack_core.c
> --- a/net/ipv4/netfilter/ip_conntrack_core.c	11 May 2004 13:07:08 -0000	1.1.1.1
> +++ b/net/ipv4/netfilter/ip_conntrack_core.c	16 Jun 2004 12:59:39 -0000
> @@ -126,11 +126,11 @@
>  }
>  
>  int
> -get_tuple(const struct iphdr *iph,
> -	  const struct sk_buff *skb,
> -	  unsigned int dataoff,
> -	  struct ip_conntrack_tuple *tuple,
> -	  const struct ip_conntrack_protocol *protocol)
> +ip_ct_get_tuple(const struct iphdr *iph,
> +		const struct sk_buff *skb,
> +		unsigned int dataoff,
> +		struct ip_conntrack_tuple *tuple,
> +		const struct ip_conntrack_protocol *protocol)
>  {
>  	/* Never happen */
>  	if (iph->frag_off & htons(IP_OFFSET)) {
> @@ -146,10 +146,10 @@
>  	return protocol->pkt_to_tuple(skb, dataoff, tuple);
>  }
>  
> -static int
> -invert_tuple(struct ip_conntrack_tuple *inverse,
> -	     const struct ip_conntrack_tuple *orig,
> -	     const struct ip_conntrack_protocol *protocol)
> +int
> +ip_ct_invert_tuple(struct ip_conntrack_tuple *inverse,
> +		   const struct ip_conntrack_tuple *orig,
> +		   const struct ip_conntrack_protocol *protocol)
>  {
>  	inverse->src.ip = orig->dst.ip;
>  	inverse->dst.ip = orig->src.ip;
> @@ -495,83 +495,6 @@
>  	return h != NULL;
>  }
>  
> -/* Returns conntrack if it dealt with ICMP, and filled in skb fields */
> -struct ip_conntrack *
> -icmp_error_track(struct sk_buff *skb,
> -		 enum ip_conntrack_info *ctinfo,
> -		 unsigned int hooknum)
> -{
> -	struct ip_conntrack_tuple innertuple, origtuple;
> -	struct {
> -		struct icmphdr icmp;
> -		struct iphdr ip;
> -	} inside;
> -	struct ip_conntrack_protocol *innerproto;
> -	struct ip_conntrack_tuple_hash *h;
> -	int dataoff;
> -
> -	IP_NF_ASSERT(skb->nfct == NULL);
> -
> -	/* Not enough header? */
> -	if (skb_copy_bits(skb, skb->nh.iph->ihl*4, &inside, sizeof(inside))!=0)
> -		return NULL;
> -
> -	if (inside.icmp.type != ICMP_DEST_UNREACH
> -	    && inside.icmp.type != ICMP_SOURCE_QUENCH
> -	    && inside.icmp.type != ICMP_TIME_EXCEEDED
> -	    && inside.icmp.type != ICMP_PARAMETERPROB
> -	    && inside.icmp.type != ICMP_REDIRECT)
> -		return NULL;
> -
> -	/* Ignore ICMP's containing fragments (shouldn't happen) */
> -	if (inside.ip.frag_off & htons(IP_OFFSET)) {
> -		DEBUGP("icmp_error_track: fragment of proto %u\n",
> -		       inside.ip.protocol);
> -		return NULL;
> -	}
> -
> -	innerproto = ip_ct_find_proto(inside.ip.protocol);
> -	dataoff = skb->nh.iph->ihl*4 + sizeof(inside.icmp) + inside.ip.ihl*4;
> -	/* Are they talking about one of our connections? */
> -	if (!get_tuple(&inside.ip, skb, dataoff, &origtuple, innerproto)) {
> -		DEBUGP("icmp_error: ! get_tuple p=%u", inside.ip.protocol);
> -		return NULL;
> -	}
> -
> -	/* Ordinarily, we'd expect the inverted tupleproto, but it's
> -	   been preserved inside the ICMP. */
> -	if (!invert_tuple(&innertuple, &origtuple, innerproto)) {
> -		DEBUGP("icmp_error_track: Can't invert tuple\n");
> -		return NULL;
> -	}
> -
> -	*ctinfo = IP_CT_RELATED;
> -
> -	h = ip_conntrack_find_get(&innertuple, NULL);
> -	if (!h) {
> -		/* Locally generated ICMPs will match inverted if they
> -		   haven't been SNAT'ed yet */
> -		/* FIXME: NAT code has to handle half-done double NAT --RR */
> -		if (hooknum == NF_IP_LOCAL_OUT)
> -			h = ip_conntrack_find_get(&origtuple, NULL);
> -
> -		if (!h) {
> -			DEBUGP("icmp_error_track: no match\n");
> -			return NULL;
> -		}
> -		/* Reverse direction from that found */
> -		if (DIRECTION(h) != IP_CT_DIR_REPLY)
> -			*ctinfo += IP_CT_IS_REPLY;
> -	} else {
> -		if (DIRECTION(h) == IP_CT_DIR_REPLY)
> -			*ctinfo += IP_CT_IS_REPLY;
> -	}
> -
> -	/* Update skb to refer to this connection */
> -	skb->nfct = &h->ctrack->infos[*ctinfo];
> -	return h->ctrack;
> -}
> -
>  /* There's a small race here where we may free a just-assured
>     connection.  Too bad: we're in trouble anyway. */
>  static inline int unreplied(const struct ip_conntrack_tuple_hash *i)
> @@ -653,7 +576,7 @@
>  		}
>  	}
>  
> -	if (!invert_tuple(&repl_tuple, tuple, protocol)) {
> +	if (!ip_ct_invert_tuple(&repl_tuple, tuple, protocol)) {
>  		DEBUGP("Can't invert tuple.\n");
>  		return NULL;
>  	}
> @@ -743,7 +666,8 @@
>  
>  	IP_NF_ASSERT((skb->nh.iph->frag_off & htons(IP_OFFSET)) == 0);
>  
> -	if (!get_tuple(skb->nh.iph, skb, skb->nh.iph->ihl*4, &tuple, proto))
> +	if (!ip_ct_get_tuple(skb->nh.iph, skb, skb->nh.iph->ihl*4, 
> +				&tuple,proto))
>  		return NULL;
>  
>  	/* look for tuple match */
> @@ -828,10 +752,11 @@
>  
>  	proto = ip_ct_find_proto((*pskb)->nh.iph->protocol);
>  
> -	/* It may be an icmp error... */
> -	if ((*pskb)->nh.iph->protocol == IPPROTO_ICMP 
> -	    && icmp_error_track(*pskb, &ctinfo, hooknum))
> -		return NF_ACCEPT;
> +	/* It may be an special packet, error, unclean...
> +	 * inverse of the return code tells to the netfilter
> +	 * core what to do with the packet. */
> +	if ((ret = proto->error(*pskb, &ctinfo, hooknum)) <= 0)
> +		return -ret;
>  
>  	if (!(ct = resolve_normal_ct(*pskb, proto,&set_reply,hooknum,&ctinfo)))
>  		/* Not valid part of a connection */
> @@ -869,7 +794,8 @@
>  int invert_tuplepr(struct ip_conntrack_tuple *inverse,
>  		   const struct ip_conntrack_tuple *orig)
>  {
> -	return invert_tuple(inverse, orig, ip_ct_find_proto(orig->dst.protonum));
> +	return ip_ct_invert_tuple(inverse, orig, 
> +				  ip_ct_find_proto(orig->dst.protonum));
>  }
>  
>  static inline int resent_expect(const struct ip_conntrack_expect *i,
> diff -u -r1.1.1.1 ip_conntrack_proto_generic.c
> --- a/net/ipv4/netfilter/ip_conntrack_proto_generic.c	11 May 2004 13:07:08 -0000	1.1.1.1
> +++ b/net/ipv4/netfilter/ip_conntrack_proto_generic.c	16 Jun 2004 12:58:02 -0000
> @@ -62,8 +62,15 @@
>  	return 1;
>  }
>  
> +static int
> +error(struct sk_buff *skb, enum ip_conntrack_info *ctinfo, 
> +      unsigned int hooknum)
> +{
> +	return NF_ACCEPT;
> +}
> +
>  struct ip_conntrack_protocol ip_conntrack_generic_protocol
>  = { { NULL, NULL }, 0, "unknown",
>      generic_pkt_to_tuple, generic_invert_tuple, generic_print_tuple,
> -    generic_print_conntrack, packet, new, NULL, NULL, NULL };
> +    generic_print_conntrack, packet, new, NULL, NULL, error, NULL };
>  
> diff -u -r1.1.1.1 ip_conntrack_proto_icmp.c
> --- a/net/ipv4/netfilter/ip_conntrack_proto_icmp.c	11 May 2004 13:07:08 -0000	1.1.1.1
> +++ b/net/ipv4/netfilter/ip_conntrack_proto_icmp.c	16 Jun 2004 18:06:57 -0000
> @@ -12,6 +12,8 @@
>  #include <linux/netfilter.h>
>  #include <linux/in.h>
>  #include <linux/icmp.h>
> +#include <net/ip.h>
> +#include <linux/netfilter_ipv4/ip_conntrack.h>
>  #include <linux/netfilter_ipv4/ip_conntrack_protocol.h>
>  
>  unsigned long ip_ct_icmp_timeout = 30*HZ;
> @@ -122,7 +124,85 @@
>  	return 1;
>  }
>  
> +/* backwards compatibility: If you modify this function you should also do 
> + * the same for icmp_error_track in ip_fw_compat_masq.c */
> +static int
> +icmp_error(struct sk_buff *skb,
> +		 enum ip_conntrack_info *ctinfo,
> +		 unsigned int hooknum)
> +{
> +	struct ip_conntrack_tuple innertuple, origtuple;
> +	struct {
> +		struct icmphdr icmp;
> +		struct iphdr ip;
> +	} inside;
> +	struct ip_conntrack_protocol *innerproto;
> +	struct ip_conntrack_tuple_hash *h;
> +	int dataoff;
> +
> +	IP_NF_ASSERT(skb->nfct == NULL);
> +
> +	/* Not enough header? */
> +	if (skb_copy_bits(skb, skb->nh.iph->ihl*4, &inside, sizeof(inside))!=0)
> +		return NF_ACCEPT;
> +
> +	if (inside.icmp.type != ICMP_DEST_UNREACH
> +	    && inside.icmp.type != ICMP_SOURCE_QUENCH
> +	    && inside.icmp.type != ICMP_TIME_EXCEEDED
> +	    && inside.icmp.type != ICMP_PARAMETERPROB
> +	    && inside.icmp.type != ICMP_REDIRECT)
> +		return NF_ACCEPT;
> +
> +	/* Ignore ICMP's containing fragments (shouldn't happen) */
> +	if (inside.ip.frag_off & htons(IP_OFFSET)) {
> +		DEBUGP("icmp_error_track: fragment of proto %u\n",
> +		       inside.ip.protocol);
> +		return NF_ACCEPT;
> +	}
> +
> +	innerproto = ip_ct_find_proto(inside.ip.protocol);
> +	dataoff = skb->nh.iph->ihl*4 + sizeof(inside.icmp) + inside.ip.ihl*4;
> +	/* Are they talking about one of our connections? */
> +	if (!ip_ct_get_tuple(&inside.ip, skb, dataoff, &origtuple, innerproto)) {
> +		DEBUGP("icmp_error: ! get_tuple p=%u", inside.ip.protocol);
> +		return NF_ACCEPT;
> +	}
> +
> +	/* Ordinarily, we'd expect the inverted tupleproto, but it's
> +	   been preserved inside the ICMP. */
> +	if (!ip_ct_invert_tuple(&innertuple, &origtuple, innerproto)) {
> +		DEBUGP("icmp_error_track: Can't invert tuple\n");
> +		return NF_ACCEPT;
> +	}
> +
> +	*ctinfo = IP_CT_RELATED;
> +
> +	h = ip_conntrack_find_get(&innertuple, NULL);
> +	if (!h) {
> +		/* Locally generated ICMPs will match inverted if they
> +		   haven't been SNAT'ed yet */
> +		/* FIXME: NAT code has to handle half-done double NAT --RR */
> +		if (hooknum == NF_IP_LOCAL_OUT)
> +			h = ip_conntrack_find_get(&origtuple, NULL);
> +
> +		if (!h) {
> +			DEBUGP("icmp_error_track: no match\n");
> +			return NF_ACCEPT;
> +		}
> +		/* Reverse direction from that found */
> +		if (DIRECTION(h) != IP_CT_DIR_REPLY)
> +			*ctinfo += IP_CT_IS_REPLY;
> +	} else {
> +		if (DIRECTION(h) == IP_CT_DIR_REPLY)
> +			*ctinfo += IP_CT_IS_REPLY;
> +	}
> +
> +	/* Update skb to refer to this connection */
> +	skb->nfct = &h->ctrack->infos[*ctinfo];
> +	return -NF_ACCEPT;
> +}
> +
>  struct ip_conntrack_protocol ip_conntrack_protocol_icmp
>  = { { NULL, NULL }, IPPROTO_ICMP, "icmp",
>      icmp_pkt_to_tuple, icmp_invert_tuple, icmp_print_tuple,
> -    icmp_print_conntrack, icmp_packet, icmp_new, NULL, NULL, NULL };
> +    icmp_print_conntrack, icmp_packet, icmp_new, NULL, NULL, icmp_error, NULL};
> diff -u -r1.1.1.1 ip_conntrack_proto_tcp.c
> --- a/net/ipv4/netfilter/ip_conntrack_proto_tcp.c	11 May 2004 13:07:08 -0000	1.1.1.1
> +++ b/net/ipv4/netfilter/ip_conntrack_proto_tcp.c	16 Jun 2004 12:57:50 -0000
> @@ -266,7 +266,14 @@
>  	return between(exp->seq, ntohl(tcph.seq), ntohl(tcph.seq) + datalen);
>  }
>  
> +static int
> +tcp_error(struct sk_buff *skb, enum ip_conntrack_info *ctinfo, 
> +	   unsigned int hooknum)
> +{
> +	return NF_ACCEPT;
> +}
> +
>  struct ip_conntrack_protocol ip_conntrack_protocol_tcp
>  = { { NULL, NULL }, IPPROTO_TCP, "tcp",
>      tcp_pkt_to_tuple, tcp_invert_tuple, tcp_print_tuple, tcp_print_conntrack,
> -    tcp_packet, tcp_new, NULL, tcp_exp_matches_pkt, NULL };
> +    tcp_packet, tcp_new, NULL, tcp_exp_matches_pkt, tcp_error, NULL };
> diff -u -r1.1.1.1 ip_conntrack_proto_udp.c
> --- a/net/ipv4/netfilter/ip_conntrack_proto_udp.c	11 May 2004 13:07:08 -0000	1.1.1.1
> +++ b/net/ipv4/netfilter/ip_conntrack_proto_udp.c	16 Jun 2004 13:02:46 -0000
> @@ -80,7 +80,14 @@
>  	return 1;
>  }
>  
> +static int
> +udp_error(struct sk_buff *skb, enum ip_conntrack_info *ctinfo,
> +	  unsigned int hooknum)
> +{
> +	return NF_ACCEPT;
> +}
> +
>  struct ip_conntrack_protocol ip_conntrack_protocol_udp
>  = { { NULL, NULL }, IPPROTO_UDP, "udp",
>      udp_pkt_to_tuple, udp_invert_tuple, udp_print_tuple, udp_print_conntrack,
> -    udp_packet, udp_new, NULL, NULL, NULL };
> +    udp_packet, udp_new, NULL, NULL, udp_error, NULL };
> diff -u -r1.1.1.1 ip_fw_compat_masq.c
> --- a/net/ipv4/netfilter/ip_fw_compat_masq.c	11 May 2004 13:07:08 -0000	1.1.1.1
> +++ b/net/ipv4/netfilter/ip_fw_compat_masq.c	16 Jun 2004 13:13:57 -0000
> @@ -24,6 +24,7 @@
>  #include <linux/proc_fs.h>
>  #include <linux/module.h>
>  #include <net/route.h>
> +#include <net/ip.h>
>  
>  #define ASSERT_READ_LOCK(x) MUST_BE_READ_LOCKED(&ip_conntrack_lock)
>  #define ASSERT_WRITE_LOCK(x) MUST_BE_WRITE_LOCKED(&ip_conntrack_lock)
> @@ -41,6 +42,83 @@
>  #define DEBUGP(format, args...)
>  #endif
>  
> +/* Returns conntrack if it dealt with ICMP, and filled in skb fields */
> +struct ip_conntrack *
> +icmp_error_track(struct sk_buff *skb,
> +		 enum ip_conntrack_info *ctinfo,
> +		 unsigned int hooknum)
> +{
> +	struct ip_conntrack_tuple innertuple, origtuple;
> +	struct {
> +		struct icmphdr icmp;
> +		struct iphdr ip;
> +	} inside;
> +	struct ip_conntrack_protocol *innerproto;
> +	struct ip_conntrack_tuple_hash *h;
> +	int dataoff;
> +
> +	IP_NF_ASSERT(skb->nfct == NULL);
> +
> +	/* Not enough header? */
> +	if (skb_copy_bits(skb, skb->nh.iph->ihl*4, &inside, sizeof(inside))!=0)
> +		return NULL;
> +
> +	if (inside.icmp.type != ICMP_DEST_UNREACH
> +	    && inside.icmp.type != ICMP_SOURCE_QUENCH
> +	    && inside.icmp.type != ICMP_TIME_EXCEEDED
> +	    && inside.icmp.type != ICMP_PARAMETERPROB
> +	    && inside.icmp.type != ICMP_REDIRECT)
> +		return NULL;
> +
> +	/* Ignore ICMP's containing fragments (shouldn't happen) */
> +	if (inside.ip.frag_off & htons(IP_OFFSET)) {
> +		DEBUGP("icmp_error_track: fragment of proto %u\n",
> +		       inside.ip.protocol);
> +		return NULL;
> +	}
> +
> +	innerproto = ip_ct_find_proto(inside.ip.protocol);
> +	dataoff = skb->nh.iph->ihl*4 + sizeof(inside.icmp) + inside.ip.ihl*4;
> +	/* Are they talking about one of our connections? */
> +	if (!ip_ct_get_tuple(&inside.ip, skb, dataoff, &origtuple, innerproto)) {
> +		DEBUGP("icmp_error: ! get_tuple p=%u", inside.ip.protocol);
> +		return NULL;
> +	}
> +
> +	/* Ordinarily, we'd expect the inverted tupleproto, but it's
> +	   been preserved inside the ICMP. */
> +	if (!ip_ct_invert_tuple(&innertuple, &origtuple, innerproto)) {
> +		DEBUGP("icmp_error_track: Can't invert tuple\n");
> +		return NULL;
> +	}
> +
> +	*ctinfo = IP_CT_RELATED;
> +
> +	h = ip_conntrack_find_get(&innertuple, NULL);
> +	if (!h) {
> +		/* Locally generated ICMPs will match inverted if they
> +		   haven't been SNAT'ed yet */
> +		/* FIXME: NAT code has to handle half-done double NAT --RR */
> +		if (hooknum == NF_IP_LOCAL_OUT)
> +			h = ip_conntrack_find_get(&origtuple, NULL);
> +
> +		if (!h) {
> +			DEBUGP("icmp_error_track: no match\n");
> +			return NULL;
> +		}
> +		/* Reverse direction from that found */
> +		if (DIRECTION(h) != IP_CT_DIR_REPLY)
> +			*ctinfo += IP_CT_IS_REPLY;
> +	} else {
> +		if (DIRECTION(h) == IP_CT_DIR_REPLY)
> +			*ctinfo += IP_CT_IS_REPLY;
> +	}
> +
> +	/* Update skb to refer to this connection */
> +	skb->nfct = &h->ctrack->infos[*ctinfo];
> +	return h->ctrack;
> +}
> +
>  unsigned int
>  do_masquerade(struct sk_buff **pskb, const struct net_device *dev)
>  {
> @@ -164,7 +242,7 @@
>  	case IPPROTO_UDP:
>  		IP_NF_ASSERT(((*pskb)->nh.iph->frag_off & htons(IP_OFFSET)) == 0);
>  
> -		if (!get_tuple((*pskb)->nh.iph, *pskb, (*pskb)->nh.iph->ihl*4, &tuple, protocol)) {
> +		if (!ip_ct_get_tuple((*pskb)->nh.iph, *pskb, (*pskb)->nh.iph->ihl*4, &tuple, protocol)) {
>  			if (net_ratelimit())
>  				printk("ip_fw_compat_masq: Can't get tuple\n");
>  			return NF_ACCEPT;
> diff -u -r1.1.1.1 ip_conntrack.h
> --- a/include/linux/netfilter_ipv4/ip_conntrack.h	11 May 2004 13:35:40 -0000	1.1.1.1
> +++ b/include/linux/netfilter_ipv4/ip_conntrack.h	16 Jun 2004 13:20:51 -0000
> @@ -264,6 +264,26 @@
>  ip_ct_selective_cleanup(int (*kill)(const struct ip_conntrack *i, void *data),
>  			void *data);
>  
> +extern struct ip_conntrack_protocol *
> +ip_ct_find_proto(u_int8_t protocol);
> +
> +extern int
> +ip_ct_get_tuple(const struct iphdr *iph,
> +		const struct sk_buff *skb,
> +		unsigned int dataoff,
> +		struct ip_conntrack_tuple *tuple,
> +		const struct ip_conntrack_protocol *protocol);
> +
> +extern int
> +ip_ct_invert_tuple(struct ip_conntrack_tuple *inverse,
> +		   const struct ip_conntrack_tuple *orig,
> +		   const struct ip_conntrack_protocol *protocol);
> +
> +/* Find a connection corresponding to a tuple. */
> +extern struct ip_conntrack_tuple_hash *
> +ip_conntrack_find_get(const struct ip_conntrack_tuple *tuple,
> +		      const struct ip_conntrack *ignored_conntrack);
> +
>  /* It's confirmed if it is, or has been in the hash table. */
>  static inline int is_confirmed(struct ip_conntrack *ct)
>  {
> diff -u -r1.1.1.1 ip_conntrack_protocol.h
> --- a/include/linux/netfilter_ipv4/ip_conntrack_protocol.h	11 May 2004 13:35:39 -0000	1.1.1.1
> +++ b/include/linux/netfilter_ipv4/ip_conntrack_protocol.h	15 Jun 2004 18:00:54 -0000
> @@ -50,6 +50,9 @@
>  	int (*exp_matches_pkt)(struct ip_conntrack_expect *exp,
>  			       const struct sk_buff *skb);
>  
> +	int (*error)(struct sk_buff *skb, enum ip_conntrack_info *ctinfo,
> +		     unsigned int hooknum);
> +
>  	/* Module (if any) which this is connected to. */
>  	struct module *me;
>  };

Re: [PATCH] modification in current protocol helper API to handle error/unclean packets

[Jozsef Kadlecsik]

Hi Pablo,

On Thu, 17 Jun 2004, Pablo Neira wrote:

> - Jozsef, I resolve to use your inverse return value but I must confess
> that I'm still thinking about another way to do that with no sucess,
> well this issue is not important. BTW, you also modified conntrack core
> in your tcp-window-tracking patch, do you think that I could add that
> part to my patch since I think that it's more related stuff and your
> patch will go on top of this?

No, I will undo those conntrack core changes in tcp-window-tracking patch
as soon as I can build on your patch.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlec@sunserv.kfki.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : KFKI Research Institute for Particle and Nuclear Physics
          H-1525 Budapest 114, POB. 49, Hungary

Re: [PATCH] modification in current protocol helper API to handle error/unclean packets

[Pablo Neira]

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

Hi Patrick,

Patrick McHardy wrote:

> What about this:
>
> ===== net/ipv4/netfilter/ip_fw_compat_masq.c 1.14 vs edited =====
> --- 1.14/net/ipv4/netfilter/ip_fw_compat_masq.c    2004-03-23 02:17:04 
> +01:00
> +++ edited/net/ipv4/netfilter/ip_fw_compat_masq.c    2004-06-17 
> 14:33:49 +02:00
> @@ -143,7 +143,9 @@
>      switch ((*pskb)->nh.iph->protocol) {
>      case IPPROTO_ICMP:
>          /* ICMP errors. */
> -        ct = icmp_error_track(*pskb, &ctinfo, NF_IP_PRE_ROUTING);
> +        ip_ct_find_proto(IPPROTO_ICMP)->error(*pskb, &ctinfo,
> +                                              NF_IP_PRE_ROUTING);
> +        ct = (struct ip_conntrack *)skb->nfct->master;


sure! I like it. I added it to this new patch.

> You could make transition for existing protocols easier by makeing the
> error function optional:
>
> if (proto->error != NULL && ret = proto->error(....))


I also added that. If any last issue, please let me know.

regards,
Pablo

[-- Attachment #2: protocol_helper-error.patch --]
[-- Type: text/x-patch, Size: 12961 bytes --]

diff -u -r1.1.1.1 ip_conntrack_core.c
--- a/net/ipv4/netfilter/ip_conntrack_core.c	11 May 2004 13:07:08 -0000	1.1.1.1
+++ b/net/ipv4/netfilter/ip_conntrack_core.c	17 Jun 2004 13:39:21 -0000
@@ -126,11 +126,11 @@
 }
 
 int
-get_tuple(const struct iphdr *iph,
-	  const struct sk_buff *skb,
-	  unsigned int dataoff,
-	  struct ip_conntrack_tuple *tuple,
-	  const struct ip_conntrack_protocol *protocol)
+ip_ct_get_tuple(const struct iphdr *iph,
+		const struct sk_buff *skb,
+		unsigned int dataoff,
+		struct ip_conntrack_tuple *tuple,
+		const struct ip_conntrack_protocol *protocol)
 {
 	/* Never happen */
 	if (iph->frag_off & htons(IP_OFFSET)) {
@@ -146,10 +146,10 @@
 	return protocol->pkt_to_tuple(skb, dataoff, tuple);
 }
 
-static int
-invert_tuple(struct ip_conntrack_tuple *inverse,
-	     const struct ip_conntrack_tuple *orig,
-	     const struct ip_conntrack_protocol *protocol)
+int
+ip_ct_invert_tuple(struct ip_conntrack_tuple *inverse,
+		   const struct ip_conntrack_tuple *orig,
+		   const struct ip_conntrack_protocol *protocol)
 {
 	inverse->src.ip = orig->dst.ip;
 	inverse->dst.ip = orig->src.ip;
@@ -495,83 +495,6 @@
 	return h != NULL;
 }
 
-/* Returns conntrack if it dealt with ICMP, and filled in skb fields */
-struct ip_conntrack *
-icmp_error_track(struct sk_buff *skb,
-		 enum ip_conntrack_info *ctinfo,
-		 unsigned int hooknum)
-{
-	struct ip_conntrack_tuple innertuple, origtuple;
-	struct {
-		struct icmphdr icmp;
-		struct iphdr ip;
-	} inside;
-	struct ip_conntrack_protocol *innerproto;
-	struct ip_conntrack_tuple_hash *h;
-	int dataoff;
-
-	IP_NF_ASSERT(skb->nfct == NULL);
-
-	/* Not enough header? */
-	if (skb_copy_bits(skb, skb->nh.iph->ihl*4, &inside, sizeof(inside))!=0)
-		return NULL;
-
-	if (inside.icmp.type != ICMP_DEST_UNREACH
-	    && inside.icmp.type != ICMP_SOURCE_QUENCH
-	    && inside.icmp.type != ICMP_TIME_EXCEEDED
-	    && inside.icmp.type != ICMP_PARAMETERPROB
-	    && inside.icmp.type != ICMP_REDIRECT)
-		return NULL;
-
-	/* Ignore ICMP's containing fragments (shouldn't happen) */
-	if (inside.ip.frag_off & htons(IP_OFFSET)) {
-		DEBUGP("icmp_error_track: fragment of proto %u\n",
-		       inside.ip.protocol);
-		return NULL;
-	}
-
-	innerproto = ip_ct_find_proto(inside.ip.protocol);
-	dataoff = skb->nh.iph->ihl*4 + sizeof(inside.icmp) + inside.ip.ihl*4;
-	/* Are they talking about one of our connections? */
-	if (!get_tuple(&inside.ip, skb, dataoff, &origtuple, innerproto)) {
-		DEBUGP("icmp_error: ! get_tuple p=%u", inside.ip.protocol);
-		return NULL;
-	}
-
-	/* Ordinarily, we'd expect the inverted tupleproto, but it's
-	   been preserved inside the ICMP. */
-	if (!invert_tuple(&innertuple, &origtuple, innerproto)) {
-		DEBUGP("icmp_error_track: Can't invert tuple\n");
-		return NULL;
-	}
-
-	*ctinfo = IP_CT_RELATED;
-
-	h = ip_conntrack_find_get(&innertuple, NULL);
-	if (!h) {
-		/* Locally generated ICMPs will match inverted if they
-		   haven't been SNAT'ed yet */
-		/* FIXME: NAT code has to handle half-done double NAT --RR */
-		if (hooknum == NF_IP_LOCAL_OUT)
-			h = ip_conntrack_find_get(&origtuple, NULL);
-
-		if (!h) {
-			DEBUGP("icmp_error_track: no match\n");
-			return NULL;
-		}
-		/* Reverse direction from that found */
-		if (DIRECTION(h) != IP_CT_DIR_REPLY)
-			*ctinfo += IP_CT_IS_REPLY;
-	} else {
-		if (DIRECTION(h) == IP_CT_DIR_REPLY)
-			*ctinfo += IP_CT_IS_REPLY;
-	}
-
-	/* Update skb to refer to this connection */
-	skb->nfct = &h->ctrack->infos[*ctinfo];
-	return h->ctrack;
-}
-
 /* There's a small race here where we may free a just-assured
    connection.  Too bad: we're in trouble anyway. */
 static inline int unreplied(const struct ip_conntrack_tuple_hash *i)
@@ -653,7 +576,7 @@
 		}
 	}
 
-	if (!invert_tuple(&repl_tuple, tuple, protocol)) {
+	if (!ip_ct_invert_tuple(&repl_tuple, tuple, protocol)) {
 		DEBUGP("Can't invert tuple.\n");
 		return NULL;
 	}
@@ -743,7 +666,8 @@
 
 	IP_NF_ASSERT((skb->nh.iph->frag_off & htons(IP_OFFSET)) == 0);
 
-	if (!get_tuple(skb->nh.iph, skb, skb->nh.iph->ihl*4, &tuple, proto))
+	if (!ip_ct_get_tuple(skb->nh.iph, skb, skb->nh.iph->ihl*4, 
+				&tuple,proto))
 		return NULL;
 
 	/* look for tuple match */
@@ -828,10 +752,12 @@
 
 	proto = ip_ct_find_proto((*pskb)->nh.iph->protocol);
 
-	/* It may be an icmp error... */
-	if ((*pskb)->nh.iph->protocol == IPPROTO_ICMP 
-	    && icmp_error_track(*pskb, &ctinfo, hooknum))
-		return NF_ACCEPT;
+	/* It may be an special packet, error, unclean...
+	 * inverse of the return code tells to the netfilter
+	 * core what to do with the packet. */
+	if (proto->error != NULL 
+	    && (ret = proto->error(*pskb, &ctinfo, hooknum)) <= 0)
+		return -ret;
 
 	if (!(ct = resolve_normal_ct(*pskb, proto,&set_reply,hooknum,&ctinfo)))
 		/* Not valid part of a connection */
@@ -869,7 +795,8 @@
 int invert_tuplepr(struct ip_conntrack_tuple *inverse,
 		   const struct ip_conntrack_tuple *orig)
 {
-	return invert_tuple(inverse, orig, ip_ct_find_proto(orig->dst.protonum));
+	return ip_ct_invert_tuple(inverse, orig, 
+				  ip_ct_find_proto(orig->dst.protonum));
 }
 
 static inline int resent_expect(const struct ip_conntrack_expect *i,
diff -u -r1.1.1.1 ip_conntrack_proto_generic.c
--- a/net/ipv4/netfilter/ip_conntrack_proto_generic.c	11 May 2004 13:07:08 -0000	1.1.1.1
+++ b/net/ipv4/netfilter/ip_conntrack_proto_generic.c	17 Jun 2004 13:19:48 -0000
@@ -65,5 +65,5 @@
 struct ip_conntrack_protocol ip_conntrack_generic_protocol
 = { { NULL, NULL }, 0, "unknown",
     generic_pkt_to_tuple, generic_invert_tuple, generic_print_tuple,
-    generic_print_conntrack, packet, new, NULL, NULL, NULL };
+    generic_print_conntrack, packet, new, NULL, NULL, NULL, NULL };
 
diff -u -r1.1.1.1 ip_conntrack_proto_icmp.c
--- a/net/ipv4/netfilter/ip_conntrack_proto_icmp.c	11 May 2004 13:07:08 -0000	1.1.1.1
+++ b/net/ipv4/netfilter/ip_conntrack_proto_icmp.c	17 Jun 2004 13:22:23 -0000
@@ -12,6 +12,8 @@
 #include <linux/netfilter.h>
 #include <linux/in.h>
 #include <linux/icmp.h>
+#include <net/ip.h>
+#include <linux/netfilter_ipv4/ip_conntrack.h>
 #include <linux/netfilter_ipv4/ip_conntrack_protocol.h>
 
 unsigned long ip_ct_icmp_timeout = 30*HZ;
@@ -122,7 +124,83 @@
 	return 1;
 }
 
+static int
+icmp_error(struct sk_buff *skb,
+		 enum ip_conntrack_info *ctinfo,
+		 unsigned int hooknum)
+{
+	struct ip_conntrack_tuple innertuple, origtuple;
+	struct {
+		struct icmphdr icmp;
+		struct iphdr ip;
+	} inside;
+	struct ip_conntrack_protocol *innerproto;
+	struct ip_conntrack_tuple_hash *h;
+	int dataoff;
+
+	IP_NF_ASSERT(skb->nfct == NULL);
+
+	/* Not enough header? */
+	if (skb_copy_bits(skb, skb->nh.iph->ihl*4, &inside, sizeof(inside))!=0)
+		return NF_ACCEPT;
+
+	if (inside.icmp.type != ICMP_DEST_UNREACH
+	    && inside.icmp.type != ICMP_SOURCE_QUENCH
+	    && inside.icmp.type != ICMP_TIME_EXCEEDED
+	    && inside.icmp.type != ICMP_PARAMETERPROB
+	    && inside.icmp.type != ICMP_REDIRECT)
+		return NF_ACCEPT;
+
+	/* Ignore ICMP's containing fragments (shouldn't happen) */
+	if (inside.ip.frag_off & htons(IP_OFFSET)) {
+		DEBUGP("icmp_error_track: fragment of proto %u\n",
+		       inside.ip.protocol);
+		return NF_ACCEPT;
+	}
+
+	innerproto = ip_ct_find_proto(inside.ip.protocol);
+	dataoff = skb->nh.iph->ihl*4 + sizeof(inside.icmp) + inside.ip.ihl*4;
+	/* Are they talking about one of our connections? */
+	if (!ip_ct_get_tuple(&inside.ip, skb, dataoff, &origtuple, innerproto)) {
+		DEBUGP("icmp_error: ! get_tuple p=%u", inside.ip.protocol);
+		return NF_ACCEPT;
+	}
+
+	/* Ordinarily, we'd expect the inverted tupleproto, but it's
+	   been preserved inside the ICMP. */
+	if (!ip_ct_invert_tuple(&innertuple, &origtuple, innerproto)) {
+		DEBUGP("icmp_error_track: Can't invert tuple\n");
+		return NF_ACCEPT;
+	}
+
+	*ctinfo = IP_CT_RELATED;
+
+	h = ip_conntrack_find_get(&innertuple, NULL);
+	if (!h) {
+		/* Locally generated ICMPs will match inverted if they
+		   haven't been SNAT'ed yet */
+		/* FIXME: NAT code has to handle half-done double NAT --RR */
+		if (hooknum == NF_IP_LOCAL_OUT)
+			h = ip_conntrack_find_get(&origtuple, NULL);
+
+		if (!h) {
+			DEBUGP("icmp_error_track: no match\n");
+			return NF_ACCEPT;
+		}
+		/* Reverse direction from that found */
+		if (DIRECTION(h) != IP_CT_DIR_REPLY)
+			*ctinfo += IP_CT_IS_REPLY;
+	} else {
+		if (DIRECTION(h) == IP_CT_DIR_REPLY)
+			*ctinfo += IP_CT_IS_REPLY;
+	}
+
+	/* Update skb to refer to this connection */
+	skb->nfct = &h->ctrack->infos[*ctinfo];
+	return -NF_ACCEPT;
+}
+
 struct ip_conntrack_protocol ip_conntrack_protocol_icmp
 = { { NULL, NULL }, IPPROTO_ICMP, "icmp",
     icmp_pkt_to_tuple, icmp_invert_tuple, icmp_print_tuple,
-    icmp_print_conntrack, icmp_packet, icmp_new, NULL, NULL, NULL };
+    icmp_print_conntrack, icmp_packet, icmp_new, NULL, NULL, icmp_error, NULL};
diff -u -r1.1.1.1 ip_conntrack_proto_tcp.c
--- a/net/ipv4/netfilter/ip_conntrack_proto_tcp.c	11 May 2004 13:07:08 -0000	1.1.1.1
+++ b/net/ipv4/netfilter/ip_conntrack_proto_tcp.c	17 Jun 2004 13:19:20 -0000
@@ -269,4 +269,4 @@
 struct ip_conntrack_protocol ip_conntrack_protocol_tcp
 = { { NULL, NULL }, IPPROTO_TCP, "tcp",
     tcp_pkt_to_tuple, tcp_invert_tuple, tcp_print_tuple, tcp_print_conntrack,
-    tcp_packet, tcp_new, NULL, tcp_exp_matches_pkt, NULL };
+    tcp_packet, tcp_new, NULL, tcp_exp_matches_pkt, NULL, NULL };
diff -u -r1.1.1.1 ip_conntrack_proto_udp.c
--- a/net/ipv4/netfilter/ip_conntrack_proto_udp.c	11 May 2004 13:07:08 -0000	1.1.1.1
+++ b/net/ipv4/netfilter/ip_conntrack_proto_udp.c	17 Jun 2004 13:19:36 -0000
@@ -83,4 +83,4 @@
 struct ip_conntrack_protocol ip_conntrack_protocol_udp
 = { { NULL, NULL }, IPPROTO_UDP, "udp",
     udp_pkt_to_tuple, udp_invert_tuple, udp_print_tuple, udp_print_conntrack,
-    udp_packet, udp_new, NULL, NULL, NULL };
+    udp_packet, udp_new, NULL, NULL, NULL, NULL };
diff -u -r1.1.1.1 ip_fw_compat_masq.c
--- a/net/ipv4/netfilter/ip_fw_compat_masq.c	11 May 2004 13:07:08 -0000	1.1.1.1
+++ b/net/ipv4/netfilter/ip_fw_compat_masq.c	17 Jun 2004 13:18:41 -0000
@@ -24,12 +24,14 @@
 #include <linux/proc_fs.h>
 #include <linux/module.h>
 #include <net/route.h>
+#include <net/ip.h>
 
 #define ASSERT_READ_LOCK(x) MUST_BE_READ_LOCKED(&ip_conntrack_lock)
 #define ASSERT_WRITE_LOCK(x) MUST_BE_WRITE_LOCKED(&ip_conntrack_lock)
 
 #include <linux/netfilter_ipv4/ip_conntrack.h>
 #include <linux/netfilter_ipv4/ip_conntrack_core.h>
+#include <linux/netfilter_ipv4/ip_conntrack_protocol.h>
 #include <linux/netfilter_ipv4/ip_nat.h>
 #include <linux/netfilter_ipv4/ip_nat_core.h>
 #include <linux/netfilter_ipv4/listhelp.h>
@@ -143,7 +145,8 @@
 	switch ((*pskb)->nh.iph->protocol) {
 	case IPPROTO_ICMP:
 		/* ICMP errors. */
-		ct = icmp_error_track(*pskb, &ctinfo, NF_IP_PRE_ROUTING);
+		protocol->error(*pskb, &ctinfo, NF_IP_PRE_ROUTING);
+		ct = (struct ip_conntrack *)(*pskb)->nfct->master;
 		if (ct) {
 			/* We only do SNAT in the compatibility layer.
 			   So we can manipulate ICMP errors from
@@ -164,7 +167,7 @@
 	case IPPROTO_UDP:
 		IP_NF_ASSERT(((*pskb)->nh.iph->frag_off & htons(IP_OFFSET)) == 0);
 
-		if (!get_tuple((*pskb)->nh.iph, *pskb, (*pskb)->nh.iph->ihl*4, &tuple, protocol)) {
+		if (!ip_ct_get_tuple((*pskb)->nh.iph, *pskb, (*pskb)->nh.iph->ihl*4, &tuple, protocol)) {
 			if (net_ratelimit())
 				printk("ip_fw_compat_masq: Can't get tuple\n");
 			return NF_ACCEPT;
diff -u -r1.1.1.1 ip_conntrack.h
--- a/include/linux/netfilter_ipv4/ip_conntrack.h	11 May 2004 13:35:40 -0000	1.1.1.1
+++ b/include/linux/netfilter_ipv4/ip_conntrack.h	16 Jun 2004 13:20:51 -0000
@@ -264,6 +264,26 @@
 ip_ct_selective_cleanup(int (*kill)(const struct ip_conntrack *i, void *data),
 			void *data);
 
+extern struct ip_conntrack_protocol *
+ip_ct_find_proto(u_int8_t protocol);
+
+extern int
+ip_ct_get_tuple(const struct iphdr *iph,
+		const struct sk_buff *skb,
+		unsigned int dataoff,
+		struct ip_conntrack_tuple *tuple,
+		const struct ip_conntrack_protocol *protocol);
+
+extern int
+ip_ct_invert_tuple(struct ip_conntrack_tuple *inverse,
+		   const struct ip_conntrack_tuple *orig,
+		   const struct ip_conntrack_protocol *protocol);
+
+/* Find a connection corresponding to a tuple. */
+extern struct ip_conntrack_tuple_hash *
+ip_conntrack_find_get(const struct ip_conntrack_tuple *tuple,
+		      const struct ip_conntrack *ignored_conntrack);
+
 /* It's confirmed if it is, or has been in the hash table. */
 static inline int is_confirmed(struct ip_conntrack *ct)
 {
diff -u -r1.1.1.1 ip_conntrack_protocol.h
--- a/include/linux/netfilter_ipv4/ip_conntrack_protocol.h	11 May 2004 13:35:39 -0000	1.1.1.1
+++ b/include/linux/netfilter_ipv4/ip_conntrack_protocol.h	15 Jun 2004 18:00:54 -0000
@@ -50,6 +50,9 @@
 	int (*exp_matches_pkt)(struct ip_conntrack_expect *exp,
 			       const struct sk_buff *skb);
 
+	int (*error)(struct sk_buff *skb, enum ip_conntrack_info *ctinfo,
+		     unsigned int hooknum);
+
 	/* Module (if any) which this is connected to. */
 	struct module *me;
 };

Re: [PATCH] modification in current protocol helper API to handle error/unclean packets

[Patrick McHardy]

Pablo Neira wrote:
> I also added that. If any last issue, please let me know.

Looks good, thanks. I'm leaving it to Jozsef to add it in case he has
any last issues.

Regards
Patrick

Re: [PATCH] modification in current protocol helper API to handle error/unclean packets

[Pablo Neira]

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

Hi,

Attached an extra patch which apply on top of my current 
protocol-helper-error patch. It performs some checking for invalid UDP 
and ICMP packets. Am I missing anything?

BTW, It also adds C99 initialization. I also modified the structure of 
error_icmp to handle in a different way special icmp messages.

Jozsef, should we define a variable called ip_ct_log_invalid for all the 
protocols or one per protocol? I mean:

+int ip_ct_icmp_log_invalid = 1;

and we should also put this definition in somewhere since other protocol 
helpers could use net_ratelimit.

+#if 0
+#define NET_RATELIMIT(foo)     (foo)
+#else
+#define NET_RATELIMIT(foo)     ((foo) && net_ratelimit())
+#endif

regards,
Pablo

[-- Attachment #2: unclean-icmp-udp-packets.patch --]
[-- Type: text/x-patch, Size: 6086 bytes --]

diff -u -r1.2 ip_conntrack_proto_icmp.c
--- a/net/ipv4/netfilter/ip_conntrack_proto_icmp.c	17 Jun 2004 13:54:41 -0000	1.2
+++ b/net/ipv4/netfilter/ip_conntrack_proto_icmp.c	17 Jun 2004 17:08:39 -0000
@@ -13,11 +13,20 @@
 #include <linux/in.h>
 #include <linux/icmp.h>
 #include <net/ip.h>
+#include <asm/checksum.h>
 #include <linux/netfilter_ipv4/ip_conntrack.h>
 #include <linux/netfilter_ipv4/ip_conntrack_protocol.h>
 
 unsigned long ip_ct_icmp_timeout = 30*HZ;
 
+/* FIXME: unify with Jozsef's tcp window tracking patch? */
+int ip_ct_icmp_log_invalid = 1;
+#if 0
+#define NET_RATELIMIT(foo)     (foo)
+#else
+#define NET_RATELIMIT(foo)     ((foo) && net_ratelimit())
+#endif
+
 #if 0
 #define DEBUGP printk
 #else
@@ -125,9 +134,8 @@
 }
 
 static int
-icmp_error(struct sk_buff *skb,
-		 enum ip_conntrack_info *ctinfo,
-		 unsigned int hooknum)
+track_error_message(struct sk_buff *skb, enum ip_conntrack_info *ctinfo,
+		    unsigned int hooknum)
 {
 	struct ip_conntrack_tuple innertuple, origtuple;
 	struct {
@@ -144,13 +152,6 @@
 	if (skb_copy_bits(skb, skb->nh.iph->ihl*4, &inside, sizeof(inside))!=0)
 		return NF_ACCEPT;
 
-	if (inside.icmp.type != ICMP_DEST_UNREACH
-	    && inside.icmp.type != ICMP_SOURCE_QUENCH
-	    && inside.icmp.type != ICMP_TIME_EXCEEDED
-	    && inside.icmp.type != ICMP_PARAMETERPROB
-	    && inside.icmp.type != ICMP_REDIRECT)
-		return NF_ACCEPT;
-
 	/* Ignore ICMP's containing fragments (shouldn't happen) */
 	if (inside.ip.frag_off & htons(IP_OFFSET)) {
 		DEBUGP("icmp_error_track: fragment of proto %u\n",
@@ -200,7 +201,64 @@
 	return -NF_ACCEPT;
 }
 
-struct ip_conntrack_protocol ip_conntrack_protocol_icmp
-= { { NULL, NULL }, IPPROTO_ICMP, "icmp",
-    icmp_pkt_to_tuple, icmp_invert_tuple, icmp_print_tuple,
-    icmp_print_conntrack, icmp_packet, icmp_new, NULL, NULL, icmp_error, NULL};
+/* Small and modified version of icmp_rcv */
+static int
+icmp_error(struct sk_buff *skb, enum ip_conntrack_info *ctinfo,
+	   unsigned int hooknum)
+{
+	struct icmphdr *icmph = skb->h.icmph;
+
+	/* ICMP header is too small! */
+	if (skb->len < 4)
+		return -NF_ACCEPT;
+	
+	switch (skb->ip_summed) {
+	case CHECKSUM_HW:
+		if (!(u16)csum_fold(skb->csum)) 
+			break;
+		if (NET_RATELIMIT(ip_ct_icmp_log_invalid))
+			nf_log_packet(PF_INET, 0, skb, NULL, NULL, 
+				      "ip_ct_icmp: bad HW ICMP checksum ");
+	case CHECKSUM_NONE:
+		if ((u16)csum_fold(skb_checksum(skb, 0, skb->len, 0))) {
+			if (NET_RATELIMIT(ip_ct_icmp_log_invalid)) {
+				nf_log_packet(PF_INET, 0, skb, NULL, NULL, 
+					      "ip_ct_udp: bad ICMP checksum ");
+			}
+			return -NF_ACCEPT;
+		}
+	default:;
+	}
+
+	/*
+	 *	18 is the highest 'known' ICMP type. Anything else is a mystery
+	 *
+	 *	RFC 1122: 3.2.2  Unknown ICMP messages types MUST be silently
+	 *		  discarded.
+	 */
+	if (icmph->type > NR_ICMP_TYPES)
+		return -NF_ACCEPT;
+
+	/* Need to track icmp error message? */
+	if (icmph->type != ICMP_DEST_UNREACH
+	    && icmph->type != ICMP_SOURCE_QUENCH
+	    && icmph->type != ICMP_TIME_EXCEEDED
+	    && icmph->type != ICMP_PARAMETERPROB
+	    && icmph->type != ICMP_REDIRECT)
+		return NF_ACCEPT;
+
+	return(track_error_message(skb, ctinfo, hooknum));
+}
+
+struct ip_conntrack_protocol ip_conntrack_protocol_icmp =
+{
+	.proto 			= IPPROTO_ICMP, 
+	.name 			= "icmp",
+	.pkt_to_tuple 		= icmp_pkt_to_tuple,
+	.invert_tuple		= icmp_invert_tuple,
+	.print_tuple 		= icmp_print_tuple,
+	.print_conntrack 	= icmp_print_conntrack,
+	.packet 		= icmp_packet,
+	.new 			= icmp_new,
+	.error 			= icmp_error,
+};
diff -u -r1.2 ip_conntrack_proto_udp.c
--- a/net/ipv4/netfilter/ip_conntrack_proto_udp.c	17 Jun 2004 13:54:41 -0000	1.2
+++ b/net/ipv4/netfilter/ip_conntrack_proto_udp.c	17 Jun 2004 17:22:18 -0000
@@ -12,11 +12,20 @@
 #include <linux/netfilter.h>
 #include <linux/in.h>
 #include <linux/udp.h>
+#include <asm/checksum.h>
 #include <linux/netfilter_ipv4/ip_conntrack_protocol.h>
 
 unsigned long ip_ct_udp_timeout = 30*HZ;
 unsigned long ip_ct_udp_timeout_stream = 180*HZ;
 
+/* FIXME: unify with Jozsef's tcp window tracking patch? */
+int ip_ct_udp_log_invalid = 1;
+#if 0
+#define NET_RATELIMIT(foo)     (foo)
+#else
+#define NET_RATELIMIT(foo)     ((foo) && net_ratelimit())
+#endif
+
 static int udp_pkt_to_tuple(const struct sk_buff *skb,
 			     unsigned int dataoff,
 			     struct ip_conntrack_tuple *tuple)
@@ -80,7 +89,52 @@
 	return 1;
 }
 
-struct ip_conntrack_protocol ip_conntrack_protocol_udp
-= { { NULL, NULL }, IPPROTO_UDP, "udp",
-    udp_pkt_to_tuple, udp_invert_tuple, udp_print_tuple, udp_print_conntrack,
-    udp_packet, udp_new, NULL, NULL, NULL, NULL };
+static int udp_error(struct sk_buff *skb, enum ip_conntrack_info *ctinfo,
+		     unsigned int hooknum)
+{
+	struct iphdr *iph = skb->nh.iph;
+	unsigned int udplen = skb->len - iph->ihl * 4;
+	struct udphdr hdr;
+
+	/* Header is too small? */
+	if (skb_copy_bits(skb, skb->nh.iph->ihl*4, &hdr, 8) != 0)
+		return -NF_ACCEPT;
+
+	/* Length field must be >= 8 */
+	if (hdr.len < 8)
+		return -NF_ACCEPT;
+	
+	/* Packet with no checksum */
+	if (!hdr.check)
+		return NF_ACCEPT;
+
+	/* Checksum invalid? Ignore.
+	 * We skip checking packets on the outgoing path
+	 * because the semantic of CHECKSUM_HW is different there 
+	 * and moreover root might send raw packets.
+	 * FIXME: Source route IP option packets --RR */
+	if (hooknum == NF_IP_PRE_ROUTING
+	    && csum_tcpudp_magic(iph->saddr, iph->daddr, udplen, IPPROTO_UDP,
+			         skb->ip_summed == CHECKSUM_HW ? skb->csum
+			      	 : skb_checksum(skb, iph->ihl*4, udplen, 0))) {
+		if (NET_RATELIMIT(ip_ct_udp_log_invalid))
+			nf_log_packet(PF_INET, 0, skb, NULL, NULL, 
+				  "ip_ct_udp: bad UDP checksum ");
+		return -NF_ACCEPT;
+	}
+	
+	return NF_ACCEPT;
+}
+
+struct ip_conntrack_protocol ip_conntrack_protocol_udp =
+{
+	.proto 			= IPPROTO_UDP,
+	.name			= "udp",
+	.pkt_to_tuple		= udp_pkt_to_tuple,
+	.invert_tuple		= udp_invert_tuple,
+	.print_tuple		= udp_print_tuple,
+	.print_conntrack	= udp_print_conntrack,
+	.packet			= udp_packet,
+	.new			= udp_new,
+	.error			= udp_error,
+};

Re: [PATCH] modification in current protocol helper API to handle error/unclean packets

[Martin Josefsson]

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

On Thu, 2004-06-17 at 14:09, Pablo Neira wrote:

[snip]

> - Jozsef, I resolve to use your inverse return value but I must confess 
> that I'm still thinking about another way to do that with no sucess, 

What's the deal with these inverse return values in tcp-window-tracking
and now conntrack_error-api?

Sometimes we return NF_ACCEPT and sometimes we return -NF_ACCEPT which
have two completely diffrent meanings. That's really really ugly.

As I've understod it we have two cases:
1. We want to return NF_* all the way back to the netfilter core.
2. We want conntrack to continue processing the packet.

And we need a way to distinguish these returnvalues.

My suggestion is that when we want NF_* we return NF_* and if we want
conntrack to continue we return CONNTRACK_CONT which is defined as -1
All NF_* macros are >= 0

Instead of checking for a negative returnvalue as we do today we check
!= CONNTRACK_CONT and then return the value previously returned to us.

In my oppinion that would greatly improve the readability of the code.
The mixing of negative and positive values using the same macro 
confuses me sometimes to say the least. I bet I'm not the only one that
sometimes has to look twice at the return value.

Have I missed something?
If not and noone objects I'll fix it up in cvs unless someone beats me
to it.

> well this issue is not important. BTW, you also modified conntrack core 
> in your tcp-window-tracking patch, do you think that I could add that 
> part to my patch since I think that it's more related stuff and your 
> patch will go on top of this?
> - I'll work on that extra patch to check for malformed udp/icmp packets 
> these days.

I fixed the length checking in the udp part of the second patch.
It compared lengths in network byteorder, added two missing ntohs()
Now udp works again :)

-- 
/Martin

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] modification in current protocol helper API to handle error/unclean packets

[Pablo Neira]

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

Hi Martin,

Martin Josefsson wrote:

>>- Jozsef, I resolve to use your inverse return value but I must confess 
>>that I'm still thinking about another way to do that with no sucess, 
>>    
>>
>
>Instead of checking for a negative returnvalue as we do today we check
>!= CONNTRACK_CONT and then return the value previously returned to us.
>
>In my oppinion that would greatly improve the readability of the code.
>The mixing of negative and positive values using the same macro 
>confuses me sometimes to say the least. I bet I'm not the only one that
>sometimes has to look twice at the return value.
>  
>

sure, I like it. If Jozsef don't tell us that we are missing something, 
I will be fine with that.

>>well this issue is not important. BTW, you also modified conntrack core 
>>in your tcp-window-tracking patch, do you think that I could add that 
>>part to my patch since I think that it's more related stuff and your 
>>patch will go on top of this?
>>- I'll work on that extra patch to check for malformed udp/icmp packets 
>>these days.
>>    
>>
>
>I fixed the length checking in the udp part of the second patch.
>It compared lengths in network byteorder, added two missing ntohs()
>Now udp works again :)
>  
>

thanks for that Martin.

Would you be willing to apply the patch attached to pom-ng? It applies 
to linux-2.6.patch_02-udp-icmp and it adds more checkings for invalid 
icmp combinations.

regards,
Pablo

[-- Attachment #2: linux-2.6.patch_02-udp-icmp-CVS.patch --]
[-- Type: text/x-patch, Size: 5329 bytes --]

--- linux-2.6.patch_02-udp-icmp	2004-06-19 18:45:11.000000000 +0200
+++ patch	2004-06-20 23:40:08.000000000 +0200
@@ -1,6 +1,6 @@
-diff -urN --exclude-from=/usr/src/diff.exclude linux-2.6.6-ct_error_api/net/ipv4/netfilter/ip_conntrack_proto_icmp.c linux-2.6.6-ct_error_api-udp/net/ipv4/netfilter/ip_conntrack_proto_icmp.c
---- linux-2.6.6-ct_error_api/net/ipv4/netfilter/ip_conntrack_proto_icmp.c	2004-06-18 08:13:51.000000000 +0200
-+++ linux-2.6.6-ct_error_api-udp/net/ipv4/netfilter/ip_conntrack_proto_icmp.c	2004-06-18 15:14:01.000000000 +0200
+diff -u -r1.4 ip_conntrack_proto_icmp.c
+--- a/net/ipv4/netfilter/ip_conntrack_proto_icmp.c	20 Jun 2004 21:22:07 -0000	1.4
++++ b/net/ipv4/netfilter/ip_conntrack_proto_icmp.c	20 Jun 2004 21:39:49 -0000
 @@ -13,6 +13,8 @@
  #include <linux/in.h>
  #include <linux/icmp.h>
@@ -10,7 +10,73 @@
  #include <linux/netfilter_ipv4/ip_conntrack.h>
  #include <linux/netfilter_ipv4/ip_conntrack_core.h>
  #include <linux/netfilter_ipv4/ip_conntrack_protocol.h>
-@@ -126,9 +128,9 @@
+@@ -25,6 +27,65 @@
+ #define DEBUGP(format, args...)
+ #endif
+ 
++#define OK 1
++#define IV 0
++
++/* ICMP types and codes described in RFC's:
++ * 	- 792: Internet Control Message Protocol. Most types.
++ * 	- 1256: ICMP Router Discovery Messages. Types 9 and 10.
++ * 	- 950: Internet Standard Subnetting Procedure. Types 17 and 18.
++ * 	- 1812: Requirements for IP Version 4 Routers. Type 3. Section 5.2.7.1
++ */
++static unsigned char icmp_valid[19][16] = 
++{
++       /* 0  1  2  3  4  5  6  7  8  9 10 11 12 13 14 15*/
++/* 0 */ {OK,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV}, /* echo reply */
++	
++	/* Type 1,2 don't exist */
++/* 1 */	{IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV},
++/* 2 */	{IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV},
++
++	/* destination host unreachable */
++/* 3 */ {OK,OK,OK,OK,OK,OK,OK,OK,OK,OK,OK,OK,OK,OK,OK,OK},
++
++	/* source quench */
++/* 4 */ {OK,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV},
++
++	/* redirect */
++/* 5 */ {OK,OK,OK,OK,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV},
++
++	/* Type 6,7 don't exist */
++/* 6 */	{IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV},
++/* 7 */	{IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV},
++
++	/* echo request */
++/* 8 */	{OK,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV},
++
++	/* router advertisement */
++/* 9 */ {OK,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV},
++	/* router selection */
++/* 10*/ {OK,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV},
++
++	/* time exceeded */
++/* 11*/ {OK,OK,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV},
++
++	/* Parameter problem */
++/* 12*/ {OK,OK,OK,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV},
++
++	/* timestamp */
++/* 13*/ {OK,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV},
++	/* timestamp reply */
++/* 14*/ {OK,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV},
++	/* Information request */
++/* 15*/ {OK,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV},
++	/* Information reply */
++/* 16*/ {OK,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV},
++	/* Address Mask request */
++/* 17*/ {OK,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV},
++	/* Address Mask reply */
++/* 18*/ {OK,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV},
++};
++
+ static int icmp_pkt_to_tuple(const struct sk_buff *skb,
+ 			     unsigned int dataoff,
+ 			     struct ip_conntrack_tuple *tuple)
+@@ -126,9 +187,9 @@
  }
  
  static int
@@ -23,7 +89,7 @@
  {
  	struct ip_conntrack_tuple innertuple, origtuple;
  	struct {
-@@ -145,13 +147,6 @@
+@@ -145,13 +206,6 @@
  	if (skb_copy_bits(skb, skb->nh.iph->ihl*4, &inside, sizeof(inside))!=0)
  		return NF_ACCEPT;
  
@@ -37,7 +103,7 @@
  	/* Ignore ICMP's containing fragments (shouldn't happen) */
  	if (inside.ip.frag_off & htons(IP_OFFSET)) {
  		DEBUGP("icmp_error_track: fragment of proto %u\n",
-@@ -201,6 +196,69 @@
+@@ -201,6 +255,85 @@
  	return -NF_ACCEPT;
  }
  
@@ -93,6 +159,22 @@
 +		return -NF_ACCEPT;
 +	}
 +
++	/* 15 is the highest 'known' ICMP code. See RFC 1812 */
++	if (icmph.code > 15) {
++		if (LOG_INVALID(IPPROTO_ICMP))
++			nf_log_packet(PF_INET, 0, skb, NULL, NULL,
++				      "ip_ct_icmp: invalid ICMP code ");
++		return -NF_ACCEPT;
++	}
++
++	/* check for invalid combinations */
++	if (!icmp_valid[icmph.type][icmph.code]) {
++		if (LOG_INVALID(IPPROTO_ICMP))
++			nf_log_packet(PF_INET, 0, skb, NULL, NULL,
++				      "ip_ct_icmp: invalid ICMP type/code ");
++		return -NF_ACCEPT;
++	}
++
 +	/* Need to track icmp error message? */
 +	if (icmph.type != ICMP_DEST_UNREACH
 +	    && icmph.type != ICMP_SOURCE_QUENCH
@@ -107,9 +189,9 @@
  struct ip_conntrack_protocol ip_conntrack_protocol_icmp =
  {
  	.proto 			= IPPROTO_ICMP,
-diff -urN --exclude-from=/usr/src/diff.exclude linux-2.6.6-ct_error_api/net/ipv4/netfilter/ip_conntrack_proto_udp.c linux-2.6.6-ct_error_api-udp/net/ipv4/netfilter/ip_conntrack_proto_udp.c
---- linux-2.6.6-ct_error_api/net/ipv4/netfilter/ip_conntrack_proto_udp.c	2004-06-18 07:37:24.000000000 +0200
-+++ linux-2.6.6-ct_error_api-udp/net/ipv4/netfilter/ip_conntrack_proto_udp.c	2004-06-18 15:39:58.000000000 +0200
+diff -u -r1.4 ip_conntrack_proto_udp.c
+--- a/net/ipv4/netfilter/ip_conntrack_proto_udp.c	20 Jun 2004 21:22:07 -0000	1.4
++++ b/net/ipv4/netfilter/ip_conntrack_proto_udp.c	20 Jun 2004 21:22:39 -0000
 @@ -12,6 +12,8 @@
  #include <linux/netfilter.h>
  #include <linux/in.h>

Re: [PATCH] modification in current protocol helper API to handle error/unclean packets

[Patrick McHardy]

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

Pablo Neira wrote:
> Would you be willing to apply the patch attached to pom-ng? It applies 
> to linux-2.6.patch_02-udp-icmp and it adds more checkings for invalid 
> icmp combinations.

You should use the defined types for better readability. Something like
this ..

Regards
Patrick

[-- Attachment #2: x --]
[-- Type: text/plain, Size: 2760 bytes --]

===== net/ipv4/netfilter/ip_conntrack_proto_icmp.c 1.8 vs edited =====
--- 1.8/net/ipv4/netfilter/ip_conntrack_proto_icmp.c	2004-06-21 01:50:41 +02:00
+++ edited/net/ipv4/netfilter/ip_conntrack_proto_icmp.c	2004-06-21 02:03:10 +02:00
@@ -27,6 +27,38 @@
 #define DEBUGP(format, args...)
 #endif
 
+#define OK 1
+#define IV 0
+
+#define ICMP_ROUTER_ADV	9
+#define ICMP_ROUTER_SEL	10
+
+/* ICMP types and codes described in RFC's:
+ * 	- 792: Internet Control Message Protocol. Most types.
+ * 	- 1256: ICMP Router Discovery Messages. Types 9 and 10.
+ * 	- 950: Internet Standard Subnetting Procedure. Types 17 and 18.
+ * 	- 1812: Requirements for IP Version 4 Routers. Type 3. Section 5.2.7.1
+ */
+static u_int8_t icmp_valid[NR_ICMP_TYPES+1][NR_ICMP_UNREACH+1] = 
+{
+	/*                          0  1  2  3  4  5  6  7  8  9 10 11 12 13 14 15 */
+	[ICMP_ECHOREPLY]	= {OK,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV},
+	[ICMP_DEST_UNREACH]	= {OK,OK,OK,OK,OK,OK,OK,OK,OK,OK,OK,OK,OK,OK,OK,OK},
+	[ICMP_SOURCE_QUENCH]	= {OK,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV},
+	[ICMP_REDIRECT] 	= {OK,OK,OK,OK,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV},
+	[ICMP_ECHO]		= {OK,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV},
+	[ICMP_ROUTER_ADV]	= {OK,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV},
+	[ICMP_ROUTER_SEL]	= {OK,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV},
+	[ICMP_TIME_EXCEEDED]	= {OK,OK,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV},
+	[ICMP_PARAMETERPROB]	= {OK,OK,OK,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV},
+	[ICMP_TIMESTAMP]	= {OK,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV},
+	[ICMP_TIMESTAMPREPLY]	= {OK,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV},
+	[ICMP_INFO_REQUEST]	= {OK,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV},
+	[ICMP_INFO_REPLY]	= {OK,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV},
+	[ICMP_ADDRESS]		= {OK,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV},
+	[ICMP_ADDRESSREPLY]	= {OK,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV},
+};
+
 static int icmp_pkt_to_tuple(const struct sk_buff *skb,
 			     unsigned int dataoff,
 			     struct ip_conntrack_tuple *tuple)
@@ -245,6 +277,22 @@
 		if (LOG_INVALID(IPPROTO_ICMP))
 			nf_log_packet(PF_INET, 0, skb, NULL, NULL,
 				      "ip_ct_icmp: invalid ICMP type ");
+		return -NF_ACCEPT;
+	}
+
+	/* 15 is the highest 'known' ICMP code. See RFC 1812 */
+	if (icmph.code > NR_ICMP_UNREACH) {
+		if (LOG_INVALID(IPPROTO_ICMP))
+			nf_log_packet(PF_INET, 0, skb, NULL, NULL,
+				      "ip_ct_icmp: invalid ICMP code ");
+		return -NF_ACCEPT;
+	}
+
+	/* check for invalid combinations */
+	if (!icmp_valid[icmph.type][icmph.code]) {
+		if (LOG_INVALID(IPPROTO_ICMP))
+			nf_log_packet(PF_INET, 0, skb, NULL, NULL,
+				      "ip_ct_icmp: invalid ICMP type/code ");
 		return -NF_ACCEPT;
 	}
 

Re: [PATCH] modification in current protocol helper API to handle error/unclean packets

[Willy Tarreau]

Hi Pablo,

On Mon, Jun 14, 2004 at 02:10:39AM +0200, Pablo Neira wrote:
 
> This way, Jozsef could call unclean() here in his tcp-window-tracking 
> patch and we won't need to add protocol specific function to the core of 
> the conntrack system like icmp_error_track to handle special ICMP 
> messages. We could also check for unclean udp and icmp packets.

There is something else I've been thinking about for a long time. In all
my configs, I first match the packet against 'unclean' in the PREROUTING
chain, and since I also use tcp_window-tracking, the check is performed
twice (at least on TCP). Wouldn't it be interesting to put a flag somewhere
on the packet being evaluated to mark it 'already checked', and that a few
matches could set ? That way, we could avoid the same test being done twice.

Just a few thoughts anyway.

Cheers,
Willy

Re: [PATCH] modification in current protocol helper API to handle error/unclean packets

[Jozsef Kadlecsik]

Hi Martin,

On Sun, 20 Jun 2004, Martin Josefsson wrote:

> > - Jozsef, I resolve to use your inverse return value but I must confess
> > that I'm still thinking about another way to do that with no sucess,
>
> What's the deal with these inverse return values in tcp-window-tracking
> and now conntrack_error-api?
>
> Sometimes we return NF_ACCEPT and sometimes we return -NF_ACCEPT which
> have two completely diffrent meanings. That's really really ugly.

Yes, it's ugly. That was introduced as a (quick) hack to express what you
describe:

> As I've understod it we have two cases:
> 1. We want to return NF_* all the way back to the netfilter core.
> 2. We want conntrack to continue processing the packet.
>
> And we need a way to distinguish these returnvalues.
>
> My suggestion is that when we want NF_* we return NF_* and if we want
> conntrack to continue we return CONNTRACK_CONT which is defined as -1
> All NF_* macros are >= 0
>
> Instead of checking for a negative returnvalue as we do today we check
> != CONNTRACK_CONT and then return the value previously returned to us.
>
> In my oppinion that would greatly improve the readability of the code.
> The mixing of negative and positive values using the same macro
> confuses me sometimes to say the least. I bet I'm not the only one that
> sometimes has to look twice at the return value.
>
> Have I missed something?

No, nothing at all.

> If not and noone objects I'll fix it up in cvs unless someone beats me
> to it.

That'd be great, Martin!

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlec@sunserv.kfki.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : KFKI Research Institute for Particle and Nuclear Physics
          H-1525 Budapest 114, POB. 49, Hungary

Re: [PATCH] modification in current protocol helper API to handle error/unclean packets

[Jozsef Kadlecsik]

Hi Pablo,

On Mon, 21 Jun 2004, Pablo Neira wrote:

> Would you be willing to apply the patch attached to pom-ng? It applies
> to linux-2.6.patch_02-udp-icmp and it adds more checkings for invalid
> icmp combinations.

Let us be very careful here and not add half of the unclean match to
conntrack.

We always have to remind ourselves that the sole task of the connection
tracking subsystem is to keep track of connections as good as possible.
We drop packets directly only when letting trough the packet would render
connection tracking useless (therefore it's so rare). We *advice* the
admin to drop packets by flagging it as INVALID when we can surely say
that the packet in question is invalid in the context of the connection
where it seems to belong to. But it's practically as strong a decision
as dropping the packet directly, so we have to be careful there as well.

I had already hesitated over the implications of the code

++      /* 15 is the highest 'known' ICMP code. See RFC 1812 */
++      if (icmph.code > 15) {
++              if (LOG_INVALID(IPPROTO_ICMP))
++                      nf_log_packet(PF_INET, 0, skb, NULL, NULL,
++                                    "ip_ct_icmp: invalid ICMP code ");
++              return -NF_ACCEPT;
++      }

because it means that if a new ICMP code is introduced, even if we modify
the source code, all previously installed systems runnin netfilter
(estimate that number) would refuse to handle the new ICMP code: we
created a forward-incompatibility.

I'd better not add the code checkings and would even remove the one above,
unless you have strong arguments against doing so.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlec@sunserv.kfki.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : KFKI Research Institute for Particle and Nuclear Physics
          H-1525 Budapest 114, POB. 49, Hungary

Re: [PATCH] modification in current protocol helper API to handle error/unclean packets

[Henrik Nordstrom]

On Mon, 21 Jun 2004, Jozsef Kadlecsik wrote:

> We always have to remind ourselves that the sole task of the connection
> tracking subsystem is to keep track of connections as good as possible.
> We drop packets directly only when letting trough the packet would render
> connection tracking useless (therefore it's so rare). We *advice* the
> admin to drop packets by flagging it as INVALID when we can surely say
> that the packet in question is invalid in the context of the connection
> where it seems to belong to. But it's practically as strong a decision
> as dropping the packet directly, so we have to be careful there as well.

I stronly agree with the above.

conntrack MUST NOT drop packets only because the meaning of the packet is 
undefined today.

conntrack MAY drop packets if the packet is in conflict with already 
tracked sessions, but I prefer not unless there is no other way to solve 
it (for example in MASQUERADE if there is no free tuple).

The only case I know of where it may be considered OK for conntrack to 
drop packets is the hack in various protocol helpers if a command is 
fragmented and in the packet reassembly. The helpers need to temporarily 
drop packets to cause TCP stream reassembly, and packet reassembly needs 
to drop packets to avoid DoS.

In all other situations involving packets which could not be 
properly tracked INVALID should be returned unless there is very 
strong reasons in terms of other tracked connections to do otherwise.

> I had already hesitated over the implications of the code
> 
> ++      /* 15 is the highest 'known' ICMP code. See RFC 1812 */
> ++      if (icmph.code > 15) {
> ++              if (LOG_INVALID(IPPROTO_ICMP))
> ++                      nf_log_packet(PF_INET, 0, skb, NULL, NULL,
> ++                                    "ip_ct_icmp: invalid ICMP code ");
> ++              return -NF_ACCEPT;
> ++      }

This is a good case where INVALID is the appropriate response. As the code
is not defined today it can not be defined how/if it should be tracked,
but this alone is not a reason to drop the packet.

This principle is also strongly advocated in RFC 1812 as "MUST NOT drop
(receive or forward) a packet merely becasue one or more of these reserved
bits has a non-zero value". This specific quote is from the IP header
section, but just empazises the golden rule of Internet networking: "be
strict in what you send, liberal in what you accept/forward".

Unless I have misunderstood the prior discussion regarding return values
the above code is actaully correct save for the confusing use of negative
NF_ACCEPT..  These ICMP codes is undefined and it can not be defined how
these should be tracked so these packets needs to be marked as INVALID in
terms of conntrack status. Logging them as "invalid ICMP code" is perhaps
a little harsh and should maybe read "unknown ICMP code" and the return
value should be fixed to return CONNTRACK_INVALID or similar instead of
overloading NF_ACCEPT as per the prior discussion regarding return 
values..

Regards
Henrik

Re: [PATCH] modification in current protocol helper API to handle error/unclean packets

[Jozsef Kadlecsik]

On Mon, 21 Jun 2004, Henrik Nordstrom wrote:

> > ++      /* 15 is the highest 'known' ICMP code. See RFC 1812 */
> > ++      if (icmph.code > 15) {
> > ++              if (LOG_INVALID(IPPROTO_ICMP))
> > ++                      nf_log_packet(PF_INET, 0, skb, NULL, NULL,
> > ++                                    "ip_ct_icmp: invalid ICMP code ");
> > ++              return -NF_ACCEPT;
> > ++      }
>
> This is a good case where INVALID is the appropriate response. As the code
> is not defined today it can not be defined how/if it should be tracked,
> but this alone is not a reason to drop the packet.

So you could live happily if such packets are marked as INVALID by
conntrack, which implies then that the ICMP code/type checking would be
acceptable as well? [Pablo's patch did not want to drop the packets but
mark as INVALID.]

It's time to add documentation exactly which packets are dropped or marked
as INVALID by conntrack.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlec@sunserv.kfki.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : KFKI Research Institute for Particle and Nuclear Physics
          H-1525 Budapest 114, POB. 49, Hungary

Re: [PATCH] modification in current protocol helper API to handle error/unclean packets

[Pablo Neira]

Hi Jozsef,

Jozsef Kadlecsik wrote:

>>Would you be willing to apply the patch attached to pom-ng? It applies
>>to linux-2.6.patch_02-udp-icmp and it adds more checkings for invalid
>>icmp combinations.
>>    
>>
>
>Let us be very careful here and not add half of the unclean match to
>conntrack.
>  
>

thanks, it's always ok having someone who has the feet on the ground :-)

>We always have to remind ourselves that the sole task of the connection
>tracking subsystem is to keep track of connections as good as possible.
>We drop packets directly only when letting trough the packet would render
>connection tracking useless (therefore it's so rare). We *advice* the
>admin to drop packets by flagging it as INVALID when we can surely say
>that the packet in question is invalid in the context of the connection
>where it seems to belong to. But it's practically as strong a decision
>as dropping the packet directly, so we have to be careful there as well.
>
>I had already hesitated over the implications of the code
>
>++      /* 15 is the highest 'known' ICMP code. See RFC 1812 */
>++      if (icmph.code > 15) {
>++              if (LOG_INVALID(IPPROTO_ICMP))
>++                      nf_log_packet(PF_INET, 0, skb, NULL, NULL,
>++                                    "ip_ct_icmp: invalid ICMP code ");
>++              return -NF_ACCEPT;ç
>++      }
>
>because it means that if a new ICMP code is introduced, even if we modify
>the source code, all previously installed systems runnin netfilter
>(estimate that number) would refuse to handle the new ICMP code: we
>created a forward-incompatibility.
>  
>

that's true but, if that day comes, we can provide different ways to 
work around the problem:

- The admin could disable dropping invalid ICMP.
- We could use some some /proc stuff or implement custom options in 
iptables like:
iptables setting strong-icmp-tracking on
iptables setting strong-tcp-tracking off
and so on. This way the admin could choose to use your current 
tcp-window-tracking or the old one which is softer.
- We could use that NOTRACK target to untrack icmp packets which have 
those new codes, actually I don't love that target but it could be ok.

And of course, document these issues: Let everyone know what conntrack 
is setting as INVALID. I'll be please to do that, even your 
tcp-window-tracking patch.

If you still consider that we can't provide any methods to work around 
to possible problem, I'll drop checkings and we could leave them in a 
patch in pom-ng as something optional.

The reason why I like adding those pieces of codes is that I like the 
idea of having a strong and smart conntrack system that we could be 
proud of, but always letting the administrator to decide what he wants 
to drop or not.

regards,
Pablo

Re: [PATCH] modification in current protocol helper API to handle error/unclean packets

[Jozsef Kadlecsik]

Hi Pablo,

On Mon, 21 Jun 2004, Pablo Neira wrote:

> >Let us be very careful here and not add half of the unclean match to
> >conntrack.
>
> thanks, it's always ok having someone who has the feet on the ground :-)

That sentence was in plural because it refers to me as well. :-)

> >I had already hesitated over the implications of the code
[...]
> >because it means that if a new ICMP code is introduced, even if we modify
> >the source code, all previously installed systems runnin netfilter
> >(estimate that number) would refuse to handle the new ICMP code: we
> >created a forward-incompatibility.
>
> that's true but, if that day comes, we can provide different ways to
> work around the problem:
>
> - The admin could disable dropping invalid ICMP.
> - We could use some some /proc stuff or implement custom options in
> iptables like:
> iptables setting strong-icmp-tracking on
> iptables setting strong-tcp-tracking off
> and so on. This way the admin could choose to use your current
> tcp-window-tracking or the old one which is softer.
> - We could use that NOTRACK target to untrack icmp packets which have
> those new codes, actually I don't love that target but it could be ok.

Probably the first one is just enough: if upgrade/recompiling is not an
option, the admin can anytime escape selected INVALID packets not to drop.

> And of course, document these issues: Let everyone know what conntrack
> is setting as INVALID. I'll be please to do that, even your
> tcp-window-tracking patch.

Documentation would be really, really great, as part of the iptables
manpage. Luckily it's already splitted up and handled by pom-ng.

> If you still consider that we can't provide any methods to work around
> to possible problem, I'll drop checkings and we could leave them in a
> patch in pom-ng as something optional.

OK, I'm convinced :-). Please send an updated patch, based on
conntrack_error-api/linux-2.6.patch_02-udp-icmp from cvs (there were some
modification in the patch by me and Martin).

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlec@sunserv.kfki.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : KFKI Research Institute for Particle and Nuclear Physics
          H-1525 Budapest 114, POB. 49, Hungary

Re: [PATCH] modification in current protocol helper API to handle error/unclean packets

[Jozsef Kadlecsik]

Hi Willy,

On Mon, 21 Jun 2004, Willy Tarreau wrote:

> > This way, Jozsef could call unclean() here in his tcp-window-tracking
> > patch and we won't need to add protocol specific function to the core of
> > the conntrack system like icmp_error_track to handle special ICMP
> > messages. We could also check for unclean udp and icmp packets.
>
> There is something else I've been thinking about for a long time. In all
> my configs, I first match the packet against 'unclean' in the PREROUTING
> chain, and since I also use tcp_window-tracking, the check is performed
> twice (at least on TCP). Wouldn't it be interesting to put a flag somewhere
> on the packet being evaluated to mark it 'already checked', and that a few
> matches could set ? That way, we could avoid the same test being done twice.

The unclean match was removed from the 2.6 tree because nobody came up
with a configurable version with liberal default settings (hint, hint! :-)
Otherwise you're right and such optimization could easily be added.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlec@sunserv.kfki.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : KFKI Research Institute for Particle and Nuclear Physics
          H-1525 Budapest 114, POB. 49, Hungary

Re: [PATCH] modification in current protocol helper API to handle error/unclean packets

[Willy Tarreau]

Hi Jozsef,

On Mon, Jun 21, 2004 at 12:51:41PM +0200, Jozsef Kadlecsik wrote:
 
> So you could live happily if such packets are marked as INVALID by
> conntrack, which implies then that the ICMP code/type checking would be
> acceptable as well? [Pablo's patch did not want to drop the packets but
> mark as INVALID.]
> 
> It's time to add documentation exactly which packets are dropped or marked
> as INVALID by conntrack.

May I ask that we could have a new result other than INVALID for such
packets ? It's becoming difficult to differenciate :

  - valid packets for which there is no session
  - valid packets for which there is a session but which are invalid wrt
    this session (wrong flags, sequence numbers, retransmits, ...)
  - invalid packets (in the 'unclean' sense)

and it's either not possible to differentiate between :
  - suspicious packets which we must let go through to try to re-establish
    a session or close a session (eg SYN), which currently generate a
    message such as "INVALID SYN (ignored)"
  - totally valid packets.

Perhaps we would need to add NOSESSION for the first case, keep INVALID
for the second case, add UNCLEAN for the third case, and IGNORED for the
fourth case. We could even add this only to ctstate so that setups relying
on the state match don't see any change.

It would really help for complicated setups and for people who want a
high level of tracability, such as a bank I know of.

Any thoughts ?

Regards,
Willy

Re: [PATCH] modification in current protocol helper API to handle error/unclean packets

[Pablo Neira]

Hi Willy,

Willy Tarreau wrote:

>Hi Jozsef,
>
>On Mon, Jun 21, 2004 at 12:51:41PM +0200, Jozsef Kadlecsik wrote:
> 
>  
>
>>So you could live happily if such packets are marked as INVALID by
>>conntrack, which implies then that the ICMP code/type checking would be
>>acceptable as well? [Pablo's patch did not want to drop the packets but
>>mark as INVALID.]
>>
>>It's time to add documentation exactly which packets are dropped or marked
>>as INVALID by conntrack.
>>    
>>
>
>May I ask that we could have a new result other than INVALID for such
>packets ? It's becoming difficult to differenciate :
>
>  - valid packets for which there is no session
>  - valid packets for which there is a session but which are invalid wrt
>    this session (wrong flags, sequence numbers, retransmits, ...)
>  - invalid packets (in the 'unclean' sense)
>  
>

I have an idea, in the case of unclean packets, perhaps we could use the 
nfcache field in skb to set a special flag like "NFC_CLEAN". Could we 
use nfcache for that purpose?

regards,
Pablo

Re: [PATCH] modification in current protocol helper API to handle error/unclean packets

[Pablo Neira]

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

Hi Patrick,

Patrick McHardy wrote:

> You should use the defined types for better readability. Something like
> this ..


now it uses the defined types. It applies to pom-ng, thanks.

regards,
Pablo

[-- Attachment #2: linux-2.6.patch_02-udp-icmp-CVS.patch --]
[-- Type: text/x-patch, Size: 4933 bytes --]

--- linux-2.6.patch_02-udp-icmp	2004-06-22 13:43:04.000000000 +0200
+++ patch	2004-06-22 13:53:02.000000000 +0200
@@ -1,6 +1,6 @@
-diff -urN --exclude-from=/usr/src/diff.exclude linux-2.6.6-ct_error_api/net/ipv4/netfilter/ip_conntrack_proto_icmp.c linux-2.6.6-ct_error_api-udp/net/ipv4/netfilter/ip_conntrack_proto_icmp.c
---- linux-2.6.6-ct_error_api/net/ipv4/netfilter/ip_conntrack_proto_icmp.c	2004-06-18 08:13:51.000000000 +0200
-+++ linux-2.6.6-ct_error_api-udp/net/ipv4/netfilter/ip_conntrack_proto_icmp.c	2004-06-18 15:14:01.000000000 +0200
+diff -u -r1.2 ip_conntrack_proto_icmp.c
+--- a/net/ipv4/netfilter/ip_conntrack_proto_icmp.c	22 Jun 2004 11:44:08 -0000	1.2
++++ b/net/ipv4/netfilter/ip_conntrack_proto_icmp.c	22 Jun 2004 11:52:17 -0000
 @@ -13,6 +13,8 @@
  #include <linux/in.h>
  #include <linux/icmp.h>
@@ -10,7 +10,46 @@
  #include <linux/netfilter_ipv4/ip_conntrack.h>
  #include <linux/netfilter_ipv4/ip_conntrack_core.h>
  #include <linux/netfilter_ipv4/ip_conntrack_protocol.h>
-@@ -126,9 +128,9 @@
+@@ -25,6 +27,38 @@
+ #define DEBUGP(format, args...)
+ #endif
+ 
++#define OK 1
++#define IV 0
++
++#define ICMP_ROUTER_ADV	9
++#define ICMP_ROUTER_SEL	10
++
++/* ICMP types and codes described in RFC's:
++ * 	- 792: Internet Control Message Protocol. Most types.
++ * 	- 1256: ICMP Router Discovery Messages. Types 9 and 10.
++ * 	- 950: Internet Standard Subnetting Procedure. Types 17 and 18.
++ * 	- 1812: Requirements for IP Version 4 Routers. Type 3. Section 5.2.7.1
++ */
++static u_int8_t icmp_valid[NR_ICMP_TYPES+1][NR_ICMP_UNREACH+1] = 
++{
++	/*                          0  1  2  3  4  5  6  7  8  9 10 11 12 13 14 15 */
++	[ICMP_ECHOREPLY]	= {OK,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV},
++	[ICMP_DEST_UNREACH]	= {OK,OK,OK,OK,OK,OK,OK,OK,OK,OK,OK,OK,OK,OK,OK,OK},
++	[ICMP_SOURCE_QUENCH]	= {OK,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV},
++	[ICMP_REDIRECT] 	= {OK,OK,OK,OK,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV},
++	[ICMP_ECHO]		= {OK,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV},
++	[ICMP_ROUTER_ADV]	= {OK,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV},
++	[ICMP_ROUTER_SEL]	= {OK,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV},
++	[ICMP_TIME_EXCEEDED]	= {OK,OK,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV},
++	[ICMP_PARAMETERPROB]	= {OK,OK,OK,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV},
++	[ICMP_TIMESTAMP]	= {OK,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV},
++	[ICMP_TIMESTAMPREPLY]	= {OK,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV},
++	[ICMP_INFO_REQUEST]	= {OK,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV},
++	[ICMP_INFO_REPLY]	= {OK,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV},
++	[ICMP_ADDRESS]		= {OK,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV},
++	[ICMP_ADDRESSREPLY]	= {OK,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV,IV},
++};
++
+ static int icmp_pkt_to_tuple(const struct sk_buff *skb,
+ 			     unsigned int dataoff,
+ 			     struct ip_conntrack_tuple *tuple)
+@@ -126,9 +160,9 @@
  }
  
  static int
@@ -23,7 +62,7 @@
  {
  	struct ip_conntrack_tuple innertuple, origtuple;
  	struct {
-@@ -145,13 +147,6 @@
+@@ -145,13 +179,6 @@
  	if (skb_copy_bits(skb, skb->nh.iph->ihl*4, &inside, sizeof(inside))!=0)
  		return NF_ACCEPT;
  
@@ -37,7 +76,7 @@
  	/* Ignore ICMP's containing fragments (shouldn't happen) */
  	if (inside.ip.frag_off & htons(IP_OFFSET)) {
  		DEBUGP("icmp_error_track: fragment of proto %u\n",
-@@ -201,6 +196,69 @@
+@@ -201,6 +228,85 @@
  	return -NF_ACCEPT;
  }
  
@@ -93,6 +132,22 @@
 +		return -NF_ACCEPT;
 +	}
 +
++	/* 15 is the highest 'known' ICMP code. See RFC 1812 */
++	if (icmph.code > NR_ICMP_UNREACH) {
++		if (LOG_INVALID(IPPROTO_ICMP))
++			nf_log_packet(PF_INET, 0, skb, NULL, NULL,
++				      "ip_ct_icmp: invalid ICMP code ");
++		return -NF_ACCEPT;
++	}
++
++	/* check for invalid combinations */
++	if (!icmp_valid[icmph.type][icmph.code]) {
++		if (LOG_INVALID(IPPROTO_ICMP))
++			nf_log_packet(PF_INET, 0, skb, NULL, NULL,
++				      "ip_ct_icmp: invalid ICMP type/code ");
++ 		return -NF_ACCEPT;
++ 	}
++
 +	/* Need to track icmp error message? */
 +	if (icmph.type != ICMP_DEST_UNREACH
 +	    && icmph.type != ICMP_SOURCE_QUENCH
@@ -107,9 +162,9 @@
  struct ip_conntrack_protocol ip_conntrack_protocol_icmp =
  {
  	.proto 			= IPPROTO_ICMP,
-diff -urN --exclude-from=/usr/src/diff.exclude linux-2.6.6-ct_error_api/net/ipv4/netfilter/ip_conntrack_proto_udp.c linux-2.6.6-ct_error_api-udp/net/ipv4/netfilter/ip_conntrack_proto_udp.c
---- linux-2.6.6-ct_error_api/net/ipv4/netfilter/ip_conntrack_proto_udp.c	2004-06-18 07:37:24.000000000 +0200
-+++ linux-2.6.6-ct_error_api-udp/net/ipv4/netfilter/ip_conntrack_proto_udp.c	2004-06-18 15:39:58.000000000 +0200
+diff -u -r1.2 ip_conntrack_proto_udp.c
+--- a/net/ipv4/netfilter/ip_conntrack_proto_udp.c	22 Jun 2004 11:44:09 -0000	1.2
++++ b/net/ipv4/netfilter/ip_conntrack_proto_udp.c	22 Jun 2004 11:48:49 -0000
 @@ -12,6 +12,8 @@
  #include <linux/netfilter.h>
  #include <linux/in.h>

Re: [PATCH] modification in current protocol helper API to handle error/unclean packets

[Jozsef Kadlecsik]

Hi Willy,

On Tue, 22 Jun 2004, Willy Tarreau wrote:

> May I ask that we could have a new result other than INVALID for such
> packets ? It's becoming difficult to differenciate :
>
>   - valid packets for which there is no session
>   - valid packets for which there is a session but which are invalid wrt
>     this session (wrong flags, sequence numbers, retransmits, ...)
>   - invalid packets (in the 'unclean' sense)
>
> and it's either not possible to differentiate between :
>   - suspicious packets which we must let go through to try to re-establish
>     a session or close a session (eg SYN), which currently generate a
>     message such as "INVALID SYN (ignored)"
>   - totally valid packets.
>
> Perhaps we would need to add NOSESSION for the first case, keep INVALID
> for the second case, add UNCLEAN for the third case, and IGNORED for the
> fourth case. We could even add this only to ctstate so that setups relying
> on the state match don't see any change.
>
> It would really help for complicated setups and for people who want a
> high level of tracability, such as a bank I know of.

I'd split the packets in question into two big categories strictly
according to wether it's an unclean packet or not.

Unclean packets are easy and could simply and elegantly be handled by
a configurable unclean match, no problem with those.

The TCP window tracking patch splits the "other" heap further into
several categories:

- drop the packet on the floor directly (conntrack is out of sync and we
  want the client to resend the SYN so that we can pick the connection up
  properly),
- packet marked as invalid (out of window packet or packet would
  lead to an invalid state),
- packet accepted by conntrack but its data is ignored with respect of
  the current connection entry (SYN we let through to detect ouf of sync
  connections and RST packets answering it when the connection is fine
  and the SYN was invalid).

Without a refreshed dropped-table patch, you are unable to handle
(i.e. log) the first case. But conntrack can log the packets for you just
fine.

In the second and third case you are unable to override the decision of
conntrack on how it treated the packet. If you let trough the second
category of the packet, it won't hurt. However, if the SYNs in the third
case are dropped by your rule, then conntrack's attempt to do its job
properly is defeated. In all case conntrack can log the packets.

What would be the purpose of the extension you propose? Logging is already
supported if one wish to monitor such packets.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlec@sunserv.kfki.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : KFKI Research Institute for Particle and Nuclear Physics
          H-1525 Budapest 114, POB. 49, Hungary

Re: [PATCH] modification in current protocol helper API to handle error/unclean packets

[Jozsef Kadlecsik]

On Tue, 22 Jun 2004, Jozsef Kadlecsik wrote:

>
> On Tue, 22 Jun 2004, Willy Tarreau wrote:
>
> > May I ask that we could have a new result other than INVALID for such
> > packets ? It's becoming difficult to differenciate :
> >
> >   - valid packets for which there is no session
> >   - valid packets for which there is a session but which are invalid wrt
> >     this session (wrong flags, sequence numbers, retransmits, ...)
> >   - invalid packets (in the 'unclean' sense)
[...]
> > It would really help for complicated setups and for people who want a
> > high level of tracability, such as a bank I know of.
[...]
> What would be the purpose of the extension you propose? Logging is already
> supported if one wish to monitor such packets.

If the point is just fine tuned selective logging, then a new target (or
better a generic NFCACHE target) in the raw table would be the most
simple solution.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlec@sunserv.kfki.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : KFKI Research Institute for Particle and Nuclear Physics
          H-1525 Budapest 114, POB. 49, Hungary

Re: [PATCH] modification in current protocol helper API to handle error/unclean packets

[Willy Tarreau]

Hi Jozsef,

On Tue, Jun 22, 2004 at 03:17:55PM +0200, Jozsef Kadlecsik wrote:
 
> In the second and third case you are unable to override the decision of
> conntrack on how it treated the packet. If you let trough the second
> category of the packet, it won't hurt. However, if the SYNs in the third
> case are dropped by your rule, then conntrack's attempt to do its job
> properly is defeated. In all case conntrack can log the packets.
> 
> What would be the purpose of the extension you propose? Logging is already
> supported if one wish to monitor such packets.

There are several uses to this :
  - being able to log *every* ignored packet with the same mechanism as the
    other ones (LOG, ULOG, QUEUE, ...). For that, the cleanest solution for
    the administrator would be a match. But indeed, it might not need to be
    a state match, but sort of a sub-state. Something like "ok, the packet
    is valid, but here are some precisions : it has been ignored".

  - being able to send a TCP RST for packets which don't belong to any
    session, and/or for those with invalid flags for the session state,
    but not for those which belong to a known session with bad sequence
    numbers. This would allow the firewall to help some hosts to close
    dead sessions without revealing anything about the configuration.

Something such as the following might sometimes be helpful :

----

# only log ignored SYNs but not ignored RSTs
iptables -A FORWARD -m state --state ESTABLISHED --reason IGNORED \
    -p tcp --syn -j LOG --log-prefix "Ignoring this SYN: "
# send an RST for session-less packets
iptables -A FORWARD -p tcp --flags RST NONE -m state --state INVALID \
    --reason NOSESSION -j send_reset
# send an RST for out-of-sync SYN/ACK packets
iptables -A FORWARD -p tcp --flags ALL SYN,ACK -m state --state INVALID \
    --reason BADFLAGS -j send_reset
# send an RST for out-of-sync ACK packets in SYN_SENT/SYN_RECV state
iptables -A FORWARD -p tcp --flags SYN,ACK,FIN,RST ACK -m state \
    --state INVALID --reason BADFLAGS -j send_reset
iptables -A send_reset -m limit --limit 100/s -j REJECT --reject-with tcp-reset
iptables -A send_reset -m limit --limit 10/s -j LOG --log-prefix "possible scan or old packet "
iptables -A send_reset -j DROP

----

These are all cases I encounter in production at a rate of about ten times a
second at the peak, and which could be greatly reduced if I had the ability
to tell the client that it's worthless retransmitting.

I know what Linus says, talk is cheap, show me the code... I have no code
for this, but since I have had this idea in mind now for a long time, I
might come up one day with a few lines, and I'd like to do it cleanly.

Regards,
Willy