ACK: [SRU][P/T/U/V/W][PATCH] UBUNTU: SAUCE: (noup) netfilter: x_tables: don't rely on well-behaving userspace
Brad Figg
brad.figg at canonical.com
Thu Mar 10 20:49:16 UTC 2016
On Thu, Mar 10, 2016 at 02:44:06PM -0600, Chris J Arges wrote:
> From: Florian Westphal <fw at strlen.de>
>
> BugLink: http://bugs.launchpad.net/bugs/1555338
>
> 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.
>
> Also, check that the offset meets the xtables_entry alignment.
>
> Reported-by: Ben Hawkes <hawkes at google.com>
> Signed-off-by: Florian Westphal <fw at strlen.de>
> Signed-off-by: Chris J Arges <chris.j.arges at canonical.com>
> ---
> net/ipv4/netfilter/arp_tables.c | 25 +++++++++++++++++--------
> net/ipv4/netfilter/ip_tables.c | 24 +++++++++++++++++-------
> net/ipv6/netfilter/ip6_tables.c | 24 +++++++++++++++++-------
> 3 files changed, 51 insertions(+), 22 deletions(-)
>
> diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
> index 59da7cd..88334b1 100644
> --- a/net/ipv4/netfilter/arp_tables.c
> +++ b/net/ipv4/netfilter/arp_tables.c
> @@ -362,6 +362,17 @@ static inline bool unconditional(const struct arpt_arp *arp)
> return memcmp(arp, &uncond, sizeof(uncond)) == 0;
> }
>
> +static bool next_offset_ok(const struct xt_table_info *t, unsigned int newpos)
> +{
> + if (newpos > t->size - sizeof(struct arpt_entry))
> + return false;
> +
> + if (newpos % __alignof__(struct arpt_entry) != 0)
> + return false;
> +
> + return true;
> +}
> +
> /* Figures out from what hook each rule can be called: returns 0 if
> * there are loops. Puts hook bitmask in comefrom.
> */
> @@ -433,6 +444,8 @@ static int mark_source_chains(const struct xt_table_info *newinfo,
>
> /* Move along one */
> size = e->next_offset;
> + if (!next_offset_ok(newinfo, pos + size))
> + return 0;
> e = (struct arpt_entry *)
> (entry0 + pos + size);
> e->counters.pcnt = pos;
> @@ -443,14 +456,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);
> @@ -458,6 +463,10 @@ static int mark_source_chains(const struct xt_table_info *newinfo,
> /* ... this is a fallthru */
> newpos = pos + e->next_offset;
> }
> +
> + if (!next_offset_ok(newinfo, newpos))
> + 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 718dfbd..ee17b65 100644
> --- a/net/ipv4/netfilter/ip_tables.c
> +++ b/net/ipv4/netfilter/ip_tables.c
> @@ -439,6 +439,17 @@ ipt_do_table(struct sk_buff *skb,
> #endif
> }
>
> +static bool next_offset_ok(const struct xt_table_info *t, unsigned int newpos)
> +{
> + if (newpos > t->size - sizeof(struct ipt_entry))
> + return false;
> +
> + if (newpos % __alignof__(struct ipt_entry) != 0)
> + return false;
> +
> + return true;
> +}
> +
> /* Figures out from what hook each rule can be called: returns 0 if
> there are loops. Puts hook bitmask in comefrom. */
> static int
> @@ -515,6 +526,8 @@ mark_source_chains(const struct xt_table_info *newinfo,
>
> /* Move along one */
> size = e->next_offset;
> + if (!next_offset_ok(newinfo, pos + size))
> + return 0;
> e = (struct ipt_entry *)
> (entry0 + pos + size);
> e->counters.pcnt = pos;
> @@ -525,13 +538,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);
> @@ -539,6 +545,10 @@ mark_source_chains(const struct xt_table_info *newinfo,
> /* ... this is a fallthru */
> newpos = pos + e->next_offset;
> }
> +
> + if (!next_offset_ok(newinfo, newpos))
> + 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 710238f..5b4b008 100644
> --- a/net/ipv6/netfilter/ip6_tables.c
> +++ b/net/ipv6/netfilter/ip6_tables.c
> @@ -449,6 +449,17 @@ ip6t_do_table(struct sk_buff *skb,
> #endif
> }
>
> +static bool next_offset_ok(const struct xt_table_info *t, unsigned int newpos)
> +{
> + if (newpos > t->size - sizeof(struct ip6t_entry))
> + return false;
> +
> + if (newpos % __alignof__(struct ip6t_entry) != 0)
> + return false;
> +
> + return true;
> +}
> +
> /* Figures out from what hook each rule can be called: returns 0 if
> there are loops. Puts hook bitmask in comefrom. */
> static int
> @@ -525,6 +536,8 @@ mark_source_chains(const struct xt_table_info *newinfo,
>
> /* Move along one */
> size = e->next_offset;
> + if (!next_offset_ok(newinfo, pos + size))
> + return 0;
> e = (struct ip6t_entry *)
> (entry0 + pos + size);
> e->counters.pcnt = pos;
> @@ -535,13 +548,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);
> @@ -549,6 +555,10 @@ mark_source_chains(const struct xt_table_info *newinfo,
> /* ... this is a fallthru */
> newpos = pos + e->next_offset;
> }
> +
> + if (!next_offset_ok(newinfo, newpos))
> + return 0;
> +
> e = (struct ip6t_entry *)
> (entry0 + newpos);
> e->counters.pcnt = pos;
> --
> 2.7.0
>
>
> --
> kernel-team mailing list
> kernel-team at lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
Looks good
--
Brad Figg brad.figg at canonical.com http://www.canonical.com
More information about the kernel-team
mailing list