> > The MNT_USER flag is not copied on any kind of mount cloning:
> > namespace creation, binding or propagation.
> 
> I half agree, and as an initial approximation this works.
> Ultimately we should be at the point that for mount propagation
> that we copy the owner of the from the owner of our parent mount
> at the propagation destination.

Yes, that sounds the most sane.

Ram, what do you think?

> > +	if (mnt->mnt_flags & MNT_USER)
> > +		seq_printf(m, ",user=%i", mnt->mnt_uid);
> How about making the test "if (mnt->mnt_user != &root_user)"

We don't want to treat root_user special.  That's what capabilities
were invented for.

> > Index: linux/include/linux/fs.h
> > ===================================================================
> > --- linux.orig/include/linux/fs.h	2007-04-20 11:55:02.000000000 +0200
> > +++ linux/include/linux/fs.h	2007-04-20 11:55:05.000000000 +0200
> > @@ -123,6 +123,7 @@ extern int dir_notify_enable;
> >  #define MS_SLAVE	(1<<19)	/* change to slave */
> >  #define MS_SHARED	(1<<20)	/* change to shared */
> >  #define MS_RELATIME	(1<<21)	/* Update atime relative to mtime/ctime. */
> > +#define MS_SETUSER	(1<<22) /* set mnt_uid to current user */
> 
> If we unconditionally use the fsuid I think we can get away without
> this flag.

That coudl work if we wouldn't have to worry about breaking the user
interface.  As it is, we cannot be sure, that existing callers of
mount(2) don't have fsuid set to some random value.

> >  #define MNT_SHRINKABLE	0x100
> > +#define MNT_USER	0x200
> 
> If we assign a user to all mount points and root gets to own the
> initial set of mounts then we don't need the internal MNT_USER
> flag.

I think we do want to treat "owned" mounts special, rather than
treating user=0 mounts special.

> > +
> > +	uid_t mnt_uid;			/* owner of the mount */
> 
> Can we please make this a user struct.   That requires a bit of
> reference counting but it has uid namespace benefits as well
> as making it easy to implement per user mount rlimits.

OK, can you ellaborate, what the uid namespace benifits are?

Miklos
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

>   I suspect we can allow MNT_FORCE for non-privileged users 
>   as well if we can trust the filesystem.

I don't think so.  MNT_FORCE has side effects on the superblock.  So a
user shouldn't be able to force an unmount on a bind mount s/he did,
but there's no problem with allowing plain/lazy unmounts.

We could possibly allow MNT_FORCE, for FS_SAFE filesystems.

Miklos
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

> > From: Miklos Szeredi <mszeredi@suse.cz>
> >
> > Add sysctl variables for accounting and limiting the number of user
> > mounts.
> >
> > The maximum number of user mounts is set to 1024 by default.  This
> > won't in itself enable user mounts, setting a mount to be owned by a
> > user is first needed
> 
> Since each mount has a user can we just make this a per user rlimit?
> 
> If we are going to implement a sysctl at this point I think it should
> be a global limit that doesn't care if who you are.  Even root can
> have recursive mounts that attempt to get out of control.

Recursive bind mounts are done carefully enough, so they don't get out
of control.

Recursive mount propagations can get out of control.  But root can
shoot itself in the foot any number of ways, and it's not for the
kernel to police that.

> Also currently you are not checking the max_users.  It looks like
> you do this in a later patch but still it is a little strange to
> allow user own mounts and have accounting but to not check the
> limit at this state.

Yeah, but at this stage user mounts are not yet allowed, so this is
safe.

Miklos
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

> > From: Miklos Szeredi <mszeredi@suse.cz>
> >
> > Allow bind mounts to unprivileged users if the following conditions
> > are met:
> >
> >   - mountpoint is not a symlink or special file
> 
> Why?  This sounds like a left over from when we were checking permissions.

Hmm, yes.  Don't know.  Maybe only the symlink check.

Bind mounts of directory over non-directy, and vica versa are already
excluded, even for root.

