New: Off-by-two stack buffer overflow in function rpc_uaddr2sockaddr() of net/sunrpc/addr.c

Bugme-new New Andrew Morton 7 participants 8 messages Nov 10, 2009 – Nov 12, 2009 3 messages not in the archive

Subsystems (4)

Networking Network drivers NFS client NFS server

Share & tools

A message that is not in the archive (sent off-list or not yet received): [email protected]/

A message that is not in the archive (sent off-list or not yet received): bug-14546-10286-V0hAGp6uBxO456/isadD/[email protected]/

Andrew Morton

author

2009-11-10 23:29 UTC #2 lore raw

(switched to email. Please respond via emailed reply-to-all, not via the bugzilla web interface).

Quoted 30 lines · On Thu, 5 Nov 2009 10:31:03 GMT bugzilla-daemon-590EEB7GvNiWaY/ihj7yzEB+6BGkLq7…

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: [email protected]
ReportedBy: [email protected]
CC: [email protected]
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:

Quoted 8 lines

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 renamed from net/sunrpc/addr.c.orig +1 -1

@@ -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);

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Chuck Lever

2009-11-10 23:38 UTC #3 lore raw

Quoted 54 lines · Andrew Morton

(switched to email. Please respond via emailed reply-to-all, not via the bugzilla web interface).
Quoted 30 lines · On Thu, 5 Nov 2009 10:31:03 GMT [email protected] 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: [email protected]
ReportedBy: [email protected]
CC: [email protected]
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:
Quoted 8 lines
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 line 320

Quoted 6 lines

net/sunrpc/addr.c renamed from net/sunrpc/addr.c.orig +1 -1

@@ -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.

net/sunrpc/addr.c line 322

Quoted 1 line

net/sunrpc/addr.c renamed from net/sunrpc/addr.c.orig +0 -0
 	memcpy(buf, uaddr, uaddr_len);

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

A message that is not in the archive (sent off-list or not yet received): 967DC2CE-588D-4207-BF2D-59727454DC2E-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org

Patroklos Argyroudis

2009-11-11 07:51 UTC #5 lore raw

Quoted 5 lines · Andrew Morton

Quoted 5 lines
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.

Quoted 5 lines · Chuck Lever

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.
Quoted 1 line
	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
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Fabio Olive Leite

2009-11-11 12:11 UTC #6 lore raw

Quoted 10 lines · On 2009-11-11 Patroklos Argyroudis wrote:

Quoted 5 lines · Chuck Lever
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.
Quoted 1 line
	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é

Signature · 7 lines

ex sed lex awk yacc, e pluribus unix, amem
na matemática das idéias, permuta é igual a adição
e um debate inteligente implementa a multiplicação
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Fabio Olive Leite

2009-11-11 12:34 UTC #7 lore raw

In reply to Patroklos Argyroudis

Quoted 3 lines · 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é

Signature · 7 lines

ex sed lex awk yacc, e pluribus unix, amem
na matemática das idéias, permuta é igual a adição
e um debate inteligente implementa a multiplicação
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Chuck Lever

2009-11-11 15:53 UTC #8 lore raw

In reply to a message not in the archive

Quoted 8 lines · On 2009-11-11 Fabio Olive Leite wrote:

Quoted 3 lines · 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
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Neil Brown

2009-11-12 05:55 UTC #9 lore raw

Quoted 8 lines · On Wednesday November 11, [email protected] wrote:

Quoted 6 lines · On 2009-11-11 Fabio Olive Leite wrote:
Quoted 3 lines · 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'

Quoted 6 lines

Quoted 4 lines
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

Patroklos Argyroudis

2009-11-11 11:02 UTC #10 lore raw

PATCH 1/1 sunrpc: off-by-two stack buffer overflow in function rpc_uaddr2sockaddr()

In reply to Andrew Morton

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 <[email protected]>

---

net/sunrpc/addr.c 2009-11-11 12:33:32.000000000 +0200 renamed from net/sunrpc/addr.c.orig 2009-11-11 12:33:04.000000000 +0200 +1 -1

@@ -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);