Re: [Coverity] Untrusted user data in kernel

Re: [Coverity] Untrusted user data in kernel

[James Morris]

This at least needs CAP_NET_ADMIN.


On Thu, 16 Dec 2004, Bryan Fulton wrote:
> ////////////////////////////////////////////////////////
> // 3:   /net/ipv6/netfilter/ip6_tables.c::do_replace  //
> ////////////////////////////////////////////////////////
>  
> - tainted unsigned scalar tmp.num_counters multiplied and passed to
> vmalloc (1161) and memset (1166) which could overflow or be too large
> 
> Call to function "copy_from_user" TAINTS argument "tmp"
> 
> 1143            if (copy_from_user(&tmp, user, sizeof(tmp)) != 0)
> 1144                    return -EFAULT;
> 
> ...
> 
> TAINTED variable "((tmp).num_counters * 16)" was passed to a tainted
> sink.
> 
> 1161            counters = vmalloc(tmp.num_counters * sizeof(struct
> ip6t_counters));
> 1162            if (!counters) {
> 1163                    ret = -ENOMEM;
> 1164                    goto free_newinfo;
> 1165            }
> 
> TAINTED variable "((tmp).num_counters * 16)" was passed to a tainted
> sink.
> 
> 1166            memset(counters, 0, tmp.num_counters * sizeof(struct
> ip6t_counters));
> 

-- 
James Morris
<jmorris@redhat.com>

Re: [Coverity] Untrusted user data in kernel

[Patrick McHardy]

James Morris wrote:

>This at least needs CAP_NET_ADMIN.
>
It is already checked in do_ip6t_set_ctl(). Otherwise anyone could
replace iptables rules :)

Regards
Patrick

>
>On Thu, 16 Dec 2004, Bryan Fulton wrote:
>  
>
>>////////////////////////////////////////////////////////
>>// 3:   /net/ipv6/netfilter/ip6_tables.c::do_replace  //
>>////////////////////////////////////////////////////////
>> 
>>- tainted unsigned scalar tmp.num_counters multiplied and passed to
>>vmalloc (1161) and memset (1166) which could overflow or be too large
>>
>>Call to function "copy_from_user" TAINTS argument "tmp"
>>
>>1143            if (copy_from_user(&tmp, user, sizeof(tmp)) != 0)
>>1144                    return -EFAULT;
>>
>>...
>>
>>TAINTED variable "((tmp).num_counters * 16)" was passed to a tainted
>>sink.
>>
>>1161            counters = vmalloc(tmp.num_counters * sizeof(struct
>>ip6t_counters));
>>1162            if (!counters) {
>>1163                    ret = -ENOMEM;
>>1164                    goto free_newinfo;
>>1165            }
>>
>>TAINTED variable "((tmp).num_counters * 16)" was passed to a tainted
>>sink.
>>
>>1166            memset(counters, 0, tmp.num_counters * sizeof(struct
>>ip6t_counters));
>>
>>    
>>
>
>  
>

Re: [Coverity] Untrusted user data in kernel

[James Morris]

On Fri, 17 Dec 2004, Patrick McHardy wrote:

> James Morris wrote:
> 
> >This at least needs CAP_NET_ADMIN.
> >
> It is already checked in do_ip6t_set_ctl(). Otherwise anyone could
> replace iptables rules :)

That's what I meant, you need the capability to do anything bad :-)


- James
-- 
James Morris
<jmorris@redhat.com>

Re: [Coverity] Untrusted user data in kernel

[Tomas Carnecky]

James Morris wrote:
> That's what I meant, you need the capability to do anything bad :-)
> 

But.. even if you have the 'permission' to do bad things, it shouldn't 
be possible.

It's a bug, and only because you can't exploit it if you haven't the 
right capabilities doesn't make the bug disappear.

IMHO such things (passing values between user/kernel space) should 
always be checked.

tom

Re: [Coverity] Untrusted user data in kernel

[David S. Miller]

On Fri, 17 Dec 2004 14:18:52 +0100
Tomas Carnecky <tom@dbservice.com> wrote:

> IMHO such things (passing values between user/kernel space) should 
> always be checked.

As per Patrick's posting, which James was responding to, it is
checked at the level above this function.

Re: [Coverity] Untrusted user data in kernel

[Tomas Carnecky]

David S. Miller wrote:
> On Fri, 17 Dec 2004 14:18:52 +0100
> Tomas Carnecky <tom@dbservice.com> wrote:
> 
> 
>>IMHO such things (passing values between user/kernel space) should 
>>always be checked.
> 
> 
> As per Patrick's posting, which James was responding to, it is
> checked at the level above this function.

