Re: [RFC][PATCH 4/3] vfs: Allow rmdir to remove mounts in all but the current mount namespace

[ebiederm@xmission.com (Eric W. Biederman)]

Andy Lutomirski <luto@amacapital.net> writes:

> On Mon, Oct 7, 2013 at 7:55 AM, Eric W. Biederman <ebiederm@xmission.com> wrote:
>> "Serge E. Hallyn" <serge@hallyn.com> writes:
>>
>>> Quoting Eric W. Biederman (ebiederm@xmission.com):
>>>>
>>>> Programs have been known to test for empty directories by attempting
>>>> to remove them.  To keep from violating the principle of least
>>>> surprise don't let directories the caller can see with someting
>>>> mounted on them be deleted.
>>>
>>> Do you think we should do the same thing for over-mounted file at
>>> vfs_unlink()?
>>
>> We easily could.
>>
>> The point of the patch is to just preserve the directory is empty don't
>> allow rmdir to succeed semantics, and as typically we can see something
>> in the directory because of the mount it doesn't make sense for rmdir to
>> succeed.
>>
>> unlink doesn't have any occassions when the permissions are sufficient
>> to remove a directory where it will fail.  So I don't see the point of
>> doing this for anything except directories.
>>
>> Except for possibly the oddball rmdir semantics mentioned I don't think
>> this patch should be part of anyone's correctness analysis.
>>
>>
>>
>> It is easiest to see that this series of changes is semantically safe if
>> we are safe to run unprivileged code in a mount namespace where root has
>> locally unmounted every mount point.
>>
>> We do have the restriction that in a user namespace we can't unmount
>> anything root was mounted outside the user namespace.  Which combined
>> with the above patch would be roughly equivalent to todays mount
>> restrictions for the common case.  Unfortunately being only roughly
>> equivalent the analysis gets very complicated, and complicated reasoning
>> usually means invalid reasoning.
>>
>>
>> So if we can feel safe just depending on the parent directory
>> permissions (which are not hidden by a mount) protecting our mount
>> points, I feel much better about this patchset.
>>
>
> I feel safe about this (in the safe-against-attack sense, not
> necessary the safe-against-confused-users sense) because the whole
> point of preventing unmount of things that were mounted by root is to
> prevent untrusted users from seeing what's under the mount.  But rmdir
> and unlink don't let you see what was under the mount because they
> remove it.

My premise is that the directory permissions of the directory that
contains the mountpoint are already restrictive enough that it safe to
rely on them for permission to unlink and thus unmount.

I believe my premise is correct however I am human and make mistakes.

As far as I can tell a world writable / or /dev has a lot of other potential
issues and similarly with other directories where we put mount points,
so people just won't create those directories with bad permissions.

I am reitterating just so that what I depend upon is clear and if anyone
knows a counter example they can tell me.

I think by preventing rmdir (when the mount is in the same mount
namespace) and allowing rename and unlink (that do not unlink a
directory) we present a minimal surprise to users.

> (Hmm.  I wonder if rename on a mountpoint could work.  I've wanted
> this on occasion.)

With this set of patches rename on a mountpoint is supported.  That will
break /etc/mtab but in the cases where it really matters renaming a
mount point will break other things.  Which makes in most cases renaming 
a mount point a case of don't do that then, while still being well
defined for the cases where it is not a problem.

Eric