Re: [patch 2/8] allow unprivileged umount
> > 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 2/8] allow unprivileged umount
Miklos Szeredi <[EMAIL PROTECTED]> writes: > 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. >> 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. >> 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. >> 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. Hopefully my other more detail replies helped with this. Eric - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 2/8] allow unprivileged umount
> 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 2/8] allow unprivileged umount
> > On Sat, 21 Apr 2007 10:09:42 +0200 Miklos Szeredi <[EMAIL PROTECTED]> wrote:
> >
> >> > > +static bool permit_umount(struct vfsmount *mnt, int flags)
> >> > > +{
> >> > >
> >> > > ...
> >> > >
> >> > > + return mnt->mnt_uid == current->uid;
> >> > > +}
> >> >
> >> > Yes, this seems very wrong. I'd have thought that comparing
> >> > user_struct*'s
> >> > would get us a heck of a lot closer to being able to support aliasing of
> >> > UIDs between different namespaces.
> >> >
> >>
> >> OK, I'll fix this up.
> >>
> >> Actually an earlier version of this patch did use user_struct's but
> >> I'd changed it to uids, because it's simpler.
> >
> > OK..
> >
> >> I didn't think about
> >> this being contrary to the id namespaces thing.
> >
> > Well I was madly assuming that when serarate UID namespaces are in use, UID
> > 42 in container A will have a different user_struct from UID 42 in
> > container B. I'd suggest that we provoke an opinion from Eric & co before
> > you do work on this.
>
> That is what I what I have been thinking as well,
Does this mean, that containers will need this? Or that you don't
know yet?
> 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...
> 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...
> 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().
Changing the capability check for mount() would break the userspace
ABI.
Miklos
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 2/8] allow unprivileged umount
Miklos Szeredi <[EMAIL PROTECTED]> writes:
> From: Miklos Szeredi <[EMAIL PROTECTED]>
>
> The owner doesn't need sysadmin capabilities to call umount().
>
> Similar behavior as umount(8) on mounts having "user=UID" option in
> /etc/mtab. The difference is that umount also checks /etc/fstab,
> presumably to exclude another mount on the same mountpoint.
>
> Signed-off-by: Miklos Szeredi <[EMAIL PROTECTED]>
> ---
>
> Index: linux/fs/namespace.c
> ===
> --- linux.orig/fs/namespace.c 2007-04-20 11:55:05.0 +0200
> +++ linux/fs/namespace.c 2007-04-20 11:55:06.0 +0200
> @@ -659,6 +659,25 @@ static int do_umount(struct vfsmount *mn
> }
>
> /*
> + * umount is permitted for
> + * - sysadmin
> + * - mount owner, if not forced umount
> + */
> +static bool permit_umount(struct vfsmount *mnt, int flags)
> +{
> + if (capable(CAP_SYS_ADMIN))
> + return true;
> +
> + if (!(mnt->mnt_flags & MNT_USER))
> + return false;
> +
> + if (flags & MNT_FORCE)
> + return false;
> +
> + return mnt->mnt_uid == current->uid;
> +}
I think this should be:
static bool permit_umount(struct vfsmount *mnt, int flags)
{
if ((mnt->mnt_uid != current->fsuid) && !capable(CAP_SETUID))
return false;
if ((flags & MNT_FORCE) && !capable(CAP_SYS_ADMIN))
return false;
return true;
}
I.e.
MNT_USER gone.
compare against fsuid.
Only require setuid for unmounts unless force is specified.
I suspect we can allow MNT_FORCE for non-privileged users
as well if we can trust the filesystem.
> +/*
> * Now umount can handle mount points as well as block devices.
> * This is important for filesystems which use unnamed block devices.
> *
> @@ -681,7 +700,7 @@ asmlinkage long sys_umount(char __user *
> goto dput_and_out;
>
> retval = -EPERM;
> - if (!capable(CAP_SYS_ADMIN))
> + if (!permit_umount(nd.mnt, flags))
> goto dput_and_out;
>
> retval = do_umount(nd.mnt, flags);
>
> --
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 2/8] allow unprivileged umount
Andrew Morton <[EMAIL PROTECTED]> writes:
> On Sat, 21 Apr 2007 10:09:42 +0200 Miklos Szeredi <[EMAIL PROTECTED]> wrote:
>
>> > > +static bool permit_umount(struct vfsmount *mnt, int flags)
>> > > +{
>> > >
>> > > ...
>> > >
>> > > +return mnt->mnt_uid == current->uid;
>> > > +}
>> >
>> > Yes, this seems very wrong. I'd have thought that comparing user_struct*'s
>> > would get us a heck of a lot closer to being able to support aliasing of
>> > UIDs between different namespaces.
>> >
>>
>> OK, I'll fix this up.
>>
>> Actually an earlier version of this patch did use user_struct's but
>> I'd changed it to uids, because it's simpler.
>
> OK..
>
>> I didn't think about
>> this being contrary to the id namespaces thing.
>
> Well I was madly assuming that when serarate UID namespaces are in use, UID
> 42 in container A will have a different user_struct from UID 42 in
> container B. I'd suggest that we provoke an opinion from Eric & co before
> you do work on this.
That is what I what I have been thinking as well, 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.
To get the user we should be user fs_uid as HPA suggested.
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.
With the CAP_SETUID thing handled I'm not currently seeing any adverse
implications to using this in containers.
Ok. Now that I have a reasonable approximation of the 10,000 foot
view now to see how the patches match up.
Eric
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 2/8] allow unprivileged umount
On Sat, 21 Apr 2007 10:09:42 +0200 Miklos Szeredi <[EMAIL PROTECTED]> wrote:
> > > +static bool permit_umount(struct vfsmount *mnt, int flags)
> > > +{
> > >
> > > ...
> > >
> > > + return mnt->mnt_uid == current->uid;
> > > +}
> >
> > Yes, this seems very wrong. I'd have thought that comparing user_struct*'s
> > would get us a heck of a lot closer to being able to support aliasing of
> > UIDs between different namespaces.
> >
>
> OK, I'll fix this up.
>
> Actually an earlier version of this patch did use user_struct's but
> I'd changed it to uids, because it's simpler.
OK..
> I didn't think about
> this being contrary to the id namespaces thing.
Well I was madly assuming that when serarate UID namespaces are in use, UID
42 in container A will have a different user_struct from UID 42 in
container B. I'd suggest that we provoke an opinion from Eric & co before
you do work on this.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 2/8] allow unprivileged umount
> > +static bool permit_umount(struct vfsmount *mnt, int flags)
> > +{
> >
> > ...
> >
> > + return mnt->mnt_uid == current->uid;
> > +}
>
> Yes, this seems very wrong. I'd have thought that comparing user_struct*'s
> would get us a heck of a lot closer to being able to support aliasing of
> UIDs between different namespaces.
>
OK, I'll fix this up.
Actually an earlier version of this patch did use user_struct's but
I'd changed it to uids, because it's simpler. I didn't think about
this being contrary to the id namespaces thing.
Miklos
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 2/8] allow unprivileged umount
Andrew Morton wrote:
On Fri, 20 Apr 2007 12:25:34 +0200 Miklos Szeredi <[EMAIL PROTECTED]> wrote:
+static bool permit_umount(struct vfsmount *mnt, int flags)
+{
...
+ return mnt->mnt_uid == current->uid;
+}
Yes, this seems very wrong. I'd have thought that comparing user_struct*'s
would get us a heck of a lot closer to being able to support aliasing of
UIDs between different namespaces.
Not to mention it should be fsuid, not uid.
-hpa
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 2/8] allow unprivileged umount
On Fri, 20 Apr 2007 12:25:34 +0200 Miklos Szeredi <[EMAIL PROTECTED]> wrote:
> +static bool permit_umount(struct vfsmount *mnt, int flags)
> +{
>
> ...
>
> + return mnt->mnt_uid == current->uid;
> +}
Yes, this seems very wrong. I'd have thought that comparing user_struct*'s
would get us a heck of a lot closer to being able to support aliasing of
UIDs between different namespaces.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[patch 2/8] allow unprivileged umount
From: Miklos Szeredi <[EMAIL PROTECTED]>
The owner doesn't need sysadmin capabilities to call umount().
Similar behavior as umount(8) on mounts having "user=UID" option in
/etc/mtab. The difference is that umount also checks /etc/fstab,
presumably to exclude another mount on the same mountpoint.
Signed-off-by: Miklos Szeredi <[EMAIL PROTECTED]>
---
Index: linux/fs/namespace.c
===
--- linux.orig/fs/namespace.c 2007-04-20 11:55:05.0 +0200
+++ linux/fs/namespace.c2007-04-20 11:55:06.0 +0200
@@ -659,6 +659,25 @@ static int do_umount(struct vfsmount *mn
}
/*
+ * umount is permitted for
+ * - sysadmin
+ * - mount owner, if not forced umount
+ */
+static bool permit_umount(struct vfsmount *mnt, int flags)
+{
+ if (capable(CAP_SYS_ADMIN))
+ return true;
+
+ if (!(mnt->mnt_flags & MNT_USER))
+ return false;
+
+ if (flags & MNT_FORCE)
+ return false;
+
+ return mnt->mnt_uid == current->uid;
+}
+
+/*
* Now umount can handle mount points as well as block devices.
* This is important for filesystems which use unnamed block devices.
*
@@ -681,7 +700,7 @@ asmlinkage long sys_umount(char __user *
goto dput_and_out;
retval = -EPERM;
- if (!capable(CAP_SYS_ADMIN))
+ if (!permit_umount(nd.mnt, flags))
goto dput_and_out;
retval = do_umount(nd.mnt, flags);
--
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
