Skip to content

Commit

Permalink
netfilter: x_tables: fix unconditional helper
Browse files Browse the repository at this point in the history 
commit 54d83fc upstream.

Ben Hawkes says:

 In the mark_source_chains function (net/ipv4/netfilter/ip_tables.c) it
 is possible for a user-supplied ipt_entry structure to have a large
 next_offset field. This field is not bounds checked prior to writing a
 counter value at the supplied offset.

Problem is that mark_source_chains should not have been called --
the rule doesn't have a next entry, so its supposed to return
an absolute verdict of either ACCEPT or DROP.

However, the function conditional() doesn't work as the name implies.
It only checks that the rule is using wildcard address matching.

However, an unconditional rule must also not be using any matches
(no -m args).

The underflow validator only checked the addresses, therefore
passing the 'unconditional absolute verdict' test, while
mark_source_chains also tested for presence of matches, and thus
proceeeded to the next (not-existent) rule.

Unify this so that all the callers have same idea of 'unconditional rule'.

Reported-by: Ben Hawkes <hawkes@google.com>
Signed-off-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
  • Loading branch information
Florian Westphal authored and Ben Hutchings committed Apr 30, 2016
1 parent 9647875 commit 366d36a
Show file tree
Hide file tree
Showing 3 changed files with 31 additions and 33 deletions.
18 changes: 9 additions & 9 deletions net/ipv4/netfilter/arp_tables.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -355,11 +355,12 @@ unsigned int arpt_do_table(struct sk_buff *skb,
}

/* All zeroes == unconditional rule. */
static inline bool unconditional(const struct arpt_arp *arp)
static inline bool unconditional(const struct arpt_entry *e)
{
static const struct arpt_arp uncond;

return memcmp(arp, &uncond, sizeof(uncond)) == 0;
return e->target_offset == sizeof(struct arpt_entry) &&
memcmp(&e->arp, &uncond, sizeof(uncond)) == 0;
}

