linux-kernel - Re: [PATCH 0/3] vfs: plug some holes involving LAST_BIND symlinks and file bind mounts (try #5)

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:	Mon, 23 Nov 2009 19:34:26 -0500
From:	Jeff Layton <jlayton@...hat.com>
To:	ebiederm@...ssion.com (Eric W. Biederman)
Cc:	Jamie Lokier <jamie@...reable.org>, linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org, pavel@....cz, miklos@...redi.hu,
	viro@...IV.linux.org.uk
Subject: Re: [PATCH 0/3] vfs: plug some holes involving LAST_BIND symlinks
 and file bind mounts (try #5)

On Mon, 23 Nov 2009 15:35:44 -0800
ebiederm@...ssion.com (Eric W. Biederman) wrote:

> Jeff Layton <jlayton@...hat.com> writes:
> 
> > On Mon, 23 Nov 2009 22:49:48 +0000
> > Jamie Lokier <jamie@...reable.org> wrote:
> >
> >> Jeff Layton wrote:
> >> > > check_path_accessible seems pretty horrible.   If a process is running
> >> > > inside of a subdirectory it doesn't have permissions to access, say
> >> > > a chroot, /proc/self/fd/XXX becomes completely unusable.
> >> > > 
> >> > 
> >> > Hmm...I have this in there:
> >> > 
> >> > +		/* are we at global root or root of namespace? */
> >> > +		if ((tdentry == root.dentry && vfsmnt == root.mnt) ||
> >> > +		    vfsmnt->mnt_parent == vfsmnt)
> >> > +			break;
> >> > 
> >> > ...In the case of a chroot, wouldn't "current->fs->root" point to the
> >> > root of the process' namespace? Or am I misunderstanding what
> >> > current->fs actually represents?
> >> 
> >> A process can run inside a subdirectory it doesn't have permissions to
> >> access without that being a chroot.
> >> 
> >
> > Certainly.
> >
> >> It can also run inside a subdirectory that isn't accessible from it's
> >> root, if that's how it was started - as well as having other
> >> descriptors pointing to things outside its root.
> >> 
> >
> > Yes.
> >
> >> It can also be passed file descriptors from outside it's root while
> >> it's running.
> >> 
> >
> > Yep.
> >
> >> Really, I think the /proc/PID/fd/N check should restrict the open to
> >> the O_* limitations that were used to open fd N before, and not have
> >> any connection to actual paths at the time of this check.
> >> 
> >
> > The big question with all of this is: Should a task have the ability
> > to follow a /proc/pid symlink to a path that it wouldn't ordinarily be
> > able to resolve with a path lookup. The concensus that I got from the
> > bugtraq discussion was that it should not, and this patch is an attempt
> > to prevent that.
> >
> > I take it from you and Eric's comments that you disagree? If so, what's
> > your rationale for allowing a task to resolve this symlink when it
> > wouldn't ordinarily be able to do so if it were a "normal" symlink?
> 
> For myself I start with the simple fact that the code has worked the
> way it currently does since around linux 0.99.  We have to be very
> careful if we change this to avoid breaking existing applications.
> 
> So if we change the existing behaviour it must be done in such a way
> that legitimate applications do not break.
> 

I certainly don't want to break existing apps. That said, applications
that are depending on /proc/pid symlinks to allow them to bypass
directory permissions or access files that aren't in their namespace
would seem to be unsafe, no?

I think all we can reasonably do is try to clearly lay out how these
symlinks are intended to work. I think it's logical that the result of
following these links should be more or less the same as if you were to
resolve the results of the readlink.

Is there some reason that we should expect them to provide anything
more? Do you have apps in mind that you think will break with this
change? If you think this is unreasonable, perhaps you could suggest an
alternative?

If this approach is reasonable, there is one thing I think that I'm
pretty sure will need to be fixed. It'll need to detect when the file
lies outside of its namespace altogether. I'm not quite sure how to
do that yet. I've not done much work with multiple namespaces, so I
could certainly use some guidance here.

-- 
Jeff Layton <jlayton@...hat.com>
--
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/