[CHECKER] copy_*_user length bugs?

[CHECKER] copy_*_user length bugs?

[Dawson Engler]

Hi All,

at the suggestion of Chris (chris@ferret.lmh.ox.ac.uk) I wrote a simple
checker to warn when the length parameter to copy_*_user was (1) an
integer and (2) not checked < 0.    

As an example, the ipv6 routine rawv6_geticmpfilter gets an integer 'len'
from user space, checks that it is smaller than a struct size and then
uses length as an argument to copy_to_user: 

                if (get_user(len, optlen))
                        return -EFAULT;
                if (len > sizeof(struct icmp6_filter))
                        len = sizeof(struct icmp6_filter);
                if (put_user(len, optlen))
                        return -EFAULT;
                if (copy_to_user(optval, &sk->tp_pinfo.tp_raw.filter, len))
                        return -EFAULT;

Is this a real bug?  Or is the checked rule only applicable to
__copy_*_user routines rather than copy_*_user routines?  (If its a real
bug, theres about 8 others that we found).

Thanks,
Dawson

Re: [CHECKER] copy_*_user length bugs?

[David Schleef]

On Tue, Apr 17, 2001 at 09:39:15PM -0700, Dawson Engler wrote:
> Hi All,
> 
> at the suggestion of Chris (chris@ferret.lmh.ox.ac.uk) I wrote a simple
> checker to warn when the length parameter to copy_*_user was (1) an
> integer and (2) not checked < 0.    
> 
> As an example, the ipv6 routine rawv6_geticmpfilter gets an integer 'len'
> from user space, checks that it is smaller than a struct size and then
> uses length as an argument to copy_to_user: 
> 
>                 if (get_user(len, optlen))
>                         return -EFAULT;
>                 if (len > sizeof(struct icmp6_filter))
>                         len = sizeof(struct icmp6_filter);
>                 if (put_user(len, optlen))
>                         return -EFAULT;
>                 if (copy_to_user(optval, &sk->tp_pinfo.tp_raw.filter, len))
>                         return -EFAULT;
> 
> Is this a real bug?  Or is the checked rule only applicable to
> __copy_*_user routines rather than copy_*_user routines?  (If its a real
> bug, theres about 8 others that we found).


The len parameter is an unsigned value, so this code is ok as
long as access_ok() correctly checks that the range to copy
doesn't stray outside of the userspace range, including the
possible wraparound for a very large len.  access_ok() on i386
checks for the wraparound.  m68k doesn't use it.  PowerPC
is correct, but only because TASK_SIZE is 0x80000000.  If it
is ever changed, there could be a problem.  I didn't check
other architectures, because I don't understand the asm.




dave...

Re: [CHECKER] copy_*_user length bugs?

[Chris Evans]

On Wed, 18 Apr 2001, David Schleef wrote:

> On Tue, Apr 17, 2001 at 09:39:15PM -0700, Dawson Engler wrote:
> > Hi All,
> >
> > at the suggestion of Chris (chris@ferret.lmh.ox.ac.uk) I wrote a simple
> > checker to warn when the length parameter to copy_*_user was (1) an
> > integer and (2) not checked < 0.
> >
> > As an example, the ipv6 routine rawv6_geticmpfilter gets an integer 'len'
> > from user space, checks that it is smaller than a struct size and then
> > uses length as an argument to copy_to_user:
> >
> >                 if (get_user(len, optlen))
> >                         return -EFAULT;
> >                 if (len > sizeof(struct icmp6_filter))
> >                         len = sizeof(struct icmp6_filter);
> >                 if (put_user(len, optlen))
> >                         return -EFAULT;
> >                 if (copy_to_user(optval, &sk->tp_pinfo.tp_raw.filter, len))
> >                         return -EFAULT;
> >
> > Is this a real bug?  Or is the checked rule only applicable to
> > __copy_*_user routines rather than copy_*_user routines?  (If its a real
> > bug, theres about 8 others that we found).
>
> The len parameter is an unsigned value, so this code is ok as
> long as access_ok() correctly checks that the range to copy
> doesn't stray outside of the userspace range, including the
> possible wraparound for a very large len.  access_ok() on i386
> checks for the wraparound.  m68k doesn't use it.  PowerPC
> is correct, but only because TASK_SIZE is 0x80000000.  If it
> is ever changed, there could be a problem.  I didn't check
> other architectures, because I don't understand the asm.

Incorrect - if the "len" variable is a signed integer, this is a nasty
bug.

To justify this, consider if len were set to minus 2 billion. This will
pass the sanity check, and pass the value straight on to copy_to_user. The
copy_to_user parameter is unsigned, so this value because approximately
+2Gb.

