Bug 14546 - Off-by-two stack buffer overflow in function rpc_uaddr2sockaddr() of net/sunrpc/addr.c
Off-by-two stack buffer overflow in function rpc_uaddr2sockaddr() of net/sunr...
Status: CLOSED CODE_FIX
Product: Networking
Classification: Unclassified
Component: Other
All Linux
: P1 normal
Assigned To: Arnaldo Carvalho de Melo
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-11-05 10:31 UTC by Patroklos Argyroudis
Modified: 2009-11-20 17:02 UTC (History)
2 users (show)

See Also:
Kernel Version: 2.6.32-rc6
Tree: Mainline
Regression: No


Attachments
Patch to fix the off-by-two stack buffer overflow (316 bytes, patch)
2009-11-05 10:32 UTC, Patroklos Argyroudis
Details | Diff

Description Patroklos Argyroudis 2009-11-05 10:31:00 UTC
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.
Comment 1 Patroklos Argyroudis 2009-11-05 10:32:31 UTC
Created attachment 23659 [details]
Patch to fix the off-by-two stack buffer overflow
Comment 2 Andrew Morton 2009-11-10 23:29:13 UTC
(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);
Comment 3 Chuck Lever 2009-11-10 23:39:30 UTC
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
Comment 4 Patroklos Argyroudis 2009-11-11 07:42:42 UTC
(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.
Comment 5 Patroklos Argyroudis 2009-11-11 07:51:41 UTC
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
Comment 6 Anonymous Emailer 2009-11-11 12:12:14 UTC
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é
Comment 7 Anonymous Emailer 2009-11-11 12:34:56 UTC
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é
Comment 8 Chuck Lever 2009-11-11 15:55:37 UTC
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
Comment 9 Neil Brown 2009-11-12 05:55:21 UTC
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
Comment 10 Trond Myklebust 2009-11-20 17:01:49 UTC
The fix has now been merged into mainline. See commit 1e360a60b24ad8f8685af66fa6de10ce46693a4b (SUNRPC: Address buffer overrun in rpc_uaddr2sockaddr()).

Note You need to log in before you can comment on or make changes to this bug.