[PATCH] NETFILTER: Fix {ip, ip6, arp}_tables hook validation

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: Dmitry Mishin <dim@openvz.org>
Signed-off-by: Patrick McHardy <kaber@trash.net>
Signed-off-by: Chris Wright <chrisw@sous-sol.org>

diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
index 413c2d0a1f3dee117db48289865279113f70e348..71b76ade00e13fa9dee0bd00ec05578e910825b5 100644 (file)
--- a/net/ipv4/netfilter/arp_tables.c
+++ b/net/ipv4/netfilter/arp_tables.c
@@ -375,6 +375,13 @@ static int mark_source_chains(struct xt_table_info *newinfo,
                            && 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.
                                 */
@@ -404,6 +411,14 @@ static int mark_source_chains(struct xt_table_info *newinfo,
                                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);
@@ -426,8 +441,6 @@ static int mark_source_chains(struct xt_table_info *newinfo,
 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))) {
@@ -437,18 +450,6 @@ static inline int standard_check(const struct arpt_entry_target *t,
                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;
 }
 
@@ -627,18 +628,20 @@ static int translate_table(const char *name,
                }
        }
 
+       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)
-               goto cleanup;
-
-       ret = -ELOOP;
-       if (!mark_source_chains(newinfo, valid_hooks, entry0)) {
-               duprintf("Looping hook\n");
-               goto cleanup;
+       if (ret != 0) {
+               ARPT_ENTRY_ITERATE(entry0, newinfo->size,
+                               cleanup_entry, &i);
+               return ret;
        }
 
        /* And one copy for every other CPU */
@@ -647,9 +650,6 @@ static int translate_table(const char *name,
                        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 8a455439b128258de3e42926640834975579d5af..2bddf8491982f31e032ed6dab0972fa90d191c22 100644 (file)
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -401,6 +401,13 @@ mark_source_chains(struct xt_table_info *newinfo,
                            && unconditional(&e->ip)) {
                                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 {
@@ -438,6 +445,13 @@ mark_source_chains(struct xt_table_info *newinfo,
                                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);
@@ -469,27 +483,6 @@ cleanup_match(struct ipt_entry_match *m, unsigned int *i)
        return 0;
 }
 
-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,
@@ -576,12 +569,7 @@ check_entry(struct ipt_entry *e, const char *name, unsigned int size,
        if (ret)
                goto err;
 
-       if (t->u.kernel.target == &ipt_standard_target) {
-               if (!standard_check(t, size)) {
-                       ret = -EINVAL;
-                       goto err;
-               }
-       } else if (t->u.kernel.target->checkentry
+       if (t->u.kernel.target->checkentry
                   && !t->u.kernel.target->checkentry(name, e, target, t->data,
                                                      e->comefrom)) {
                duprintf("ip_tables: check failed for `%s'.\n",
@@ -718,17 +706,19 @@ 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)
-               goto cleanup;
-
-       ret = -ELOOP;
-       if (!mark_source_chains(newinfo, valid_hooks, entry0))
-               goto cleanup;
+       if (ret != 0) {
+               IPT_ENTRY_ITERATE(entry0, newinfo->size,
+                               cleanup_entry, &i);
+               return ret;
+       }
 
        /* And one copy for every other CPU */
        for_each_possible_cpu(i) {
@@ -736,9 +726,6 @@ 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;
 }
 
@@ -1591,18 +1578,13 @@ static int compat_copy_entry_from_user(struct ipt_entry *e, void **dstptr,
        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, de->comefrom)) {
                duprintf("ip_tables: compat: check failed for `%s'.\n",
                         t->u.kernel.target->name);
-               goto err;
+               ret = -EINVAL;
        }
-       ret = 0;
 err:
        return ret;
 }

diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c
index 204e02162d494ab24c0a1663d2e359db471f89e3..626dcaf0378d658ad5c48a52a9099b3ce4a68ae8 100644 (file)
--- a/net/ipv6/netfilter/ip6_tables.c
+++ b/net/ipv6/netfilter/ip6_tables.c
@@ -440,6 +440,13 @@ mark_source_chains(struct xt_table_info *newinfo,
                            && 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 {
@@ -477,6 +484,13 @@ mark_source_chains(struct xt_table_info *newinfo,
                                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);
@@ -508,27 +522,6 @@ cleanup_match(struct ip6t_entry_match *m, unsigned int *i)
        return 0;
 }
 
-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,
@@ -616,12 +609,7 @@ check_entry(struct ip6t_entry *e, const char *name, unsigned int size,
        if (ret)
                goto err;
 
-       if (t->u.kernel.target == &ip6t_standard_target) {
-               if (!standard_check(t, size)) {
-                       ret = -EINVAL;
-                       goto err;
-               }
-       } else if (t->u.kernel.target->checkentry
+       if (t->u.kernel.target->checkentry
                   && !t->u.kernel.target->checkentry(name, e, target, t->data,
                                                      e->comefrom)) {
                duprintf("ip_tables: check failed for `%s'.\n",
@@ -758,17 +746,19 @@ 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)
-               goto cleanup;
-
-       ret = -ELOOP;
-       if (!mark_source_chains(newinfo, valid_hooks, entry0))
-               goto cleanup;
+       if (ret != 0) {
+               IP6T_ENTRY_ITERATE(entry0, newinfo->size,
+                                  cleanup_entry, &i);
+               return ret;
+       }
 
        /* And one copy for every other CPU */
        for_each_possible_cpu(i) {
@@ -777,9 +767,6 @@ translate_table(const char *name,
        }
 
        return 0;
-cleanup:
-       IP6T_ENTRY_ITERATE(entry0, newinfo->size, cleanup_entry, &i);
-       return ret;
 }
 
 /* Gets counters. */