Re: [PATCH v3 1/2] fs: add loopback/bind mount specific security hook

["Mickaël Salaün" <mic@digikod.net>]

On Fri, Jan 10, 2025 at 04:42:19PM +0100, Christian Brauner wrote:

> On Thu, Jan 09, 2025 at 08:14:07PM -0800, Shervin Oloumi wrote:
> > On Fri, Jan 3, 2025 at 3:11 AM Jan Kara <jack@suse.cz> wrote:
> > >
> > > On Mon 30-12-24 17:46:31, Shervin Oloumi wrote:

> > > > diff --git a/fs/namespace.c b/fs/namespace.c
> > > > index 23e81c2a1e3f..c902608c9759 100644
> > > > --- a/fs/namespace.c
> > > > +++ b/fs/namespace.c
> > > > @@ -2765,6 +2765,10 @@ static int do_loopback(struct path *path, const char *old_name,
> > > >       if (err)
> > > >               return err;
> > > >
> > > > +     err = security_sb_bindmount(&old_path, path);
> > > > +     if (err)
> > > > +             goto out;
> > > > +
> > >
> > > So this gets triggered for the legacy mount API path (mount(2) syscall).
> > > For the new mount API, I can see open_tree() does not have any security
> > > hook. Is that intented? Are you catching equivalent changes done through
> > > the new mount API inside security_move_mount() hook?
> > 
> > I am not very familiar with the new API and when it is used, but LandLock does
> > listen to security_move_mount() and it rejects all such requests. It also
> > listens to and rejects remount and pivotroot. Are there cases where bind mount
> > requests go through open_tree() and this hook is bypassed?
> 
> Whether or not Landlock currently blocks security_move_mount()
> unconditionally doesn't matter. Introducing this api will have to do it
> for the old and the new mount api. First, because we don't implement new
> features for the old mount api that aren't also available in the new
> mount api. Second, because this asymmetry just begs for bugs when
> security_move_mount() isn't blocked anymore.
> 
> And third, there seems to be a misconception here.
> open_tree(OPEN_TREE_CLONE) gives you an unattached bind-mount.
> move_mount() is just sugar on top to attach it to a mount namespace.
> 
> But a file descriptor from open_tree(OPEN_TREE_CLONE) can be interacted
> with just fine, i.e., read, write, create, shared with other processes.
> You could create a bind-mount that is never attached anywhere. So I'm
> not sure what security guarantees it would give you to block MS_BIND but
> not OPEN_TREE_CLONE.
> 
> It should be done for both.

Yes, the new hook should probably be called by attach_recursive_mnt().

Landlock tests should check with the legacy mount(2) and the new
move_mount(2) (e.g. with test variants).

> 
> > 
> > > Also what caught my eye is that the LSM doesn't care at all whether this is
> > > a recursive bind mount (copying all mounts beneath the specified one) or
> > > just a normal one (copying only a single mount). Maybe that's fine but it
> > > seems a bit counter-intuitive to me as AFAIU it implicitly places a
> > > requirement on the policy that if doing some bind mount is forbidden, then
> > > doing bind mount of any predecessor must be forbidden as well (otherwise
> > > the policy will be inconsistent).
> > 
> > I need to double check this with Mickaël, but I think it is safe to allow
> > recursive bind mounts. If a bind mount was allowed, let's say /A -> /home/B,
> > then we already verified that the process did not gain more access (or even
> > dropped some access) through the new mount point, e.g. accessing /A/a through
> > /home/B/a. So with a recursive bind mount, let's say /home -> /C, once we check
> > for privilege escalation as usual, it should be safe to clone any existing sub
> > mounts in the new destination. Because if access through B <= A and C <= B then
> > access through C <= A. So back to your point, there should never exist an
> > illegal bind mount action that can implicitly happen through a legal recursive
> > bind mount of its predecessor. Regardless, I think it might be useful to know if
> > a mount is recursive for other use cases so I added a boolean for surfacing
> > MS_REC in the new patches.
> 
> Say /home/hidden is covered by a bind-mount of /dev/null and you're
> doing mount --bind /home /somewhere then /home/hidden will be uncovered
> (There's cases where the kernel itself fuses mounts together or "locks"
> them so things like this cannot happen e.g., when creating an
> unprivileged mount namespace.). If your policy blocks unmounting
> /home/hidden to protect the underlying file then a non-recursive
> bind-mount would be able to circumvent that restriction.

That would be a valid attack scenario.  For Landlock, to make it simple,
non-recursive bind mounts should always be denied.

Shervin, please add this scenario in a comment for the Landlock
implementation of the hook.