> >   - parent mount is owned by the user
> >   - the number of user mounts is below the maximum
> >
> > Unprivileged mounts imply MS_SETUSER, and will also have the "nosuid"
> > and "nodev" mount flags set.
> 
> So in principle I agree, but in detail I disagree.
> 
> capable(CAP_SETUID) should be required to leave MNT_NOSUID clear.
> capable(CAP_MKNOD) should be required to leave MNT_NODEV clear.
> 
> I.e.  We should not special case this as a user mount but rather
> simply check to see if the user performing the mount has the appropriate
> capabilities to allow the flags.

Sounds sane.  Will fix.

Miklos
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

> > +	/*
> > +	 * For unprivileged mounts use current uid/gid.  Still allow
> > +	 * "user_id" and "group_id" options for compatibility, but
> > +	 * only if they match these values.
> > +	 */
> > +	if (!capable(CAP_SYS_ADMIN)) {
> > +		d->user_id = current->uid;
> > +		d->user_id_present = 1;
> > +		d->group_id = current->gid;
> > +		d->group_id_present = 1;
> > +
> > +	}
> 
> CAP_SETUID is the appropriate capability...
> 
> This is not a dimension we have not fully explored.
> What is the problem with a user controlled mount having different
> uid and gid values.
> 
> Yes they map into different users but how is this a problem.
> The only problem that I can recall is the historic chown problem
> where you could give files to other users and mess up their quotas.
> 
> Or is the problem other users writing to this user controlled
> filesystem?

Yes.  Or even just a suid process trying to access the user controlled
filesystem.  See Documentation/filesystems/fuse.txt for the gory
details.

Eric, thanks for the detailed review :)

Miklos
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

> > Does this mean, that containers will need this?  Or that you don't
> > know yet?
> 
> The uid namespace is something we have to handle carefully and we
> have not decided on the final design.
> 
> What is clear is that all permission checks will need to become
> either (uid namspace, uid) tuple comparisons.  Or struct user
> pointer comparisons.  To see if we are talking about the same
> uid.
> 
> So the eventual uid namespace combined with the possibility
> for rlimits if we use struct user *.  See to make using a struct
> user a clear win.

OK, if we don't yet know, I'd rather leave this for later.  It will be
trivial to change to user_struct if we want per-user rlimits.

> >> storing a user struct on each mount point seems sane, plus it allows
> >> per user mount rlimits which is major plus.  Especially since we
> >> seem to be doing accounting only for user mounts a per user rlimit
> >> seems good.
> >
> > I'm not against per-user rlimits for mounts, but I'd rather do this
> > later...
> 
> Then let's add a non-discriminate limit.  Instead of a limit that
> applies only to root.

See reply to relevant patch.

> >> To get the user we should be user fs_uid as HPA suggested.
> >
> > fsuid is exclusively used for checking file permissions, which we
> > don't do here anymore.  So while it could be argued, that mount() _is_
> > a filesystem operation, it is really a different sort of filesystem
> > operation than the rest.
> >
> > OTOH it wouldn't hurt to use fsuid instead of ruid...
> 
> Yes.  I may be confused but I'm pretty certain we want either
> the fsuid or the euid to be the mount owner.  ruid just looks wrong.
> The fsuid is a special case of the effective uid.  Which is who
> we should perform operations as.  Unless I'm just confused.

Definitely not euid.  Euid is the one which is effective, i.e. it will
basically always be zero for a privileged mount().

Ruid is the one which is returned by getuid().  If a user execs a
suid-root program, then ruid will be the id of the user, while euid
will be zero.


> >> Finally I'm pretty certain the capability we should care about in
> >> this context is CAP_SETUID.  Instead of CAP_SYS_ADMIN.
> >> 
> >> If we have CAP_SETUID we can become which ever user owns the mount,
> >> and the root user in a container needs this, so he can run login
> >> programs.  So changing the appropriate super user checks from
> >> CAP_SYS_ADMIN to CAP_SETUID I think is the right thing todo.
> >
> > That's a flawed logic.  If you want to mount as a specific user, and
> > you have CAP_SETUID, then just use set*uid() and then mount().
> 
> Totally agreed for mount.
> 
> > Changing the capability check for mount() would break the userspace
> > ABI.
> 
> Sorry I apparently wasn't clear.  CAP_SETUID should be the capability
> check for umount.

