From patchwork Thu Mar 10 20:44:06 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Chris J Arges X-Patchwork-Id: 595952 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from huckleberry.canonical.com (huckleberry.canonical.com [91.189.94.19]) by ozlabs.org (Postfix) with ESMTP id 147DA14031D; Fri, 11 Mar 2016 07:44:26 +1100 (AEDT) Received: from localhost ([127.0.0.1] helo=huckleberry.canonical.com) by huckleberry.canonical.com with esmtp (Exim 4.76) (envelope-from ) id 1ae7RQ-0003X4-Q2; Thu, 10 Mar 2016 20:44:16 +0000 Received: from youngberry.canonical.com ([91.189.89.112]) by huckleberry.canonical.com with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.76) (envelope-from ) id 1ae7RL-0003Wp-Hz for kernel-team@lists.ubuntu.com; Thu, 10 Mar 2016 20:44:11 +0000 Received: from 1.general.arges.us.vpn ([10.172.65.250] helo=localhost.localdomain) by youngberry.canonical.com with esmtpsa (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.76) (envelope-from ) id 1ae7RL-0006TP-03 for kernel-team@lists.ubuntu.com; Thu, 10 Mar 2016 20:44:11 +0000 From: Chris J Arges To: kernel-team@lists.ubuntu.com Subject: [SRU][P/T/U/V/W][PATCH] UBUNTU: SAUCE: (noup) netfilter: x_tables: don't rely on well-behaving userspace Date: Thu, 10 Mar 2016 14:44:06 -0600 Message-Id: <1457642646-17454-1-git-send-email-chris.j.arges@canonical.com> X-Mailer: git-send-email 2.7.0 X-BeenThere: kernel-team@lists.ubuntu.com X-Mailman-Version: 2.1.14 Precedence: list List-Id: Kernel team discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: kernel-team-bounces@lists.ubuntu.com Sender: kernel-team-bounces@lists.ubuntu.com From: Florian Westphal 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 Signed-off-by: Florian Westphal Signed-off-by: Chris J Arges --- 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;