/* Figures out from what hook each rule can be called: returns 0 if
Expand Down Expand Up @@ -398,11 +399,10 @@ static int mark_source_chains(const struct xt_table_info *newinfo,
|= ((1 << hook) | (1 << NF_ARP_NUMHOOKS));

/* Unconditional return/END. */
if ((e->target_offset == sizeof(struct arpt_entry) &&
if ((unconditional(e) &&
(strcmp(t->target.u.user.name,
XT_STANDARD_TARGET) == 0) &&
t->verdict < 0 && unconditional(&e->arp)) ||
visited) {
t->verdict < 0) || visited) {
unsigned int oldpos, size;

if ((strcmp(t->target.u.user.name,
Expand Down Expand Up @@ -541,7 +541,7 @@ static bool check_underflow(const struct arpt_entry *e)
const struct xt_entry_target *t;
unsigned int verdict;

if (!unconditional(&e->arp))
if (!unconditional(e))
return false;
t = arpt_get_target_c(e);
if (strcmp(t->u.user.name, XT_STANDARD_TARGET) != 0)
Expand Down Expand Up @@ -588,9 +588,9 @@ static inline int check_entry_size_and_hooks(struct arpt_entry *e,
newinfo->hook_entry[h] = hook_entries[h];
if ((unsigned char *)e - base == underflows[h]) {
if (!check_underflow(e)) {
pr_err("Underflows must be unconditional and "
"use the STANDARD target with "
"ACCEPT/DROP\n");
pr_debug("Underflows must be unconditional and "
"use the STANDARD target with "
"ACCEPT/DROP\n");
return -EINVAL;
}
newinfo->underflow[h] = underflows[h];
Expand Down
23 changes: 11 additions & 12 deletions net/ipv4/netfilter/ip_tables.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -168,11 +168,12 @@ get_entry(const void *base, unsigned int offset)

/* All zeroes == unconditional rule. */
/* Mildly perf critical (only if packet tracing is on) */
static inline bool unconditional(const struct ipt_ip *ip)
static inline bool unconditional(const struct ipt_entry *e)
{
static const struct ipt_ip uncond;

return memcmp(ip, &uncond, sizeof(uncond)) == 0;
return e->target_offset == sizeof(struct ipt_entry) &&
memcmp(&e->ip, &uncond, sizeof(uncond)) == 0;
#undef FWINV
}

Expand Down Expand Up @@ -229,11 +230,10 @@ get_chainname_rulenum(const struct ipt_entry *s, const struct ipt_entry *e,
} else if (s == e) {
(*rulenum)++;

if (s->target_offset == sizeof(struct ipt_entry) &&
if (unconditional(s) &&
strcmp(t->target.u.kernel.target->name,
XT_STANDARD_TARGET) == 0 &&
t->verdict < 0 &&
unconditional(&s->ip)) {
t->verdict < 0) {
/* Tail of chains: STANDARD target (return/policy) */
*comment = *chainname == hookname
? comments[NF_IP_TRACE_COMMENT_POLICY]
Expand Down Expand Up @@ -472,11 +472,10 @@ mark_source_chains(const struct xt_table_info *newinfo,
e->comefrom |= ((1 << hook) | (1 << NF_INET_NUMHOOKS));

/* Unconditional return/END. */
if ((e->target_offset == sizeof(struct ipt_entry) &&
if ((unconditional(e) &&
(strcmp(t->target.u.user.name,
XT_STANDARD_TARGET) == 0) &&
t->verdict < 0 && unconditional(&e->ip)) ||
visited) {
t->verdict < 0) || visited) {
unsigned int oldpos, size;

if ((strcmp(t->target.u.user.name,
Expand Down Expand Up @@ -703,7 +702,7 @@ static bool check_underflow(const struct ipt_entry *e)
const struct xt_entry_target *t;
unsigned int verdict;

if (!unconditional(&e->ip))
if (!unconditional(e))
return false;
t = ipt_get_target_c(e);
if (strcmp(t->u.user.name, XT_STANDARD_TARGET) != 0)
Expand Down Expand Up @@ -751,9 +750,9 @@ check_entry_size_and_hooks(struct ipt_entry *e,
newinfo->hook_entry[h] = hook_entries[h];
if ((unsigned char *)e - base == underflows[h]) {
if (!check_underflow(e)) {
pr_err("Underflows must be unconditional and "
"use the STANDARD target with "
"ACCEPT/DROP\n");
pr_debug("Underflows must be unconditional and "
"use the STANDARD target with "
"ACCEPT/DROP\n");
return -EINVAL;
}
newinfo->underflow[h] = underflows[h];
Expand Down
23 changes: 11 additions & 12 deletions net/ipv6/netfilter/ip6_tables.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -195,11 +195,12 @@ get_entry(const void *base, unsigned int offset)

/* All zeroes == unconditional rule. */
/* Mildly perf critical (only if packet tracing is on) */
static inline bool unconditional(const struct ip6t_ip6 *ipv6)
static inline bool unconditional(const struct ip6t_entry *e)
{
static const struct ip6t_ip6 uncond;

return memcmp(ipv6, &uncond, sizeof(uncond)) == 0;
return e->target_offset == sizeof(struct ip6t_entry) &&
memcmp(&e->ipv6, &uncond, sizeof(uncond)) == 0;
}

static inline const struct xt_entry_target *
Expand Down Expand Up @@ -255,11 +256,10 @@ get_chainname_rulenum(const struct ip6t_entry *s, const struct ip6t_entry *e,
} else if (s == e) {
(*rulenum)++;

if (s->target_offset == sizeof(struct ip6t_entry) &&
if (unconditional(s) &&
strcmp(t->target.u.kernel.target->name,
XT_STANDARD_TARGET) == 0 &&
t->verdict < 0 &&
unconditional(&s->ipv6)) {
t->verdict < 0) {
/* Tail of chains: STANDARD target (return/policy) */
*comment = *chainname == hookname
? comments[NF_IP6_TRACE_COMMENT_POLICY]
Expand Down Expand Up @@ -482,11 +482,10 @@ mark_source_chains(const struct xt_table_info *newinfo,
e->comefrom |= ((1 << hook) | (1 << NF_INET_NUMHOOKS));

/* Unconditional return/END. */
if ((e->target_offset == sizeof(struct ip6t_entry) &&
if ((unconditional(e) &&
(strcmp(t->target.u.user.name,
XT_STANDARD_TARGET) == 0) &&
t->verdict < 0 &&
unconditional(&e->ipv6)) || visited) {
t->verdict < 0) || visited) {
unsigned int oldpos, size;

if ((strcmp(t->target.u.user.name,
Expand Down Expand Up @@ -714,7 +713,7 @@ static bool check_underflow(const struct ip6t_entry *e)
const struct xt_entry_target *t;
unsigned int verdict;

if (!unconditional(&e->ipv6))
if (!unconditional(e))
return false;
t = ip6t_get_target_c(e);
if (strcmp(t->u.user.name, XT_STANDARD_TARGET) != 0)
Expand Down Expand Up @@ -762,9 +761,9 @@ check_entry_size_and_hooks(struct ip6t_entry *e,
newinfo->hook_entry[h] = hook_entries[h];
if ((unsigned char *)e - base == underflows[h]) {
if (!check_underflow(e)) {
pr_err("Underflows must be unconditional and "
"use the STANDARD target with "
"ACCEPT/DROP\n");
pr_debug("Underflows must be unconditional and "
"use the STANDARD target with "
"ACCEPT/DROP\n");
return -EINVAL;
}
newinfo->underflow[h] = underflows[h];
Expand Down

0 comments on commit 366d36a

Please sign in to comment.