Message-ID: <20100823091708.6f03c42b@notabene>
Date:	Mon, 23 Aug 2010 09:17:08 +1000
From:	Neil Brown <neilb@...e.de>
To:	Nick Piggin <npiggin@...nel.dk>
Cc:	Al Viro <viro@...IV.linux.org.uk>,
	Christoph Hellwig <hch@...radead.org>,
	"Aneesh Kumar K.V" <aneesh.kumar@...ux.vnet.ibm.com>,
	adilger@....com, corbet@....net, 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 18:30:24 +1000
Nick Piggin <npiggin@...nel.dk> wrote:

> Thanks, I had both of the same concerns as Christoph with API
> change and exposing symlink fds last time I looked at the patces,
> actually.
> 
> But they can probably be worked around or avoided. I think the more
> important thing is whether it is worth supporting. This is
> all restricted to root (or CAP_DAC_READ_SEARCH) only, right, and
> what exact semantics they want. I would like to see more discussion
> of what this enables and some results.

They allow a credible user-space implementation of the server for some
network filesystem protocols such as NFS and apparently 9P.

> 
> For the case of avoiding expensive network revalidations in path name
> lookup, do we even need to open symlinks? Could the security issues be
> avoided by always having handle attached to an open fd?

I don't see what you are getting at here... which particular security isses,
and what fd would you use?

As I understand it there are only two security issues that have been noted.
1/ lookup-by-filehandle can bypass any 'search' permission tests on ancestor
   directories.  I cannot see any way to avoid this except require
   CAP_DAC_READ_SEARCH
2/ Creating a hardlink to an 'fd'  allows a process that was given an 'fd'
   that it could not have opened itself to prevent that file from being
   removed (and space reclaimed) by creating a private hardlink.
   This could be avoided by requiring CAP_DAC_READ_SEARCH for that particular
   operation (and probably requiring i_nlink > 0 anyway) but that feels like
   a very special-case restriction.

Was it one of these that you were referring to?

Thanks,
NeilBrown


> 
> On Sat, Aug 21, 2010 at 10:09:00AM +1000, Neil Brown 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.
> > 
> > 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.
> > 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.
> > 
> > One problem is that there is no way to pass 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.
> > 
> > NeilBrown
> --
> 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/

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

Powered by blists - more mailing lists