Message-ID: <CAHC9VhTHviBcqhC=iOgD0R2Z4XqQifd-F1NysaX2C8oaF00oXA@mail.gmail.com>
Date: Fri, 3 Jan 2025 00:11:06 -0500
From: Paul Moore <paul@...l-moore.com>
To: Shervin Oloumi <enlightened@...omium.org>
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

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