[PATCH nf] netfilter: x_tables: deal with bogus nextoffset values

[PATCH nf] netfilter: x_tables: deal with bogus nextoffset values

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

With malformed data thats not necessarily the case so we can
write outside of allocated area later as we might not have walked the
entire blob.

Fix this by simplifying mark_source_chains -- it already has to check
if nextoff is in range to catch invalid jumps, so just do the check
when we move to a next entry as well.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/ipv4/netfilter/arp_tables.c | 16 ++++++++--------
 net/ipv4/netfilter/ip_tables.c  | 15 ++++++++-------
 net/ipv6/netfilter/ip6_tables.c | 13 ++++++-------
 3 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
index b488cac..5a0b591 100644
--- a/net/ipv4/netfilter/arp_tables.c
+++ b/net/ipv4/netfilter/arp_tables.c
@@ -437,6 +437,10 @@ static int mark_source_chains(const struct xt_table_info *newinfo,
 
 				/* Move along one */
 				size = e->next_offset;
+
+				if (pos + size > newinfo->size - sizeof(*e))
+					return 0;
+
 				e = (struct arpt_entry *)
 					(entry0 + pos + size);
 				e->counters.pcnt = pos;
@@ -447,14 +451,6 @@ static int mark_source_chains(const struct xt_table_info *newinfo,
 				if (strcmp(t->target.u.user.name,
 					   XT_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);
@@ -462,6 +458,10 @@ static int mark_source_chains(const struct xt_table_info *newinfo,
 					/* ... this is a fallthru */
 					newpos = pos + e->next_offset;
 				}
+
+				if (newpos > newinfo->size - sizeof(*e))
+					return 0;
+
 				e = (struct arpt_entry *)
 					(entry0 + newpos);
 				e->counters.pcnt = pos;
diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
index b99affa..ceb995f 100644
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -519,6 +519,10 @@ mark_source_chains(const struct xt_table_info *newinfo,
 
 				/* Move along one */
 				size = e->next_offset;
+
+				if (pos + size > newinfo->size - sizeof(*e))
+					return 0;
+
 				e = (struct ipt_entry *)
 					(entry0 + pos + size);
 				e->counters.pcnt = pos;
@@ -529,13 +533,6 @@ mark_source_chains(const struct xt_table_info *newinfo,
 				if (strcmp(t->target.u.user.name,
 					   XT_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);
@@ -543,6 +540,10 @@ mark_source_chains(const struct xt_table_info *newinfo,
 					/* ... this is a fallthru */
 					newpos = pos + e->next_offset;
 				}
+
+				if (newpos > newinfo->size - sizeof(*e))
+					return 0;
+
 				e = (struct ipt_entry *)
 					(entry0 + newpos);
 				e->counters.pcnt = pos;
diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c
index 99425cf..d88a794 100644
--- a/net/ipv6/netfilter/ip6_tables.c
+++ b/net/ipv6/netfilter/ip6_tables.c
@@ -531,6 +531,8 @@ mark_source_chains(const struct xt_table_info *newinfo,
 
 				/* Move along one */
 				size = e->next_offset;
+				if (pos + size > newinfo->size - sizeof(*e))
+					return 0;
 				e = (struct ip6t_entry *)
 					(entry0 + pos + size);
 				e->counters.pcnt = pos;
@@ -541,13 +543,6 @@ mark_source_chains(const struct xt_table_info *newinfo,
 				if (strcmp(t->target.u.user.name,
 					   XT_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);
@@ -555,6 +550,10 @@ mark_source_chains(const struct xt_table_info *newinfo,
 					/* ... this is a fallthru */
 					newpos = pos + e->next_offset;
 				}
+
+				if (newpos > newinfo->size - sizeof(*e))
+					return 0;
+
 				e = (struct ip6t_entry *)
 					(entry0 + newpos);
 				e->counters.pcnt = pos;
-- 
2.4.10

Re: [PATCH nf] netfilter: x_tables: deal with bogus nextoffset values

[Pablo Neira Ayuso]

On Thu, Mar 10, 2016 at 01:56:02AM +0100, Florian Westphal 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.
> 
> With malformed data thats not necessarily the case so we can
> write outside of allocated area later as we might not have walked the
> entire blob.
> 
> Fix this by simplifying mark_source_chains -- it already has to check
> if nextoff is in range to catch invalid jumps, so just do the check
> when we move to a next entry as well.

Thanks for posting this patch so fast Florian.

It's sad that Ben didn't even take the time to reach the people that
the MAINTAINERS file shows in first place *sigh*.

I'll place this in nf-next together with remaining pending fixes, it
seems we'll have 4.5 just after this -rc7 so I don't think we'll get
there in time for this.

I'll pass this to -stable once this hits master, these patches apply
cleanly to every kernel starting 3.2.

Re: [PATCH nf] netfilter: x_tables: deal with bogus nextoffset values

[Ben Hawkes]

On Thu, Mar 10, 2016 at 6:12 AM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Thu, Mar 10, 2016 at 01:56:02AM +0100, Florian Westphal 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.
>>
>> With malformed data thats not necessarily the case so we can
>> write outside of allocated area later as we might not have walked the
>> entire blob.
>>
>> Fix this by simplifying mark_source_chains -- it already has to check
>> if nextoff is in range to catch invalid jumps, so just do the check
>> when we move to a next entry as well.
>
> Thanks for posting this patch so fast Florian.
>
> It's sad that Ben didn't even take the time to reach the people that
> the MAINTAINERS file shows in first place *sigh*.

What is sad about this precisely? I followed the documented process
for reporting a security issue
(https://www.kernel.org/doc/Documentation/SecurityBugs), and then
followed the instructions I received from this list. If you have a
problem with my actions, then I suggest you raise this with
security@kernel.org.

> I'll place this in nf-next together with remaining pending fixes, it
> seems we'll have 4.5 just after this -rc7 so I don't think we'll get
> there in time for this.
>
> I'll pass this to -stable once this hits master, these patches apply
> cleanly to every kernel starting 3.2.
>

Re: [PATCH nf] netfilter: x_tables: deal with bogus nextoffset values

[Pablo Neira Ayuso]

On Thu, Mar 10, 2016 at 06:41:24AM -0800, Ben Hawkes wrote:
> On Thu, Mar 10, 2016 at 6:12 AM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Thu, Mar 10, 2016 at 01:56:02AM +0100, Florian Westphal 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.
> >>
> >> With malformed data thats not necessarily the case so we can
> >> write outside of allocated area later as we might not have walked the
> >> entire blob.
> >>
> >> Fix this by simplifying mark_source_chains -- it already has to check
> >> if nextoff is in range to catch invalid jumps, so just do the check
> >> when we move to a next entry as well.
> >
> > Thanks for posting this patch so fast Florian.
> >
> > It's sad that Ben didn't even take the time to reach the people that
> > the MAINTAINERS file shows in first place *sigh*.
> 
> What is sad about this precisely? I followed the documented process
> for reporting a security issue
> (https://www.kernel.org/doc/Documentation/SecurityBugs), and then
> followed the instructions I received from this list. If you have a
> problem with my actions, then I suggest you raise this with
> security@kernel.org.

As in any kind of bug, you should Cc maintainers of the corresponding
subsystem. In that sense, as in any kind of bug, it would be nice if
you participate testing and reviewing the patches that were posted to
address the bug.

Thanks.

Re: [PATCH nf] netfilter: x_tables: deal with bogus nextoffset values

[Michal Kubecek]

On Thu, Mar 10, 2016 at 03:12:31PM +0100, Pablo Neira Ayuso wrote:
> On Thu, Mar 10, 2016 at 01:56:02AM +0100, Florian Westphal 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.
> > 
> > With malformed data thats not necessarily the case so we can
> > write outside of allocated area later as we might not have walked the
> > entire blob.
> > 
> > Fix this by simplifying mark_source_chains -- it already has to check
> > if nextoff is in range to catch invalid jumps, so just do the check
> > when we move to a next entry as well.
...
> I'll place this in nf-next together with remaining pending fixes, it
> seems we'll have 4.5 just after this -rc7 so I don't think we'll get
> there in time for this.

Hi,

I can't see this patch neither in nf nor in nf-next even if the other
one (netfilter: x_tables: check for size overflow) is in nf-next. Was it
omitted on purpose or is it a mistake?

                                                          Michal Kubecek

Re: [PATCH nf] netfilter: x_tables: deal with bogus nextoffset values

[Pablo Neira Ayuso]

Hi Michal,

On Fri, Mar 18, 2016 at 02:03:40PM +0100, Michal Kubecek wrote:
> I can't see this patch neither in nf nor in nf-next even if the other
> one (netfilter: x_tables: check for size overflow) is in nf-next. Was it
> omitted on purpose or is it a mistake?

We've been considering different solutions for this, so this is still
under debate, just proposed a new one here:

http://patchwork.ozlabs.org/patch/599721/

Thanks.