Now, providing the malicious user passes a low user space pointer (e.g.
just above 0), the kernel's virtual address space wrap check will not
trigger because ~0 + ~2Gb does not exceed 4G. And the result is the user
being able to read kernel memory.

Cheers
Chris

Re: [CHECKER] copy_*_user length bugs?

[Russell King]

On Wed, Apr 18, 2001 at 12:14:56PM +0100, Chris Evans wrote:
> To justify this, consider if len were set to minus 2 billion. This will
> pass the sanity check, and pass the value straight on to copy_to_user. The
> copy_to_user parameter is unsigned, so this value because approximately
> +2Gb.

For ARM, this isn't a problem (we do 33-bit arithmetic in access_ok
specifically to catch this type of thing).  x86 does the same thing (or
did when I wrote the code for ARM.

> Now, providing the malicious user passes a low user space pointer (e.g.
> just above 0), the kernel's virtual address space wrap check will not
> trigger because ~0 + ~2Gb does not exceed 4G. And the result is the user
> being able to read kernel memory.

But ~0 + ~2GB = ~2GB.  Last time I checked, ~2GB is less than 3GB, and 3GB
is the start of kernel memory on x86.  Therefore, I don't see that the
user will be able to read kernel memory.

--
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: [CHECKER] copy_*_user length bugs?

[Chris Evans]

On Wed, 18 Apr 2001, Russell King wrote:

> > Now, providing the malicious user passes a low user space pointer (e.g.
> > just above 0), the kernel's virtual address space wrap check will not
> > trigger because ~0 + ~2Gb does not exceed 4G. And the result is the user
> > being able to read kernel memory.
>
> But ~0 + ~2GB = ~2GB.  Last time I checked, ~2GB is less than 3GB, and 3GB
> is the start of kernel memory on x86.  Therefore, I don't see that the
> user will be able to read kernel memory.

The problem is that (up to) a 2Gb copy is attempted into userspace. The
source is a kernel object which is not 2Gb large! So, we read off the end
of some kernel object, and there is often something very interesting after
it ;-)

For a good real-world example, please see my Bugtraq post regarding
sysctl():

http://www.securityfocus.com/archive/1/161764

Cheers
Chris

Re: [CHECKER] copy_*_user length bugs?

[Andreas Schwab]

Chris Evans <chris@scary.beasts.org> writes:

|> To justify this, consider if len were set to minus 2 billion. This will
|> pass the sanity check, and pass the value straight on to copy_to_user. The
|> copy_to_user parameter is unsigned, so this value because approximately
|> +2Gb.
|> 
|> Now, providing the malicious user passes a low user space pointer (e.g.
|> just above 0), the kernel's virtual address space wrap check will not
|> trigger because ~0 + ~2Gb does not exceed 4G. And the result is the user
|> being able to read kernel memory.

On m68k this is not a problem, since kernel and user address space are
strictly distinct, even in the kernel.  The luser will get an EFAULT
eventually.

Andreas.

-- 
Andreas Schwab                                  "And now for something
SuSE Labs                                        completely different."
Andreas.Schwab@suse.de
SuSE GmbH, Schanzäckerstr. 10, D-90443 Nürnberg
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5

Re: [CHECKER] copy_*_user length bugs?

[Richard B. Johnson]

On Tue, 17 Apr 2001, Dawson Engler wrote:

> Hi All,
> 
> at the suggestion of Chris (chris@ferret.lmh.ox.ac.uk) I wrote a simple
> checker to warn when the length parameter to copy_*_user was (1) an
> integer and (2) not checked < 0.    
> 
> As an example, the ipv6 routine rawv6_geticmpfilter gets an integer 'len'
> from user space, checks that it is smaller than a struct size and then
> uses length as an argument to copy_to_user: 
> 
>                 if (get_user(len, optlen))
>                         return -EFAULT;
>                 if (len > sizeof(struct icmp6_filter))
>                         len = sizeof(struct icmp6_filter);
>                 if (put_user(len, optlen))
>                         return -EFAULT;
>                 if (copy_to_user(optval, &sk->tp_pinfo.tp_raw.filter, len))
>                         return -EFAULT;
> 
> Is this a real bug?  Or is the checked rule only applicable to
> __copy_*_user routines rather than copy_*_user routines?  (If its a real
> bug, theres about 8 others that we found).
> 
> Thanks,
> Dawson

Length is type size_t (unsigned). It can't be less than zero.
copy_to_user() should fault with invalid lengths. That's one
of the things it's supposed to do.


Cheers,
Dick Johnson

Penguin : Linux version 2.4.1 on an i686 machine (799.53 BogoMips).

"Memory is like gasoline. You use it up when you are running. Of
course you get it all back when you reboot..."; Actual explanation
obtained from the Micro$oft help desk.