From 74c9c0c17dea729d6089c0c82762babd02e65f84 Mon Sep 17 00:00:00 2001 From: Dmitry Mishin Date: Tue, 5 Dec 2006 13:43:50 -0800 Subject: [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 Signed-off-by: Patrick McHardy Signed-off-by: David S. Miller --- net/ipv4/netfilter/arp_tables.c | 48 ++++++++++++++--------------- net/ipv4/netfilter/ip_tables.c | 68 +++++++++++++++-------------------------- 2 files changed, 49 insertions(+), 67 deletions(-) (limited to 'net/ipv4/netfilter') diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c index 413c2d0a1f3..71b76ade00e 100644 --- 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 8a455439b12..2bddf849198 100644 --- 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; } -- cgit v1.2.3 From f6677f4312ee74f8ca68c4cc4060465607b72b41 Mon Sep 17 00:00:00 2001 From: Dmitry Mishin Date: Tue, 5 Dec 2006 13:44:07 -0800 Subject: [NETFILTER]: Fix iptables compat hook validation In compat mode, matches and targets valid hooks checks always successful due to not initialized e->comefrom field yet. This patch separates this checks from translation code and moves them after mark_source_chains() call, where these marks are initialized. Signed-off-by: Dmitry Mishin Signed-off-by; Patrick McHardy Signed-off-by: David S. Miller --- net/ipv4/netfilter/ip_tables.c | 78 +++++++++++++++++++++++++++--------------- 1 file changed, 51 insertions(+), 27 deletions(-) (limited to 'net/ipv4/netfilter') diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c index 2bddf849198..0ff2956d35e 100644 --- a/net/ipv4/netfilter/ip_tables.c +++ b/net/ipv4/netfilter/ip_tables.c @@ -1516,25 +1516,8 @@ 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) { - struct ipt_entry_match *dm; - struct ipt_match *match; - int ret; - - dm = (struct ipt_entry_match *)*dstptr; - match = m->u.kernel.match; xt_compat_match_from_user(m, dstptr, size); - - ret = xt_check_match(match, AF_INET, dm->u.match_size - sizeof(*dm), - name, hookmask, ip->proto, - ip->invflags & IPT_INV_PROTO); - 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; - } - return ret; + return 0; } static int compat_copy_entry_from_user(struct ipt_entry *e, void **dstptr, @@ -1556,7 +1539,7 @@ static int compat_copy_entry_from_user(struct ipt_entry *e, void **dstptr, ret = IPT_MATCH_ITERATE(e, compat_copy_match_from_user, dstptr, size, name, &de->ip, de->comefrom); if (ret) - goto err; + return ret; de->target_offset = e->target_offset - (origsize - *size); t = ipt_get_target(e); target = t->u.kernel.target; @@ -1569,26 +1552,62 @@ static int compat_copy_entry_from_user(struct ipt_entry *e, void **dstptr, if ((unsigned char *)de - base < newinfo->underflow[h]) newinfo->underflow[h] -= origsize - *size; } + return ret; +} + +static inline int compat_check_match(struct ipt_entry_match *m, const char *name, + const struct ipt_ip *ip, unsigned int hookmask) +{ + struct ipt_match *match; + int ret; + + match = m->u.kernel.match; + ret = xt_check_match(match, AF_INET, m->u.match_size - sizeof(*m), + name, hookmask, ip->proto, + ip->invflags & IPT_INV_PROTO); + if (!ret && m->u.kernel.match->checkentry + && !m->u.kernel.match->checkentry(name, ip, match, m->data, + hookmask)) { + duprintf("ip_tables: compat: check failed for `%s'.\n", + m->u.kernel.match->name); + ret = -EINVAL; + } + return ret; +} + +static inline int compat_check_target(struct ipt_entry *e, const char *name) +{ + struct ipt_entry_target *t; + struct ipt_target *target; + int ret; - t = ipt_get_target(de); + t = ipt_get_target(e); target = t->u.kernel.target; ret = xt_check_target(target, AF_INET, t->u.target_size - sizeof(*t), name, e->comefrom, e->ip.proto, e->ip.invflags & IPT_INV_PROTO); - if (ret) - goto err; - - if (t->u.kernel.target->checkentry - && !t->u.kernel.target->checkentry(name, de, target, - t->data, de->comefrom)) { + if (!ret && t->u.kernel.target->checkentry + && !t->u.kernel.target->checkentry(name, e, target, + t->data, e->comefrom)) { duprintf("ip_tables: compat: check failed for `%s'.\n", t->u.kernel.target->name); ret = -EINVAL; } -err: return ret; } +static inline int compat_check_entry(struct ipt_entry *e, const char *name) +{ + int ret; + + ret = IPT_MATCH_ITERATE(e, compat_check_match, name, &e->ip, + e->comefrom); + if (ret) + return ret; + + return compat_check_target(e, name); +} + static int translate_compat_table(const char *name, unsigned int valid_hooks, @@ -1677,6 +1696,11 @@ translate_compat_table(const char *name, if (!mark_source_chains(newinfo, valid_hooks, entry1)) goto free_newinfo; + ret = IPT_ENTRY_ITERATE(entry1, newinfo->size, compat_check_entry, + name); + if (ret) + goto free_newinfo; + /* And one copy for every other CPU */ for_each_possible_cpu(i) if (newinfo->entries[i] && newinfo->entries[i] != entry1) -- cgit v1.2.3