Is only the capability checked or also the data passed to the kernel?
It's not clear from Patricks reply:
 > It is already checked in do_ip6t_set_ctl(). Otherwise anyone could
 > replace iptables rules :)
For me it seems that only CAP_NET_ADMIN is checked and not the data.

tom

Re: [Coverity] Untrusted user data in kernel

[David S. Miller]

On Fri, 17 Dec 2004 20:34:55 +0100
Tomas Carnecky <tom@dbservice.com> wrote:

>  > It is already checked in do_ip6t_set_ctl(). Otherwise anyone could
>  > replace iptables rules :)
> For me it seems that only CAP_NET_ADMIN is checked and not the data.

If that's the case then I agree with you Tomas.

Re: [Coverity] Untrusted user data in kernel

[Bill Davidsen]

James Morris wrote:
> On Fri, 17 Dec 2004, Patrick McHardy wrote:
> 
> 
>>James Morris wrote:
>>
>>
>>>This at least needs CAP_NET_ADMIN.
>>>
>>
>>It is already checked in do_ip6t_set_ctl(). Otherwise anyone could
>>replace iptables rules :)
> 
> 
> That's what I meant, you need the capability to do anything bad :-)

Are you saying that processes with capability don't make mistakes? This 
isn't a bug related to untrusted users doing privileged operations, it's 
a case of using unchecked user data.


-- 
    -bill davidsen (davidsen@tmr.com)
"The secret to procrastination is to put things off until the
  last possible moment - but no longer"  -me

Re: [Coverity] Untrusted user data in kernel

[linux-os]

On Fri, 17 Dec 2004, Bill Davidsen wrote:

> James Morris wrote:
>> On Fri, 17 Dec 2004, Patrick McHardy wrote:
>> 
>> 
>>> James Morris wrote:
>>> 
>>> 
>>>> This at least needs CAP_NET_ADMIN.
>>>> 
>>> 
>>> It is already checked in do_ip6t_set_ctl(). Otherwise anyone could
>>> replace iptables rules :)
>> 
>> 
>> That's what I meant, you need the capability to do anything bad :-)
>
> Are you saying that processes with capability don't make mistakes? This isn't 
> a bug related to untrusted users doing privileged operations, it's a case of 
> using unchecked user data.
>

But isn't there always the possibility of "unchecked user data"?
I can, as root, do `cp /dev/zero /dev/mem` and have the most
spectacular crask you've evet seen. I can even make my file-
systems unrecoverable.



Cheers,
Dick Johnson
Penguin : Linux version 2.6.9 on an i686 machine (5537.79 BogoMips).
  Notice : All mail here is now cached for review by Dictator Bush.
                  98.36% of all statistics are fiction.

Re: [Coverity] Untrusted user data in kernel

[Oliver Neukum]

> > Are you saying that processes with capability don't make mistakes? This isn't 
> > a bug related to untrusted users doing privileged operations, it's a case of 
> > using unchecked user data.
> >
> 
> But isn't there always the possibility of "unchecked user data"?
> I can, as root, do `cp /dev/zero /dev/mem` and have the most
> spectacular crask you've evet seen. I can even make my file-
> systems unrecoverable.

Only if you have the capability for raw hardware access.
The same is true for the firmware interface. What other subsystems might
be dangerous?

	Regards
		Oliver

Re: [Coverity] Untrusted user data in kernel

[Bill Davidsen]

linux-os wrote:
> On Fri, 17 Dec 2004, Bill Davidsen wrote:
> 
>> James Morris wrote:
>>
>>> On Fri, 17 Dec 2004, Patrick McHardy wrote:
>>>
>>>
>>>> James Morris wrote:
>>>>
>>>>
>>>>> This at least needs CAP_NET_ADMIN.
>>>>>
>>>>
>>>> It is already checked in do_ip6t_set_ctl(). Otherwise anyone could
>>>> replace iptables rules :)
>>>
>>>
>>>
>>> That's what I meant, you need the capability to do anything bad :-)
>>
>>
>> Are you saying that processes with capability don't make mistakes? 
>> This isn't a bug related to untrusted users doing privileged 
>> operations, it's a case of using unchecked user data.
>>
> 
> But isn't there always the possibility of "unchecked user data"?
> I can, as root, do `cp /dev/zero /dev/mem` and have the most
> spectacular crask you've evet seen. I can even make my file-
> systems unrecoverable.

But that's not the type of thing you would do by accident. The kernel 
can't protect against deliberate abuse by trusted users, nor should it. 
But the type of problem caused by an application program bug can, and I 
believe should, be caught.

The difference between "oops" and "take that!"

-- 
    -bill davidsen (davidsen@tmr.com)
"The secret to procrastination is to put things off until the
  last possible moment - but no longer"  -me

