linux-kernel - Re: [PATCH -V18 04/13] vfs: Allow handle based open on symlinks

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]

Message-ID: <m362z4s46c.fsf@linux.vnet.ibm.com>
Date:	Sat, 21 Aug 2010 15:01:07 +0530
From:	"Aneesh Kumar K. V" <aneesh.kumar@...ux.vnet.ibm.com>
To:	Neil Brown <neilb@...e.de>, Al Viro <viro@...IV.linux.org.uk>
Cc:	Christoph Hellwig <hch@...radead.org>, adilger@....com,
	corbet@....net, npiggin@...nel.dk, hooanon05@...oo.co.jp,
	bfields@...ldses.org, miklos@...redi.hu,
	linux-fsdevel@...r.kernel.org, sfrench@...ibm.com,
	philippe.deniel@....FR, linux-kernel@...r.kernel.org
Subject: Re: [PATCH -V18 04/13] vfs: Allow handle based open on symlinks

On Sat, 21 Aug 2010 10:09:00 +1000, Neil Brown <neilb@...e.de> wrote:
> [[email address for Nick Piggin changed to npiggin@...nel.dk]]
> 
> On Fri, 20 Aug 2010 12:51:35 +0100
> Al Viro <viro@...IV.linux.org.uk> wrote:
> 
> > On Fri, Aug 20, 2010 at 07:53:03PM +1000, Neil Brown wrote:
> > > On Fri, 20 Aug 2010 04:30:57 -0400
> > > Christoph Hellwig <hch@...radead.org> wrote:
> > > 
> > > > Suddenly getting an file pointer for a symlink which could never happen
> > > > before is a really bad idea.  Just add a proper readlink_by_handle
> > > > system call, similar to what's done in the XFS interface.
> > > 
> > > Why is that?
> > > With futexes we suddenly get a file descriptor for something we could never
> > > get a file descriptor on before and that doesn't seem to be a problem.
> > > 
> > > Why should symlinks be special as the only thing that you cannot have a file
> > > descriptor for?  Uniformity of interface is a very valuable property.
> > 
> > You are welcome to review the codepaths around pathname resolution for
> > assumptions of presense of ->follow_link() and friends; there _are_
> > subtle cases and dumping your "opened symlinks" in there is far from
> > a trivial change.  Note that it affects more than just the starting
> > points of lookups; /proc/*/fd/* stuff is also involved.
> 
> So as I understand it you aren't rejecting the concept in principle, but you
> believe non-trivial code review is required before it can be considered an
> acceptable change?
> That's quite reasonable.  I hope to find time to have a look at the code.
> 
> > 
> > BTW, speaking of NULL pathname, linkat() variant that allows creating a link
> > to an opened file is also a very dubious thing; at the very least, you get
> > non-trivial security implications, since now a process that got an opened
> > descriptor passed to it by somebody else may create hardlinks to the
> > sucker.

Why would that be an issue ? Even though a hardlink can be created,
only process with right privileges can access them. One problem is;
being able to create hardlinks at different directory location as above
implies that there is no way to guarantee that the file got completely
removed from the system. Is this the security risk that you are pointing
above ?

> 
> Fair comment, and while one could imagine ways around this (such as requiring
> some Capability to link an fd) they wouldn't be very elegant.


a sys_handle_link that limits to CAP_DAC_READ_SEARCH is a nice compromise.


> But then nor is inventing a pile of new syscalls for doing different things
> with handles such as the list Aneesh posted.
> 
> Maybe a different approach is needed.
> 
> How about a new AT flag:  AT_FILE_HANDLE
> 
> Meaning is that the 'dirfd' is used only to identify a filesystem (vfsmnt) and
> the 'name' pointer actually points to a filehandle fragment interpreted in
> that filesystem.


But that would imply once can guess the handle and create a hardlink to it.

> 
> One problem is that there is no way to pass the length...


struct file_handle already include the length. 

> Options:
>    fragment is at most 64 bytes nul padded at the end
>    fragment is hex encoded and nul terminated
>    ??
> 
> I think I prefer the hex encoding, but I'm hoping someone else has a better
> idea.
> 

-aneesh
--
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/