Skip to content

L
linux
Commit fec74820 authored by Patrick McHardy's avatar Patrick McHardy Committed by Chris Wright
Browse files
Options

[PATCH] 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
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.

Upstream commit 590bdf7f 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: default avatarPatrick McHardy <kaber@trash.net>
Signed-off-by: default avatarChris Wright <chrisw@sous-sol.org>
parent 76f0a94e
Hide whitespace changes
Inline Side-by-side
Showing with 77 additions and 78 deletions
net/ipv4/netfilter/arp_tables.c
View file @ fec74820
......@@ -380,6 +380,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.
*/
......@@ -409,6 +416,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);
......@@ -431,8 +446,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))) {
......@@ -442,18 +455,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;
}
......@@ -471,7 +472,13 @@ static inline int check_entry(struct arpt_entry *e, const char *name, unsigned i
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);
......@@ -641,7 +648,7 @@ static int translate_table(const char *name,
if (ret != 0) {
ARPT_ENTRY_ITERATE(entry0, newinfo->size,
cleanup_entry, &i);
cleanup_entry, &i);
return ret;
}
......
net/ipv4/netfilter/ip_tables.c
View file @ fec74820
......@@ -404,6 +404,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 {
......@@ -441,6 +448,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);
......@@ -473,27 +487,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,
......@@ -552,12 +545,18 @@ check_entry(struct ipt_entry *e, const char *name, unsigned int size,
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),
......@@ -575,12 +574,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 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,
cleanup_entry, &i);
cleanup_entry, &i);
return ret;
}
......@@ -1531,6 +1525,10 @@ check_compat_entry_size_and_hooks(struct ipt_entry *e,
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;
......@@ -1540,6 +1538,9 @@ check_compat_entry_size_and_hooks(struct ipt_entry *e,
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),
......@@ -1656,19 +1657,15 @@ 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, 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;
}
......
net/ipv6/netfilter/ip6_tables.c
View file @ fec74820
......@@ -444,6 +444,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 {
......@@ -481,6 +488,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);
......@@ -513,27 +527,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,
......@@ -592,12 +585,19 @@ check_entry(struct ip6t_entry *e, const char *name, unsigned int size,
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),
......@@ -615,12 +615,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 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,
cleanup_entry, &i);
cleanup_entry, &i);
return ret;
}
......@@ -780,7 +775,7 @@ translate_table(const char *name,
memcpy(newinfo->entries[i], entry0, newinfo->size);
}
return ret;
return 0;
}
/* Gets counters. */
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7