[PATCH 0/2] Handle invalid packets in conntrack consistently

[PATCH 0/2] Handle invalid packets in conntrack consistently

[Jozsef Kadlecsik]

Hi Pablo,

As it was discussed, at the moment conntrack handles invalid packets
differently: IPv6 conntrack marks the packets as INVALID and lets
the user to drop them by an explicit rule, while IPv4 conntrack
simply drops such packets.

The next two patches bring conntrack in sync by changing IPv4 conntrack
behaviour to follow IPv6 conntrack. Invalid packet logging support is
also added.

The patches are follow-up of the second version of the patch I sent on
Tuesday, with the cover letter subject "Drop malformed IPv4 packets in
conntrack, 2nd try".

Best regards,
Jozsef

Jozsef Kadlecsik (2):
  net: netfilter: prepare conntrack for consistent invalid packet
    handling
  net: netfilter: handle invalid packets consistently in conntrack

 include/net/netfilter/nf_conntrack.h           |    3 +-
 include/net/netfilter/nf_conntrack_l3proto.h   |    5 ++-
 net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c |   29 ++++++++++++++++-------
 net/ipv4/netfilter/nf_conntrack_proto_icmp.c   |    2 +-
 net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c |   11 ++++++--
 net/ipv6/netfilter/nf_conntrack_proto_icmpv6.c |    2 +-
 net/netfilter/nf_conntrack_core.c              |    9 ++++---
 net/netfilter/nf_conntrack_l3proto_generic.c   |    3 +-
 net/netfilter/xt_connlimit.c                   |    2 +-
 9 files changed, 43 insertions(+), 23 deletions(-)

[PATCH 1/2] net: netfilter: prepare conntrack for consistent invalid packet handling

[Jozsef Kadlecsik]

Add the net struct as argument to nf_ct_get_tuplepr and get_l4proto
in order to be able to check the setting of nf_conntrack_log_invalid.

Signed-off-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
---
 include/net/netfilter/nf_conntrack.h           |    3 ++-
 include/net/netfilter/nf_conntrack_l3proto.h   |    5 +++--
 net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c |    3 ++-
 net/ipv4/netfilter/nf_conntrack_proto_icmp.c   |    2 +-
 net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c |    3 ++-
 net/ipv6/netfilter/nf_conntrack_proto_icmpv6.c |    2 +-
 net/netfilter/nf_conntrack_core.c              |    9 +++++----
 net/netfilter/nf_conntrack_l3proto_generic.c   |    3 ++-
 net/netfilter/xt_connlimit.c                   |    2 +-
 9 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/include/net/netfilter/nf_conntrack.h b/include/net/netfilter/nf_conntrack.h
index ab86036..4e6af0a 100644
--- a/include/net/netfilter/nf_conntrack.h
+++ b/include/net/netfilter/nf_conntrack.h
@@ -215,7 +215,8 @@ extern void nf_ct_insert_dying_list(struct nf_conn *ct);
 
 extern void nf_conntrack_flush_report(struct net *net, u32 pid, int report);
 
