* [PATCH nf RFC] netfilter: x_tables: only allow jumps to user-defined chains
@ 2018-02-07 13:20 Florian Westphal
2018-02-14 20:04 ` Pablo Neira Ayuso
0 siblings, 1 reply; 2+ messages in thread
From: Florian Westphal @ 2018-02-07 13:20 UTC (permalink / raw)
To: netfilter-devel; +Cc: Florian Westphal
This rejects rulesets where a jump occurs to a non-user defined chain.
This isn't limited in any way in the binary format (you can jump to
any rule you want within the blob structure), but iptables tools
do not offset such a feature.
Sending as RFC as this limits features that might be used by programs
that don't call xtables(-restore) tools.
This change also prevents the syzkaller reported crash as
ruleset gets rejected.
Reported-by: syzbot+e783f671527912cd9403@syzkaller.appspotmail.com
Signed-off-by: Florian Westphal <fw@strlen.de>
---
net/ipv4/netfilter/arp_tables.c | 20 ++++++++++++++------
net/ipv4/netfilter/ip_tables.c | 21 +++++++++++++++------
net/ipv6/netfilter/ip6_tables.c | 22 ++++++++++++++++------
3 files changed, 45 insertions(+), 18 deletions(-)
diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
index e3e420f3ba7b..2df708e5cf42 100644
--- a/net/ipv4/netfilter/arp_tables.c
+++ b/net/ipv4/netfilter/arp_tables.c
@@ -368,10 +368,14 @@ static int mark_source_chains(const struct xt_table_info *newinfo,
if (strcmp(t->target.u.user.name,
XT_STANDARD_TARGET) == 0 &&
newpos >= 0) {
- /* This a jump; chase it. */
- if (!xt_find_jump_offset(offsets, newpos,
- newinfo->number))
+ if (newpos >= newinfo->size)
return 0;
+ if (entry0 + newpos != ipt_next_entry(e)) {
+ /* This a jump; chase it. */
+ if (!xt_find_jump_offset(offsets, newpos,
+ newinfo->stacksize))
+ return 0;
+ }
} else {
/* ... this is a fallthru */
newpos = pos + e->next_offset;
@@ -523,6 +527,7 @@ static int translate_table(struct xt_table_info *newinfo, void *entry0,
struct arpt_entry *iter;
unsigned int *offsets;
unsigned int i;
+ bool userchain;
int ret = 0;
newinfo->size = repl->size;
@@ -548,12 +553,15 @@ static int translate_table(struct xt_table_info *newinfo, void *entry0,
repl->valid_hooks);
if (ret != 0)
goto out_free;
- if (i < repl->num_entries)
- offsets[i] = (void *)iter - entry0;
++i;
+ if (userchain) {
+ offsets[newinfo->stacksize] = (void *)iter - entry0;
+ ++newinfo->stacksize;
+ userchain = false;
+ }
if (strcmp(arpt_get_target(iter)->u.user.name,
XT_ERROR_TARGET) == 0)
- ++newinfo->stacksize;
+ userchain = true;
}
ret = -EINVAL;
diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
index e38395a8dcf2..c8adab24a883 100644
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -435,10 +435,14 @@ mark_source_chains(const struct xt_table_info *newinfo,
if (strcmp(t->target.u.user.name,
XT_STANDARD_TARGET) == 0 &&
newpos >= 0) {
- /* This a jump; chase it. */
- if (!xt_find_jump_offset(offsets, newpos,
- newinfo->number))
+ if (newpos >= newinfo->size)
return 0;
+ if (entry0 + newpos != ipt_next_entry(e)) {
+ /* This a jump; chase it. */
+ if (!xt_find_jump_offset(offsets, newpos,
+ newinfo->stacksize))
+ return 0;
+ }
} else {
/* ... this is a fallthru */
newpos = pos + e->next_offset;
@@ -671,6 +675,7 @@ translate_table(struct net *net, struct xt_table_info *newinfo, void *entry0,
struct ipt_entry *iter;
unsigned int *offsets;
unsigned int i;
+ bool userchain;
int ret = 0;
newinfo->size = repl->size;
@@ -686,6 +691,7 @@ translate_table(struct net *net, struct xt_table_info *newinfo, void *entry0,
if (!offsets)
return -ENOMEM;
i = 0;
+ userchain = false;
/* Walk through entries, checking offsets. */
xt_entry_foreach(iter, entry0, newinfo->size) {
ret = check_entry_size_and_hooks(iter, newinfo, entry0,
@@ -695,12 +701,15 @@ translate_table(struct net *net, struct xt_table_info *newinfo, void *entry0,
repl->valid_hooks);
if (ret != 0)
goto out_free;
- if (i < repl->num_entries)
- offsets[i] = (void *)iter - entry0;
++i;
+ if (userchain) {
+ offsets[newinfo->stacksize] = (void *)iter - entry0;
+ ++newinfo->stacksize;
+ userchain = false;
+ }
if (strcmp(ipt_get_target(iter)->u.user.name,
XT_ERROR_TARGET) == 0)
- ++newinfo->stacksize;
+ userchain = true;
}
ret = -EINVAL;
diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c
index 62358b93bbac..d4932b18ad14 100644
--- a/net/ipv6/netfilter/ip6_tables.c
+++ b/net/ipv6/netfilter/ip6_tables.c
@@ -453,10 +453,15 @@ mark_source_chains(const struct xt_table_info *newinfo,
if (strcmp(t->target.u.user.name,
XT_STANDARD_TARGET) == 0 &&
newpos >= 0) {
- /* This a jump; chase it. */
- if (!xt_find_jump_offset(offsets, newpos,
- newinfo->number))
+ if (newpos >= newinfo->size)
return 0;
+
+ if (entry0 + newpos != ip6t_next_entry(e)) {
+ /* This a jump; chase it. */
+ if (!xt_find_jump_offset(offsets, newpos,
+ newinfo->stacksize))
+ return 0;
+ }
} else {
/* ... this is a fallthru */
newpos = pos + e->next_offset;
@@ -689,6 +694,7 @@ translate_table(struct net *net, struct xt_table_info *newinfo, void *entry0,
struct ip6t_entry *iter;
unsigned int *offsets;
unsigned int i;
+ bool userchain;
int ret = 0;
newinfo->size = repl->size;
@@ -704,6 +710,7 @@ translate_table(struct net *net, struct xt_table_info *newinfo, void *entry0,
if (!offsets)
return -ENOMEM;
i = 0;
+ userchain = false;
/* Walk through entries, checking offsets. */
xt_entry_foreach(iter, entry0, newinfo->size) {
ret = check_entry_size_and_hooks(iter, newinfo, entry0,
@@ -713,12 +720,15 @@ translate_table(struct net *net, struct xt_table_info *newinfo, void *entry0,
repl->valid_hooks);
if (ret != 0)
goto out_free;
- if (i < repl->num_entries)
- offsets[i] = (void *)iter - entry0;
++i;
+ if (userchain) {
+ offsets[newinfo->stacksize] = (void *)iter - entry0;
+ ++newinfo->stacksize;
+ userchain = false;
+ }
if (strcmp(ip6t_get_target(iter)->u.user.name,
XT_ERROR_TARGET) == 0)
- ++newinfo->stacksize;
+ userchain = true;
}
ret = -EINVAL;
--
2.13.6
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH nf RFC] netfilter: x_tables: only allow jumps to user-defined chains
2018-02-07 13:20 [PATCH nf RFC] netfilter: x_tables: only allow jumps to user-defined chains Florian Westphal
@ 2018-02-14 20:04 ` Pablo Neira Ayuso
0 siblings, 0 replies; 2+ messages in thread
From: Pablo Neira Ayuso @ 2018-02-14 20:04 UTC (permalink / raw)
To: Florian Westphal; +Cc: netfilter-devel
On Wed, Feb 07, 2018 at 02:20:41PM +0100, Florian Westphal wrote:
> This rejects rulesets where a jump occurs to a non-user defined chain.
> This isn't limited in any way in the binary format (you can jump to
> any rule you want within the blob structure), but iptables tools
> do not offset such a feature.
>
> Sending as RFC as this limits features that might be used by programs
> that don't call xtables(-restore) tools.
>
> This change also prevents the syzkaller reported crash as
> ruleset gets rejected.
My original intention was to go for this, given our official interface
since the beginning has been iptables-restore. But given this
description makes it clear that we have chance to break third
applications relying on this binary layout, better go conservative and
keep allowing this.
My only concern so far is if this sort of flexibility, allowing us
arbitrary jumps, can cause us more problems later on.
Let me know,
Thanks!
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2018-02-14 20:04 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-07 13:20 [PATCH nf RFC] netfilter: x_tables: only allow jumps to user-defined chains Florian Westphal
2018-02-14 20:04 ` Pablo Neira Ayuso
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.