On 2023-08-14, Sargun Dhillon <sargun@xxxxxxxxx> wrote:
> On Sun, Aug 13, 2023 at 10:41 PM Aleksa Sarai <cyphar@xxxxxxxxxx> wrote:
> >
> > It just occurred to me that the whole MNT_LOCK_* machinery has the
> > unfortunate consequence of restricting the host root user from being
> > able to modify the locked flags. Since this change will let you do this
> > without creating a userns, do we want to make can_change_locked_flags()
> > do capable(CAP_SYS_MOUNT)?
> >
> Doesn't mount_setattr already require that the user has CAP_SYS_ADMIN
> in the mount's user namespace?
>
> I'm not sure how this lets us bypass that.
>
> Or are you saying we should check for
> CAP_SYS_MOUNT && CAP_SYS_ADMIN?
I was talking about the fact that can_change_locked_flags() doesn't have
an escape catch for capable(CAP_SYS_whatever). But as I mentioned in a
later mail, this might be safe but it would have other downsides.
> > > + if ((new_mount_flags & kattr->attr_lock) != kattr->attr_lock) {
> > > + err = -EINVAL;
> > > + break;
> > > + }
> >
> > Since the MNT_LOCK_* flags are invisible to userspace, it seems more
> > reasonable to have the attr_lock set be added to the existing set rather
> > than requiring userspace to pass the same set of flags.
> >
> IMHO, it's nice to be able to use the existing set of flags. I don't mind
> adding new flags though.
This was related to the later paragraph. I agree passing the existing
MOUNT_ATTR_* flags to attr_lock is preferable.
> > Actually, AFAICS this implementation breaks backwards compatibility
> > because with this change you now need to pass MNT_LOCK_* flags if
> > operating on a mount that has locks applied already. So existing
> > programs (which have .attr_lock=0) will start getting -EINVAL when
> > operating on mounts with locked flags (such as those locked in the
> > userns case). Or am I missing something?
> I don't think so, because if attr_lock is 0, then
> new_mount_flags & kattr->attr_lock is 0. kattr->attr_lock is only
> flags to *newly lock*, and doesn't inherit the set of current locks.
Ah, I misread this (I was confused why this check was needed which made
me think it must be checking something else, but looking at it again,
the check is actually making sure that if you lock a flag that the flag
is also set) -- in that case the behaviour is exactly what I was
describing. Oops!
> > In any case, the most reasonable behaviour would be to OR the requested
> > lock flags with the existing ones IMHO.
> >
> I can append this to the mount_attr_do_lock test in the next patch, and it
> lets me flip the NOSUID bit on and off without attr_lock being set, unless
> you're referring to something else.
The behaviour I was talking about was:
memset(&attr, 0, sizeof(attr));
attr.attr_set = attr.attr_lock = MOUNT_ATTR_NOSUID;
ASSERT_EQ(sys_mount_setattr(-1, "/tmp/C", 0, &attr, sizeof(attr)), 0);
memset(&attr, 0, sizeof(attr));
attr.attr_set = attr.attr_lock = MOUNT_ATTR_NOEXEC;
ASSERT_EQ(sys_mount_setattr(-1, "/tmp/C", 0, &attr, sizeof(attr)), 0); // should work
/* mount should have MOUNT_ATTR_NOSUID|MOUNT_ATTR_NOEXEC now */
(Which is what the current implementation should do.)
> You shouldn't be able to attr_clr a locked attribute (even as
> CAP_SYS_ADMIN in the init_user_ns).
This is what I was referring to in the escape hatch bit above (whether
we should allow this).
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
Attachment:
signature.asc
Description: PGP signature