-extern bool nf_ct_get_tuplepr(const struct sk_buff *skb,
+extern bool nf_ct_get_tuplepr(const struct net *net,
+			      const struct sk_buff *skb,
 			      unsigned int nhoff, u_int16_t l3num,
 			      struct nf_conntrack_tuple *tuple);
 extern bool nf_ct_invert_tuplepr(struct nf_conntrack_tuple *inverse,
diff --git a/include/net/netfilter/nf_conntrack_l3proto.h b/include/net/netfilter/nf_conntrack_l3proto.h
index e8010f4..eddb720 100644
--- a/include/net/netfilter/nf_conntrack_l3proto.h
+++ b/include/net/netfilter/nf_conntrack_l3proto.h
@@ -46,8 +46,9 @@ struct nf_conntrack_l3proto {
 	 *	*dataoff: offset of protocol header (TCP, UDP,...) in skb
 	 *	*protonum: protocol number
 	 */
-	int (*get_l4proto)(const struct sk_buff *skb, unsigned int nhoff,
-			   unsigned int *dataoff, u_int8_t *protonum);
+	int (*get_l4proto)(const struct net *net, const struct sk_buff *skb,
+			   unsigned int nhoff, unsigned int *dataoff,
+			   u_int8_t *protonum);
 
 	int (*tuple_to_nlattr)(struct sk_buff *skb,
 			       const struct nf_conntrack_tuple *t);
diff --git a/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c b/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c
index 6a5f038..af7cdc7 100644
--- a/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c
+++ b/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c
@@ -66,7 +66,8 @@ static int ipv4_print_tuple(struct seq_file *s,
 			  &tuple->src.u3.ip, &tuple->dst.u3.ip);
 }
 
-static int ipv4_get_l4proto(const struct sk_buff *skb, unsigned int nhoff,
+static int ipv4_get_l4proto(const struct net *net,
+			    const struct sk_buff *skb, unsigned int nhoff,
 			    unsigned int *dataoff, u_int8_t *protonum)
 {
 	const struct iphdr *iph;
diff --git a/net/ipv4/netfilter/nf_conntrack_proto_icmp.c b/net/ipv4/netfilter/nf_conntrack_proto_icmp.c
index ab5b27a..21ca0e9 100644
--- a/net/ipv4/netfilter/nf_conntrack_proto_icmp.c
+++ b/net/ipv4/netfilter/nf_conntrack_proto_icmp.c
@@ -127,7 +127,7 @@ icmp_error_message(struct net *net, struct nf_conn *tmpl, struct sk_buff *skb,
 	NF_CT_ASSERT(skb->nfct == NULL);
 
 	/* Are they talking about one of our connections? */
-	if (!nf_ct_get_tuplepr(skb,
+	if (!nf_ct_get_tuplepr(net, skb,
 			       skb_network_offset(skb) + ip_hdrlen(skb)
 						       + sizeof(struct icmphdr),
 			       PF_INET, &origtuple)) {
diff --git a/net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c b/net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c
index 4111050..c65c060 100644
--- a/net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c
+++ b/net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c
@@ -116,7 +116,8 @@ static int nf_ct_ipv6_skip_exthdr(const struct sk_buff *skb, int start,
 	return start;
 }
 
-static int ipv6_get_l4proto(const struct sk_buff *skb, unsigned int nhoff,
+static int ipv6_get_l4proto(const struct net *net,
+			    const struct sk_buff *skb, unsigned int nhoff,
 			    unsigned int *dataoff, u_int8_t *protonum)
 {
 	unsigned int extoff = nhoff + sizeof(struct ipv6hdr);
diff --git a/net/ipv6/netfilter/nf_conntrack_proto_icmpv6.c b/net/ipv6/netfilter/nf_conntrack_proto_icmpv6.c
index 7c05e7e..d87c868 100644
--- a/net/ipv6/netfilter/nf_conntrack_proto_icmpv6.c
+++ b/net/ipv6/netfilter/nf_conntrack_proto_icmpv6.c
@@ -143,7 +143,7 @@ icmpv6_error_message(struct net *net, struct nf_conn *tmpl,
 	NF_CT_ASSERT(skb->nfct == NULL);
 
 	/* Are they talking about one of our connections? */
-	if (!nf_ct_get_tuplepr(skb,
+	if (!nf_ct_get_tuplepr(net, skb,
 			       skb_network_offset(skb)
 				+ sizeof(struct ipv6hdr)
 				+ sizeof(struct icmp6hdr),
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index ed86a3b..180f443 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -128,8 +128,9 @@ nf_ct_get_tuple(const struct sk_buff *skb,
 }
 EXPORT_SYMBOL_GPL(nf_ct_get_tuple);
 
-bool nf_ct_get_tuplepr(const struct sk_buff *skb, unsigned int nhoff,
-		       u_int16_t l3num, struct nf_conntrack_tuple *tuple)
+bool nf_ct_get_tuplepr(const struct net *net, const struct sk_buff *skb,
+		       unsigned int nhoff, u_int16_t l3num,
+		       struct nf_conntrack_tuple *tuple)
 {
 	struct nf_conntrack_l3proto *l3proto;
 	struct nf_conntrack_l4proto *l4proto;
@@ -140,7 +141,7 @@ bool nf_ct_get_tuplepr(const struct sk_buff *skb, unsigned int nhoff,
 	rcu_read_lock();
 
 	l3proto = __nf_ct_l3proto_find(l3num);
-	ret = l3proto->get_l4proto(skb, nhoff, &protoff, &protonum);
+	ret = l3proto->get_l4proto(net, skb, nhoff, &protoff, &protonum);
 	if (ret != NF_ACCEPT) {
 		rcu_read_unlock();
 		return false;
@@ -926,7 +927,7 @@ nf_conntrack_in(struct net *net, u_int8_t pf, unsigned int hooknum,
 
 	/* rcu_read_lock()ed by nf_hook_slow */
 	l3proto = __nf_ct_l3proto_find(pf);
-	ret = l3proto->get_l4proto(skb, skb_network_offset(skb),
+	ret = l3proto->get_l4proto(net, skb, skb_network_offset(skb),
 				   &dataoff, &protonum);
 	if (ret <= 0) {
 		pr_debug("not prepared to track yet or error occurred\n");
diff --git a/net/netfilter/nf_conntrack_l3proto_generic.c b/net/netfilter/nf_conntrack_l3proto_generic.c
index e7eb807..ebed8db 100644
--- a/net/netfilter/nf_conntrack_l3proto_generic.c
+++ b/net/netfilter/nf_conntrack_l3proto_generic.c
@@ -55,7 +55,8 @@ static int generic_print_tuple(struct seq_file *s,
 	return 0;
 }
 
-static int generic_get_l4proto(const struct sk_buff *skb, unsigned int nhoff,
+static int generic_get_l4proto(const struct net *net,
+			       const struct sk_buff *skb, unsigned int nhoff,
 			       unsigned int *dataoff, u_int8_t *protonum)
 {
 	/* Never track !!! */
diff --git a/net/netfilter/xt_connlimit.c b/net/netfilter/xt_connlimit.c
index c6d5a83..36baf0e 100644
--- a/net/netfilter/xt_connlimit.c
+++ b/net/netfilter/xt_connlimit.c
@@ -188,7 +188,7 @@ connlimit_mt(const struct sk_buff *skb, struct xt_action_param *par)
 	ct = nf_ct_get(skb, &ctinfo);
 	if (ct != NULL)
 		tuple_ptr = &ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple;
-	else if (!nf_ct_get_tuplepr(skb, skb_network_offset(skb),
+	else if (!nf_ct_get_tuplepr(net, skb, skb_network_offset(skb),
 				    par->family, &tuple))
 		goto hotdrop;
 
-- 
1.7.0.4

[PATCH 2/2] net: netfilter: handle invalid packets consistently in conntrack

[Jozsef Kadlecsik]

IPv6 conntrack marked invalid packets as INVALID and let the user
drop those by an explicit rule, while IPv4 conntrack dropped such
packets itself.

IPv4 conntrack is changed so that it marks INVALID packets and lets
the user to drop them.

Invalid packet logging support added to catch why the packet is marked
as INVALID.

Signed-off-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
---
 net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c |   26 ++++++++++++++++-------
 net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c |    8 +++++-
 2 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c b/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c
index af7cdc7..97ad520 100644
--- a/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c
+++ b/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c
@@ -74,23 +74,33 @@ static int ipv4_get_l4proto(const struct net *net,
 	struct iphdr _iph;
 
 	iph = skb_header_pointer(skb, nhoff, sizeof(_iph), &_iph);
-	if (iph == NULL)
-		return -NF_DROP;
+	if (iph == NULL) {
+		if (LOG_INVALID(net, IPPROTO_RAW))
+			nf_log_packet(NFPROTO_IPV4, 0, skb, NULL, NULL, NULL,
+				"nf_conntrack_ipv4: can't get IP header\n");
+		return -NF_ACCEPT;
+	}
 
 	/* Conntrack defragments packets, we might still see fragments
 	 * inside ICMP packets though. */
-	if (iph->frag_off & htons(IP_OFFSET))
-		return -NF_DROP;
+	if (iph->frag_off & htons(IP_OFFSET)) {
+		if (LOG_INVALID(net, IPPROTO_RAW))
+			nf_log_packet(NFPROTO_IPV4, 0, skb, NULL, NULL, NULL,
+				"nf_conntrack_ipv4: can't handle fragment\n");
+		return -NF_ACCEPT;
+	}
 
 	*dataoff = nhoff + (iph->ihl << 2);
 	*protonum = iph->protocol;
 
 	/* Check bogus IP headers */
 	if (*dataoff > skb->len) {
-		pr_debug("nf_conntrack_ipv4: drop bogus IPv4 packet: "
-			 "nhoff %u, ihl %u, skblen %u\n",
-			 nhoff, iph->ihl << 2, skb->len);
-		return -NF_DROP;
+		if (LOG_INVALID(net, IPPROTO_RAW))
+			nf_log_packet(NFPROTO_IPV4, 0, skb, NULL, NULL, NULL,
+				"nf_conntrack_ipv4: bogus IPv4 packet: "
+				"nhoff %u, ihl %u, skblen %u\n",
+				nhoff, iph->ihl << 2, skb->len);
+		return -NF_ACCEPT;
 	}
 
 	return NF_ACCEPT;
diff --git a/net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c b/net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c
index c65c060..c106fab 100644
--- a/net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c
+++ b/net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c
@@ -126,7 +126,9 @@ static int ipv6_get_l4proto(const struct net *net,
 
 	if (skb_copy_bits(skb, nhoff + offsetof(struct ipv6hdr, nexthdr),
 			  &pnum, sizeof(pnum)) != 0) {
-		pr_debug("ip6_conntrack_core: can't get nexthdr\n");
+		if (LOG_INVALID(net, IPPROTO_RAW))
+			nf_log_packet(NFPROTO_IPV6, 0, skb, NULL, NULL, NULL,
+				"nf_conntrack_ipv6: can't get nexthdr\n");
 		return -NF_ACCEPT;
 	}
 	protoff = nf_ct_ipv6_skip_exthdr(skb, extoff, &pnum, skb->len - extoff);
@@ -135,7 +137,9 @@ static int ipv6_get_l4proto(const struct net *net,
 	 * except of IPv6 & ext headers. but it's tracked anyway. - YK
 	 */
 	if ((protoff < 0) || (protoff > skb->len)) {
-		pr_debug("ip6_conntrack_core: can't find proto in pkt\n");
+		if (LOG_INVALID(net, IPPROTO_RAW))
+			nf_log_packet(NFPROTO_IPV6, 0, skb, NULL, NULL, NULL,
+				"nf_conntrack_ipv6: can't find proto in pkt\n");
 		return -NF_ACCEPT;
 	}
 
-- 
1.7.0.4

Re: [PATCH 0/2] Handle invalid packets in conntrack consistently

[Pablo Neira Ayuso]

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

Hi Jozsef,

On Fri, Apr 06, 2012 at 04:57:31PM +0200, Jozsef Kadlecsik wrote:
> Hi Pablo,
> 
> As it was discussed, at the moment conntrack handles invalid packets
> differently: IPv6 conntrack marks the packets as INVALID and lets
> the user to drop them by an explicit rule, while IPv4 conntrack
> simply drops such packets.
> 
> The next two patches bring conntrack in sync by changing IPv4 conntrack
> behaviour to follow IPv6 conntrack. Invalid packet logging support is
> also added.
> 
> The patches are follow-up of the second version of the patch I sent on
> Tuesday, with the cover letter subject "Drop malformed IPv4 packets in
> conntrack, 2nd try".

If you don't mind, I'll submit the following reworked patches.
Basically, it initially fixes the inconsistency in the handling of bad
packets between IPv4 and IPv6, then the packets with wrong ihl.

I have left the change to add logging out, I think that belongs to
net-next. I'll recover that once I start collecting patches for it
(that will happen along this week).

I have kept you as author, they are basically yours with little
changes, I hope that you don't mind.

[-- Attachment #2: 0001-netfilter-nf_ct_ipv4-handle-invalid-IPv4-and-IPv6-pa.patch --]
[-- Type: text/x-diff, Size: 1454 bytes --]

>From 8430eac2f6a3c2adce22d490e2ab8bb50d59077a Mon Sep 17 00:00:00 2001
From: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
Date: Mon, 9 Apr 2012 16:32:16 +0200
Subject: [PATCH 1/2] netfilter: nf_ct_ipv4: handle invalid IPv4 and IPv6 packets consistently

IPv6 conntrack marked invalid packets as INVALID and let the user
drop those by an explicit rule, while IPv4 conntrack dropped such
packets itself.

IPv4 conntrack is changed so that it marks INVALID packets and let
the user to drop them.

Signed-off-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c b/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c
index de9da21..750b06a 100644
--- a/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c
+++ b/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c
@@ -74,12 +74,12 @@ static int ipv4_get_l4proto(const struct sk_buff *skb, unsigned int nhoff,
 
 	iph = skb_header_pointer(skb, nhoff, sizeof(_iph), &_iph);
 	if (iph == NULL)
-		return -NF_DROP;
+		return -NF_ACCEPT;
 
 	/* Conntrack defragments packets, we might still see fragments
 	 * inside ICMP packets though. */
 	if (iph->frag_off & htons(IP_OFFSET))
-		return -NF_DROP;
+		return -NF_ACCEPT;
 
 	*dataoff = nhoff + (iph->ihl << 2);
 	*protonum = iph->protocol;
-- 
1.7.2.5


[-- Attachment #3: 0002-netfilter-nf_ct_ipv4-packets-with-wrong-ihl-are-inva.patch --]
[-- Type: text/x-diff, Size: 1969 bytes --]

>From 22c6a3f87f4c36866269af66f26dd640ff2adc16 Mon Sep 17 00:00:00 2001
From: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
Date: Tue, 3 Apr 2012 22:02:01 +0200
Subject: [PATCH 2/2] netfilter: nf_ct_ipv4: packets with wrong ihl are invalid

It was reported that the Linux kernel sometimes logs:

klogd: [2629147.402413] kernel BUG at net / netfilter /
nf_conntrack_proto_tcp.c: 447!
klogd: [1072212.887368] kernel BUG at net / netfilter /
nf_conntrack_proto_tcp.c: 392

ipv4_get_l4proto() in nf_conntrack_l3proto_ipv4.c and tcp_error() in
nf_conntrack_proto_tcp.c should catch malformed packets, so the errors
at the indicated lines - TCP options parsing - should not happen.
However, tcp_error() relies on the "dataoff" offset to the TCP header,
calculated by ipv4_get_l4proto().  But ipv4_get_l4proto() does not check
bogus ihl values in IPv4 packets, which then can slip through tcp_error()
and get caught at the TCP options parsing routines.

The patch fixes ipv4_get_l4proto() by dropping packets with bogus
ihl value.

The patch closes netfilter bugzilla id 771.

Signed-off-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c b/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c
index 750b06a..7437832 100644
--- a/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c
+++ b/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c
@@ -84,6 +84,14 @@ static int ipv4_get_l4proto(const struct sk_buff *skb, unsigned int nhoff,
 	*dataoff = nhoff + (iph->ihl << 2);
 	*protonum = iph->protocol;
 
+	/* Check bogus IP headers */
+	if (*dataoff > skb->len) {
+		pr_debug("nf_conntrack_ipv4: drop bogus IPv4 packet: "
+			 "nhoff %u, ihl %u, skblen %u\n",
+			 nhoff, iph->ihl << 2, skb->len);
+		return -NF_ACCEPT;
+	}
+
 	return NF_ACCEPT;
 }
 
-- 
1.7.2.5