Re: [Bugme-new] [Bug 14546] New: Off-by-two stack buffer overflow in function rpc_uaddr2sockaddr() of net/sunrpc/addr.c

Re: [Bugme-new] [Bug 14546] New: Off-by-two stack buffer overflow in function rpc_uaddr2sockaddr() of net/sunrpc/addr.c

[Andrew Morton]

(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-590EEB7GvNiWaY/ihj7yzEB+6BGkLq7r@public.gmane.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-f8uhVLnGfZaxAyOMLChx1axOck334EZe@public.gmane.org
>         ReportedBy: argp-YZAGAMbGdGKGw+nKnLezzg@public.gmane.org
>                 CC: argp-YZAGAMbGdGKGw+nKnLezzg@public.gmane.org
>         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);

Re: [Bugme-new] [Bug 14546] New: Off-by-two stack buffer overflow in function rpc_uaddr2sockaddr() of net/sunrpc/addr.c

[Chuck Lever]

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

Re: [Bugme-new] [Bug 14546] New: Off-by-two stack buffer overflow in function rpc_uaddr2sockaddr() of net/sunrpc/addr.c

[Patroklos Argyroudis]

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

Re: [Bugme-new] [Bug 14546] New: Off-by-two stack buffer overflow in function rpc_uaddr2sockaddr() of net/sunrpc/addr.c

[Fabio Olive Leite]

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.
> >=20
> > >	memcpy(buf, uaddr, uaddr_len);
>=20
> 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.

=46ailing 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,
=46=C3=A1bio Oliv=C3=A9
--=20
ex sed lex awk yacc, e pluribus unix, amem
na matem=C3=A1tica das id=C3=A9ias, permuta =C3=A9 igual a adi=C3=A7=C3=
=A3o
e um debate inteligente implementa a multiplica=C3=A7=C3=A3o

Re: [Bugme-new] [Bug 14546] New: Off-by-two stack buffer overflow in function rpc_uaddr2sockaddr() of net/sunrpc/addr.c

[Fabio Olive Leite]

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 use=
d
to convert the address.

I believe you could go with buf[RPCBIND_MAXUADDRLEN+1] for the extra
NUL only.

Cheers,
=46=C3=A1bio Oliv=C3=A9
--=20
ex sed lex awk yacc, e pluribus unix, amem
na matem=C3=A1tica das id=C3=A9ias, permuta =C3=A9 igual a adi=C3=A7=C3=
=A3o
e um debate inteligente implementa a multiplica=C3=A7=C3=A3o

Re: [Bugme-new] [Bug 14546] New: Off-by-two stack buffer overflow in function rpc_uaddr2sockaddr() of net/sunrpc/addr.c

[Chuck Lever]

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

Re: [Bugme-new] [Bug 14546] New: Off-by-two stack buffer overflow in function rpc_uaddr2sockaddr() of net/sunrpc/addr.c

[Neil Brown]

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

[PATCH] sunrpc: off-by-two stack buffer overflow in function rpc_uaddr2sockaddr()

[Patroklos Argyroudis]

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';

Signed-off-by: Patroklos Argyroudis <argp@census-labs.com>
---

--- linux-2.6/net/sunrpc/addr.c.orig    2009-11-11 12:33:04.000000000 +0200
+++ linux-2.6/net/sunrpc/addr.c 2009-11-11 12:33:32.000000000 +0200
@@ -316,7 +316,7 @@ size_t rpc_uaddr2sockaddr(const char *ua
    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);