-From stable-bounces@linux.kernel.org Thu Nov 16 21:40:15 2006
+From stable-bounces@linux.kernel.org Tue Nov 21 02:39:12 2006
+Message-ID: <4562D63F.5060207@trash.net>
+Date: Tue, 21 Nov 2006 11:34:39 +0100
From: Patrick McHardy <kaber@trash.net>
-To: stable@kernel.org
-Message-Id: <20061117053545.10231.94652.sendpatchset@localhost.localdomain>
-Date: Fri, 17 Nov 2006 06:35:45 +0100 (MET)
-Cc: Patrick McHardy <kaber@trash.net>, davem@davemloft.net
-Subject: NETFILTER: Missed and reordered checks in {arp, ip, ip6}_tables
+To: Chris Wright <chrisw@sous-sol.org>
+Cc: dim@openvz.org, dev@openvz.org, davem@davemloft.net, stable@kernel.org
+Subject: NETFILTER: Missed and reordered checks in {arp,ip,ip6}_tables
+
+Backport fix for missing ruleset validation in {arp,ip,ip6}_tables
+and a fix on top which fixes a regression in the first patch.
There is a number of issues in parsing user-provided table in
translate_table(). Malicious user with CAP_NET_ADMIN may crash system by
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>
-Signed-off-by: David S. Miller <davem@davemloft.net>
+Upstream commit 590bdf7fd2292b47c428111cb1360e312eff207e introduced a
+regression in match/target hook validation. mark_source_chains builds
+a bitmask for each rule representing the hooks it can be reached from,
+which is then used by the matches and targets to make sure they are
+only called from valid hooks. The patch moved the match/target specific
+validation before the mark_source_chains call, at which point the mask
+is always zero.
+
+This patch returns back to the old order and moves the standard checks
+to mark_source_chains. This allows to get rid of a special case for
+standard targets as a nice side-effect.
+
+Signed-off-by: Patrick McHardy
Signed-off-by: Chris Wright <chrisw@sous-sol.org>
---
-commit 0ef4760e162ea44c847cca7393b36e5bcac5414e
-tree 7036ce51d75aaf46d5c4abca281956c39caced10
-parent 94a3d63f9ca6cb404f62ee4186d20fec3e8bdc97
-author Patrick McHardy <kaber@trash.net> Fri, 17 Nov 2006 06:24:10 +0100
-committer Patrick McHardy <kaber@trash.net> Fri, 17 Nov 2006 06:24:10 +0100
+commit 1cfcb663c5a6d8b4b1172ff481af1b597bc8b54e
+tree 61c5b135ee292681f38945a3549cb9005aec1d7c
+parent b2ab160e1a3a1eb3fcc80132d8d7db5687a7a113
+author Patrick McHardy <kaber@trash.net> Tue, 21 Nov 2006 11:17:03 +0100
+committer Patrick McHardy <kaber@trash.net> Tue, 21 Nov 2006 11:24:51 +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(-)
+ net/ipv4/netfilter/arp_tables.c | 37 +++++++++++++---------
+ net/ipv4/netfilter/ip_tables.c | 65 +++++++++++++++++++---------------------
+ net/ipv6/netfilter/ip6_tables.c | 53 ++++++++++++++------------------
+ 3 files changed, 77 insertions(+), 78 deletions(-)
--- linux-2.6.18.3.orig/net/ipv4/netfilter/arp_tables.c
+++ linux-2.6.18.3/net/ipv4/netfilter/arp_tables.c
-@@ -471,7 +471,13 @@ static inline int check_entry(struct arp
+@@ -380,6 +380,13 @@ static int mark_source_chains(struct xt_
+ && unconditional(&e->arp)) {
+ unsigned int oldpos, size;
+
++ if (t->verdict < -NF_MAX_VERDICT - 1) {
++ duprintf("mark_source_chains: bad "
++ "negative verdict (%i)\n",
++ t->verdict);
++ return 0;
++ }
++
+ /* Return: backtrack through the last
+ * big jump.
+ */
+@@ -409,6 +416,14 @@ static int mark_source_chains(struct xt_
+ if (strcmp(t->target.u.user.name,
+ ARPT_STANDARD_TARGET) == 0
+ && newpos >= 0) {
++ if (newpos > newinfo->size -
++ sizeof(struct arpt_entry)) {
++ duprintf("mark_source_chains: "
++ "bad verdict (%i)\n",
++ newpos);
++ return 0;
++ }
++
+ /* This a jump; chase it. */
+ duprintf("Jump rule %u -> %u\n",
+ pos, newpos);
+@@ -431,8 +446,6 @@ static int mark_source_chains(struct xt_
+ static inline int standard_check(const struct arpt_entry_target *t,
+ unsigned int max_offset)
+ {
+- struct arpt_standard_target *targ = (void *)t;
+-
+ /* Check standard info. */
+ if (t->u.target_size
+ != ARPT_ALIGN(sizeof(struct arpt_standard_target))) {
+@@ -442,18 +455,6 @@ static inline int standard_check(const s
+ return 0;
+ }
+
+- if (targ->verdict >= 0
+- && targ->verdict > max_offset - sizeof(struct arpt_entry)) {
+- duprintf("arpt_standard_check: bad verdict (%i)\n",
+- targ->verdict);
+- return 0;
+- }
+-
+- if (targ->verdict < -NF_MAX_VERDICT - 1) {
+- duprintf("arpt_standard_check: bad negative verdict (%i)\n",
+- targ->verdict);
+- return 0;
+- }
+ return 1;
+ }
+
+@@ -471,7 +472,13 @@ static inline int check_entry(struct arp
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);
-@@ -629,20 +635,18 @@ static int translate_table(const char *n
- }
- }
+@@ -641,7 +648,7 @@ 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,
+ 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;
++ cleanup_entry, &i);
+ return ret;
}
- /* And one copy for every other CPU */
-@@ -651,6 +655,9 @@ static int translate_table(const char *n
- memcpy(newinfo->entries[i], entry0, newinfo->size);
- }
+--- linux-2.6.18.3.orig/net/ipv4/netfilter/ip_tables.c
++++ linux-2.6.18.3/net/ipv4/netfilter/ip_tables.c
+@@ -404,6 +404,13 @@ mark_source_chains(struct xt_table_info
+ && unconditional(&e->ip)) {
+ unsigned int oldpos, size;
-+ return 0;
-+cleanup:
-+ ARPT_ENTRY_ITERATE(entry0, newinfo->size, cleanup_entry, &i);
- return ret;
++ if (t->verdict < -NF_MAX_VERDICT - 1) {
++ duprintf("mark_source_chains: bad "
++ "negative verdict (%i)\n",
++ t->verdict);
++ return 0;
++ }
++
+ /* Return: backtrack through the last
+ big jump. */
+ do {
+@@ -441,6 +448,13 @@ mark_source_chains(struct xt_table_info
+ if (strcmp(t->target.u.user.name,
+ IPT_STANDARD_TARGET) == 0
+ && newpos >= 0) {
++ if (newpos > newinfo->size -
++ sizeof(struct ipt_entry)) {
++ duprintf("mark_source_chains: "
++ "bad verdict (%i)\n",
++ newpos);
++ return 0;
++ }
+ /* This a jump; chase it. */
+ duprintf("Jump rule %u -> %u\n",
+ pos, newpos);
+@@ -474,27 +488,6 @@ cleanup_match(struct ipt_entry_match *m,
}
---- linux-2.6.18.3.orig/net/ipv4/netfilter/ip_tables.c
-+++ linux-2.6.18.3/net/ipv4/netfilter/ip_tables.c
-@@ -552,12 +552,18 @@ check_entry(struct ipt_entry *e, const c
+ static inline int
+-standard_check(const struct ipt_entry_target *t,
+- unsigned int max_offset)
+-{
+- struct ipt_standard_target *targ = (void *)t;
+-
+- /* Check standard info. */
+- if (targ->verdict >= 0
+- && targ->verdict > max_offset - sizeof(struct ipt_entry)) {
+- duprintf("ipt_standard_check: bad verdict (%i)\n",
+- targ->verdict);
+- return 0;
+- }
+- if (targ->verdict < -NF_MAX_VERDICT - 1) {
+- duprintf("ipt_standard_check: bad negative verdict (%i)\n",
+- targ->verdict);
+- return 0;
+- }
+- return 1;
+-}
+-
+-static inline int
+ check_match(struct ipt_entry_match *m,
+ const char *name,
+ const struct ipt_ip *ip,
+@@ -552,12 +545,18 @@ check_entry(struct ipt_entry *e, const c
return -EINVAL;
}
target = try_then_request_module(xt_find_target(AF_INET,
t->u.user.name,
t->u.user.revision),
-@@ -720,19 +726,17 @@ translate_table(const char *name,
- }
- }
+@@ -575,12 +574,7 @@ check_entry(struct ipt_entry *e, const c
+ if (ret)
+ goto err;
-- 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 (t->u.kernel.target == &ipt_standard_target) {
+- if (!standard_check(t, size)) {
+- ret = -EINVAL;
+- goto cleanup_matches;
+- }
+- } else if (t->u.kernel.target->checkentry
++ if (t->u.kernel.target->checkentry
+ && !t->u.kernel.target->checkentry(name, e, target, t->data,
+ t->u.target_size
+ - sizeof(*t),
+@@ -730,7 +724,7 @@ translate_table(const char *name,
-- if (ret != 0) {
-- IPT_ENTRY_ITERATE(entry0, newinfo->size,
+ 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) {
-@@ -740,6 +744,9 @@ translate_table(const char *name,
- memcpy(newinfo->entries[i], entry0, newinfo->size);
++ cleanup_entry, &i);
+ return ret;
}
-+ return 0;
-+cleanup:
-+ IPT_ENTRY_ITERATE(entry0, newinfo->size, cleanup_entry, &i);
- return ret;
- }
-
-@@ -1531,6 +1538,10 @@ check_compat_entry_size_and_hooks(struct
+@@ -1531,6 +1525,10 @@ check_compat_entry_size_and_hooks(struct
return -EINVAL;
}
off = 0;
entry_offset = (void *)e - (void *)base;
j = 0;
-@@ -1540,6 +1551,9 @@ check_compat_entry_size_and_hooks(struct
+@@ -1540,6 +1538,9 @@ check_compat_entry_size_and_hooks(struct
goto cleanup_matches;
t = ipt_get_target(e);
target = try_then_request_module(xt_find_target(AF_INET,
t->u.user.name,
t->u.user.revision),
+@@ -1656,19 +1657,15 @@ static int compat_copy_entry_from_user(s
+ if (ret)
+ goto err;
+
+- ret = -EINVAL;
+- if (t->u.kernel.target == &ipt_standard_target) {
+- if (!standard_check(t, *size))
+- goto err;
+- } else if (t->u.kernel.target->checkentry
++ if (t->u.kernel.target->checkentry
+ && !t->u.kernel.target->checkentry(name, de, target,
+ t->data, t->u.target_size - sizeof(*t),
+ de->comefrom)) {
+ duprintf("ip_tables: compat: check failed for `%s'.\n",
+ t->u.kernel.target->name);
++ ret = -EINVAL;
+ goto err;
+ }
+- ret = 0;
+ err:
+ return ret;
+ }
--- linux-2.6.18.3.orig/net/ipv6/netfilter/ip6_tables.c
+++ linux-2.6.18.3/net/ipv6/netfilter/ip6_tables.c
-@@ -592,12 +592,19 @@ check_entry(struct ip6t_entry *e, const
+@@ -444,6 +444,13 @@ mark_source_chains(struct xt_table_info
+ && unconditional(&e->ipv6)) {
+ unsigned int oldpos, size;
+
++ if (t->verdict < -NF_MAX_VERDICT - 1) {
++ duprintf("mark_source_chains: bad "
++ "negative verdict (%i)\n",
++ t->verdict);
++ return 0;
++ }
++
+ /* Return: backtrack through the last
+ big jump. */
+ do {
+@@ -481,6 +488,13 @@ mark_source_chains(struct xt_table_info
+ if (strcmp(t->target.u.user.name,
+ IP6T_STANDARD_TARGET) == 0
+ && newpos >= 0) {
++ if (newpos > newinfo->size -
++ sizeof(struct ip6t_entry)) {
++ duprintf("mark_source_chains: "
++ "bad verdict (%i)\n",
++ newpos);
++ return 0;
++ }
+ /* This a jump; chase it. */
+ duprintf("Jump rule %u -> %u\n",
+ pos, newpos);
+@@ -514,27 +528,6 @@ cleanup_match(struct ip6t_entry_match *m
+ }
+
+ static inline int
+-standard_check(const struct ip6t_entry_target *t,
+- unsigned int max_offset)
+-{
+- struct ip6t_standard_target *targ = (void *)t;
+-
+- /* Check standard info. */
+- if (targ->verdict >= 0
+- && targ->verdict > max_offset - sizeof(struct ip6t_entry)) {
+- duprintf("ip6t_standard_check: bad verdict (%i)\n",
+- targ->verdict);
+- return 0;
+- }
+- if (targ->verdict < -NF_MAX_VERDICT - 1) {
+- duprintf("ip6t_standard_check: bad negative verdict (%i)\n",
+- targ->verdict);
+- return 0;
+- }
+- return 1;
+-}
+-
+-static inline int
+ check_match(struct ip6t_entry_match *m,
+ const char *name,
+ const struct ip6t_ip6 *ipv6,
+@@ -592,12 +585,19 @@ check_entry(struct ip6t_entry *e, const
return -EINVAL;
}
target = try_then_request_module(xt_find_target(AF_INET6,
t->u.user.name,
t->u.user.revision),
-@@ -760,19 +767,17 @@ translate_table(const char *name,
- }
- }
+@@ -615,12 +615,7 @@ check_entry(struct ip6t_entry *e, const
+ if (ret)
+ goto err;
-- 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 (t->u.kernel.target == &ip6t_standard_target) {
+- if (!standard_check(t, size)) {
+- ret = -EINVAL;
+- goto cleanup_matches;
+- }
+- } else if (t->u.kernel.target->checkentry
++ if (t->u.kernel.target->checkentry
+ && !t->u.kernel.target->checkentry(name, e, target, t->data,
+ t->u.target_size
+ - sizeof(*t),
+@@ -770,7 +765,7 @@ translate_table(const char *name,
-- if (ret != 0) {
-- IP6T_ENTRY_ITERATE(entry0, newinfo->size,
+ 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;
++ cleanup_entry, &i);
+ return ret;
+ }
- /* And one copy for every other CPU */
- for_each_possible_cpu(i) {
-@@ -780,6 +785,9 @@ translate_table(const char *name,
+@@ -780,7 +775,7 @@ translate_table(const char *name,
memcpy(newinfo->entries[i], entry0, newinfo->size);
}
+- return ret;
+ return 0;
-+cleanup:
-+ IP6T_ENTRY_ITERATE(entry0, newinfo->size, cleanup_entry, &i);
- return ret;
}
+ /* Gets counters. */
projects / thirdparty / kernel / stable-queue.git / commitdiff
