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

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

------------------------------------------------------------------------

diff -u -r1.1.1.1 ip_conntrack_core.c

net/ipv4/netfilter/ip_conntrack_core.c +1 -78

@@ -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)

net/ipv4/netfilter/ip_conntrack_core.c +1 -0

+		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

net/ipv4/netfilter/ip_conntrack_proto_generic.c +8 -1

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

net/ipv4/netfilter/ip_conntrack_proto_icmp.c +18 -0

@@ -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);
+

net/ipv4/netfilter/ip_conntrack_proto_icmp.c +26 -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);

net/ipv4/netfilter/ip_conntrack_proto_icmp.c +15 -1

+		}
+		/* 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

net/ipv4/netfilter/ip_conntrack_proto_tcp.c +5 -1

+{
+	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

net/ipv4/netfilter/ip_conntrack_proto_udp.c +8 -0

@@ -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;
 }

net/ipv4/netfilter/ip_conntrack_proto_udp.c +16 -1

+	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

net/ipv4/netfilter/ip_fw_compat_masq.c +78 -0

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

include/linux/netfilter_ipv4/ip_conntrack.h +20 -0

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

include/linux/netfilter_ipv4/ip_conntrack_protocol.h +3 -0

@@ -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;
 };

Quoted 1 line · Pablo Neira

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

Some more comments below.

Regards
Patrick

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.

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

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

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

invert_tuple and find_tuple probably should be prefixed with ip_ct_ then.

Quoted 2 lines

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

net/ipv4/netfilter/ip_conntrack_core.c line 756

Quoted 3 lines

net/ipv4/netfilter/ip_conntrack_core.c +3 -0

+       /* 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?

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

------------------------------------------------------------------------

diff -u -r1.1.1.1 ip_conntrack_core.c

net/ipv4/netfilter/ip_conntrack_core.c +19 -93

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

net/ipv4/netfilter/ip_conntrack_proto_generic.c +8 -1

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

net/ipv4/netfilter/ip_conntrack_proto_icmp.c +81 -1

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

net/ipv4/netfilter/ip_conntrack_proto_tcp.c +8 -1

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

net/ipv4/netfilter/ip_conntrack_proto_udp.c +8 -1

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

net/ipv4/netfilter/ip_fw_compat_masq.c +79 -1

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

include/linux/netfilter_ipv4/ip_conntrack.h +20 -0

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

include/linux/netfilter_ipv4/ip_conntrack_protocol.h +3 -0

@@ -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;
 };

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

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

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.

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

Quoted 4 lines

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

Quoted 5 lines

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 :)

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.

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.

Quoted 2 lines

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

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.

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;
++ }

Quoted 7 lines

++ /* 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.

Quoted 5 lines

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.

Quoted 2 lines

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 :-)

Quoted 1 line

I had already hesitated over the implications of the code

Quoted 4 lines

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.

Quoted 4 lines

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.

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.

Hi Jozsef,

Quoted 9 lines · Jozsef Kadlecsik

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)

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

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.

Quoted 7 lines · Willy Tarreau

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)

Quoted 2 lines

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.

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.