[NETFILTER 00/05]: Netfilter fixes

[NETFILTER 00/05]: Netfilter fixes

[Patrick McHardy]

Hi Dave,

the following patches contain a few important iptables fixes from the OpenVZ
guys, a fix for a nf_conntrack regression from the listhelp.h removal and
a small Kconfig update. I'll pass the important ones on to -stable once
I've caught up with all the previous fixes.

Please apply, thanks.


 net/ipv4/netfilter/arp_tables.c   |   25 +++++++++-----
 net/ipv4/netfilter/ip_tables.c    |   67 +++++++++++++++++++-------------------
 net/ipv6/netfilter/Kconfig        |    2 -
 net/ipv6/netfilter/ip6_tables.c   |   24 +++++++++----
 net/netfilter/nf_conntrack_core.c |    3 +
 5 files changed, 69 insertions(+), 52 deletions(-)

Dmitry Mishin:
      [NETFILTER]: Missed and reordered checks in {arp,ip,ip6}_tables
      [NETFILTER]: ip_tables: compat code module refcounting fix

Martin Josefsson:
      [NETFILTER]: nf_conntrack: add missing unlock in get_next_corpse()

Peter Bieringer:
      [NETFILTER]: remove masq/NAT from ip6tables Kconfig help

Vasily Averin:
      [NETFILTER]: ip_tables: compat error way cleanup

[NETFILTER 01/05]: remove masq/NAT from ip6tables Kconfig help

[Patrick McHardy]

[NETFILTER]: remove masq/NAT from ip6tables Kconfig help

Signed-off-by: Patrick McHardy <kaber@trash.net>

---
commit 68c5f8eb167a363d431d3ed02c052a60b3902ab7
tree ba8809081f5c40a80855d4737e78a2fd3d482086
parent c55145450fa6a2632a4f0c8941dd04678e06fb3c
author Peter Bieringer <pb@bieringer.de> Wed, 25 Oct 2006 18:53:40 +0200
committer Patrick McHardy <kaber@trash.net> Wed, 25 Oct 2006 18:53:40 +0200

 net/ipv6/netfilter/Kconfig |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/ipv6/netfilter/Kconfig b/net/ipv6/netfilter/Kconfig
index 4bc4e5b..d7c45a9 100644
--- a/net/ipv6/netfilter/Kconfig
+++ b/net/ipv6/netfilter/Kconfig
@@ -40,7 +40,7 @@ config IP6_NF_QUEUE
 	  To compile it as a module, choose M here.  If unsure, say N.
 
 config IP6_NF_IPTABLES
-	tristate "IP6 tables support (required for filtering/masq/NAT)"
+	tristate "IP6 tables support (required for filtering)"
 	depends on NETFILTER_XTABLES
 	help
 	  ip6tables is a general, extensible packet identification framework.

Re: [NETFILTER 01/05]: remove masq/NAT from ip6tables Kconfig help

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Mon, 30 Oct 2006 19:18:57 +0100 (MET)

> [NETFILTER]: remove masq/NAT from ip6tables Kconfig help
> 
> Signed-off-by: Patrick McHardy <kaber@trash.net>

Applied.

[NETFILTER 02/05]: Missed and reordered checks in {arp, ip, ip6}_tables

[Patrick McHardy]

[NETFILTER]: Missed and reordered checks in {arp,ip,ip6}_tables

There is a number of issues in parsing user-provided table in
translate_table(). Malicious user with CAP_NET_ADMIN may crash system by
passing special-crafted table to the *_tables.

The first issue is that mark_source_chains() function is called before entry
content checks. In case of standard target, mark_source_chains() function
uses t->verdict field in order to determine new position. But the check, that
this field leads no further, than the table end, is in check_entry(), which
is called later, than mark_source_chains().

The second issue, that there is no check that target_offset points inside
entry. If so, *_ITERATE_MATCH macro will follow further, than the entry
ends. As a result, we'll have oops or memory disclosure.

And the third issue, that there is no check that the target is completely
inside entry. Results are the same, as in previous issue.

Signed-off-by: Dmitry Mishin <dim@openvz.org>
Acked-by: Kirill Korotaev <dev@openvz.org>
Signed-off-by: Patrick McHardy <kaber@trash.net>