The argument applies to umount as well.  For compatibility, we _need_
the CAP_SYS_ADMIN check.  And if program has CAP_SETUID but not
CAP_SYS_ADMIN, it can just set the id to the mount owner before
calling umount.

Miklos
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

> >> > +	if (mnt->mnt_flags & MNT_USER)
> >> > +		seq_printf(m, ",user=%i", mnt->mnt_uid);
> >> How about making the test "if (mnt->mnt_user != &root_user)"
> >
> > We don't want to treat root_user special.  That's what capabilities
> > were invented for.
> 
> For the print statement?  What ever it is minor.

It is a user interface, not a print statement.  Your suggested change
would be vetoed by any number of people.  

So either we have all mounts having owners, AND have /proc/mounts add
"user=0" to all mounts.  While I don't _think_ this would actually
break userspace, it would definitely make people complain.

The other choice is what the current patchset does: is to have
"legacy" mounts without owners, and "new generation" mounts with
owners having "user=UID" in /proc/mounts, regardless of the value of
UID.

> So I want to minimize the changes needed to existing programs.
> Now if all we have to do is specify MS_SETUSER when root a
> user with CAP_SETUID is setting up a mount as a user other
> then himself then I don't much care.  If we have to call MS_SETUSER
> as unprivileged users

You don't.  Unprivileged mounts _imply_ MS_SETUSER.

> >> > +
> >> > +	uid_t mnt_uid;			/* owner of the mount */
> >> 
> >> Can we please make this a user struct.   That requires a bit of
> >> reference counting but it has uid namespace benefits as well
> >> as making it easy to implement per user mount rlimits.
> >
> > OK, can you ellaborate, what the uid namespace benifits are?
> 
> In the uid namespace the comparison is simpler as are the propagations
> rules.  Basically if you use a struct user you will never need to
> care about a uid namespace.  If you don't we will have to tear through
> this code another time.

Well, OK.  I'll do the user_struct thing then.

Miklos
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

> >> > From: Miklos Szeredi <mszeredi@suse.cz>
> >> >
> >> > Add sysctl variables for accounting and limiting the number of user
> >> > mounts.
> >> >
> >> > The maximum number of user mounts is set to 1024 by default.  This
> >> > won't in itself enable user mounts, setting a mount to be owned by a
> >> > user is first needed
> >> 
> >> Since each mount has a user can we just make this a per user rlimit?
> >> 
> >> If we are going to implement a sysctl at this point I think it should
> >> be a global limit that doesn't care if who you are.  Even root can
> >> have recursive mounts that attempt to get out of control.
> >
> > Recursive bind mounts are done carefully enough, so they don't get out
> > of control.
> >
> > Recursive mount propagations can get out of control.  But root can
> > shoot itself in the foot any number of ways, and it's not for the
> > kernel to police that.
> 
> Yes.  It is.
> 
> This is mostly about removing special cases.
> 
> We routinely have limits on resources that are global and apply
> to root along with every one else.  Root can change them but
> they still apply to root.  Things like the number of inodes
> in the system or the total number of files.

There's no max_inodes any more.  As for max_files:

