Message-ID: <CAMb9sTgEjNk4X+FLwpmi56z+LDV=gYOO=qN5AjQSN4Erw-SUaw@mail.gmail.com>
Date: Thu, 9 Jan 2025 20:14:07 -0800
From: Shervin Oloumi <enlightened@...omium.org>
To: Jan Kara <jack@...e.cz>
Cc: mic@...ikod.net, viro@...iv.linux.org.uk, brauner@...nel.org, 
	paul@...l-moore.com, jmorris@...ei.org, serge@...lyn.com, 
	linux-kernel@...r.kernel.org, linux-security-module@...r.kernel.org, 
	gnoack@...gle.com, shuah@...nel.org, jorgelo@...omium.org, 
	allenwebb@...omium.org
Subject: Re: [PATCH 1/2] fs: add loopback/bind mount specific security hook

On Fri, Jan 3, 2025 at 3:11 AM Jan Kara <jack@...e.cz> wrote:
>
> On Mon 30-12-24 17:46:31, Shervin Oloumi wrote:
> > The main mount security hook (security_sb_mount) is called early in the
> > process before the mount type is determined and the arguments are
> > validated and converted to the appropriate format. Specifically, the
> > source path is surfaced as a string, which is not appropriate for
> > checking bind mount requests. For bind mounts the source should be
> > validated and passed as a path struct (same as destination), after the
> > mount type is determined. This allows the hook users to evaluate the
> > mount attributes without the need to perform any validations or
> > conversions out of band, which can introduce a TOCTOU race condition.
> >
> > The newly introduced hook is invoked only if the security_sb_mount hook
> > passes, and only if the MS_BIND flag is detected. At this point the
> > source of the mount has been successfully converted to a path struct
> > using the kernel's kern_path API. This allows LSMs to target bind mount
> > requests at the right stage, and evaluate the attributes in the right
> > format, based on the type of mount.
> >
> > This does not affect the functionality of the existing mount security
> > hooks, including security_sb_mount. The new hook, can be utilized as a
> > supplement to the main hook for further analyzing bind mount requests.
> > This means that there is still the option of only using the main hook
> > function, if all one wants to do is indiscriminately reject all bind
> > mount requests, regardless of the source and destination arguments.
> > However, if one needs to evaluate the source and destination of a bind
> > mount request before making a decision, this hook function should be
> > preferred. Of course, if a bind mount request does not make it past the
> > security_sb_mount check, the bind mount hook function is never invoked.
> >
> > Signed-off-by: Shervin Oloumi <enlightened@...omium.org>
>
> Christian is much more experienced in this area than me but let me share my
> thoughts before he returns from vacation.
>
> > 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?

> 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.

>
>                                                                 Honza
> --
> Jan Kara <jack@...e.com>
> SUSE Labs, CR

Powered by blists - more mailing lists