---
commit b59a94e9119db15b4d28869997f615f236081e58
tree 2dada64d71bbd5606aec0960643e11f633b1513b
parent 68c5f8eb167a363d431d3ed02c052a60b3902ab7
author Dmitry Mishin <dim@openvz.org> Mon, 30 Oct 2006 16:28:13 +0100
committer Patrick McHardy <kaber@trash.net> Mon, 30 Oct 2006 16:28:13 +0100

 net/ipv4/netfilter/arp_tables.c |   25 ++++++++++++++++---------
 net/ipv4/netfilter/ip_tables.c  |   30 ++++++++++++++++++++++--------
 net/ipv6/netfilter/ip6_tables.c |   24 ++++++++++++++++--------
 3 files changed, 54 insertions(+), 25 deletions(-)

diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
index 0849f1c..413c2d0 100644
--- a/net/ipv4/netfilter/arp_tables.c
+++ b/net/ipv4/netfilter/arp_tables.c
@@ -466,7 +466,13 @@ static inline int check_entry(struct arp
 		return -EINVAL;
 	}
 
+	if (e->target_offset + sizeof(struct arpt_entry_target) > e->next_offset)
+		return -EINVAL;
+
 	t = arpt_get_target(e);
+	if (e->target_offset + t->u.target_size > e->next_offset)
+		return -EINVAL;
+
 	target = try_then_request_module(xt_find_target(NF_ARP, t->u.user.name,
 							t->u.user.revision),
 					 "arpt_%s", t->u.user.name);
@@ -621,20 +627,18 @@ static int translate_table(const char *n
 		}
 	}
 
-	if (!mark_source_chains(newinfo, valid_hooks, entry0)) {
-		duprintf("Looping hook\n");
-		return -ELOOP;
-	}
-
 	/* Finally, each sanity check must pass */
 	i = 0;
 	ret = ARPT_ENTRY_ITERATE(entry0, newinfo->size,
 				 check_entry, name, size, &i);
 
-	if (ret != 0) {
-		ARPT_ENTRY_ITERATE(entry0, newinfo->size,
-				   cleanup_entry, &i);
-		return ret;
+	if (ret != 0)
+		goto cleanup;
+
+	ret = -ELOOP;
+	if (!mark_source_chains(newinfo, valid_hooks, entry0)) {
+		duprintf("Looping hook\n");
+		goto cleanup;
 	}
 
 	/* And one copy for every other CPU */
@@ -643,6 +647,9 @@ static int translate_table(const char *n
 			memcpy(newinfo->entries[i], entry0, newinfo->size);
 	}
 
+	return 0;
+cleanup:
+	ARPT_ENTRY_ITERATE(entry0, newinfo->size, cleanup_entry, &i);
 	return ret;
 }
 
diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
index 4b90927..e2c7f6e 100644
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -547,12 +547,18 @@ check_entry(struct ipt_entry *e, const c
 		return -EINVAL;
 	}
 
+	if (e->target_offset + sizeof(struct ipt_entry_target) > e->next_offset)
+		return -EINVAL;
+
 	j = 0;
 	ret = IPT_MATCH_ITERATE(e, check_match, name, &e->ip, e->comefrom, &j);
 	if (ret != 0)
 		goto cleanup_matches;
 
 	t = ipt_get_target(e);
+	ret = -EINVAL;
+	if (e->target_offset + t->u.target_size > e->next_offset)
+			goto cleanup_matches;
 	target = try_then_request_module(xt_find_target(AF_INET,
 						     t->u.user.name,
 						     t->u.user.revision),
@@ -712,19 +718,17 @@ translate_table(const char *name,
 		}
 	}
 
-	if (!mark_source_chains(newinfo, valid_hooks, entry0))
-		return -ELOOP;
-
 	/* Finally, each sanity check must pass */
 	i = 0;
 	ret = IPT_ENTRY_ITERATE(entry0, newinfo->size,
 				check_entry, name, size, &i);
 
-	if (ret != 0) {
-		IPT_ENTRY_ITERATE(entry0, newinfo->size,
-				  cleanup_entry, &i);
-		return ret;
-	}
+	if (ret != 0)
+		goto cleanup;
+
+	ret = -ELOOP;
+	if (!mark_source_chains(newinfo, valid_hooks, entry0))
+		goto cleanup;
 
 	/* And one copy for every other CPU */
 	for_each_possible_cpu(i) {
@@ -732,6 +736,9 @@ translate_table(const char *name,
 			memcpy(newinfo->entries[i], entry0, newinfo->size);
 	}
 
+	return 0;
+cleanup:
+	IPT_ENTRY_ITERATE(entry0, newinfo->size, cleanup_entry, &i);
 	return ret;
 }
 
@@ -1463,6 +1470,10 @@ check_compat_entry_size_and_hooks(struct
 		return -EINVAL;
 	}
 
