Message-ID: <CAMb9sTj5qp3fxba8hRpUHAB5ShTXL7XgxebS2gihPpJTFOCxpQ@mail.gmail.com>
Date: Thu, 9 Jan 2025 20:11:34 -0800
From: Shervin Oloumi <enlightened@...omium.org>
To: Paul Moore <paul@...l-moore.com>
Cc: mic@...ikod.net, viro@...iv.linux.org.uk, brauner@...nel.org, jack@...e.cz, 
	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

Thanks for the feedback Paul, the latest patch should include the
recommendations now.

On Thu, Jan 2, 2025 at 9:11 PM Paul Moore <paul@...l-moore.com> wrote:
>
> On Mon, Dec 30, 2024 at 8:46 PM Shervin Oloumi <enlightened@...omium.org> 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>
> > ---
> >  fs/namespace.c                |  4 ++++
> >  include/linux/lsm_hook_defs.h |  1 +
> >  include/linux/security.h      |  1 +
> >  security/security.c           | 16 ++++++++++++++++
> >  4 files changed, 22 insertions(+)
>
> Like Casey I'm not really excited about such a specific LSM hook, but
> unfortunately we can't simply call kern_path() in the existing
> security_sb_mount() callback as that could end up resolving to
> something different than the call in do_loopback() which would be bad.
> Moving the kern_path() call up into path_mount() just looks like a bad
> idea anyway you look at it.  Unfortunately I don't really see an
> alternative to what you're proposing, so I guess we're kinda stuck
> with this as a solution, unless someone can think of something better.
>
> I'm going to need to see an ACK from the VFS folks on this before I
> merge the new hook.
>
> I'd also stick with the security_sb_bindmount() name as opposed to the
> XXX_path() suggestion from Casey simply to help distinguish it from
> the pathname based LSM hooks.  Yes, this is operating on the pathname,
> but bind mounts are a bit of a special case.
>
> Unrelated, but I just noticed that we are calling security_sb_mount()
> *before* may_mount(); that's the opposite order for most LSM hook
> placements where we do the discretionary/capabilities checks first and
> the LSM checks.  That's something we should look at, perhaps there is
> a good reason for the ordering being different, perhaps it's a
> mistake.
>
> > 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;
>
> I might make a mention in the commit description that the
> do_reconfigure_mnt() case (MS_REMOUNT|MS_BIND) should be able to be
> handled using the existing security_sb_mount() hook.
>
> >         err = -EINVAL;
> >         if (mnt_ns_loop(old_path.dentry))
> >                 goto out;
>
> ...
>
> > diff --git a/security/security.c b/security/security.c
> > index 09664e09fec9..bd7cb3df16f4 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -1564,6 +1564,22 @@ int security_sb_mount(const char *dev_name, const struct path *path,
> >         return call_int_hook(sb_mount, dev_name, path, type, flags, data);
> >  }
> >
> > +/**
> > + * security_sb_bindmount() - Loopback/bind mount specific permission check
> > + * @old_path: source of loopback/bind mount
> > + * @path: mount point
> > + *
> > + * This check is performed in addition to security_sb_mount and only if the
> > + * mount type is determined to be loopback/bind mount (flags & MS_BIND).  It
> > + * surfaces the mount source as a path struct.
>
> I wouldn't mention security_sb_mount() above as that makes the comment
> somewhat fragile in the face of changing hooks.  I would suggest
> something along these lines:
>
> "Beyond any general mounting hooks, this check is performed on an
>  initial loopback/bind mount (MS_BIND) with the mount source presented
>  as a path struct in @old_path."
>
> > + * Return: Returns 0 if permission is granted.
> > + */
> > +int security_sb_bindmount(const struct path *old_path, const struct path *path)
> > +{
> > +       return call_int_hook(sb_bindmount, old_path, path);
> > +}
> > +
> >  /**
> >   * security_sb_umount() - Check permission for unmounting a filesystem
> >   * @mnt: mounted filesystem
> >
> > base-commit: fc033cf25e612e840e545f8d5ad2edd6ba613ed5
> > --
> > 2.47.1.613.gc27f4b7a9f-goog
>
> --
> paul-moore.com

Powered by blists - more mailing lists