Re: unprivileged mounts git tree

[Serge E. Hallyn]

Quoting Miklos Szeredi (miklos@xxxxxxxxxx):
> On Thu, 4 Sep 2008, Serge E. Hallyn wrote:
> > Quoting Miklos Szeredi (miklos@xxxxxxxxxx):
> > > On Thu, 4 Sep 2008, Serge E. Hallyn wrote:
> > > > but you're still doing
> > > > 
> > > > 	if (IS_MNT_SHARED(old_nd.path.mnt) && !capable(CAP_SYS_ADMIN))
> > > > 		goto out;
> > > > 
> > > > shouldn't it be something like
> > > > 
> > > > 	if (IS_MNT_SHARED(old_nd.path.mnt) && (old_nd.path.mnt & MNT_USER))
> > > > 		goto out;
> > > > 
> > > > ?
> > > 
> > > Why would that be an error?  There's no real security gain to be had
> > > from restricting a privileged user, but could cause a lot of
> > > annoyance.  If we think this is dangerous, then protection should be
> > > built into mount(8) with an option to override.  But not into the
> > > kernel, IMO.
> > 
> > We disagree on that.  But can we agree that the check you added is wrong?
> 
> No :)
> 
> > There is no reason why a user mount should not be able to do shared
> > mounts, is there?
> 
> I don't know.  It's something to think about in the future, but not
> essential.  We know that without the above check the user can do bad
> things: propagate mounts back into the source, and we don't want that.

When is propagating mounts back into the source bad?  Because you are
not preventing it.

You are preventing future propagation back into the user's own mounts,
but not into mounts not owned by the user.

It's not right.

> We could allow binding a shared mount if
> 
>   a) the owners of the source and destination match
>   b) the destination is made a slave of the source

Well that's what I've been saying...

> But the current patchset doesn't allow _any_ changes to propagation
> without CAP_SYS_ADMIN, so why should bind be an exception?

Because it's not a change in propagation among existing mounts, instead
it's defining propagation for the new user mounts.  And since user
mounts don't currently exist, we're in no position to talk about
exceptions to existing behavior.

> And yes, this is something to think about, but I think it's a rather
> uncommon corner case, and so the patchset very much makes sense
> without having to deal with unprivileged mount propagation changes.

I'm willing to accept that if we simply leave the patchset as it was
before, but your new check just adds inconsistencies for absolutely zero
security gain.

> >  So should the check above just go away then?
> 
> No, we'd be back with the original problem.

We still have the original problem.

When root does

	mount -bind /mnt /mnt
	mount --make-rshared /mnt
	mount --bind -o user=hallyn /mnt /home/hallyn/mnt

and hallyn does

	mount --bind /usr /home/hallyn/mnt/usr

then the kernel happily propagates the mount to /mnt/usr.
Now if hallyn does

	mount --bind /home/hallyn/mnt/usr /home/hallyn/mnt/usr2

THAT gives him a -EPERM.

To what end?

-serge
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/