Missing check for CAP_SYS_ADMIN in do_reconfigure_mnt()

Missing check for CAP_SYS_ADMIN in do_reconfigure_mnt()

[tianyu zhou]

Hi, function do_remount() in fs/namespace.c checks the CAP_SYS_ADMIN
before it calls set_mount_attributes().

--------------------
// fs/namespace.c
static int do_remount(struct path *path, int ms_flags, int sb_flags,
              int mnt_flags, void *data)
{
        ....
        if (ns_capable(sb->s_user_ns, CAP_SYS_ADMIN)) {
            err = reconfigure_super(fc);
            if (!err) {
                lock_mount_hash();
                set_mount_attributes(mnt, mnt_flags);       // <===
protected function
                unlock_mount_hash();
            }
        ...
}
--------------------

However, in another caller of set_mount_attributes(),
do_reconfigure_mnt(), I have not found any check for CAP_SYS_ADMIN.
So, is there a missing check bug inside do_reconfigure_mnt() ? (which
makes it possible for normal user to reach set_mount_attributes())

Thanks!

Best regards,
Tianyu

Re: Missing check for CAP_SYS_ADMIN in do_reconfigure_mnt()

[Al Viro]

On Mon, May 31, 2021 at 10:59:54PM +0800, tianyu zhou wrote:
> Hi, function do_remount() in fs/namespace.c checks the CAP_SYS_ADMIN
> before it calls set_mount_attributes().
> 
> --------------------
> // fs/namespace.c
> static int do_remount(struct path *path, int ms_flags, int sb_flags,
>               int mnt_flags, void *data)
> {
>         ....
>         if (ns_capable(sb->s_user_ns, CAP_SYS_ADMIN)) {
>             err = reconfigure_super(fc);
>             if (!err) {
>                 lock_mount_hash();
>                 set_mount_attributes(mnt, mnt_flags);       // <===
> protected function
>                 unlock_mount_hash();
>             }
>         ...
> }
> --------------------
> 
> However, in another caller of set_mount_attributes(),
> do_reconfigure_mnt(), I have not found any check for CAP_SYS_ADMIN.
> So, is there a missing check bug inside do_reconfigure_mnt() ? (which
> makes it possible for normal user to reach set_mount_attributes())

IDGI.  By the same token, there are callers of e.g. memcpy() with
CAP_SYS_ADMIN checks upstream of those, as well as those that are
called without any such checks whatsoever.  The answer to such
observation would obviously be "what of that?" and I really wonder
what your criteria are.

For another example, in the same function you have lock_mount_hash()
calls as well; are you going to report the calls of that made without
CAP_SYS_ADMIN?

IOW, what are the heuristics you are using to select the functions
you deem suspicious?

Re: Missing check for CAP_SYS_ADMIN in do_reconfigure_mnt()

[Matthew Wilcox]

On Mon, May 31, 2021 at 10:59:54PM +0800, tianyu zhou wrote:
> Hi, function do_remount() in fs/namespace.c checks the CAP_SYS_ADMIN
> before it calls set_mount_attributes().
> 
> However, in another caller of set_mount_attributes(),
> do_reconfigure_mnt(), I have not found any check for CAP_SYS_ADMIN.
> So, is there a missing check bug inside do_reconfigure_mnt() ? (which
> makes it possible for normal user to reach set_mount_attributes())

You weren't looking hard enough ...

path_mount()
        if (!may_mount())
                return -EPERM;
...
        if ((flags & (MS_REMOUNT | MS_BIND)) == (MS_REMOUNT | MS_BIND))
                return do_reconfigure_mnt(path, mnt_flags);

(this is the only call to do_reconfigure_mnt())

and:

static inline bool may_mount(void)
{
        return ns_capable(current->nsproxy->mnt_ns->user_ns, CAP_SYS_ADMIN);
}

Re: Missing check for CAP_SYS_ADMIN in do_reconfigure_mnt()

[tianyu zhou]

Thanks for reminding!

But here is one more question about the CAP_SYS_ADMIN check inside
may_mount(): why does it check the CAP in
current->nsproxy->mnt_ns->user_ns?

for do_remount(), it checks CAP_SYS_ADMIN in path->mnt->mnt_sb->s_user_ns;
for path_mount(), it checks CAP_SYS_ADMIN in current->nsproxy->mnt_ns->user_ns.

Is these two user ns are same during runtime? (If they are same, then
it will be redundant check in path_mount() and its callee
do_remount(); if they are not same, maybe do_reconfigure_mnt() need
more check for path->mnt->mnt_sb->s_user_ns)

Tianyu

Matthew Wilcox <willy@infradead.org> 于2021年6月1日周二 上午1:07写道：
>
> On Mon, May 31, 2021 at 10:59:54PM +0800, tianyu zhou wrote:
> > Hi, function do_remount() in fs/namespace.c checks the CAP_SYS_ADMIN
> > before it calls set_mount_attributes().
> >
> > However, in another caller of set_mount_attributes(),
> > do_reconfigure_mnt(), I have not found any check for CAP_SYS_ADMIN.
> > So, is there a missing check bug inside do_reconfigure_mnt() ? (which
> > makes it possible for normal user to reach set_mount_attributes())
>
> You weren't looking hard enough ...
>
> path_mount()
>         if (!may_mount())
>                 return -EPERM;
> ...
>         if ((flags & (MS_REMOUNT | MS_BIND)) == (MS_REMOUNT | MS_BIND))
>                 return do_reconfigure_mnt(path, mnt_flags);
>
> (this is the only call to do_reconfigure_mnt())
>
> and:
>
> static inline bool may_mount(void)
> {
>         return ns_capable(current->nsproxy->mnt_ns->user_ns, CAP_SYS_ADMIN);
> }
>

Re: Missing check for CAP_SYS_ADMIN in do_reconfigure_mnt()

[Christian Brauner]

On Tue, Jun 01, 2021 at 03:02:42AM +0800, tianyu zhou wrote:
> Thanks for reminding!
> 
> But here is one more question about the CAP_SYS_ADMIN check inside
> may_mount(): why does it check the CAP in
> current->nsproxy->mnt_ns->user_ns?

Because the caller needs to be privileged enough to create mounts in
that mount namespace. That's the first requirement. You don't want a
caller creating mounts or altering mounts even though someone explicitly
wanted to prevent them from doing that. Or from someone creating a new
user namespace mapping their own uid and gaining cap_sys_admin in the
userns and then starting to change mount options for everyone in that
mount namespace.

> 
> for do_remount(), it checks CAP_SYS_ADMIN in path->mnt->mnt_sb->s_user_ns;
> for path_mount(), it checks CAP_SYS_ADMIN in current->nsproxy->mnt_ns->user_ns.

You can't reach do_remount() other via path_mount(). So you get:

1. may_mount()
2. check_mnt()

which translates to:

1. Is the caller allowed to create/destroy etc. mounts in that mnt
   namespace?
2. Is the caller in the same mnt namespace that the mount is visible in
   they try to operate on?

The it comes down to the question whether the caller is trying to change
an option that affects the superblock, i.e. _all places where that
filesystem is visible_ or whether they try to change mount specific
options, i.e. options that only affect a specific mount.

For do_remount() it affects the whole filesystem so you need to check
whether the caller is privileged over the filesystem itself, i.e.
whether they are capable in the s_user_ns of the filesystem. That should
also answer the next question:

> 
> Is these two user ns are same during runtime? (If they are same, then

No they don't need to be the same. Take a look at in do_remount():

	fc->oldapi = true;
	err = parse_monolithic_mount_data(fc, data);
	if (!err) {
		down_write(&sb->s_umount);
		err = -EPERM;
		if (ns_capable(sb->s_user_ns, CAP_SYS_ADMIN)) {

^^
Here is were we check that the caller is allowed to alter the
superblock. So if you end up there you have checked 3 things so far:

[x] Allowed to mount in that mnt namespace 
[x] Located in the same mnt namespace as the mount
[x] Privileged wrt to the superblock


> it will be redundant check in path_mount() and its callee
> do_remount(); if they are not same, maybe do_reconfigure_mnt() need

No, do_reconfigure_mnt() affects mount specific options, not the
superblock. So it's enough to know:

[x] Allowed to mount in that mnt namespace
[x] Located in the same mnt namespace as the mount

> more check for path->mnt->mnt_sb->s_user_ns)
> 
> Tianyu
> 
> Matthew Wilcox <willy@infradead.org> 于2021年6月1日周二 上午1:07写道：
> >
> > On Mon, May 31, 2021 at 10:59:54PM +0800, tianyu zhou wrote:
> > > Hi, function do_remount() in fs/namespace.c checks the CAP_SYS_ADMIN
> > > before it calls set_mount_attributes().
> > >
> > > However, in another caller of set_mount_attributes(),
> > > do_reconfigure_mnt(), I have not found any check for CAP_SYS_ADMIN.
> > > So, is there a missing check bug inside do_reconfigure_mnt() ? (which
> > > makes it possible for normal user to reach set_mount_attributes())
> >
> > You weren't looking hard enough ...
> >
> > path_mount()
> >         if (!may_mount())
> >                 return -EPERM;
> > ...
> >         if ((flags & (MS_REMOUNT | MS_BIND)) == (MS_REMOUNT | MS_BIND))
> >                 return do_reconfigure_mnt(path, mnt_flags);
> >
> > (this is the only call to do_reconfigure_mnt())
> >
> > and:
> >
> > static inline bool may_mount(void)
> > {
> >         return ns_capable(current->nsproxy->mnt_ns->user_ns, CAP_SYS_ADMIN);
> > }
> >