Skip to content

Commit

Permalink
[NETFILTER]: Fix {ip,ip6,arp}_tables hook validation
Browse files Browse the repository at this point in the history
Commit 590bdf7 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: David S. Miller <davem@davemloft.net>
  • Loading branch information
Dmitry Mishin authored and David S. Miller committed Dec 7, 2006
1 parent 79066ad commit 74c9c0c
Show file tree
Hide file tree
Showing 3 changed files with 72 additions and 103 deletions.
48 changes: 24 additions & 24 deletions net/ipv4/netfilter/arp_tables.c
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*/
Expand Down Expand Up @@ -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);
Expand All @@ -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))) {
Expand All @@ -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;
}

Expand Down Expand Up @@ -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 */
Expand All @@ -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;
}

Expand Down
68 changes: 25 additions & 43 deletions net/ipv4/netfilter/ip_tables.c
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -718,27 +706,26 @@ 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) {
if (newinfo->entries[i] && newinfo->entries[i] != entry0)
memcpy(newinfo->entries[i], entry0, newinfo->size);
}

return 0;
cleanup:
IPT_ENTRY_ITERATE(entry0, newinfo->size, cleanup_entry, &i);
return ret;
}

Expand Down Expand Up @@ -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;
}
Expand Down
59 changes: 23 additions & 36 deletions net/ipv6/netfilter/ip6_tables.c
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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) {
Expand All @@ -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. */
Expand Down

0 comments on commit 74c9c0c

Please sign in to comment.