+	if (e->target_offset + sizeof(struct compat_xt_entry_target) >
+								e->next_offset)
+		return -EINVAL;
+
 	off = 0;
 	entry_offset = (void *)e - (void *)base;
 	j = 0;
@@ -1472,6 +1483,9 @@ check_compat_entry_size_and_hooks(struct
 		goto cleanup_matches;
 
 	t = ipt_get_target(e);
+	ret = -EINVAL;
+	if (e->target_offset + t->u.target_size > e->next_offset)
+			goto cleanup_matches;
 	target = try_then_request_module(xt_find_target(AF_INET,
 						     t->u.user.name,
 						     t->u.user.revision),
diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c
index 53bf977..167c2ea 100644
--- a/net/ipv6/netfilter/ip6_tables.c
+++ b/net/ipv6/netfilter/ip6_tables.c
@@ -586,12 +586,19 @@ check_entry(struct ip6t_entry *e, const 
 		return -EINVAL;
 	}
 
+	if (e->target_offset + sizeof(struct ip6t_entry_target) >
+								e->next_offset)
+		return -EINVAL;
+
 	j = 0;
 	ret = IP6T_MATCH_ITERATE(e, check_match, name, &e->ipv6, e->comefrom, &j);
 	if (ret != 0)
 		goto cleanup_matches;
 
 	t = ip6t_get_target(e);
+	ret = -EINVAL;
+	if (e->target_offset + t->u.target_size > e->next_offset)
+			goto cleanup_matches;
 	target = try_then_request_module(xt_find_target(AF_INET6,
 							t->u.user.name,
 							t->u.user.revision),
@@ -751,19 +758,17 @@ translate_table(const char *name,
 		}
 	}
 
-	if (!mark_source_chains(newinfo, valid_hooks, entry0))
-		return -ELOOP;
-
 	/* Finally, each sanity check must pass */
 	i = 0;
 	ret = IP6T_ENTRY_ITERATE(entry0, newinfo->size,
 				check_entry, name, size, &i);
 
-	if (ret != 0) {
-		IP6T_ENTRY_ITERATE(entry0, newinfo->size,
-				  cleanup_entry, &i);
-		return ret;
-	}
+	if (ret != 0)
+		goto cleanup;
+
+	ret = -ELOOP;
+	if (!mark_source_chains(newinfo, valid_hooks, entry0))
+		goto cleanup;
 
 	/* And one copy for every other CPU */
 	for_each_possible_cpu(i) {
@@ -771,6 +776,9 @@ translate_table(const char *name,
 			memcpy(newinfo->entries[i], entry0, newinfo->size);
 	}
 
+	return 0;
+cleanup:
+	IP6T_ENTRY_ITERATE(entry0, newinfo->size, cleanup_entry, &i);
 	return ret;
 }
 

Re: [NETFILTER 02/05]: Missed and reordered checks in {arp,ip,ip6}_tables

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Mon, 30 Oct 2006 19:18:58 +0100 (MET)

> [NETFILTER]: Missed and reordered checks in {arp,ip,ip6}_tables
 ...
> Signed-off-by: Dmitry Mishin <dim@openvz.org>
> Acked-by: Kirill Korotaev <dev@openvz.org>
> Signed-off-by: Patrick McHardy <kaber@trash.net>

Applied.

[NETFILTER 03/05]: ip_tables: compat error way cleanup

[Patrick McHardy]

[NETFILTER]: ip_tables: compat error way cleanup

This patch adds forgotten compat_flush_offset() call to error way of
translate_compat_table().  May lead to table corruption on the next
compat_do_replace().

Signed-off-by: Vasily Averin <vvs@openvz.org>
Acked-by: Dmitry Mishin <dim@openvz.org>
Acked-by: Kirill Korotaev <dev@openvz.org>
Signed-off-by: Patrick McHardy <kaber@trash.net>

---
commit 2e3d7fd325d9327881f17aec50619caaf1aeab00
tree a2f9d2588fbe0740bab2df52a45a44539a9b6488
parent b59a94e9119db15b4d28869997f615f236081e58
author Vasily Averin <vvs@openvz.org> Mon, 30 Oct 2006 16:28:51 +0100
committer Patrick McHardy <kaber@trash.net> Mon, 30 Oct 2006 16:28:51 +0100

 net/ipv4/netfilter/ip_tables.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
index e2c7f6e..0f4835c 100644
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -1725,6 +1725,7 @@ free_newinfo:
 out:
 	return ret;
 out_unlock:
+	compat_flush_offsets();
 	xt_compat_unlock(AF_INET);
 	goto out;
 }

Re: [NETFILTER 03/05]: ip_tables: compat error way cleanup

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Mon, 30 Oct 2006 19:19:00 +0100 (MET)

> [NETFILTER]: ip_tables: compat error way cleanup
> 
> This patch adds forgotten compat_flush_offset() call to error way of
> translate_compat_table().  May lead to table corruption on the next
> compat_do_replace().
> 
> Signed-off-by: Vasily Averin <vvs@openvz.org>
> Acked-by: Dmitry Mishin <dim@openvz.org>
> Acked-by: Kirill Korotaev <dev@openvz.org>
> Signed-off-by: Patrick McHardy <kaber@trash.net>

Applied.

[NETFILTER 04/05]: nf_conntrack: add missing unlock in get_next_corpse()

[Patrick McHardy]

[NETFILTER]: nf_conntrack: add missing unlock in get_next_corpse()

Add missing unlock in get_next_corpse() in nf_conntrack. It was missed
during the removal of listhelp.h . Also remove an unneeded use of
nf_ct_tuplehash_to_ctrack() in the same function.

Should be applied before 2.6.19 is released.

Signed-off-by: Martin Josefsson <gandalf@wlug.westbo.se>
Signed-off-by: Patrick McHardy <kaber@trash.net>

---
commit 4de9f9e71c273a4f53f82c5dd3e48f25cb3893f9
tree 7ec3e2872f3820ed76117ea463adab561d9645b9
parent 2e3d7fd325d9327881f17aec50619caaf1aeab00
author Martin Josefsson <gandalf@wlug.westbo.se> Mon, 30 Oct 2006 16:31:19 +0100
committer Patrick McHardy <kaber@trash.net> Mon, 30 Oct 2006 16:31:19 +0100

 net/netfilter/nf_conntrack_core.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 093b3dd..836541e 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -1520,9 +1520,10 @@ get_next_corpse(int (*iter)(struct nf_co
 		if (iter(ct, data))
 			goto found;
 	}
+	write_unlock_bh(&nf_conntrack_lock);
 	return NULL;
 found:
-	atomic_inc(&nf_ct_tuplehash_to_ctrack(h)->ct_general.use);
+	atomic_inc(&ct->ct_general.use);
 	write_unlock_bh(&nf_conntrack_lock);
 	return ct;
 }

Re: [NETFILTER 04/05]: nf_conntrack: add missing unlock in get_next_corpse()

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Mon, 30 Oct 2006 19:19:01 +0100 (MET)

> [NETFILTER]: nf_conntrack: add missing unlock in get_next_corpse()
> 
> Add missing unlock in get_next_corpse() in nf_conntrack. It was missed
> during the removal of listhelp.h . Also remove an unneeded use of
> nf_ct_tuplehash_to_ctrack() in the same function.
> 
> Should be applied before 2.6.19 is released.
> 
> Signed-off-by: Martin Josefsson <gandalf@wlug.westbo.se>
> Signed-off-by: Patrick McHardy <kaber@trash.net>

Applied.

[NETFILTER 05/05]: ip_tables: compat code module refcounting fix

[Patrick McHardy]

[NETFILTER]: ip_tables: compat code module refcounting fix

This patch fixes bug in iptables modules refcounting on compat error way.

As we are getting modules in check_compat_entry_size_and_hooks(), in case of
later error, we should put them all in translate_compat_table(), not  in the
compat_copy_entry_from_user() or compat_copy_match_from_user(), as it is now.

Signed-off-by: Dmitry Mishin <dim@openvz.org>
Acked-by: Vasily Averin <vvs@openvz.org>
Acked-by: Kirill Korotaev <dev@openvz.org>
Signed-off-by: Patrick McHardy <kaber@trash.net>

---
commit 56909696850411199f03d51b86d919a30d2db7e1
tree fe9013200a32706da219e287903909fcfd5c6e14
parent 4de9f9e71c273a4f53f82c5dd3e48f25cb3893f9
author Dmitry Mishin <dim@openvz.org> Mon, 30 Oct 2006 17:03:10 +0100
committer Patrick McHardy <kaber@trash.net> Mon, 30 Oct 2006 17:03:10 +0100

 net/ipv4/netfilter/ip_tables.c |   36 +++++++++++-------------------------
 1 files changed, 11 insertions(+), 25 deletions(-)

diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
index 0f4835c..8a45543 100644
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -1527,7 +1527,7 @@ cleanup_matches:
 
 static inline int compat_copy_match_from_user(struct ipt_entry_match *m,
 	void **dstptr, compat_uint_t *size, const char *name,
-	const struct ipt_ip *ip, unsigned int hookmask, int *i)
+	const struct ipt_ip *ip, unsigned int hookmask)
 {
 	struct ipt_entry_match *dm;
 	struct ipt_match *match;
@@ -1540,22 +1540,13 @@ static inline int compat_copy_match_from
 	ret = xt_check_match(match, AF_INET, dm->u.match_size - sizeof(*dm),
 			     name, hookmask, ip->proto,
 			     ip->invflags & IPT_INV_PROTO);
-	if (ret)
-		goto err;
-
-	if (m->u.kernel.match->checkentry
+	if (!ret && m->u.kernel.match->checkentry
 	    && !m->u.kernel.match->checkentry(name, ip, match, dm->data,
 					      hookmask)) {
 		duprintf("ip_tables: check failed for `%s'.\n",
 			 m->u.kernel.match->name);
 		ret = -EINVAL;
-		goto err;
 	}
-	(*i)++;
-	return 0;
-
-err:
-	module_put(m->u.kernel.match->me);
 	return ret;
 }
 
@@ -1567,19 +1558,18 @@ static int compat_copy_entry_from_user(s
 	struct ipt_target *target;
 	struct ipt_entry *de;
 	unsigned int origsize;
-	int ret, h, j;
+	int ret, h;
 
 	ret = 0;
 	origsize = *size;
 	de = (struct ipt_entry *)*dstptr;
 	memcpy(de, e, sizeof(struct ipt_entry));
 
-	j = 0;
 	*dstptr += sizeof(struct compat_ipt_entry);
 	ret = IPT_MATCH_ITERATE(e, compat_copy_match_from_user, dstptr, size,
-			name, &de->ip, de->comefrom, &j);
+			name, &de->ip, de->comefrom);
 	if (ret)
-		goto cleanup_matches;
+		goto err;
 	de->target_offset = e->target_offset - (origsize - *size);
 	t = ipt_get_target(e);
 	target = t->u.kernel.target;
@@ -1613,12 +1603,7 @@ static int compat_copy_entry_from_user(s
 		goto err;
 	}
 	ret = 0;
-	return ret;
-
 err:
-	module_put(t->u.kernel.target->me);
-cleanup_matches:
-	IPT_MATCH_ITERATE(e, cleanup_match, &j);
 	return ret;
 }
 
