From mboxrd@z Thu Jan 1 00:00:00 1970 From: Patrick McHardy Subject: [NETFILTER 04/08]: Missed and reordered checks in {arp, ip, ip6}_tables Date: Fri, 17 Nov 2006 06:35:45 +0100 (MET) Message-ID: <20061117053545.10231.94652.sendpatchset@localhost.localdomain> References: <20061117053540.10231.92379.sendpatchset@localhost.localdomain> Cc: netfilter-devel@lists.netfilter.org, Patrick McHardy , davem@davemloft.net Return-path: To: stable@kernel.org In-Reply-To: <20061117053540.10231.92379.sendpatchset@localhost.localdomain> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: netfilter-devel-bounces@lists.netfilter.org Errors-To: netfilter-devel-bounces@lists.netfilter.org List-Id: netfilter-devel.vger.kernel.org [NETFILTER]: Missed and reordered checks in {arp,ip,ip6}_tables There is a number of issues in parsing user-provided table in translate_table(). Malicious user with CAP_NET_ADMIN may crash system by passing special-crafted table to the *_tables. The first issue is that mark_source_chains() function is called before entry content checks. In case of standard target, mark_source_chains() function uses t->verdict field in order to determine new position. But the check, that this field leads no further, than the table end, is in check_entry(), which is called later, than mark_source_chains(). The second issue, that there is no check that target_offset points inside entry. If so, *_ITERATE_MATCH macro will follow further, than the entry ends. As a result, we'll have oops or memory disclosure. And the third issue, that there is no check that the target is completely inside entry. Results are the same, as in previous issue. Signed-off-by: Dmitry Mishin Acked-by: Kirill Korotaev Signed-off-by: Patrick McHardy Signed-off-by: David S. Miller --- commit 0ef4760e162ea44c847cca7393b36e5bcac5414e tree 7036ce51d75aaf46d5c4abca281956c39caced10 parent 94a3d63f9ca6cb404f62ee4186d20fec3e8bdc97 author Patrick McHardy Fri, 17 Nov 2006 06:24:10 +0100 committer Patrick McHardy Fri, 17 Nov 2006 06:24:10 +0100 net/ipv4/netfilter/arp_tables.c | 25 ++++++++++++++++--------- net/ipv4/netfilter/ip_tables.c | 30 ++++++++++++++++++++++-------- net/ipv6/netfilter/ip6_tables.c | 24 ++++++++++++++++-------- 3 files changed, 54 insertions(+), 25 deletions(-) diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c index 8d1d7a6..9ea7869 100644 --- a/net/ipv4/netfilter/arp_tables.c +++ b/net/ipv4/netfilter/arp_tables.c @@ -471,7 +471,13 @@ static inline int check_entry(struct arp return -EINVAL; } + if (e->target_offset + sizeof(struct arpt_entry_target) > e->next_offset) + return -EINVAL; + t = arpt_get_target(e); + if (e->target_offset + t->u.target_size > e->next_offset) + return -EINVAL; + target = try_then_request_module(xt_find_target(NF_ARP, t->u.user.name, t->u.user.revision), "arpt_%s", t->u.user.name); @@ -629,20 +635,18 @@ static int translate_table(const char *n } } - if (!mark_source_chains(newinfo, valid_hooks, entry0)) { - duprintf("Looping hook\n"); - return -ELOOP; - } - /* Finally, each sanity check must pass */ i = 0; ret = ARPT_ENTRY_ITERATE(entry0, newinfo->size, check_entry, name, size, &i); - if (ret != 0) { - ARPT_ENTRY_ITERATE(entry0, newinfo->size, - cleanup_entry, &i); - return ret; + if (ret != 0) + goto cleanup; + + ret = -ELOOP; + if (!mark_source_chains(newinfo, valid_hooks, entry0)) { + duprintf("Looping hook\n"); + goto cleanup; } /* And one copy for every other CPU */ @@ -651,6 +655,9 @@ static int translate_table(const char *n memcpy(newinfo->entries[i], entry0, newinfo->size); } + return 0; +cleanup: + ARPT_ENTRY_ITERATE(entry0, newinfo->size, cleanup_entry, &i); return ret; } diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c index b22e4d3..dcfb7f7 100644 --- a/net/ipv4/netfilter/ip_tables.c +++ b/net/ipv4/netfilter/ip_tables.c @@ -552,12 +552,18 @@ check_entry(struct ipt_entry *e, const c return -EINVAL; } + if (e->target_offset + sizeof(struct ipt_entry_target) > e->next_offset) + return -EINVAL; + j = 0; ret = IPT_MATCH_ITERATE(e, check_match, name, &e->ip, e->comefrom, &j); if (ret != 0) goto cleanup_matches; t = ipt_get_target(e); + ret = -EINVAL; + if (e->target_offset + t->u.target_size > e->next_offset) + goto cleanup_matches; target = try_then_request_module(xt_find_target(AF_INET, t->u.user.name, t->u.user.revision), @@ -720,19 +726,17 @@ translate_table(const char *name, } } - if (!mark_source_chains(newinfo, valid_hooks, entry0)) - return -ELOOP; - /* Finally, each sanity check must pass */ i = 0; ret = IPT_ENTRY_ITERATE(entry0, newinfo->size, check_entry, name, size, &i); - if (ret != 0) { - IPT_ENTRY_ITERATE(entry0, newinfo->size, - cleanup_entry, &i); - return ret; - } + if (ret != 0) + goto cleanup; + + ret = -ELOOP; + if (!mark_source_chains(newinfo, valid_hooks, entry0)) + goto cleanup; /* And one copy for every other CPU */ for_each_possible_cpu(i) { @@ -740,6 +744,9 @@ translate_table(const char *name, memcpy(newinfo->entries[i], entry0, newinfo->size); } + return 0; +cleanup: + IPT_ENTRY_ITERATE(entry0, newinfo->size, cleanup_entry, &i); return ret; } @@ -1531,6 +1538,10 @@ check_compat_entry_size_and_hooks(struct return -EINVAL; } + if (e->target_offset + sizeof(struct compat_xt_entry_target) > + e->next_offset) + return -EINVAL; + off = 0; entry_offset = (void *)e - (void *)base; j = 0; @@ -1540,6 +1551,9 @@ check_compat_entry_size_and_hooks(struct goto cleanup_matches; t = ipt_get_target(e); + ret = -EINVAL; + if (e->target_offset + t->u.target_size > e->next_offset) + goto cleanup_matches; target = try_then_request_module(xt_find_target(AF_INET, t->u.user.name, t->u.user.revision), diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c index c9d6b23..9a27156 100644 --- a/net/ipv6/netfilter/ip6_tables.c +++ b/net/ipv6/netfilter/ip6_tables.c @@ -592,12 +592,19 @@ check_entry(struct ip6t_entry *e, const return -EINVAL; } + if (e->target_offset + sizeof(struct ip6t_entry_target) > + e->next_offset) + return -EINVAL; + j = 0; ret = IP6T_MATCH_ITERATE(e, check_match, name, &e->ipv6, e->comefrom, &j); if (ret != 0) goto cleanup_matches; t = ip6t_get_target(e); + ret = -EINVAL; + if (e->target_offset + t->u.target_size > e->next_offset) + goto cleanup_matches; target = try_then_request_module(xt_find_target(AF_INET6, t->u.user.name, t->u.user.revision), @@ -760,19 +767,17 @@ translate_table(const char *name, } } - if (!mark_source_chains(newinfo, valid_hooks, entry0)) - return -ELOOP; - /* Finally, each sanity check must pass */ i = 0; ret = IP6T_ENTRY_ITERATE(entry0, newinfo->size, check_entry, name, size, &i); - if (ret != 0) { - IP6T_ENTRY_ITERATE(entry0, newinfo->size, - cleanup_entry, &i); - return ret; - } + if (ret != 0) + goto cleanup; + + ret = -ELOOP; + if (!mark_source_chains(newinfo, valid_hooks, entry0)) + goto cleanup; /* And one copy for every other CPU */ for_each_possible_cpu(i) { @@ -780,6 +785,9 @@ translate_table(const char *name, memcpy(newinfo->entries[i], entry0, newinfo->size); } + return 0; +cleanup: + IP6T_ENTRY_ITERATE(entry0, newinfo->size, cleanup_entry, &i); return ret; }