| Message ID | 20250106235022.3859523-1-enlightened@chromium.org (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [v2,1/2] fs: add loopback/bind mount specific security hook | expand |
On Mon, Jan 6, 2025 at 6:50 PM Shervin Oloumi <enlightened@chromium.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@chromium.org> > --- > > 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 >
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
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@chromium.org> --- 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 - 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(+) base-commit: fc033cf25e612e840e545f8d5ad2edd6ba613ed5