@@ -1632,7 +1617,7 @@ translate_compat_table(const char *name,
 		unsigned int *hook_entries,
 		unsigned int *underflows)
 {
-	unsigned int i;
+	unsigned int i, j;
 	struct xt_table_info *newinfo, *info;
 	void *pos, *entry0, *entry1;
 	unsigned int size;
@@ -1650,21 +1635,21 @@ translate_compat_table(const char *name,
 	}
 
 	duprintf("translate_compat_table: size %u\n", info->size);
-	i = 0;
+	j = 0;
 	xt_compat_lock(AF_INET);
 	/* Walk through entries, checking offsets. */
 	ret = IPT_ENTRY_ITERATE(entry0, total_size,
 				check_compat_entry_size_and_hooks,
 				info, &size, entry0,
 				entry0 + total_size,
-				hook_entries, underflows, &i, name);
+				hook_entries, underflows, &j, name);
 	if (ret != 0)
 		goto out_unlock;
 
 	ret = -EINVAL;
-	if (i != number) {
+	if (j != number) {
 		duprintf("translate_compat_table: %u not %u entries\n",
-			 i, number);
+			 j, number);
 		goto out_unlock;
 	}
 
@@ -1723,6 +1708,7 @@ translate_compat_table(const char *name,
 free_newinfo:
 	xt_free_table_info(newinfo);
 out:
+	IPT_ENTRY_ITERATE(entry0, total_size, cleanup_entry, &j);
 	return ret;
 out_unlock:
 	compat_flush_offsets();

Re: [NETFILTER 05/05]: ip_tables: compat code module refcounting fix

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Mon, 30 Oct 2006 19:19:03 +0100 (MET)

> [NETFILTER]: ip_tables: compat code module refcounting fix
> 
> This patch fixes bug in iptables modules refcounting on compat error way.
> 
> As we are getting modules in check_compat_entry_size_and_hooks(), in case of
> later error, we should put them all in translate_compat_table(), not  in the
> compat_copy_entry_from_user() or compat_copy_match_from_user(), as it is now.
> 
> Signed-off-by: Dmitry Mishin <dim@openvz.org>
> Acked-by: Vasily Averin <vvs@openvz.org>
> Acked-by: Kirill Korotaev <dev@openvz.org>
> Signed-off-by: Patrick McHardy <kaber@trash.net>

Applied.