Bug 2135
Summary: | setsockopt() syscall IP_MSFILTER option logic error leads to leaking kernel memory information | ||
---|---|---|---|
Product: | Networking | Reporter: | hsj |
Component: | IPV4 | Assignee: | Nivedita Singhvi (niv) |
Status: | CLOSED CODE_FIX | ||
Severity: | normal | ||
Priority: | P2 | ||
Hardware: | i386 | ||
OS: | Linux | ||
Kernel Version: | 2.6.3 | Subsystem: | |
Regression: | --- | Bisected commit-id: | |
Attachments: | Patch from Dave for 2135 |
Description
hsj
2004-02-18 08:50:51 UTC
Hi, Am entering the following from Dave Stevens, who looked into the problem. Closing the bug, let me know if you have any questions, or contact Dave at dlstevens at us dot ibm dot com. Also attaching the patch he provided. thanks, Nivedita ---- I was able to reproduce the bug that allowed read-access to kernel buffers beyond the allocated temporary buffer, and have added that portion of your proposed patch (thanks!). The test for overflow in your patch confused me, because it failed to take into account that numsrc is an address count and the macros produce byte counts. Also, IP_SFLSIZE is an unrelated macro-- it should be IP_MSFILTERSIZE() for that test. These are corrected and simplified in the patch below, since an overflow has occurred any time IP_MSFILTER_SIZE(msf->numsrc) is less than that of "0". Bogus values for optlen are protected by the fact that they are sock_kmalloc'ed (a limit size), but the temporary buffer can be arbitrarily large, though it's ok to fail. I've added a test to restrict the size of optlen, as well. I have not addressed the issue you mention of optlen being larger than imsf_numsrc implies causing unnecessary data to be copied from user space. That's true, but it avoids doing two copyin's and sticking them together. Users that use the macros for setsockopt()'s optlen will have the exact size and users that don't will not. Thanks for reporting the problem! +-DLS Created attachment 2218 [details]
Patch from Dave for 2135
i'm sorry that explanation is insufficient...
> (IP_SFLSIZE(msf->imsf_numsrc) - IP_SFLSIZE(0)) < msf->imsf_numsrc
this is a check for guaranteeing that the value of msf->imsf_numsrc is smaller
than 0x40000000 (on 32bit environment)...
# yes, i know, this method may be lame :)
this (or similar check) is required.
e.g.
on 32bit environment, IP_MSFILTER_SIZE(0) == IP_MSFILTER_SIZE(0x40000000) ==
IP_MSFILTER_SIZE(0x80000000) == IP_MSFILTER_SIZE(0xc0000000)
cause integer overflow...
in net/ipv4/igmp.c ip_mc_msfilter() func,
--------------------------------------------------------------------------------
newpsl = (struct ip_sf_socklist *)sock_kmalloc(sk,
IP_SFLSIZE(msf->imsf_numsrc), GFP_KERNEL);
--- snip ---
newpsl->sl_max = newpsl->sl_count = msf->imsf_numsrc;
memcpy(newpsl->sl_addr, msf->imsf_slist,
msf->imsf_numsrc * sizeof(msf->imsf_slist[0]));
--------------------------------------------------------------------------------
thanks for ip_msfilter's imsf_slist and ip_sf_socklist's sl_addr are the same
sizes (__u32), memcpy() is no problem.
# btw, since the code of ipv6 is processed by the loop, it crashes here.
however, newpsl->sl_count is corrupted by the bogus value.
in net/ipv4/igmp.c ip_mc_msfget() func,
--------------------------------------------------------------------------------
if (!psl) {
len = 0;
count = 0;
} else {
count = psl->sl_count;
}
copycount = count < msf->imsf_numsrc ? count : msf->imsf_numsrc;
len = copycount * sizeof(psl->sl_addr[0]);
msf->imsf_numsrc = count;
if (put_user(IP_MSFILTER_SIZE(copycount), optlen) ||
copy_to_user((void *)optval, msf, IP_MSFILTER_SIZE(0))) {
return -EFAULT;
}
if (len &&
copy_to_user((void *)&optval->imsf_slist[0], psl->sl_addr, len))
return -EFAULT;
return 0;
--------------------------------------------------------------------------------
copycount is set to msf->imsf_numsrc when msf->imsf_numsrc is smaller than
count (this is above mentioned bogus value).
# count is signed, but msf->imsf_numsrc is unsigned,
# so comparison is performed as unsigned...
consequently, almost arbitrary values can be assigned to len.
// this is a sample.c -> sample2.c patch
// sample2.c reproduces a problem even if Dave's patch is applied
--------------------------------------------------------------------------------
--- sample.c Thu Feb 26 02:12:25 2004
+++ sample2.c Thu Feb 26 02:13:21 2004
@@ -53,12 +53,13 @@
imsfp->imsf_multiaddr = (unsigned int)inet_addr(MULTICAST);
imsfp->imsf_interface = (unsigned int)inet_addr(ADDR);
imsfp->imsf_fmode = MCAST_INCLUDE;
- imsfp->imsf_numsrc = SIZE;
+ imsfp->imsf_numsrc = 0x80000000; // this assumes the
32bit environment...
ret = setsockopt(s,SOL_IP,IP_MSFILTER,buf,IP_MSFILTER_SIZE(0));
printf("setsockopt2 ret[%d]\n",ret);
if(ret)
break;
+ imsfp->imsf_numsrc = SIZE;
len = sizeof(buf);
ret = getsockopt(s,SOL_IP,IP_MSFILTER,buf,&len);
printf("getsockopt ret[%d] len[%d]\n",ret,len);
--------------------------------------------------------------------------------
primarily, my patch should have been checked as follows.
--------------------------------------------------------------------------------
if(((IP_SFLSIZE(msf->imsf_numsrc) - IP_SFLSIZE(0)) < msf->imsf_numsrc)
||
((IP_MSFILTER_SIZE(msf->imsf_numsrc) - IP_MSFILTER_SIZE(0)) <
msf->imsf_numsrc) ||
(IP_MSFILTER_SIZE(msf->imsf_numsrc) > optlen)) {
--------------------------------------------------------------------------------
however, since ip_msfilter's imsf_slist and ip_sf_socklist's sl_addr are the
same sizes, the 2nd line is omissible.
in ipv6 code, since group_filter's gf_slist (struct __kernel_sockaddr_storage)
and ip6_sf_socklist's sl_addr (struct in6_addr) sizes differ, it is necessary
to check firmly.
hope this helps. :)
Respectfully,
hsj
Thanks for the testing and info..I'll have Dave look into it :). Nivedita I think that this was already resolved. If so, please close this entry. thanks. sorry, i forgot to close an entry. this bug was already fixed. :) Respectfully, hsj |