get_empty_filp():

	/*
	 * Privileged users can go above max_files
	 */
	if (get_nr_files() >= files_stat.max_files && !capable(CAP_SYS_ADMIN)) {

Miklos
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

> > On Apr 21 2007 10:57, Eric W. Biederman wrote:
> >>
> >>> tmpfs!
> >>
> >>tmpfs is a possible problem because it can consume lots of ram/swap. 
> >>Which is why it has limits on the amount of space it can consume. 
> >
> > Users can gobble up all RAM and swap already today. (Unless they are
> > confined into an rlimit, which, in most systems, is not the case.)
> > And in case /dev/shm exists, they can already fill it without running
> > into an rlimit early.
> 
> There are systems that care about rlimits and there is strong intersection
> between caring about rlimits and user mounts.  Although I do agree that
> it looks like we have gotten lazy with the default mount options for
> /dev/shm.
> 
> Going a little farther any filesystem that is safe to put on a usb
> stick and mount automatically should ultimately be safe for unprivileged
> mounts as well.

Actually, that's not as simple.

For the usb stick or cdrom you need physical access to the machine.
And once you have that you basically have full control over the system
anyway.

But with block filesystems, the user would still need access to the
device (currently kernel doesn't even check this I think).

So it may make sense to mark all block based filesystems safe, and
defer permission checking to user access on the block device.

But the safe flag is still needed for filesystems, which don't have
such an additional access checking, such as network filesystems.

Miklos
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

> >> > +
> >> > +	uid_t mnt_uid;			/* owner of the mount */
> >> 
> >> Can we please make this a user struct.   That requires a bit of
> >> reference counting but it has uid namespace benefits as well
> >> as making it easy to implement per user mount rlimits.
> >
> > OK, can you ellaborate, what the uid namespace benifits are?
> 
> In the uid namespace the comparison is simpler as are the propagations
> rules.  Basically if you use a struct user you will never need to
> care about a uid namespace.

I tried to implement it but got stuck on this: fsuid doesn't have a
user_struct in task_struct (yet), so we'd now have to convert
current->fsuid to a user_struct.  This can be done with alloc_uid(),
but this can fail, bringing in extra error handling complexity.

Also we'd have to compare current->fsuid with a user_struct, which we
don't yet know how will actually be done in the future.

So it seems, we still have to care about the uid namespace, at least
if fsuid is preferred to ruid.

Anyway, here's a patch fixing the other things you brought up, and
which I agree with.  Does this look OK?

Thanks,
Miklos


Index: linux/fs/namespace.c
===================================================================
--- linux.orig/fs/namespace.c	2007-04-22 17:48:18.000000000 +0200
+++ linux/fs/namespace.c	2007-04-22 18:19:51.000000000 +0200
@@ -252,10 +252,12 @@ static int reserve_user_mount(void)
 static void __set_mnt_user(struct vfsmount *mnt)
 {
 	BUG_ON(mnt->mnt_flags & MNT_USER);
-	mnt->mnt_uid = current->uid;
+	mnt->mnt_uid = current->fsuid;
 	mnt->mnt_flags |= MNT_USER;
-	if (!capable(CAP_SYS_ADMIN))
-		mnt->mnt_flags |= MNT_NOSUID | MNT_NODEV;
+	if (!capable(CAP_SETUID))
+		mnt->mnt_flags |= MNT_NOSUID;
+	if (!capable(CAP_MKNOD))
+		mnt->mnt_flags |= MNT_NODEV;
 }
 
 static void set_mnt_user(struct vfsmount *mnt)
@@ -725,10 +727,10 @@ static bool permit_umount(struct vfsmoun
 	if (!(mnt->mnt_flags & MNT_USER))
 		return false;
 
-	if (flags & MNT_FORCE)
+	if ((flags & MNT_FORCE) && !(mnt->mnt_sb->s_type->fs_flags & FS_SAFE))
 		return false;
 
-	return mnt->mnt_uid == current->uid;
+	return mnt->mnt_uid == current->fsuid;
 }
 
 /*
@@ -792,13 +794,13 @@ static bool permit_mount(struct nameidat
 	if (type && !(type->fs_flags & FS_SAFE))
 		return false;
 
-	if (!S_ISDIR(inode->i_mode) && !S_ISREG(inode->i_mode))
+	if (S_ISLNK(inode->i_mode))
 		return false;
 
 	if (!(nd->mnt->mnt_flags & MNT_USER))
 		return false;
 
-	if (nd->mnt->mnt_uid != current->uid)
+	if (nd->mnt->mnt_uid != current->fsuid)
 		return false;
 
 	*flags |= MS_SETUSER;
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Karel Zak <kzak@redhat.com> writes:

> On Fri, Apr 20, 2007 at 12:25:32PM +0200, Miklos Szeredi wrote:
>> The following extra security measures are taken for unprivileged
>> mounts:
>> 
>>  - usermounts are limited by a sysctl tunable
>>  - force "nosuid,nodev" mount options on the created mount
>
>  The original userspace "user=" solution also implies the "noexec"
>  option by default (you can override the default by "exec" option).
>  
>  It means the kernel based solution is not fully compatible ;-(

Why noexec?  Either it was a silly or arbitrary decision, or
our kernel design may be incomplete.

Now I can see not wanting to support executables if you are locking
down a system.  The classic don't execute a program from a CD just because
the CD was stuck in the drive problem.

So I can see how executing code from an untrusted source could prevent
exploitation of other problems, and we certainly don't want to do it
automatically.

Eric
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

On Fri, Apr 20, 2007 at 12:25:32PM +0200, Miklos Szeredi wrote:
> The following extra security measures are taken for unprivileged
> mounts:
> 
>  - usermounts are limited by a sysctl tunable
>  - force "nosuid,nodev" mount options on the created mount

 The original userspace "user=" solution also implies the "noexec"
 option by default (you can override the default by "exec" option).
 
 It means the kernel based solution is not fully compatible ;-(

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 
 Red Hat Czech s.r.o.
 Purkynova 99/71, 612 45 Brno, Czech Republic
 Reg.id: CZ27690016
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

> > The following extra security measures are taken for unprivileged
> > mounts:
> > 
> >  - usermounts are limited by a sysctl tunable
> >  - force "nosuid,nodev" mount options on the created mount
> 
>  The original userspace "user=" solution also implies the "noexec"
>  option by default (you can override the default by "exec" option).

Unlike "nosuid" and "nodev", I don't think "noexec" has real security
benefits.

>  It means the kernel based solution is not fully compatible ;-(

Oh, I don't think that matters.  For traditional /etc/fstab based user
mounts, mount(8) will have to remain suid-root, the kernel can't
replace the fstab check.

In fact the latest patches don't even support these "legacy" user
mounts too well: setting the owner of a mount gives not only umount
privilege, but the ability to submount.  This is not necessarily a
good thing for these kinds of user mounts.

We could add a new "nosubmount" or similar flag, to prevent
submounting, but that again would go against the simplicity of the
current approach, so I'm not sure it's worth it.

Miklos
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers

On Wed, Apr 25, 2007 at 09:18:28AM +0200, Miklos Szeredi wrote:
> > > The following extra security measures are taken for unprivileged
> > > mounts:
> > > 
> > >  - usermounts are limited by a sysctl tunable
> > >  - force "nosuid,nodev" mount options on the created mount
> > 
> >  The original userspace "user=" solution also implies the "noexec"
> >  option by default (you can override the default by "exec" option).
> 
> Unlike "nosuid" and "nodev", I don't think "noexec" has real security
> benefits.

 Yes. I agree. 

> >  It means the kernel based solution is not fully compatible ;-(
> 
> Oh, I don't think that matters.  For traditional /etc/fstab based user
> mounts, mount(8) will have to remain suid-root, the kernel can't
> replace the fstab check.

 Ok, it makes sense. You're right that for the mount(8) is more
 important the fstab check. 
 
 Please, prepare a mount(8) patch -- with the patch it will be more
 clear.

> We could add a new "nosubmount" or similar flag, to prevent
> submounting, but that again would go against the simplicity of the
> current approach, so I'm not sure it's worth it.

 The "nosubmount" is probably good idea.
 
 The patches seem much better in v4. I'm fun for the feature in the
 kernel (and also for every change that makes mtab more and more
 obsolete :-).

    Karel



> 
> Miklos
> -
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
 Karel Zak  <kzak@redhat.com>
 
 Red Hat Czech s.r.o.
 Purkynova 99/71, 612 45 Brno, Czech Republic
 Reg.id: CZ27690016
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers