Hi Bryan,
First of all, thanks very much for this effort.
Davem: Please check the networking ones, there are several.
On Thu, Dec 16, 2004 at 05:33:32PM -0800, Bryan Fulton wrote:
> Hi, recently we ran a security checker over linux and discovered some
> flaws in the Linux 2.6.9 kernel. I've inserted into this post a few
> examples of what we found. These functions copy in user data
> (copy_from_user) and use it as an array index, loop bound or memory
> function argument without proper bounds checking.
>
> This posting just involves bugs in /fs, /net and /drivers/net. There
> will be more postings for similar flaws in the drivers, as well as
> network exploitable bugs and bugs in system calls.
We're anxious to see those.
> Some can be viewed as minor as they might involve directly passing an
> unsigned tainted scalar to kmalloc. I was under the impression that
> kmalloc has an implicit bounds check as it returns null if attempting to
> allocate >64kb (or at least it used to). Can someone confirm/disconfirm
> that?
On v2.6 the kmalloc limit is 128k for most machines.
!CONFIG_MMU allows up to 1Mb and CONFIG_LARGE_ALLOCS allows up to 32Mb, so better
not rely on kmalloc checking. ;)
> Suggestions for other security properties to check are welcome. We
> appreciate your feedback as a method to improve and expand our
> security checkers.
Please run the checker on v2.4.29-pre3.
Would be really nice if you could do it periodically say on each new kernel release (v2.6
and v2.4) or a monthly basis.
> Thanks,
> .:Bryan Fulton and Ted Unangst of Coverity, Inc.
>
> Quick location summary:
>
> /fs/coda/pioctl.c::coda_pioctl
> /fs/xfs/linux-2.6/xfs_ioctl.c::xfs_attrmulti_by_handle
> /net/ipv6/netfilter/ip6_tables.c::do_replace
> /net/bridge/br_ioctl.c::old_deviceless
> /net/rose/rose_route.c::rose_rt_ioctl
> /drivers/net/wan/sdla.c::sdla_xfer
>
> /////////////////////////////////////////////////////
> // 1: /fs/coda/pioctl.c::coda_pioctl //
> /////////////////////////////////////////////////////
> - tainted scalars (signed shorts) data->vi.in_size and data->vi.out_size
> are used to copy memory from and to user space
> - neither are properly upper/lower bounds checked (in_size only
> upper-bound checked, out_size only lower-bound checked)
<snip>
> TAINTED variable "((data)->vi).in_size" passed to tainted data sink
> "copy_from_user"
>
> 572 if ( copy_from_user((char*)inp + (long)inp->coda_ioctl.data,
> 573 data->vi.in, data->vi.in_size) ) {
> 574 error = -EINVAL;
> 575 goto exit;
> 576 }
>
> ...
>
> Checked lower bounds of signed scalar "((data)->vi).out_size" by
> "((outp)->coda_ioctl).len >
> ((data)->vi).out_size"
>
> 588 if (outp->coda_ioctl.len > data->vi.out_size) {
> 589 error = -EINVAL;
> 590 } else {
>
> TAINTED variable "((data)->vi).out_size" passed to tainted data sink
> "copy_to_user"
>
> 591 if (copy_to_user(data->vi.out,
> 592 (char *)outp +
> (long)outp->coda_ioctl.data,
> 593 data->vi.out_size)) {
> 594 error = -EFAULT;
> 595 goto exit;
> 596 }
Correct, fix for both v2.4 and v2.6 attached. Adds bound checking:
Jan Harkes, please check correctness so we can apply it.
--- linux-2.6.10-rc3/fs/coda/upcall.c.orig 2005-01-05 10:30:24.575445152 -0200
+++ linux-2.6.10-rc3/fs/coda/upcall.c 2005-01-05 10:30:26.623133856 -0200
@@ -550,10 +550,15 @@
UPARG(CODA_IOCTL);
/* build packet for Venus */
- if (data->vi.in_size > VC_MAXDATASIZE) {
+ if (data->vi.in_size > VC_MAXDATASIZE || data->vi.in_size < 0) {
error = -EINVAL;
goto exit;
- }
+ }
+
+ if (data->vi.out_size > VC_MAXDATASIZE || data->vi.out_size < 0) {
+ error = -EINVAL;
+ goto exit;
+ }
inp->coda_ioctl.VFid = *fid;
> ////////////////////////////////////////////////////////////////////
> // 2: /fs/xfs/linux-2.6/xfs_ioctl.c::xfs_attrmulti_by_handle //
> ////////////////////////////////////////////////////////////////////
XFS people, can you take care of this one please. Not a security threat,
protected under ADMIN caps.
> ////////////////////////////////////////////////////////
> // 3: /net/ipv6/netfilter/ip6_tables.c::do_replace //
> ////////////////////////////////////////////////////////
This one lacks bound checking as people discussed, but is not
a security threat since region is protected under NET_ADMIN caps.
> //////////////////////////////////////////////////
> // 4: /net/bridge/br_ioctl.c::old_deviceless //
> //////////////////////////////////////////////////
Lacks bound checking but is not a security threat since region
is protectedunder NET_ADMIN caps.
Something similar to this should do it. Not sure if "65535" is a
sane value, though.
--- br_ioctl.c.orig 2005-01-05 11:02:28.301994264 -0200
+++ br_ioctl.c 2005-01-05 11:02:30.552652112 -0200
@@ -324,6 +324,9 @@
int *indices;
int ret = 0;
+ if (args[2] > 65535)
+ return -EFAULT;
+
indices = kmalloc(args[2]*sizeof(int), GFP_KERNEL);
if (indices == NULL)
return -ENOMEM;
> //////////////////////////////////////////////////
> // 5: /net/rose/rose_route.c::rose_rt_ioctl //
> //////////////////////////////////////////////////
Not a security threat because requires NET_ADMIN caps.
Alan, Arnaldo, proper bound checking is required nevertheless.
Can you take a look at this please?
> //////////////////////////////////////////////
> // 6: /drivers/net/wan/sdla.c::sdla_xfer //
> //////////////////////////////////////////////
>
> - tainted signed scalar mem.len passed to kmalloc and memset (1206 and
> 1211, or 1220 and 1223). Possibly minor because of kmalloc's
> implicit size check
Protected by NET_ADMIN caps, but definately needs some bound checking.
Jan, I think SDLA_MAX_DATA is the correct bound to check for here, can
you confirm please?
--- linux-2.4.28.orig/drivers/net/wan/sdla.c 2004-12-31 15:21:16.000000000 -0200
+++ linux-2.4.28/drivers/net/wan/sdla.c 2005-01-05 11:23:15.089453760 -0200
@@ -1195,6 +1195,9 @@
if(copy_from_user(&mem, info, sizeof(mem)))
return -EFAULT;
+
+ if (mem.len > SDLA_MAX_DATA || mem.len < 0)
+ return -EFAULT;
if (read)
{
On Wed, Jan 05, 2005 at 10:04:23AM -0200, Marcelo Tosatti wrote:
> On Thu, Dec 16, 2004 at 05:33:32PM -0800, Bryan Fulton wrote:
> Correct, fix for both v2.4 and v2.6 attached. Adds bound checking:
>
> Jan Harkes, please check correctness so we can apply it.
I was looking at this and actually think that both in_size and out_size
should just be changed to unsigned short instead of signed short (the
values should never be negative, period).
That fixes the bounds checking on the in_size, but the out_size one is
still questionable. I'm not even sure the code is actually testing the
right thing there.
> --- linux-2.6.10-rc3/fs/coda/upcall.c.orig 2005-01-05 10:30:24.575445152 -0200
> +++ linux-2.6.10-rc3/fs/coda/upcall.c 2005-01-05 10:30:26.623133856 -0200
> @@ -550,10 +550,15 @@
> UPARG(CODA_IOCTL);
>
> /* build packet for Venus */
> - if (data->vi.in_size > VC_MAXDATASIZE) {
> + if (data->vi.in_size > VC_MAXDATASIZE || data->vi.in_size < 0) {
> error = -EINVAL;
> goto exit;
> - }
> + }
This part would work, but changing to the variable to unsigned short
in include/linux/coda.h works just as well.
> + if (data->vi.out_size > VC_MAXDATASIZE || data->vi.out_size < 0) {
> + error = -EINVAL;
> + goto exit;
> + }
We might be overwriting out_size when making the upcall to venus, so
checking out_size here probably doesn't help all that much. I'm still
looking at what exactly is going on with that. I should have a patch by
the end of the week.
Jan
On Wed, Jan 05, 2005 at 10:04:23AM -0200, Marcelo Tosatti wrote:
> > ////////////////////////////////////////////////////////////////////
> > // 2: /fs/xfs/linux-2.6/xfs_ioctl.c::xfs_attrmulti_by_handle //
> > ////////////////////////////////////////////////////////////////////
>
> XFS people, can you take care of this one please. Not a security threat,
> protected under ADMIN caps.
Fixed in our local tree coupla weeks ago, it'll be in the next update.
cheers.
--
Nathan
This patch adds bounds checking for tainted scalars.
(reported by Brian Fulton and Ted Unangst, Coverity Inc.)
Signed-off-by: Jan Harkes <[email protected]>
Index: linux-2.4.29-pre3-bk4/include/linux/coda.h
===================================================================
--- linux-2.4.29-pre3-bk4.orig/include/linux/coda.h 2005-01-06 15:37:01.576583328 -0500
+++ linux-2.4.29-pre3-bk4/include/linux/coda.h 2005-01-06 09:12:40.000000000 -0500
@@ -767,8 +767,8 @@
#define PIOCPARM_MASK 0x0000ffff
struct ViceIoctl {
caddr_t in, out; /* Data to be transferred in, or out */
- short in_size; /* Size of input buffer <= 2K */
- short out_size; /* Maximum size of output buffer, <= 2K */
+ u_short in_size; /* Size of input buffer <= 2K */
+ u_short out_size; /* Maximum size of output buffer, <= 2K */
};
struct PioctlData {
Index: linux-2.4.29-pre3-bk4/fs/coda/upcall.c
===================================================================
--- linux-2.4.29-pre3-bk4.orig/fs/coda/upcall.c 2005-01-06 15:37:01.609578312 -0500
+++ linux-2.4.29-pre3-bk4/fs/coda/upcall.c 2005-01-06 15:36:24.849166744 -0500
@@ -543,6 +543,11 @@
goto exit;
}
+ if (data->vi.out_size > VC_MAXDATASIZE) {
+ error = -EINVAL;
+ goto exit;
+ }
+
inp->coda_ioctl.VFid = *fid;
/* the cmd field was mutated by increasing its size field to
@@ -571,26 +576,30 @@
error, coda_f2s(fid));
goto exit;
}
-
- /* Copy out the OUT buffer. */
+
+ if (outsize < (long)outp->coda_ioctl.data + outp->coda_ioctl.len) {
+ CDEBUG(D_FILE, "reply size %d < reply len %ld\n", outsize,
+ (long)outp->coda_ioctl.data + outp->coda_ioctl.len);
+ error = -EINVAL;
+ goto exit;
+ }
+
if (outp->coda_ioctl.len > data->vi.out_size) {
- CDEBUG(D_FILE, "return len %d <= request len %d\n",
- outp->coda_ioctl.len,
- data->vi.out_size);
+ CDEBUG(D_FILE, "return len %d > request len %d\n",
+ outp->coda_ioctl.len, data->vi.out_size);
error = -EINVAL;
- } else {
- error = verify_area(VERIFY_WRITE, data->vi.out,
- data->vi.out_size);
- if ( error ) goto exit;
-
- if (copy_to_user(data->vi.out,
- (char *)outp + (long)outp->coda_ioctl.data,
- data->vi.out_size)) {
- error = -EINVAL;
- goto exit;
- }
+ goto exit;
}
+ /* Copy out the OUT buffer. */
+ error = verify_area(VERIFY_WRITE, data->vi.out, outp->coda_ioctl.len);
+ if ( error ) goto exit;
+
+ if (copy_to_user(data->vi.out,
+ (char *)outp + (long)outp->coda_ioctl.data,
+ outp->coda_ioctl.len)) {
+ error = -EINVAL;
+ }
exit:
CODA_FREE(inp, insize);
return error;
This patch adds bounds checks for tainted scalars
(reported by Brian Fulton and Ted Unangst, Coverity Inc.).
Signed-off-by: Jan Harkes <[email protected]>
Index: linux-2.6.10-mm2/include/linux/coda.h
===================================================================
--- linux-2.6.10-mm2.orig/include/linux/coda.h 2005-01-07 16:36:03.000000000 -0500
+++ linux-2.6.10-mm2/include/linux/coda.h 2005-01-07 16:42:20.000000000 -0500
@@ -761,8 +761,8 @@
struct ViceIoctl {
void __user *in; /* Data to be transferred in */
void __user *out; /* Data to be transferred out */
- short in_size; /* Size of input buffer <= 2K */
- short out_size; /* Maximum size of output buffer, <= 2K */
+ u_short in_size; /* Size of input buffer <= 2K */
+ u_short out_size; /* Maximum size of output buffer, <= 2K */
};
struct PioctlData {
Index: linux-2.6.10-mm2/fs/coda/upcall.c
===================================================================
--- linux-2.6.10-mm2.orig/fs/coda/upcall.c 2005-01-07 16:36:03.000000000 -0500
+++ linux-2.6.10-mm2/fs/coda/upcall.c 2005-01-07 16:53:03.074276720 -0500
@@ -555,6 +555,11 @@
goto exit;
}
+ if (data->vi.out_size > VC_MAXDATASIZE) {
+ error = -EINVAL;
+ goto exit;
+ }
+
inp->coda_ioctl.VFid = *fid;
/* the cmd field was mutated by increasing its size field to
@@ -583,19 +588,26 @@
error, coda_f2s(fid));
goto exit;
}
+
+ if (outsize < (long)outp->coda_ioctl.data + outp->coda_ioctl.len) {
+ error = -EINVAL;
+ goto exit;
+ }
/* Copy out the OUT buffer. */
if (outp->coda_ioctl.len > data->vi.out_size) {
error = -EINVAL;
- } else {
- if (copy_to_user(data->vi.out,
- (char *)outp + (long)outp->coda_ioctl.data,
- data->vi.out_size)) {
- error = -EFAULT;
- goto exit;
- }
+ goto exit;
}
+ /* Copy out the OUT buffer. */
+ if (copy_to_user(data->vi.out,
+ (char *)outp + (long)outp->coda_ioctl.data,
+ outp->coda_ioctl.len)) {
+ error = -EFAULT;
+ goto exit;
+ }
+
exit:
CODA_FREE(inp, insize);
return error;