Bug 14546
| Summary: | Off-by-two stack buffer overflow in function rpc_uaddr2sockaddr() of net/sunrpc/addr.c | ||
|---|---|---|---|
| Product: | Networking | Reporter: | Patroklos Argyroudis (argp) |
| Component: | Other | Assignee: | Arnaldo Carvalho de Melo (acme) |
| Status: | CLOSED CODE_FIX | ||
| Severity: | normal | CC: | argp, trondmy |
| Priority: | P1 | ||
| Hardware: | All | ||
| OS: | Linux | ||
| Kernel Version: | 2.6.32-rc6 | Subsystem: | |
| Regression: | No | Bisected commit-id: | |
| Attachments: | Patch to fix the off-by-two stack buffer overflow | ||
Created attachment 23659 [details]
Patch to fix the off-by-two stack buffer overflow
(switched to email. Please respond via emailed reply-to-all, not via the bugzilla web interface). On Thu, 5 Nov 2009 10:31:03 GMT bugzilla-daemon@bugzilla.kernel.org wrote: > http://bugzilla.kernel.org/show_bug.cgi?id=14546 > > Summary: Off-by-two stack buffer overflow in function > rpc_uaddr2sockaddr() of net/sunrpc/addr.c > Product: Networking > Version: 2.5 > Kernel Version: 2.6.32-rc6 > Platform: All > OS/Version: Linux > Tree: Mainline > Status: NEW > Severity: normal > Priority: P1 > Component: Other > AssignedTo: acme@ghostprotocols.net > ReportedBy: argp@census-labs.com > CC: argp@census-labs.com > Regression: No > > > There is an off-by-two stack buffer overflow in function rpc_uaddr2sockaddr() > of file net/sunrpc/addr.c in the Linux kernel SUNRPC implementation. > > The function rpc_uaddr2sockaddr() that is used to convert a universal address > to a socket address takes as an argument the size_t variable uaddr_len (the > length of the universal address string). The stack buffer buf is declared in > line 315 to be of size RPCBIND_MAXUADDRLEN. If the passed argument uaddr_len > is > equal to RPCBIND_MAXUADDRLEN then the check at line 319 passes and then at > lines 324 and 325 there are two out-of-bounds assignments: > > 319 if (uaddr_len > sizeof(buf)) > 320 return 0; > ... > 324 buf[uaddr_len] = '\n'; > 325 buf[uaddr_len + 1] = '\0'; > > To fix it please see the attached patch. > Please don't submit patches via bugzilla. Please prepare this patch as per Documentation/SubmittingPatches and email it to all the recipients of this email, thanks. --- ./net/sunrpc/addr.c.orig 2009-11-05 11:55:45.000000000 +0200 +++ ./net/sunrpc/addr.c 2009-11-05 12:09:34.000000000 +0200 @@ -316,7 +316,7 @@ unsigned long portlo, porthi; unsigned short port; - if (uaddr_len > sizeof(buf)) + if (uaddr_len > sizeof(buf) - 2) return 0; memcpy(buf, uaddr, uaddr_len); On Nov 10, 2009, at 6:29 PM, Andrew Morton wrote: > > (switched to email. Please respond via emailed reply-to-all, not > via the > bugzilla web interface). > > On Thu, 5 Nov 2009 10:31:03 GMT > bugzilla-daemon@bugzilla.kernel.org wrote: > >> http://bugzilla.kernel.org/show_bug.cgi?id=14546 >> >> Summary: Off-by-two stack buffer overflow in function >> rpc_uaddr2sockaddr() of net/sunrpc/addr.c >> Product: Networking >> Version: 2.5 >> Kernel Version: 2.6.32-rc6 >> Platform: All >> OS/Version: Linux >> Tree: Mainline >> Status: NEW >> Severity: normal >> Priority: P1 >> Component: Other >> AssignedTo: acme@ghostprotocols.net >> ReportedBy: argp@census-labs.com >> CC: argp@census-labs.com >> Regression: No >> >> >> There is an off-by-two stack buffer overflow in function >> rpc_uaddr2sockaddr() >> of file net/sunrpc/addr.c in the Linux kernel SUNRPC implementation. >> >> The function rpc_uaddr2sockaddr() that is used to convert a >> universal address >> to a socket address takes as an argument the size_t variable >> uaddr_len (the >> length of the universal address string). The stack buffer buf is >> declared in >> line 315 to be of size RPCBIND_MAXUADDRLEN. If the passed argument >> uaddr_len is >> equal to RPCBIND_MAXUADDRLEN then the check at line 319 passes and >> then at >> lines 324 and 325 there are two out-of-bounds assignments: >> >> 319 if (uaddr_len > sizeof(buf)) >> 320 return 0; >> ... >> 324 buf[uaddr_len] = '\n'; >> 325 buf[uaddr_len + 1] = '\0'; >> >> To fix it please see the attached patch. >> > > Please don't submit patches via bugzilla. > > Please prepare this patch as per Documentation/SubmittingPatches and > email it to all the recipients of this email, thanks. > > --- ./net/sunrpc/addr.c.orig 2009-11-05 11:55:45.000000000 +0200 > +++ ./net/sunrpc/addr.c 2009-11-05 12:09:34.000000000 +0200 > @@ -316,7 +316,7 @@ > unsigned long portlo, porthi; > unsigned short port; > > - if (uaddr_len > sizeof(buf)) > + if (uaddr_len > sizeof(buf) - 2) > return 0; Why wouldn't you bump the size of the buffer by two as well? Otherwise valid universal addresses that are RPCBIND_MAXUADDRLEN bytes long will fail here. > memcpy(buf, uaddr, uaddr_len); -- Chuck Lever chuck[dot]lever[at]oracle[dot]com (In reply to comment #2) > Please prepare this patch as per Documentation/SubmittingPatches and > email it to all the recipients of this email, thanks. Ok, I will do so. (In reply to comment #3) > Why wouldn't you bump the size of the buffer by two as well? > Otherwise valid universal addresses that are RPCBIND_MAXUADDRLEN bytes > long will fail here. > > > memcpy(buf, uaddr, uaddr_len); There is no need to increase the size of the buffer since the new check (if (uaddr_len > sizeof(buf) - 2)) will terminate the function in case the valid universal address is RPCBIND_MAXUADDRLEN bytes. On Nov 10, 2009, at 6:29 PM, Andrew Morton wrote: > > > >Please don't submit patches via bugzilla. > > > >Please prepare this patch as per Documentation/SubmittingPatches and > >email it to all the recipients of this email, thanks. Ok, I will do so. On Tue, Nov 10, 2009 at 06:38:05PM -0500, Chuck Lever wrote: > Why wouldn't you bump the size of the buffer by two as well? > Otherwise valid universal addresses that are RPCBIND_MAXUADDRLEN > bytes long will fail here. > > > memcpy(buf, uaddr, uaddr_len); There is no need to increase the size of the buffer since the new check (if (uaddr_len > sizeof(buf) - 2)) will terminate the function in case the valid universal address is RPCBIND_MAXUADDRLEN bytes. Cheers, Patroklos Reply-To: fabio.olive@gmail.com On 2009-11-11 Patroklos Argyroudis wrote: > On Tue, Nov 10, 2009 at 06:38:05PM -0500, Chuck Lever wrote: > > Why wouldn't you bump the size of the buffer by two as well? > > Otherwise valid universal addresses that are RPCBIND_MAXUADDRLEN > > bytes long will fail here. > > > > > memcpy(buf, uaddr, uaddr_len); > > There is no need to increase the size of the buffer since the new > check (if (uaddr_len > sizeof(buf) - 2)) will terminate the function > in case the valid universal address is RPCBIND_MAXUADDRLEN bytes. Failing to convert a valid address is incorrect and unexpected. What Chuck meant is that since it is valid to have an address up to RPCBIND_MAXUADDRLEN bytes long, the function should be able to work on that, by having an internal buffer that allows for the extra "\n\0". Cheers, Fábio Olivé Reply-To: fabio.olive@gmail.com On 2009-11-11 Patroklos Argyroudis wrote: > There is no need to increase the size of the buffer since the new > check (if (uaddr_len > sizeof(buf) - 2)) will terminate the function > in case the valid universal address is RPCBIND_MAXUADDRLEN bytes. On a second note, why is '\n' needed there? You should only need '\0', as a '\n' at the end is not required by any of the string functions used to convert the address. I believe you could go with buf[RPCBIND_MAXUADDRLEN+1] for the extra NUL only. Cheers, Fábio Olivé On 2009-11-11 Fabio Olive Leite wrote: > On 2009-11-11 Patroklos Argyroudis wrote: >> There is no need to increase the size of the buffer since the new >> check (if (uaddr_len > sizeof(buf) - 2)) will terminate the function >> in case the valid universal address is RPCBIND_MAXUADDRLEN bytes. > On a second note, why is '\n' needed there? You should only need > '\0', as a '\n' > at the end is not required by any of the string functions used to > convert the > address. I believe you could go with buf[RPCBIND_MAXUADDRLEN+1] for > the extra NUL only. AFAICT, strict_strtoul() requires the '\n\0' termination. -- Chuck Lever chuck[dot]lever[at]oracle[dot]com On Wednesday November 11, chuck.lever@oracle.com wrote: > On 2009-11-11 Fabio Olive Leite wrote: > > On 2009-11-11 Patroklos Argyroudis wrote: > >> There is no need to increase the size of the buffer since the new > >> check (if (uaddr_len > sizeof(buf) - 2)) will terminate the function > >> in case the valid universal address is RPCBIND_MAXUADDRLEN bytes. > > On a second note, why is '\n' needed there? You should only need > > '\0', as a '\n' > > > at the end is not required by any of the string functions used to > > convert the > > address. I believe you could go with buf[RPCBIND_MAXUADDRLEN+1] for > > the extra NUL only. > > AFAICT, strict_strtoul() requires the '\n\0' termination. if ((*tail == '\0') || ((len == (size_t)(tail - cp) + 1) && (*tail == '\n'))) { *res = val; return 0; } allows, not requires. Though admittedly that code isn't as clear as one might like: if (tail[0] == 0 || (tail[0] == '\n' && tail[1] == 0) { ..... } NeilBrown The fix has now been merged into mainline. See commit 1e360a60b24ad8f8685af66fa6de10ce46693a4b (SUNRPC: Address buffer overrun in rpc_uaddr2sockaddr()). |
There is an off-by-two stack buffer overflow in function rpc_uaddr2sockaddr() of file net/sunrpc/addr.c in the Linux kernel SUNRPC implementation. The function rpc_uaddr2sockaddr() that is used to convert a universal address to a socket address takes as an argument the size_t variable uaddr_len (the length of the universal address string). The stack buffer buf is declared in line 315 to be of size RPCBIND_MAXUADDRLEN. If the passed argument uaddr_len is equal to RPCBIND_MAXUADDRLEN then the check at line 319 passes and then at lines 324 and 325 there are two out-of-bounds assignments: 319 if (uaddr_len > sizeof(buf)) 320 return 0; ... 324 buf[uaddr_len] = '\n'; 325 buf[uaddr_len + 1] = '\0'; To fix it please see the attached patch.