* [PATCH 1/4] netfilter: x_tables: validate e->target_offset early
@ 2016-03-19 21:51 Florian Westphal
2016-03-19 21:51 ` [PATCH 2/4] netfilter: x_tables: don't move to non-existent next rule Florian Westphal
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Florian Westphal @ 2016-03-19 21:51 UTC (permalink / raw)
To: netfilter-devel; +Cc: Florian Westphal
We should check that e->target_offset is sane before
mark_source_chains gets called since it will fetch the target entry
for loop detection.
Signed-off-by: Florian Westphal <fw@strlen.de>
---
net/ipv4/netfilter/arp_tables.c | 17 ++++++++---------
net/ipv4/netfilter/ip_tables.c | 17 ++++++++---------
net/ipv6/netfilter/ip6_tables.c | 17 ++++++++---------
3 files changed, 24 insertions(+), 27 deletions(-)
diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
index bf08192..830bbe8 100644
--- a/net/ipv4/netfilter/arp_tables.c
+++ b/net/ipv4/netfilter/arp_tables.c
@@ -474,14 +474,12 @@ next:
return 1;
}
-static inline int check_entry(const struct arpt_entry *e, const char *name)
+static inline int check_entry(const struct arpt_entry *e)
{
const struct xt_entry_target *t;
- if (!arp_checkentry(&e->arp)) {
- duprintf("arp_tables: arp check failed %p %s.\n", e, name);
+ if (!arp_checkentry(&e->arp))
return -EINVAL;
- }
if (e->target_offset + sizeof(struct xt_entry_target) > e->next_offset)
return -EINVAL;
@@ -522,10 +520,6 @@ find_check_entry(struct arpt_entry *e, const char *name, unsigned int size)
struct xt_target *target;
int ret;
- ret = check_entry(e, name);
- if (ret)
- return ret;
-
e->counters.pcnt = xt_percpu_counter_alloc();
if (IS_ERR_VALUE(e->counters.pcnt))
return -ENOMEM;
@@ -576,6 +570,7 @@ static inline int check_entry_size_and_hooks(struct arpt_entry *e,
unsigned int valid_hooks)
{
unsigned int h;
+ int err;
if ((unsigned long)e % __alignof__(struct arpt_entry) != 0 ||
(unsigned char *)e + sizeof(struct arpt_entry) >= limit) {
@@ -590,6 +585,10 @@ static inline int check_entry_size_and_hooks(struct arpt_entry *e,
return -EINVAL;
}
+ err = check_entry(e);
+ if (err)
+ return err;
+
/* Check hooks & underflows */
for (h = 0; h < NF_ARP_NUMHOOKS; h++) {
if (!(valid_hooks & (1 << h)))
@@ -1246,7 +1245,7 @@ check_compat_entry_size_and_hooks(struct compat_arpt_entry *e,
}
/* For purposes of check_entry casting the compat entry is fine */
- ret = check_entry((struct arpt_entry *)e, name);
+ ret = check_entry((struct arpt_entry *)e);
if (ret)
return ret;
diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
index e53f8d6..1d72a3c 100644
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -569,14 +569,12 @@ static void cleanup_match(struct xt_entry_match *m, struct net *net)
}
static int
-check_entry(const struct ipt_entry *e, const char *name)
+check_entry(const struct ipt_entry *e)
{
const struct xt_entry_target *t;
- if (!ip_checkentry(&e->ip)) {
- duprintf("ip check failed %p %s.\n", e, name);
+ if (!ip_checkentry(&e->ip))
return -EINVAL;
- }
if (e->target_offset + sizeof(struct xt_entry_target) >
e->next_offset)
@@ -666,10 +664,6 @@ find_check_entry(struct ipt_entry *e, struct net *net, const char *name,
struct xt_mtchk_param mtpar;
struct xt_entry_match *ematch;
- ret = check_entry(e, name);
- if (ret)
- return ret;
-
e->counters.pcnt = xt_percpu_counter_alloc();
if (IS_ERR_VALUE(e->counters.pcnt))
return -ENOMEM;
@@ -741,6 +735,7 @@ check_entry_size_and_hooks(struct ipt_entry *e,
unsigned int valid_hooks)
{
unsigned int h;
+ int err;
if ((unsigned long)e % __alignof__(struct ipt_entry) != 0 ||
(unsigned char *)e + sizeof(struct ipt_entry) >= limit) {
@@ -755,6 +750,10 @@ check_entry_size_and_hooks(struct ipt_entry *e,
return -EINVAL;
}
+ err = check_entry(e);
+ if (err)
+ return err;
+
/* Check hooks & underflows */
for (h = 0; h < NF_INET_NUMHOOKS; h++) {
if (!(valid_hooks & (1 << h)))
@@ -1506,7 +1505,7 @@ check_compat_entry_size_and_hooks(struct compat_ipt_entry *e,
}
/* For purposes of check_entry casting the compat entry is fine */
- ret = check_entry((struct ipt_entry *)e, name);
+ ret = check_entry((struct ipt_entry *)e);
if (ret)
return ret;
diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c
index 84f9baf..26a5ad1 100644
--- a/net/ipv6/netfilter/ip6_tables.c
+++ b/net/ipv6/netfilter/ip6_tables.c
@@ -581,14 +581,12 @@ static void cleanup_match(struct xt_entry_match *m, struct net *net)
}
static int
-check_entry(const struct ip6t_entry *e, const char *name)
+check_entry(const struct ip6t_entry *e)
{
const struct xt_entry_target *t;
- if (!ip6_checkentry(&e->ipv6)) {
- duprintf("ip_tables: ip check failed %p %s.\n", e, name);
+ if (!ip6_checkentry(&e->ipv6))
return -EINVAL;
- }
if (e->target_offset + sizeof(struct xt_entry_target) >
e->next_offset)
@@ -679,10 +677,6 @@ find_check_entry(struct ip6t_entry *e, struct net *net, const char *name,
struct xt_mtchk_param mtpar;
struct xt_entry_match *ematch;
- ret = check_entry(e, name);
- if (ret)
- return ret;
-
e->counters.pcnt = xt_percpu_counter_alloc();
if (IS_ERR_VALUE(e->counters.pcnt))
return -ENOMEM;
@@ -753,6 +747,7 @@ check_entry_size_and_hooks(struct ip6t_entry *e,
unsigned int valid_hooks)
{
unsigned int h;
+ int err;
if ((unsigned long)e % __alignof__(struct ip6t_entry) != 0 ||
(unsigned char *)e + sizeof(struct ip6t_entry) >= limit) {
@@ -767,6 +762,10 @@ check_entry_size_and_hooks(struct ip6t_entry *e,
return -EINVAL;
}
+ err = check_entry(e);
+ if (err)
+ return err;
+
/* Check hooks & underflows */
for (h = 0; h < NF_INET_NUMHOOKS; h++) {
if (!(valid_hooks & (1 << h)))
@@ -1518,7 +1517,7 @@ check_compat_entry_size_and_hooks(struct compat_ip6t_entry *e,
}
/* For purposes of check_entry casting the compat entry is fine */
- ret = check_entry((struct ip6t_entry *)e, name);
+ ret = check_entry((struct ip6t_entry *)e);
if (ret)
return ret;
--
2.4.10
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/4] netfilter: x_tables: don't move to non-existent next rule
2016-03-19 21:51 [PATCH 1/4] netfilter: x_tables: validate e->target_offset early Florian Westphal
@ 2016-03-19 21:51 ` Florian Westphal
2016-03-21 0:34 ` Florian Westphal
2016-03-19 21:51 ` [PATCH 3/4] netfilter: xtables: validate targets of jumps Florian Westphal
2016-03-19 21:51 ` [PATCH 4/4] netfilter: xtables: don't attempt to alloc more than 4g Florian Westphal
2 siblings, 1 reply; 5+ messages in thread
From: Florian Westphal @ 2016-03-19 21:51 UTC (permalink / raw)
To: netfilter-devel; +Cc: Florian Westphal
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 xt_entry_foreach() macro stops iterating once
e->next_offset is out of bounds, assuming this is the last entry that
will be used.
However, if the blob is malformed its possible that mark_source_chains
function attempts to move past the last entry iff this last entry
doesn't have a verdict/jump (i.e. evaluation continues with next rule).
To fix this we check that the next address isn't above the blob size.
We know from initial xt_entry_foreach that all e->next_offset values are
sane except the last entry, where last + last->next_offset brought us above
the total_size.
Reported-by: Ben Hawkes <hawkes@google.com>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
net/ipv4/netfilter/arp_tables.c | 10 +++++++---
net/ipv4/netfilter/ip_tables.c | 6 ++++++
net/ipv6/netfilter/ip6_tables.c | 6 ++++++
3 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
index 830bbe8..2347a5c 100644
--- a/net/ipv4/netfilter/arp_tables.c
+++ b/net/ipv4/netfilter/arp_tables.c
@@ -439,6 +439,8 @@ static int mark_source_chains(const struct xt_table_info *newinfo,
size = e->next_offset;
e = (struct arpt_entry *)
(entry0 + pos + size);
+ if (pos + size >= newinfo->size)
+ return 0;
e->counters.pcnt = pos;
pos += size;
} else {
@@ -458,9 +460,13 @@ static int mark_source_chains(const struct xt_table_info *newinfo,
/* This a jump; chase it. */
duprintf("Jump rule %u -> %u\n",
pos, newpos);
+ e = (struct arpt_entry *)
+ (entry0 + newpos);
} else {
/* ... this is a fallthru */
newpos = pos + e->next_offset;
+ if (newpos >= newinfo->size)
+ return 0;
}
e = (struct arpt_entry *)
(entry0 + newpos);
@@ -690,10 +696,8 @@ static int translate_table(struct xt_table_info *newinfo, void *entry0,
}
}
- if (!mark_source_chains(newinfo, repl->valid_hooks, entry0)) {
- duprintf("Looping hook\n");
+ if (!mark_source_chains(newinfo, repl->valid_hooks, entry0))
return -ELOOP;
- }
/* Finally, each sanity check must pass */
i = 0;
diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
index 1d72a3c..07ce901 100644
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -521,6 +521,8 @@ mark_source_chains(const struct xt_table_info *newinfo,
size = e->next_offset;
e = (struct ipt_entry *)
(entry0 + pos + size);
+ if (pos + size >= newinfo->size)
+ return 0;
e->counters.pcnt = pos;
pos += size;
} else {
@@ -539,9 +541,13 @@ mark_source_chains(const struct xt_table_info *newinfo,
/* This a jump; chase it. */
duprintf("Jump rule %u -> %u\n",
pos, newpos);
+ e = (struct ipt_entry *)
+ (entry0 + newpos);
} else {
/* ... this is a fallthru */
newpos = pos + e->next_offset;
+ if (newpos >= newinfo->size)
+ return 0;
}
e = (struct ipt_entry *)
(entry0 + newpos);
diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c
index 26a5ad1..99068dc 100644
--- a/net/ipv6/netfilter/ip6_tables.c
+++ b/net/ipv6/netfilter/ip6_tables.c
@@ -533,6 +533,8 @@ mark_source_chains(const struct xt_table_info *newinfo,
size = e->next_offset;
e = (struct ip6t_entry *)
(entry0 + pos + size);
+ if (pos + size >= newinfo->size)
+ return 0;
e->counters.pcnt = pos;
pos += size;
} else {
@@ -551,9 +553,13 @@ mark_source_chains(const struct xt_table_info *newinfo,
/* This a jump; chase it. */
duprintf("Jump rule %u -> %u\n",
pos, newpos);
+ e = (struct ip6t_entry *)
+ (entry0 + newpos);
} else {
/* ... this is a fallthru */
newpos = pos + e->next_offset;
+ if (newpos >= newinfo->size)
+ return 0;
}
e = (struct ip6t_entry *)
(entry0 + newpos);
--
2.4.10
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 3/4] netfilter: xtables: validate targets of jumps
2016-03-19 21:51 [PATCH 1/4] netfilter: x_tables: validate e->target_offset early Florian Westphal
2016-03-19 21:51 ` [PATCH 2/4] netfilter: x_tables: don't move to non-existent next rule Florian Westphal
@ 2016-03-19 21:51 ` Florian Westphal
2016-03-19 21:51 ` [PATCH 4/4] netfilter: xtables: don't attempt to alloc more than 4g Florian Westphal
2 siblings, 0 replies; 5+ messages in thread
From: Florian Westphal @ 2016-03-19 21:51 UTC (permalink / raw)
To: netfilter-devel; +Cc: Florian Westphal
When we see a jump also check that the offset gets us to a beginning of
a rule (an ipt_entry).
Signed-off-by: Florian Westphal <fw@strlen.de>
---
net/ipv4/netfilter/arp_tables.c | 14 ++++++++++++++
net/ipv4/netfilter/ip_tables.c | 14 ++++++++++++++
net/ipv6/netfilter/ip6_tables.c | 14 ++++++++++++++
3 files changed, 42 insertions(+)
diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
index 2347a5c..378ea80 100644
--- a/net/ipv4/netfilter/arp_tables.c
+++ b/net/ipv4/netfilter/arp_tables.c
@@ -366,6 +366,18 @@ static inline bool unconditional(const struct arpt_arp *arp)
return memcmp(arp, &uncond, sizeof(uncond)) == 0;
}
+static bool find_jump_target(const struct xt_table_info *t,
+ const struct arpt_entry *target)
+{
+ struct arpt_entry *iter;
+
+ xt_entry_foreach(iter, t->entries, t->size) {
+ if (iter == target)
+ return true;
+ }
+ return false;
+}
+
/* Figures out from what hook each rule can be called: returns 0 if
* there are loops. Puts hook bitmask in comefrom.
*/
@@ -462,6 +474,8 @@ static int mark_source_chains(const struct xt_table_info *newinfo,
pos, newpos);
e = (struct arpt_entry *)
(entry0 + newpos);
+ if (!find_jump_target(newinfo, e))
+ return 0;
} else {
/* ... this is a fallthru */
newpos = pos + e->next_offset;
diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
index 07ce901..5d66c55 100644
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -443,6 +443,18 @@ ipt_do_table(struct sk_buff *skb,
#endif
}
+static bool find_jump_target(const struct xt_table_info *t,
+ const struct ipt_entry *target)
+{
+ struct ipt_entry *iter;
+
+ xt_entry_foreach(iter, t->entries, t->size) {
+ if (iter == target)
+ return true;
+ }
+ return false;
+}
+
/* Figures out from what hook each rule can be called: returns 0 if
there are loops. Puts hook bitmask in comefrom. */
static int
@@ -543,6 +555,8 @@ mark_source_chains(const struct xt_table_info *newinfo,
pos, newpos);
e = (struct ipt_entry *)
(entry0 + newpos);
+ if (!find_jump_target(newinfo, e))
+ return 0;
} else {
/* ... this is a fallthru */
newpos = pos + e->next_offset;
diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c
index 99068dc..6fc671c 100644
--- a/net/ipv6/netfilter/ip6_tables.c
+++ b/net/ipv6/netfilter/ip6_tables.c
@@ -455,6 +455,18 @@ ip6t_do_table(struct sk_buff *skb,
#endif
}
+static bool find_jump_target(const struct xt_table_info *t,
+ const struct ip6t_entry *target)
+{
+ struct ip6t_entry *iter;
+
+ xt_entry_foreach(iter, t->entries, t->size) {
+ if (iter == target)
+ return true;
+ }
+ return false;
+}
+
/* Figures out from what hook each rule can be called: returns 0 if
there are loops. Puts hook bitmask in comefrom. */
static int
@@ -555,6 +567,8 @@ mark_source_chains(const struct xt_table_info *newinfo,
pos, newpos);
e = (struct ip6t_entry *)
(entry0 + newpos);
+ if (!find_jump_target(newinfo, e))
+ return 0;
} else {
/* ... this is a fallthru */
newpos = pos + e->next_offset;
--
2.4.10
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 4/4] netfilter: xtables: don't attempt to alloc more than 4g
2016-03-19 21:51 [PATCH 1/4] netfilter: x_tables: validate e->target_offset early Florian Westphal
2016-03-19 21:51 ` [PATCH 2/4] netfilter: x_tables: don't move to non-existent next rule Florian Westphal
2016-03-19 21:51 ` [PATCH 3/4] netfilter: xtables: validate targets of jumps Florian Westphal
@ 2016-03-19 21:51 ` Florian Westphal
2 siblings, 0 replies; 5+ messages in thread
From: Florian Westphal @ 2016-03-19 21:51 UTC (permalink / raw)
To: netfilter-devel; +Cc: Florian Westphal
We track size in unsigned int everywhere, so better don't
even bother trying to alloc this size.
Signed-off-by: Florian Westphal <fw@strlen.de>
---
net/netfilter/x_tables.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
index 582c9cf..3740717 100644
--- a/net/netfilter/x_tables.c
+++ b/net/netfilter/x_tables.c
@@ -659,7 +659,7 @@ struct xt_table_info *xt_alloc_table_info(unsigned int size)
struct xt_table_info *info = NULL;
size_t sz = sizeof(*info) + size;
- if (sz < sizeof(*info))
+ if (sz < sizeof(*info) || sz > UINT_MAX)
return NULL;
/* Pedantry: prevent them from hitting BUG() in vmalloc.c --RR */
--
2.4.10
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/4] netfilter: x_tables: don't move to non-existent next rule
2016-03-19 21:51 ` [PATCH 2/4] netfilter: x_tables: don't move to non-existent next rule Florian Westphal
@ 2016-03-21 0:34 ` Florian Westphal
0 siblings, 0 replies; 5+ messages in thread
From: Florian Westphal @ 2016-03-21 0:34 UTC (permalink / raw)
To: Florian Westphal; +Cc: netfilter-devel
Florian Westphal <fw@strlen.de> wrote:
> 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 xt_entry_foreach() macro stops iterating once
> e->next_offset is out of bounds, assuming this is the last entry that
> will be used.
>
> However, if the blob is malformed its possible that mark_source_chains
> function attempts to move past the last entry iff this last entry
> doesn't have a verdict/jump (i.e. evaluation continues with next rule).
Problem is that the underflow check thinks the last rule is terminal
but mark_source_chains sees that its in fact a conditional rule.
So we should fix the underflow detection instead to catch this, I'll send
a different patch tomorrow.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-03-21 0:34 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-19 21:51 [PATCH 1/4] netfilter: x_tables: validate e->target_offset early Florian Westphal
2016-03-19 21:51 ` [PATCH 2/4] netfilter: x_tables: don't move to non-existent next rule Florian Westphal
2016-03-21 0:34 ` Florian Westphal
2016-03-19 21:51 ` [PATCH 3/4] netfilter: xtables: validate targets of jumps Florian Westphal
2016-03-19 21:51 ` [PATCH 4/4] netfilter: xtables: don't attempt to alloc more than 4g Florian Westphal
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).