Re: [Coverity] Untrusted user data in kernel

[Tomas Carnecky]

linux-os wrote:
> On Fri, 17 Dec 2004, Bill Davidsen wrote:
> 
>> James Morris wrote:
>>
>>> On Fri, 17 Dec 2004, Patrick McHardy wrote:
 >>>
>>> That's what I meant, you need the capability to do anything bad :-)
>>
>>
>> Are you saying that processes with capability don't make mistakes? 
>> This isn't a bug related to untrusted users doing privileged 
>> operations, it's a case of using unchecked user data.
>>
> 
> But isn't there always the possibility of "unchecked user data"?
> I can, as root, do `cp /dev/zero /dev/mem` and have the most
> spectacular crask you've evet seen. I can even make my file-
> systems unrecoverable.
> 

But the difference between you example (cp /dev/zero /dev/mem) and 
passing unchecked data to the kernel is... you _can_ check the data and 
do something about it if you discover that the data is not valid/within 
a range/whatever even if the user has full permissions.
No same person would do a 'cp /dev/zero /dev/mem', but passing bad data 
is more likely to happen, badly written userspace configuration tools etc.

tom

Re: [Coverity] Untrusted user data in kernel

[Oliver Neukum]

> But the difference between you example (cp /dev/zero /dev/mem) and 
> passing unchecked data to the kernel is... you _can_ check the data and 

This is the difference:
static int open_port(struct inode * inode, struct file * filp)
{
	return capable(CAP_SYS_RAWIO) ? 0 : -EPERM;
}
(from mem.c)

	Regards
		Oliver

Re: [Coverity] Untrusted user data in kernel

[Tomas Carnecky]

Oliver Neukum wrote:
>>But the difference between you example (cp /dev/zero /dev/mem) and 
>>passing unchecked data to the kernel is... you _can_ check the data and 
> 
> 
> This is the difference:
> static int open_port(struct inode * inode, struct file * filp)
> {
> 	return capable(CAP_SYS_RAWIO) ? 0 : -EPERM;
> }
> (from mem.c)
> 

OK, but my point was, whenever you can check the 'contents' of the data 
passed to the kernel, do it. You can't check if the data someone writes 
to /dev/mem is valid or not, but you can check for out-of-range/etc. 
data in ioctl & friends.

tom

Re: [Coverity] Untrusted user data in kernel

[Horst von Brand]

linux-os <linux-os@chaos.analogic.com> said:
> On Fri, 17 Dec 2004, Bill Davidsen wrote:

[...]

> > Are you saying that processes with capability don't make mistakes? This
> > isn't a bug related to untrusted users doing privileged operations,
> > it's a case of using unchecked user data.

> But isn't there always the possibility of "unchecked user data"?

Yes. But it should be kept to a minimum.

> I can, as root, do `cp /dev/zero /dev/mem` and have the most
> spectacular crask you've evet seen. I can even make my file-
> systems unrecoverable.

Right. And you can get rid of /dev/mem if you don't want to screw yourself
this way (which is well-known). The problem at hand is _not_ in this same
league.
-- 
Dr. Horst H. von Brand                   User #22616 counter.li.org
Departamento de Informatica                     Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria              +56 32 654239
Casilla 110-V, Valparaiso, Chile                Fax:  +56 32 797513

Re: [Coverity] Untrusted user data in kernel

[Pavel Machek]

Hi!

> This at least needs CAP_NET_ADMIN.

Hmm, but that means that CAP_NET_ADMIN implies all other capabilities,
unless you fix this.

								Pavel

> > TAINTED variable "((tmp).num_counters * 16)" was passed to a tainted
> > sink.
> > 
> > 1161            counters = vmalloc(tmp.num_counters * sizeof(struct
> > ip6t_counters));
> > 1162            if (!counters) {
> > 1163                    ret = -ENOMEM;
> > 1164                    goto free_newinfo;
> > 1165            }
> > 
> > TAINTED variable "((tmp).num_counters * 16)" was passed to a tainted
> > sink.
> > 
> > 1166            memset(counters, 0, tmp.num_counters * sizeof(struct
> > ip6t_counters));


-- 
Boycott Kodak -- for their patent abuse against Java.

Re: [Coverity] Untrusted user data in kernel

[James Morris]

On Fri, 17 Dec 2004, Pavel Machek wrote:

> Hi!
> 
> > This at least needs CAP_NET_ADMIN.
> 
> Hmm, but that means that CAP_NET_ADMIN implies all other capabilities,
> unless you fix this.

I'm not saying it doesn't need to be fixed, but that it is not exploitable 
by unprivileged users.


- James
-- 
James Morris
<jmorris@redhat.com>