Re: [CHECKER] Probable security holes in 2.6.5

[Chris Wright <chrisw@osdl.org>]

* Ken Ashcraft (ken@coverity.com) wrote:
> [BUG] overflow in SMP_ALIGN?
> /home/kash/linux/linux-2.6.5/net/ipv6/netfilter/ip6_tables.c:1156:do_replace: ERROR:TAINT: 1144:1156:Passing unbounded user value "(tmp).size" as arg 2 to function "copy_from_user", which uses it unsafely in model [SOURCE_MODEL=(lib,copy_from_user,user,taintscalar)] [SINK_MODEL=(lib,copy_from_user,user,trustingsink)]    [PATH=] 
> 	struct ip6t_replace tmp;
> 	struct ip6t_table *t;
> 	struct ip6t_table_info *newinfo, *oldinfo;
> 	struct ip6t_counters *counters;
> 
> Start --->
> 	if (copy_from_user(&tmp, user, sizeof(tmp)) != 0)
> 		return -EFAULT;
> 
> 	/* Pedantry: prevent them from hitting BUG() in vmalloc.c --RR */
> 	if ((SMP_ALIGN(tmp.size) >> PAGE_SHIFT) + 2 > num_physpages)
> 		return -ENOMEM;
> 
> 	newinfo = vmalloc(sizeof(struct ip6t_table_info)
> 			  + SMP_ALIGN(tmp.size) * NR_CPUS);
> 	if (!newinfo)
> 		return -ENOMEM;
> 
> Error --->
> 	if (copy_from_user(newinfo->entries, user + sizeof(tmp),
> 			   tmp.size) != 0) {
> 		ret = -EFAULT;
> 		goto free_newinfo;

I don't think there's an overflow here.  Just possiblity of large
allocations.  Seems sane to me to have some limits.  Also nothing
seems to sanity check against the tmp.num_counters sized vmalloc().
I do believe these are protected by capable() check.

> ---------------------------------------------------------
> [BUG] overflow in SMP_ALIGN?
> /home/kash/linux/linux-2.6.5/net/ipv4/netfilter/arp_tables.c:891:do_replace: ERROR:TAINT: 875:891:Passing unbounded user value "(tmp).size" as arg 2 to function "copy_from_user", which uses it unsafely in model [SOURCE_MODEL=(lib,copy_from_user,user,taintscalar)] [SINK_MODEL=(lib,copy_from_user,user,trustingsink)]    [PATH=] 
> 	struct arpt_replace tmp;
> 	struct arpt_table *t;
> 	struct arpt_table_info *newinfo, *oldinfo;
> 	struct arpt_counters *counters;
> 
> Start --->
> 	if (copy_from_user(&tmp, user, sizeof(tmp)) != 0)
> 
> 	... DELETED 10 lines ...
> 
> 	newinfo = vmalloc(sizeof(struct arpt_table_info)
> 			  + SMP_ALIGN(tmp.size) * NR_CPUS);
> 	if (!newinfo)
> 		return -ENOMEM;
> 
> Error --->
> 	if (copy_from_user(newinfo->entries, user + sizeof(tmp),
> 			   tmp.size) != 0) {
> 		ret = -EFAULT;
> 		goto free_newinfo;

There's a couple checks here:

	/* Hack: Causes ipchains to give correct error msg --RR */
	if (len != sizeof(tmp) + tmp.size)
		return -ENOPROTOOPT;

	/* Pedantry: prevent them from hitting BUG() in vmalloc.c --RR */
	if ((SMP_ALIGN(tmp.size) >> PAGE_SHIFT) + 2 > num_physpages)
		return -ENOMEM;

Are these sufficient to limit considering the allocation is sized by
'SMP_ALIGN(tmp.size) * NR_CPUS'?  And nothing checking tmp.num_counters.
Again, protected by capable().

> ---------------------------------------------------------
> [BUG] overflow in SMP_ALIGN?
> /home/kash/linux/linux-2.6.5/net/ipv4/netfilter/ip_tables.c:1074:do_replace: ERROR:TAINT: 1058:1074:Passing unbounded user value "(tmp).size" as arg 2 to function "copy_from_user", which uses it unsafely in model [SOURCE_MODEL=(lib,copy_from_user,user,taintscalar)] [SINK_MODEL=(lib,copy_from_user,user,trustingsink)]    [PATH=] 
> 	struct ipt_replace tmp;
> 	struct ipt_table *t;
> 	struct ipt_table_info *newinfo, *oldinfo;
> 	struct ipt_counters *counters;
> 
> Start --->
> 	if (copy_from_user(&tmp, user, sizeof(tmp)) != 0)
> 
> 	... DELETED 10 lines ...
> 
> 	newinfo = vmalloc(sizeof(struct ipt_table_info)
> 			  + SMP_ALIGN(tmp.size) * NR_CPUS);
> 	if (!newinfo)
> 		return -ENOMEM;
> 
> Error --->
> 	if (copy_from_user(newinfo->entries, user + sizeof(tmp),
> 			   tmp.size) != 0) {
> 		ret = -EFAULT;
> 		goto free_newinfo;

same here.

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net