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

[Paul Moore]

On Mon, Jan 6, 2025 at 6:50 PM Shervin Oloumi <enlightened@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> ---
>
> Changes since v1:
> - Indicate whether the mount is recursive in the hook. This can be a
>   factor when deciding if a mount should be allowed
> - Add default capabilities function for the new hook in security.h. This
>   is required for some tests to pass

I'm not seeing anything like that in the patch you sent?  Am I missing
something, did you send an incomplete/outdated patch, or simply word
the change above wrong?

> - Reword the hook description to be more future proof
> ---
>  fs/namespace.c                |  4 ++++
>  include/linux/lsm_hook_defs.h |  2 ++
>  include/linux/security.h      |  7 +++++++
>  security/security.c           | 18 ++++++++++++++++++
>  4 files changed, 31 insertions(+)
>
> diff --git a/fs/namespace.c b/fs/namespace.c
> index 23e81c2a1e3f..535a1841c200 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, recurse ? true : false);
> +       if (err)
> +               goto out;
> +
>         err = -EINVAL;
>         if (mnt_ns_loop(old_path.dentry))
>                 goto out;
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index eb2937599cb0..94f5a3530b98 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -71,6 +71,8 @@ LSM_HOOK(int, 0, sb_show_options, struct seq_file *m, struct super_block *sb)
>  LSM_HOOK(int, 0, sb_statfs, struct dentry *dentry)
>  LSM_HOOK(int, 0, sb_mount, const char *dev_name, const struct path *path,
>          const char *type, unsigned long flags, void *data)
> +LSM_HOOK(int, 0, sb_bindmount, const struct path *old_path,
> +        const struct path *path, bool recurse)
>  LSM_HOOK(int, 0, sb_umount, struct vfsmount *mnt, int flags)
>  LSM_HOOK(int, 0, sb_pivotroot, const struct path *old_path,
>          const struct path *new_path)
> diff --git a/include/linux/security.h b/include/linux/security.h
> index cbdba435b798..d4a4c71865e3 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -365,6 +365,7 @@ int security_sb_show_options(struct seq_file *m, struct super_block *sb);
>  int security_sb_statfs(struct dentry *dentry);
>  int security_sb_mount(const char *dev_name, const struct path *path,
>                       const char *type, unsigned long flags, void *data);
> +int security_sb_bindmount(const struct path *old_path, const struct path *path, bool recurse);
>  int security_sb_umount(struct vfsmount *mnt, int flags);
>  int security_sb_pivotroot(const struct path *old_path, const struct path *new_path);
>  int security_sb_set_mnt_opts(struct super_block *sb,
> @@ -801,6 +802,12 @@ static inline int security_sb_mount(const char *dev_name, const struct path *pat
>         return 0;
>  }
>
> +static inline int security_sb_bindmount(const struct path *old_path,
> +                                       const struct path *path, bool recurse)
> +{
> +       return 0;
> +}
> +
>  static inline int security_sb_umount(struct vfsmount *mnt, int flags)
>  {
>         return 0;
> diff --git a/security/security.c b/security/security.c
> index 09664e09fec9..c115d8627e2d 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1564,6 +1564,24 @@ 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 permission check
> + * @old_path: source of loopback/bind mount
> + * @path: mount point
> + * @recurse: true if recursive (MS_REC)
> + *
> + * 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,
> +                         bool recurse)
> +{
> +       return call_int_hook(sb_bindmount, old_path, path, recurse);
> +}
> +
>  /**
>   * security_sb_umount() - Check permission for unmounting a filesystem
>   * @mnt: mounted filesystem
>
> base-commit: fc033cf25e612e840e545f8d5ad2edd6ba613ed5
> --
> 2.47.1.613.gc27f4b7a9f-goog
>


-- 
paul-moore.com