linux-kernel - Re: v2.6.28-rc1: readlink /proc/*/exe returns uninitialized data to userspace

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:	Tue, 4 Nov 2008 15:48:23 +0000
From:	Al Viro <viro@...IV.linux.org.uk>
To:	"Eric W. Biederman" <ebiederm@...ssion.com>
Cc:	Alexey Dobriyan <adobriyan@...il.com>,
	Vegard Nossum <vegard.nossum@...il.com>,
	"Koyama, Yoshiya" <Yoshiya.Koyama@...com>,
	"Rafael J. Wysocki" <rjw@...k.pl>, Ingo Molnar <mingo@...e.hu>,
	Pekka Enberg <penberg@...helsinki.fi>,
	LKML <linux-kernel@...r.kernel.org>, Greg KH <greg@...ah.com>,
	Kay Sievers <kay.sievers@...y.org>,
	linux-fsdevel@...r.kernel.org
Subject: Re: v2.6.28-rc1: readlink /proc/*/exe returns uninitialized data
	to userspace

On Tue, Nov 04, 2008 at 02:54:35AM -0800, Eric W. Biederman wrote:

> does a memcpy of the new name to the target name,
> but it doesn't do anything with the source name.
> Then later we swap the name lengths.
> 
> So the length on the dentry no longer matches the data
> we put in the buffer.

Aye.  BTW, here's yesterday IRC log, after eparis had pointed to
" (deleted)" in the ends of two more reproducers (auditd spew after
crond upgrade, FWIW - same issue with d_path()):

13:48 #sec-eng: < viro> eparis: it's switch_names()
13:49 #sec-eng: < viro> if both names are internal
13:49 #sec-eng: < viro> in that case we want to copy ->d_name.len instead of swapping those
13:50 #sec-eng: < viro> IOW, I understand what's going on; it's not too nasty, fortunately, but it needs fixing
13:51 #sec-eng: < viro> pathname ends on " (deleted)" and has sane path
13:51 #sec-eng: < viro> i.e. we have dentry->d_name.{name,len} responsible for everything in between
13:52 #sec-eng: < viro> and .len is more than it ought to be
13:52 #sec-eng: < viro> OTOH, it's still within the limit on name length, so it's embedded into struct dentry
13:53 #sec-eng: < viro> i.e. we had either unlink() doing something _very_ odd or rename() buggering the name/len for (unhashed) target
13:53 #sec-eng: < viro> rename() => d_move()
13:54 #sec-eng: < viro> then testing a bit with copying /bin/sh to /tmp/<different names>, starting those and doing mv(1) of one over another had happily reproduced it
13:55 #sec-eng: < viro> so that was d_move() with short-over-short and source name longer than target one
13:56 #sec-eng: < viro> since there are only two lines handling d_name there (
13:56 #sec-eng: < viro>         /* Switch the names.. */
13:56 #sec-eng: < viro>         switch_names(dentry, target);
13:56 #sec-eng: < viro>         do_switch(dentry->d_name.len, target->d_name.len);
13:57 #sec-eng: < viro> and switch_names() is 4-way choice by "is the source name short enough"/"is the target name short enough"...
13:58 #sec-eng: < viro> IOW, after noticing that " (deleted)" it had been absolutely straightforward
13:58 #sec-eng: < viro> it's not junk in the end
13:59 #sec-eng: < viro> it's junk in the _middle_

> Certainly not a resource leak or any kind of deadlock.
> And the length is right.  But it is an information leak.
> 
> I suppose a clever person could figure out how to steal
> information that way.

Not much, but that certainly needs fixing, leak or not.
--
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/