Hi,
I noticed that something weird is going on with /proc/sys/sunrpc/transports.
This file is generated in net/sunrpc/sysctl.c, function proc_do_xprt(). When
I "cat" this file, I get the expected output:
$ cat /proc/sys/sunrpc/transports
tcp 1048576
udp 32768
But I think that it does not check the length of the buffer supplied by
userspace to read(). With my original program, I found that the stack was
being overwritten by the characters above, even when the length given to
read() was just 1. So I have created a test program, see it at the bottom of
this e-mail. Here is its output:
$ gcc -Wall -std=gnu99 proc-sys-sunrpc-transports.c && ./a.out
read(0) returned 0, errno = 0
read(1) returned -1, errno = 21
read(2) returned -1, errno = 20
read(3) returned -1, errno = 19
read(4) returned -1, errno = 18
read(5) returned -1, errno = 17
read(6) returned -1, errno = 16
read(7) returned -1, errno = 15
...etc. This program just reopens the file and tries to read a different
number of characters each time. Strace can be used to verify that it really
returns these errnos.
With a small change to the program, we can also compare the buffer address
passed to read() and see that the kernel wrote the full file into the buffer
regardless of how many bytes we wanted to get back.
I don't know if this could be used in some malicious way. Maybe if a setuid
root program tried to open a user-supplied file (which could be this one in
/proc), it could crash the program quite easily. But since there is no way
to change the contents of the file... I don't know. Maybe a denial of
service attack where some user can make a daemon open a custom file and
crash? But it relies on the buffer being small enough, and the file is only
38 bytes on my machine. Who would pass a buffer to read that was smaller
than 38 bytes anyway? :-)
This file was introduced in
commit dc9a16e49dbba3dd042e6aec5d9a7929e099a89b
Author: Tom Tucker <[email protected]>
Date: Sun Dec 30 21:08:31 2007 -0600
svc: Add /proc/sys/sunrpc/transport files
Add a file that when read lists the set of registered svc
transports.
Signed-off-by: Tom Tucker <[email protected]>
Acked-by: Neil Brown <[email protected]>
Reviewed-by: Chuck Lever <[email protected]>
Reviewed-by: Greg Banks <[email protected]>
Signed-off-by: J. Bruce Fields <[email protected]>
...adding everybody to Cc.
Thanks,
Vegard
--
#include <errno.h>
#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
int
main(void)
{
for (int i = 0; i < 20; ++i) {
int fd = open("/proc/sys/sunrpc/transports", O_RDONLY);
if (fd == -1)
exit(EXIT_FAILURE);
char buf[512];
memset(buf, 0, sizeof(buf));
int ret = read(fd, buf, i);
printf("read(%d) returned %d, errno = %d\n", i, ret, errno);
close(fd);
}
return EXIT_SUCCESS;
}
[Vegard Nossum - Sat, Aug 30, 2008 at 08:44:22PM +0200]
| Hi,
|
| I noticed that something weird is going on with /proc/sys/sunrpc/transports.
| This file is generated in net/sunrpc/sysctl.c, function proc_do_xprt(). When
| I "cat" this file, I get the expected output:
|
| $ cat /proc/sys/sunrpc/transports
| tcp 1048576
| udp 32768
|
| But I think that it does not check the length of the buffer supplied by
| userspace to read(). With my original program, I found that the stack was
| being overwritten by the characters above, even when the length given to
| read() was just 1. So I have created a test program, see it at the bottom of
| this e-mail. Here is its output:
|
...
Indeed, maybe just add checking for user buffer length?
As proc_dodebug() in this file are doing. I don't think
the user would be happy with his stack burned :)
Something like:
---
Index: linux-2.6.git/net/sunrpc/sysctl.c
===================================================================
--- linux-2.6.git.orig/net/sunrpc/sysctl.c 2008-07-20 11:40:14.000000000 +0400
+++ linux-2.6.git/net/sunrpc/sysctl.c 2008-08-30 23:05:30.000000000 +0400
@@ -69,6 +69,8 @@ static int proc_do_xprt(ctl_table *table
return -EINVAL;
else {
len = svc_print_xprts(tmpbuf, sizeof(tmpbuf));
+ if (*lenp < len)
+ return -EFAULT;
if (!access_ok(VERIFY_WRITE, buffer, len))
return -EFAULT;
On Sat, Aug 30, 2008 at 9:06 PM, Cyrill Gorcunov <[email protected]> wrote:
> [Vegard Nossum - Sat, Aug 30, 2008 at 08:44:22PM +0200]
> | Hi,
> |
> | I noticed that something weird is going on with /proc/sys/sunrpc/transports.
> | This file is generated in net/sunrpc/sysctl.c, function proc_do_xprt(). When
> | I "cat" this file, I get the expected output:
> |
> | $ cat /proc/sys/sunrpc/transports
> | tcp 1048576
> | udp 32768
> |
> | But I think that it does not check the length of the buffer supplied by
> | userspace to read(). With my original program, I found that the stack was
> | being overwritten by the characters above, even when the length given to
> | read() was just 1. So I have created a test program, see it at the bottom of
> | this e-mail. Here is its output:
> |
> ...
>
> Indeed, maybe just add checking for user buffer length?
> As proc_dodebug() in this file are doing. I don't think
> the user would be happy with his stack burned :)
>
> Something like:
> ---
>
> Index: linux-2.6.git/net/sunrpc/sysctl.c
> ===================================================================
> --- linux-2.6.git.orig/net/sunrpc/sysctl.c 2008-07-20 11:40:14.000000000 +0400
> +++ linux-2.6.git/net/sunrpc/sysctl.c 2008-08-30 23:05:30.000000000 +0400
> @@ -69,6 +69,8 @@ static int proc_do_xprt(ctl_table *table
> return -EINVAL;
> else {
> len = svc_print_xprts(tmpbuf, sizeof(tmpbuf));
> + if (*lenp < len)
> + return -EFAULT;
> if (!access_ok(VERIFY_WRITE, buffer, len))
> return -EFAULT;
>
Hm. I think this is wrong. Shouldn't we copy as many bytes as the user
indicated?
Vegard
--
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
-- E. W. Dijkstra, EWD1036
[Vegard Nossum - Sat, Aug 30, 2008 at 09:15:16PM +0200]
| On Sat, Aug 30, 2008 at 9:06 PM, Cyrill Gorcunov <[email protected]> wrote:
| > [Vegard Nossum - Sat, Aug 30, 2008 at 08:44:22PM +0200]
| > | Hi,
| > |
| > | I noticed that something weird is going on with /proc/sys/sunrpc/transports.
| > | This file is generated in net/sunrpc/sysctl.c, function proc_do_xprt(). When
| > | I "cat" this file, I get the expected output:
| > |
| > | $ cat /proc/sys/sunrpc/transports
| > | tcp 1048576
| > | udp 32768
| > |
| > | But I think that it does not check the length of the buffer supplied by
| > | userspace to read(). With my original program, I found that the stack was
| > | being overwritten by the characters above, even when the length given to
| > | read() was just 1. So I have created a test program, see it at the bottom of
| > | this e-mail. Here is its output:
| > |
| > ...
| >
| > Indeed, maybe just add checking for user buffer length?
| > As proc_dodebug() in this file are doing. I don't think
| > the user would be happy with his stack burned :)
| >
| > Something like:
| > ---
| >
| > Index: linux-2.6.git/net/sunrpc/sysctl.c
| > ===================================================================
| > --- linux-2.6.git.orig/net/sunrpc/sysctl.c 2008-07-20 11:40:14.000000000 +0400
| > +++ linux-2.6.git/net/sunrpc/sysctl.c 2008-08-30 23:05:30.000000000 +0400
| > @@ -69,6 +69,8 @@ static int proc_do_xprt(ctl_table *table
| > return -EINVAL;
| > else {
| > len = svc_print_xprts(tmpbuf, sizeof(tmpbuf));
| > + if (*lenp < len)
| > + return -EFAULT;
| > if (!access_ok(VERIFY_WRITE, buffer, len))
| > return -EFAULT;
| >
|
| Hm. I think this is wrong. Shouldn't we copy as many bytes as the user
| indicated?
Well, hard to say what user-space programmer is expecting from us.
I mean - maybe he (reader) wants only part of results not the whole
contents BUT by this way he never know what the whole conetnts would be
until trying to read more (ie to check if there no more data from
kernel side). What is preferred behaviour - i don't know :)
|
|
| Vegard
|
| --
| "The animistic metaphor of the bug that maliciously sneaked in while
| the programmer was not looking is intellectually dishonest as it
| disguises that the error is the programmer's own creation."
| -- E. W. Dijkstra, EWD1036
|
- Cyrill -
[Cyrill Gorcunov - Sat, Aug 30, 2008 at 11:21:12PM +0400]
| [Vegard Nossum - Sat, Aug 30, 2008 at 09:15:16PM +0200]
| | On Sat, Aug 30, 2008 at 9:06 PM, Cyrill Gorcunov <[email protected]> wrote:
| | > [Vegard Nossum - Sat, Aug 30, 2008 at 08:44:22PM +0200]
| | > | Hi,
| | > |
| | > | I noticed that something weird is going on with /proc/sys/sunrpc/transports.
| | > | This file is generated in net/sunrpc/sysctl.c, function proc_do_xprt(). When
| | > | I "cat" this file, I get the expected output:
| | > |
| | > | $ cat /proc/sys/sunrpc/transports
| | > | tcp 1048576
| | > | udp 32768
| | > |
| | > | But I think that it does not check the length of the buffer supplied by
| | > | userspace to read(). With my original program, I found that the stack was
| | > | being overwritten by the characters above, even when the length given to
| | > | read() was just 1. So I have created a test program, see it at the bottom of
| | > | this e-mail. Here is its output:
| | > |
| | > ...
| | >
| | > Indeed, maybe just add checking for user buffer length?
| | > As proc_dodebug() in this file are doing. I don't think
| | > the user would be happy with his stack burned :)
| | >
| | > Something like:
| | > ---
| | >
| | > Index: linux-2.6.git/net/sunrpc/sysctl.c
| | > ===================================================================
| | > --- linux-2.6.git.orig/net/sunrpc/sysctl.c 2008-07-20 11:40:14.000000000 +0400
| | > +++ linux-2.6.git/net/sunrpc/sysctl.c 2008-08-30 23:05:30.000000000 +0400
| | > @@ -69,6 +69,8 @@ static int proc_do_xprt(ctl_table *table
| | > return -EINVAL;
| | > else {
| | > len = svc_print_xprts(tmpbuf, sizeof(tmpbuf));
| | > + if (*lenp < len)
| | > + return -EFAULT;
| | > if (!access_ok(VERIFY_WRITE, buffer, len))
| | > return -EFAULT;
| | >
| |
| | Hm. I think this is wrong. Shouldn't we copy as many bytes as the user
| | indicated?
|
| Well, hard to say what user-space programmer is expecting from us.
| I mean - maybe he (reader) wants only part of results not the whole
| contents BUT by this way he never know what the whole conetnts would be
| until trying to read more (ie to check if there no more data from
| kernel side). What is preferred behaviour - i don't know :)
|
| |
| |
| | Vegard
| |
| | --
| | "The animistic metaphor of the bug that maliciously sneaked in while
| | the programmer was not looking is intellectually dishonest as it
| | disguises that the error is the programmer's own creation."
| | -- E. W. Dijkstra, EWD1036
| |
|
| - Cyrill -
Btw, I didn't try to get size of sysfs file - will such an action return
size of data from kernel side?
- Cyrill -
On Sat, Aug 30, 2008 at 9:21 PM, Cyrill Gorcunov <[email protected]> wrote:
> | Hm. I think this is wrong. Shouldn't we copy as many bytes as the user
> | indicated?
>
> Well, hard to say what user-space programmer is expecting from us.
> I mean - maybe he (reader) wants only part of results not the whole
> contents BUT by this way he never know what the whole conetnts would be
> until trying to read more (ie to check if there no more data from
> kernel side). What is preferred behaviour - i don't know :)
For any other file, read(1) + read(1) should be exactly equivalent to
a read(2). What's the difference here?
(Btw, thanks for the quick reply :-))
Vegard
--
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
-- E. W. Dijkstra, EWD1036
On Sat, Aug 30, 2008 at 9:06 PM, Cyrill Gorcunov <[email protected]> wrote:
> [Vegard Nossum - Sat, Aug 30, 2008 at 08:44:22PM +0200]
> | Hi,
> |
> | I noticed that something weird is going on with /proc/sys/sunrpc/transports.
> | This file is generated in net/sunrpc/sysctl.c, function proc_do_xprt(). When
> | I "cat" this file, I get the expected output:
> |
> | $ cat /proc/sys/sunrpc/transports
> | tcp 1048576
> | udp 32768
> |
> | But I think that it does not check the length of the buffer supplied by
> | userspace to read(). With my original program, I found that the stack was
> | being overwritten by the characters above, even when the length given to
> | read() was just 1. So I have created a test program, see it at the bottom of
> | this e-mail. Here is its output:
> |
> ...
>
> Indeed, maybe just add checking for user buffer length?
> As proc_dodebug() in this file are doing. I don't think
> the user would be happy with his stack burned :)
>
> Something like:
> ---
>
> Index: linux-2.6.git/net/sunrpc/sysctl.c
> ===================================================================
> --- linux-2.6.git.orig/net/sunrpc/sysctl.c 2008-07-20 11:40:14.000000000 +0400
> +++ linux-2.6.git/net/sunrpc/sysctl.c 2008-08-30 23:05:30.000000000 +0400
> @@ -69,6 +69,8 @@ static int proc_do_xprt(ctl_table *table
> return -EINVAL;
> else {
> len = svc_print_xprts(tmpbuf, sizeof(tmpbuf));
> + if (*lenp < len)
> + return -EFAULT;
> if (!access_ok(VERIFY_WRITE, buffer, len))
> return -EFAULT;
>
>
BTW, look at this:
$ od -A x -t x1z /proc/sys/sunrpc/transports
000000 74 63 70 20 31 30 34 38 35 37 36 0a 75 64 70 20 >tcp 1048576.udp <
000010 33 32 37 36 38 0a 00 00 00 00 00 00 00 00 00 00 >32768...........<
000020 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >................<
*
0003e0 00 00 00 00 00 00 00 00 00 00 >..........<
0003ea
...and:
$ strace -e trace=read cat /proc/sys/sunrpc/transports > /dev/null
read(3, "\177ELF\1\1\1\0\0\0\0\0\0\0\0\0\3\0\3\0\1\0\0\0@G\316E4\0\0\0"...,
512) = 512
read(3, "tcp 1048576\nudp 32768\n\0\0\0\0\0\0\0\0\0\0"..., 4096) = 4074
read(3, "", 4096) = 0
...why does it have a huge return value? The output is only about 40
bytes... why add all the \0? Would your patch also fix this?
Vegard
--
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
-- E. W. Dijkstra, EWD1036
[Vegard Nossum - Sat, Aug 30, 2008 at 09:34:21PM +0200]
| On Sat, Aug 30, 2008 at 9:21 PM, Cyrill Gorcunov <[email protected]> wrote:
| > | Hm. I think this is wrong. Shouldn't we copy as many bytes as the user
| > | indicated?
| >
| > Well, hard to say what user-space programmer is expecting from us.
| > I mean - maybe he (reader) wants only part of results not the whole
| > contents BUT by this way he never know what the whole conetnts would be
| > until trying to read more (ie to check if there no more data from
| > kernel side). What is preferred behaviour - i don't know :)
|
| For any other file, read(1) + read(1) should be exactly equivalent to
| a read(2). What's the difference here?
Convinced completely :) Moreover proc_dodebug() does exactly
the same as you talking about.
|
| (Btw, thanks for the quick reply :-))
with my pleasure :)
|
|
| Vegard
|
| --
| "The animistic metaphor of the bug that maliciously sneaked in while
| the programmer was not looking is intellectually dishonest as it
| disguises that the error is the programmer's own creation."
| -- E. W. Dijkstra, EWD1036
|
- Cyrill -
---
Index: linux-2.6.git/net/sunrpc/sysctl.c
===================================================================
--- linux-2.6.git.orig/net/sunrpc/sysctl.c 2008-07-20 11:40:14.000000000 +0400
+++ linux-2.6.git/net/sunrpc/sysctl.c 2008-08-30 23:43:14.000000000 +0400
@@ -71,7 +71,8 @@ static int proc_do_xprt(ctl_table *table
len = svc_print_xprts(tmpbuf, sizeof(tmpbuf));
if (!access_ok(VERIFY_WRITE, buffer, len))
return -EFAULT;
-
+ if (*lenp < len)
+ len = *lenp;
if (__copy_to_user(buffer, tmpbuf, len))
return -EFAULT;
}
[Vegard Nossum - Sat, Aug 30, 2008 at 09:42:30PM +0200]
| On Sat, Aug 30, 2008 at 9:06 PM, Cyrill Gorcunov <[email protected]> wrote:
| > [Vegard Nossum - Sat, Aug 30, 2008 at 08:44:22PM +0200]
| > | Hi,
| > |
| > | I noticed that something weird is going on with /proc/sys/sunrpc/transports.
| > | This file is generated in net/sunrpc/sysctl.c, function proc_do_xprt(). When
| > | I "cat" this file, I get the expected output:
| > |
| > | $ cat /proc/sys/sunrpc/transports
| > | tcp 1048576
| > | udp 32768
| > |
| > | But I think that it does not check the length of the buffer supplied by
| > | userspace to read(). With my original program, I found that the stack was
| > | being overwritten by the characters above, even when the length given to
| > | read() was just 1. So I have created a test program, see it at the bottom of
| > | this e-mail. Here is its output:
| > |
| > ...
| >
| > Indeed, maybe just add checking for user buffer length?
| > As proc_dodebug() in this file are doing. I don't think
| > the user would be happy with his stack burned :)
| >
| > Something like:
| > ---
| >
| > Index: linux-2.6.git/net/sunrpc/sysctl.c
| > ===================================================================
| > --- linux-2.6.git.orig/net/sunrpc/sysctl.c 2008-07-20 11:40:14.000000000 +0400
| > +++ linux-2.6.git/net/sunrpc/sysctl.c 2008-08-30 23:05:30.000000000 +0400
| > @@ -69,6 +69,8 @@ static int proc_do_xprt(ctl_table *table
| > return -EINVAL;
| > else {
| > len = svc_print_xprts(tmpbuf, sizeof(tmpbuf));
| > + if (*lenp < len)
| > + return -EFAULT;
| > if (!access_ok(VERIFY_WRITE, buffer, len))
| > return -EFAULT;
| >
| >
|
| BTW, look at this:
|
| $ od -A x -t x1z /proc/sys/sunrpc/transports
| 000000 74 63 70 20 31 30 34 38 35 37 36 0a 75 64 70 20 >tcp 1048576.udp <
| 000010 33 32 37 36 38 0a 00 00 00 00 00 00 00 00 00 00 >32768...........<
| 000020 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >................<
| *
| 0003e0 00 00 00 00 00 00 00 00 00 00 >..........<
| 0003ea
|
| ...and:
|
| $ strace -e trace=read cat /proc/sys/sunrpc/transports > /dev/null
| read(3, "\177ELF\1\1\1\0\0\0\0\0\0\0\0\0\3\0\3\0\1\0\0\0@G\316E4\0\0\0"...,
| 512) = 512
| read(3, "tcp 1048576\nudp 32768\n\0\0\0\0\0\0\0\0\0\0"..., 4096) = 4074
| read(3, "", 4096) = 0
|
| ...why does it have a huge return value? The output is only about 40
| bytes... why add all the \0? Would your patch also fix this?
|
|
| Vegard
Get me some time to check
|
| --
| "The animistic metaphor of the bug that maliciously sneaked in while
| the programmer was not looking is intellectually dishonest as it
| disguises that the error is the programmer's own creation."
| -- E. W. Dijkstra, EWD1036
|
- Cyrill -
[Vegard Nossum - Sat, Aug 30, 2008 at 09:42:30PM +0200]
| On Sat, Aug 30, 2008 at 9:06 PM, Cyrill Gorcunov <[email protected]> wrote:
| > [Vegard Nossum - Sat, Aug 30, 2008 at 08:44:22PM +0200]
| > | Hi,
| > |
| > | I noticed that something weird is going on with /proc/sys/sunrpc/transports.
| > | This file is generated in net/sunrpc/sysctl.c, function proc_do_xprt(). When
| > | I "cat" this file, I get the expected output:
| > |
| > | $ cat /proc/sys/sunrpc/transports
| > | tcp 1048576
| > | udp 32768
| > |
| > | But I think that it does not check the length of the buffer supplied by
| > | userspace to read(). With my original program, I found that the stack was
| > | being overwritten by the characters above, even when the length given to
| > | read() was just 1. So I have created a test program, see it at the bottom of
| > | this e-mail. Here is its output:
| > |
| > ...
| >
| > Indeed, maybe just add checking for user buffer length?
| > As proc_dodebug() in this file are doing. I don't think
| > the user would be happy with his stack burned :)
| >
| > Something like:
| > ---
| >
| > Index: linux-2.6.git/net/sunrpc/sysctl.c
| > ===================================================================
| > --- linux-2.6.git.orig/net/sunrpc/sysctl.c 2008-07-20 11:40:14.000000000 +0400
| > +++ linux-2.6.git/net/sunrpc/sysctl.c 2008-08-30 23:05:30.000000000 +0400
| > @@ -69,6 +69,8 @@ static int proc_do_xprt(ctl_table *table
| > return -EINVAL;
| > else {
| > len = svc_print_xprts(tmpbuf, sizeof(tmpbuf));
| > + if (*lenp < len)
| > + return -EFAULT;
| > if (!access_ok(VERIFY_WRITE, buffer, len))
| > return -EFAULT;
| >
| >
|
| BTW, look at this:
|
| $ od -A x -t x1z /proc/sys/sunrpc/transports
| 000000 74 63 70 20 31 30 34 38 35 37 36 0a 75 64 70 20 >tcp 1048576.udp <
| 000010 33 32 37 36 38 0a 00 00 00 00 00 00 00 00 00 00 >32768...........<
| 000020 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >................<
| *
| 0003e0 00 00 00 00 00 00 00 00 00 00 >..........<
| 0003ea
|
| ...and:
|
| $ strace -e trace=read cat /proc/sys/sunrpc/transports > /dev/null
| read(3, "\177ELF\1\1\1\0\0\0\0\0\0\0\0\0\3\0\3\0\1\0\0\0@G\316E4\0\0\0"...,
| 512) = 512
| read(3, "tcp 1048576\nudp 32768\n\0\0\0\0\0\0\0\0\0\0"..., 4096) = 4074
| read(3, "", 4096) = 0
|
| ...why does it have a huge return value? The output is only about 40
| bytes... why add all the \0? Would your patch also fix this?
I think it's from strace side - it pass 4096 zero'ed buffer.
At least I don't see additional issues from kernel side in buffer
filling - except from svc_print_xprts() which walk over list.
But I think sunpc guys should know details :)
Will send short-fix patch soon :)
|
|
| Vegard
|
| --
| "The animistic metaphor of the bug that maliciously sneaked in while
| the programmer was not looking is intellectually dishonest as it
| disguises that the error is the programmer's own creation."
| -- E. W. Dijkstra, EWD1036
|
- Cyrill -
On Sat, Aug 30, 2008 at 9:56 PM, Cyrill Gorcunov <[email protected]> wrote:
> | BTW, look at this:
> |
> | $ od -A x -t x1z /proc/sys/sunrpc/transports
> | 000000 74 63 70 20 31 30 34 38 35 37 36 0a 75 64 70 20 >tcp 1048576.udp <
> | 000010 33 32 37 36 38 0a 00 00 00 00 00 00 00 00 00 00 >32768...........<
> | 000020 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >................<
> | *
> | 0003e0 00 00 00 00 00 00 00 00 00 00 >..........<
> | 0003ea
> |
> | ...and:
> |
> | $ strace -e trace=read cat /proc/sys/sunrpc/transports > /dev/null
> | read(3, "\177ELF\1\1\1\0\0\0\0\0\0\0\0\0\3\0\3\0\1\0\0\0@G\316E4\0\0\0"...,
> | 512) = 512
> | read(3, "tcp 1048576\nudp 32768\n\0\0\0\0\0\0\0\0\0\0"..., 4096) = 4074
> | read(3, "", 4096) = 0
> |
> | ...why does it have a huge return value? The output is only about 40
> | bytes... why add all the \0? Would your patch also fix this?
>
> I think it's from strace side - it pass 4096 zero'ed buffer.
"cat" passed buffer of size 4096, yes. But read() still returned 4074.
It should have returned 38 or so.
> At least I don't see additional issues from kernel side in buffer
> filling - except from svc_print_xprts() which walk over list.
> But I think sunpc guys should know details :)
> Will send short-fix patch soon :)
It looks like it's returning (sizeof(buffer) - x) where it really
should be returning x. Maybe it's this one that should be different?
*lenp -= len;
Vegard
--
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
-- E. W. Dijkstra, EWD1036
[Vegard Nossum - Sat, Aug 30, 2008 at 09:59:38PM +0200]
| On Sat, Aug 30, 2008 at 9:56 PM, Cyrill Gorcunov <[email protected]> wrote:
| > | BTW, look at this:
| > |
| > | $ od -A x -t x1z /proc/sys/sunrpc/transports
| > | 000000 74 63 70 20 31 30 34 38 35 37 36 0a 75 64 70 20 >tcp 1048576.udp <
| > | 000010 33 32 37 36 38 0a 00 00 00 00 00 00 00 00 00 00 >32768...........<
| > | 000020 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >................<
| > | *
| > | 0003e0 00 00 00 00 00 00 00 00 00 00 >..........<
| > | 0003ea
| > |
| > | ...and:
| > |
| > | $ strace -e trace=read cat /proc/sys/sunrpc/transports > /dev/null
| > | read(3, "\177ELF\1\1\1\0\0\0\0\0\0\0\0\0\3\0\3\0\1\0\0\0@G\316E4\0\0\0"...,
| > | 512) = 512
| > | read(3, "tcp 1048576\nudp 32768\n\0\0\0\0\0\0\0\0\0\0"..., 4096) = 4074
| > | read(3, "", 4096) = 0
| > |
| > | ...why does it have a huge return value? The output is only about 40
| > | bytes... why add all the \0? Would your patch also fix this?
| >
| > I think it's from strace side - it pass 4096 zero'ed buffer.
|
| "cat" passed buffer of size 4096, yes. But read() still returned 4074.
| It should have returned 38 or so.
|
| > At least I don't see additional issues from kernel side in buffer
| > filling - except from svc_print_xprts() which walk over list.
| > But I think sunpc guys should know details :)
| > Will send short-fix patch soon :)
|
| It looks like it's returning (sizeof(buffer) - x) where it really
| should be returning x. Maybe it's this one that should be different?
|
| *lenp -= len;
|
|
| Vegard
yes, but this is just a side effect, if we fix main error - it should
resolve this problem too. Did you try the fix I sent a few msgs ago?
(I don't have sunrpc on my machine)
|
| --
| "The animistic metaphor of the bug that maliciously sneaked in while
| the programmer was not looking is intellectually dishonest as it
| disguises that the error is the programmer's own creation."
| -- E. W. Dijkstra, EWD1036
|
- Cyrill -
On Sat, Aug 30, 2008 at 10:04 PM, Cyrill Gorcunov <[email protected]> wrote:
> [Vegard Nossum - Sat, Aug 30, 2008 at 09:59:38PM +0200]
> | On Sat, Aug 30, 2008 at 9:56 PM, Cyrill Gorcunov <[email protected]> wrote:
> | > | BTW, look at this:
> | > |
> | > | $ od -A x -t x1z /proc/sys/sunrpc/transports
> | > | 000000 74 63 70 20 31 30 34 38 35 37 36 0a 75 64 70 20 >tcp 1048576.udp <
> | > | 000010 33 32 37 36 38 0a 00 00 00 00 00 00 00 00 00 00 >32768...........<
> | > | 000020 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >................<
> | > | *
> | > | 0003e0 00 00 00 00 00 00 00 00 00 00 >..........<
> | > | 0003ea
> | > |
> | > | ...and:
> | > |
> | > | $ strace -e trace=read cat /proc/sys/sunrpc/transports > /dev/null
> | > | read(3, "\177ELF\1\1\1\0\0\0\0\0\0\0\0\0\3\0\3\0\1\0\0\0@G\316E4\0\0\0"...,
> | > | 512) = 512
> | > | read(3, "tcp 1048576\nudp 32768\n\0\0\0\0\0\0\0\0\0\0"..., 4096) = 4074
> | > | read(3, "", 4096) = 0
> | > |
> | > | ...why does it have a huge return value? The output is only about 40
> | > | bytes... why add all the \0? Would your patch also fix this?
> | >
> | > I think it's from strace side - it pass 4096 zero'ed buffer.
> |
> | "cat" passed buffer of size 4096, yes. But read() still returned 4074.
> | It should have returned 38 or so.
> |
> | > At least I don't see additional issues from kernel side in buffer
> | > filling - except from svc_print_xprts() which walk over list.
> | > But I think sunpc guys should know details :)
> | > Will send short-fix patch soon :)
> |
> | It looks like it's returning (sizeof(buffer) - x) where it really
> | should be returning x. Maybe it's this one that should be different?
> |
> | *lenp -= len;
> |
>
> yes, but this is just a side effect, if we fix main error - it should
> resolve this problem too. Did you try the fix I sent a few msgs ago?
> (I don't have sunrpc on my machine)
Sorry, I did it now :-)
$ uname -a
Linux grianne 2.6.27-rc5-00006-gbef69ea-dirty #4 SMP PREEMPT Sat
Aug 30 22:07:18 CEST 2008 i686 i686 i386 GNU/Linux
$ strace -e trace=read cat /proc/sys/sunrpc/transports > /dev/null
read(3, "\177ELF\1\1\1\0\0\0\0\0\0\0\0\0\3\0\3\0\1\0\0\0\320
\265\0004\0\0\0"..., 512) = 512
read(3, "tcp 1048576\nudp 32768\n\0\0\0\0\0\0\0\0\0\0"..., 4096) = 4074
read(3, "", 4096) = 0
So that problem seems to remain.
Vegard
--
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
-- E. W. Dijkstra, EWD1036
[Vegard Nossum - Sat, Aug 30, 2008 at 10:13:23PM +0200]
| On Sat, Aug 30, 2008 at 10:04 PM, Cyrill Gorcunov <[email protected]> wrote:
| > [Vegard Nossum - Sat, Aug 30, 2008 at 09:59:38PM +0200]
| > | On Sat, Aug 30, 2008 at 9:56 PM, Cyrill Gorcunov <[email protected]> wrote:
| > | > | BTW, look at this:
| > | > |
| > | > | $ od -A x -t x1z /proc/sys/sunrpc/transports
| > | > | 000000 74 63 70 20 31 30 34 38 35 37 36 0a 75 64 70 20 >tcp 1048576.udp <
| > | > | 000010 33 32 37 36 38 0a 00 00 00 00 00 00 00 00 00 00 >32768...........<
| > | > | 000020 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >................<
| > | > | *
| > | > | 0003e0 00 00 00 00 00 00 00 00 00 00 >..........<
| > | > | 0003ea
| > | > |
| > | > | ...and:
| > | > |
| > | > | $ strace -e trace=read cat /proc/sys/sunrpc/transports > /dev/null
| > | > | read(3, "\177ELF\1\1\1\0\0\0\0\0\0\0\0\0\3\0\3\0\1\0\0\0@G\316E4\0\0\0"...,
| > | > | 512) = 512
| > | > | read(3, "tcp 1048576\nudp 32768\n\0\0\0\0\0\0\0\0\0\0"..., 4096) = 4074
| > | > | read(3, "", 4096) = 0
| > | > |
| > | > | ...why does it have a huge return value? The output is only about 40
| > | > | bytes... why add all the \0? Would your patch also fix this?
| > | >
| > | > I think it's from strace side - it pass 4096 zero'ed buffer.
| > |
| > | "cat" passed buffer of size 4096, yes. But read() still returned 4074.
| > | It should have returned 38 or so.
| > |
| > | > At least I don't see additional issues from kernel side in buffer
| > | > filling - except from svc_print_xprts() which walk over list.
| > | > But I think sunpc guys should know details :)
| > | > Will send short-fix patch soon :)
| > |
| > | It looks like it's returning (sizeof(buffer) - x) where it really
| > | should be returning x. Maybe it's this one that should be different?
| > |
| > | *lenp -= len;
| > |
| >
| > yes, but this is just a side effect, if we fix main error - it should
| > resolve this problem too. Did you try the fix I sent a few msgs ago?
| > (I don't have sunrpc on my machine)
|
| Sorry, I did it now :-)
|
| $ uname -a
| Linux grianne 2.6.27-rc5-00006-gbef69ea-dirty #4 SMP PREEMPT Sat
| Aug 30 22:07:18 CEST 2008 i686 i686 i386 GNU/Linux
|
| $ strace -e trace=read cat /proc/sys/sunrpc/transports > /dev/null
| read(3, "\177ELF\1\1\1\0\0\0\0\0\0\0\0\0\3\0\3\0\1\0\0\0\320
| \265\0004\0\0\0"..., 512) = 512
| read(3, "tcp 1048576\nudp 32768\n\0\0\0\0\0\0\0\0\0\0"..., 4096) = 4074
| read(3, "", 4096) = 0
|
| So that problem seems to remain.
thanks - will check... (if not fall into sleep :)
|
|
| Vegard
|
| --
| "The animistic metaphor of the bug that maliciously sneaked in while
| the programmer was not looking is intellectually dishonest as it
| disguises that the error is the programmer's own creation."
| -- E. W. Dijkstra, EWD1036
|
- Cyrill -
Vegard Nossum wrote:
>I don't know if this could be used in some malicious way. Maybe if a setuid
>root program tried to open a user-supplied file (which could be this one in
>/proc), it could crash the program quite easily. But since there is no way
>to change the contents of the file... I don't know.
You don't necessarily need control over the values that are written past
the end of the buffer to exploit a buffer overrun bug. If those values
are semi-random but predictable, it may still be possible to exploit the
vulnerability by arranging to stash malicious code at the address that the
return address will be overwritten with. Even if those values are random
and not predictable, it may still be able to fill a large fraction of the
address space with malicious code and thus have a significant probability
of successful exploitation. See, e.g., NOP sleds, heap spraying, heap
feng shui. I would not want to rely on this bug being difficult to exploit.
Bottom line: I suspect it would be prudent to assume this bug may be
fully exploitable.
[Vegard Nossum - Sat, Aug 30, 2008 at 10:13:23PM +0200]
| On Sat, Aug 30, 2008 at 10:04 PM, Cyrill Gorcunov <[email protected]> wrote:
| > [Vegard Nossum - Sat, Aug 30, 2008 at 09:59:38PM +0200]
| > | On Sat, Aug 30, 2008 at 9:56 PM, Cyrill Gorcunov <[email protected]> wrote:
| > | > | BTW, look at this:
| > | > |
| > | > | $ od -A x -t x1z /proc/sys/sunrpc/transports
| > | > | 000000 74 63 70 20 31 30 34 38 35 37 36 0a 75 64 70 20 >tcp 1048576.udp <
| > | > | 000010 33 32 37 36 38 0a 00 00 00 00 00 00 00 00 00 00 >32768...........<
| > | > | 000020 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >................<
| > | > | *
| > | > | 0003e0 00 00 00 00 00 00 00 00 00 00 >..........<
| > | > | 0003ea
| > | > |
| > | > | ...and:
| > | > |
| > | > | $ strace -e trace=read cat /proc/sys/sunrpc/transports > /dev/null
| > | > | read(3, "\177ELF\1\1\1\0\0\0\0\0\0\0\0\0\3\0\3\0\1\0\0\0@G\316E4\0\0\0"...,
| > | > | 512) = 512
| > | > | read(3, "tcp 1048576\nudp 32768\n\0\0\0\0\0\0\0\0\0\0"..., 4096) = 4074
| > | > | read(3, "", 4096) = 0
| > | > |
| > | > | ...why does it have a huge return value? The output is only about 40
| > | > | bytes... why add all the \0? Would your patch also fix this?
| > | >
| > | > I think it's from strace side - it pass 4096 zero'ed buffer.
| > |
| > | "cat" passed buffer of size 4096, yes. But read() still returned 4074.
| > | It should have returned 38 or so.
| > |
| > | > At least I don't see additional issues from kernel side in buffer
| > | > filling - except from svc_print_xprts() which walk over list.
| > | > But I think sunpc guys should know details :)
| > | > Will send short-fix patch soon :)
| > |
| > | It looks like it's returning (sizeof(buffer) - x) where it really
| > | should be returning x. Maybe it's this one that should be different?
| > |
| > | *lenp -= len;
| > |
| >
| > yes, but this is just a side effect, if we fix main error - it should
| > resolve this problem too. Did you try the fix I sent a few msgs ago?
| > (I don't have sunrpc on my machine)
|
| Sorry, I did it now :-)
|
| $ uname -a
| Linux grianne 2.6.27-rc5-00006-gbef69ea-dirty #4 SMP PREEMPT Sat
| Aug 30 22:07:18 CEST 2008 i686 i686 i386 GNU/Linux
|
| $ strace -e trace=read cat /proc/sys/sunrpc/transports > /dev/null
| read(3, "\177ELF\1\1\1\0\0\0\0\0\0\0\0\0\3\0\3\0\1\0\0\0\320
| \265\0004\0\0\0"..., 512) = 512
| read(3, "tcp 1048576\nudp 32768\n\0\0\0\0\0\0\0\0\0\0"..., 4096) = 4074
| read(3, "", 4096) = 0
|
| So that problem seems to remain.
|
|
| Vegard
Vegard, are you sure you test it with this patch (a bit updated)?
|
| --
| "The animistic metaphor of the bug that maliciously sneaked in while
| the programmer was not looking is intellectually dishonest as it
| disguises that the error is the programmer's own creation."
| -- E. W. Dijkstra, EWD1036
|
- Cyrill -
---
Index: linux-2.6.git/net/sunrpc/sysctl.c
===================================================================
--- linux-2.6.git.orig/net/sunrpc/sysctl.c 2008-07-20 11:40:14.000000000 +0400
+++ linux-2.6.git/net/sunrpc/sysctl.c 2008-08-31 00:19:42.000000000 +0400
@@ -69,9 +69,10 @@ static int proc_do_xprt(ctl_table *table
return -EINVAL;
else {
len = svc_print_xprts(tmpbuf, sizeof(tmpbuf));
+ if (*lenp < len)
+ len = *lenp;
if (!access_ok(VERIFY_WRITE, buffer, len))
return -EFAULT;
-
if (__copy_to_user(buffer, tmpbuf, len))
return -EFAULT;
}
Cyrill Gorcunov wrote:
>Index: linux-2.6.git/net/sunrpc/sysctl.c
>===================================================================
>--- linux-2.6.git.orig/net/sunrpc/sysctl.c 2008-07-20 11:40:14.000000000 +0400
>+++ linux-2.6.git/net/sunrpc/sysctl.c 2008-08-30 23:05:30.000000000 +0400
>@@ -69,6 +69,8 @@ static int proc_do_xprt(ctl_table *table
> return -EINVAL;
> else {
> len = svc_print_xprts(tmpbuf, sizeof(tmpbuf));
>+ if (*lenp < len)
>+ return -EFAULT;
> if (!access_ok(VERIFY_WRITE, buffer, len))
> return -EFAULT;
1. Would it be better to use copy_to_user() rather than
access_ok() followed immediately by __copy_to_user()?
2. Is it OK to dereference *lenp directly? Is lenp a pointer into user
memory or kernel memory? If it points to user memory, why is it safe to
dereference it directly? (What about TOCTTOU bugs?) Should there be
some sparse annotations here to ensure the code is not dereferencing
user pointers directly? Later on, proc_do_xprt() also dereferences
*lenp and *ppos directly.
3. 'len' is declared as a signed int. len will be converted to size_t
before doing the comparison, so if len can ever be negative (e.g.,
svc_print_xprts() returns -1 because of an error), this patch will do
the wrong thing. Looks like the current definition of svc_print_xprts()
won't ever do that, as that code currently stands, so at present this
is not a bug. However from a security point of view there are benefits
to code whose correctness is 'locally obvious', all else being equal.
In particular this seems like a possible maintenance hazard. Would it be
better to use type size_t for lengths like this that are never supposed
to be negative?
4. Is proc_dostring() relevant here?
[David Wagner - Sat, Aug 30, 2008 at 10:55:51PM +0000]
| Cyrill Gorcunov wrote:
| >Index: linux-2.6.git/net/sunrpc/sysctl.c
| >===================================================================
| >--- linux-2.6.git.orig/net/sunrpc/sysctl.c 2008-07-20 11:40:14.000000000 +0400
| >+++ linux-2.6.git/net/sunrpc/sysctl.c 2008-08-30 23:05:30.000000000 +0400
| >@@ -69,6 +69,8 @@ static int proc_do_xprt(ctl_table *table
| > return -EINVAL;
| > else {
| > len = svc_print_xprts(tmpbuf, sizeof(tmpbuf));
| >+ if (*lenp < len)
| >+ return -EFAULT;
| > if (!access_ok(VERIFY_WRITE, buffer, len))
| > return -EFAULT;
|
| 1. Would it be better to use copy_to_user() rather than
| access_ok() followed immediately by __copy_to_user()?
|
| 2. Is it OK to dereference *lenp directly? Is lenp a pointer into user
| memory or kernel memory? If it points to user memory, why is it safe to
| dereference it directly? (What about TOCTTOU bugs?) Should there be
| some sparse annotations here to ensure the code is not dereferencing
| user pointers directly? Later on, proc_do_xprt() also dereferences
| *lenp and *ppos directly.
Didn't check for this - will do.
|
| 3. 'len' is declared as a signed int. len will be converted to size_t
| before doing the comparison, so if len can ever be negative (e.g.,
| svc_print_xprts() returns -1 because of an error), this patch will do
| the wrong thing. Looks like the current definition of svc_print_xprts()
| won't ever do that, as that code currently stands, so at present this
| is not a bug. However from a security point of view there are benefits
| to code whose correctness is 'locally obvious', all else being equal.
| In particular this seems like a possible maintenance hazard. Would it be
| better to use type size_t for lengths like this that are never supposed
| to be negative?
|
| 4. Is proc_dostring() relevant here?
|
Yes David, I think proc_dostring is better candidate to use here, thanks!
- Cyrill -
[David Wagner - Sat, Aug 30, 2008 at 10:55:51PM +0000]
| Cyrill Gorcunov wrote:
| >Index: linux-2.6.git/net/sunrpc/sysctl.c
| >===================================================================
| >--- linux-2.6.git.orig/net/sunrpc/sysctl.c 2008-07-20 11:40:14.000000000 +0400
| >+++ linux-2.6.git/net/sunrpc/sysctl.c 2008-08-30 23:05:30.000000000 +0400
| >@@ -69,6 +69,8 @@ static int proc_do_xprt(ctl_table *table
| > return -EINVAL;
| > else {
| > len = svc_print_xprts(tmpbuf, sizeof(tmpbuf));
| >+ if (*lenp < len)
| >+ return -EFAULT;
| > if (!access_ok(VERIFY_WRITE, buffer, len))
| > return -EFAULT;
|
| 1. Would it be better to use copy_to_user() rather than
| access_ok() followed immediately by __copy_to_user()?
Yes, thanks
|
| 2. Is it OK to dereference *lenp directly? Is lenp a pointer into user
| memory or kernel memory? If it points to user memory, why is it safe to
| dereference it directly? (What about TOCTTOU bugs?) Should there be
| some sparse annotations here to ensure the code is not dereferencing
| user pointers directly? Later on, proc_do_xprt() also dereferences
| *lenp and *ppos directly.
Not only proc_do_xprt do that so I think it's safe (check for NULL
on highr level I suspect).
|
| 3. 'len' is declared as a signed int. len will be converted to size_t
| before doing the comparison, so if len can ever be negative (e.g.,
| svc_print_xprts() returns -1 because of an error), this patch will do
| the wrong thing. Looks like the current definition of svc_print_xprts()
| won't ever do that, as that code currently stands, so at present this
| is not a bug. However from a security point of view there are benefits
| to code whose correctness is 'locally obvious', all else being equal.
| In particular this seems like a possible maintenance hazard. Would it be
| better to use type size_t for lengths like this that are never supposed
| to be negative?
thanks, I changed it to size_t and as you mentoined negative is never coming
from called svc_print_xprts.
|
| 4. Is proc_dostring() relevant here?
it's not possible to use for now this function (one of my patches
was quite wrong by using it :)
- Cyrill -
[Cyrill Gorcunov - Sun, Aug 31, 2008 at 02:30:26PM +0400]
...
|
| |
| | 2. Is it OK to dereference *lenp directly? Is lenp a pointer into user
| | memory or kernel memory? If it points to user memory, why is it safe to
| | dereference it directly? (What about TOCTTOU bugs?) Should there be
| | some sparse annotations here to ensure the code is not dereferencing
| | user pointers directly? Later on, proc_do_xprt() also dereferences
| | *lenp and *ppos directly.
on second view: will check for TOCTTOU bug (iirc vfs layer does
latch file descriptor for these kind of operations)
|
| Not only proc_do_xprt do that so I think it's safe (check for NULL
| on highr level I suspect).
|
...
- Cyrill -