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

On Thu, Jan 2, 2025 at 9:11 PM Paul Moore [off-list ref] wrote:

On Mon, Dec 30, 2024 at 8:46 PM Shervin Oloumi [off-list ref] wrote:

quoted

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

quoted

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.

quoted

        err = -EINVAL;
        if (mnt_ns_loop(old_path.dentry))
                goto out;

...

quoted

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

quoted

+ * 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