linux-kernel - Re: buffer overflow in /proc/sys/sunrpc/transports

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sat, 30 Aug 2008 23:21:12 +0400
From:	Cyrill Gorcunov <gorcunov@...il.com>
To:	Vegard Nossum <vegard.nossum@...il.com>
Cc:	Tom Tucker <tom@...ngridcomputing.com>, Neil Brown <neilb@...e.de>,
	Chuck Lever <chuck.lever@...cle.com>, Greg Banks <gnb@....com>,
	"J. Bruce Fields" <bfields@...i.umich.edu>,
	linux-kernel@...r.kernel.org
Subject: Re: buffer overflow in /proc/sys/sunrpc/transports

[Vegard Nossum - Sat, Aug 30, 2008 at 09:15:16PM +0200]
| On Sat, Aug 30, 2008 at 9:06 PM, Cyrill Gorcunov <gorcunov@...il.com> 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 -
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/