Message-ID: <u6jt67meocvhxlzx4bjt7dyba2piipfznrml46lkv5oi4ft4u5@cbqocdezi7is>
Date: Fri, 3 Jan 2025 12:10:57 +0100
From: Jan Kara <jack@...e.cz>
To: Shervin Oloumi <enlightened@...omium.org>
Cc: mic@...ikod.net, viro@...iv.linux.org.uk, brauner@...nel.org, 
	jack@...e.cz, paul@...l-moore.com, 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 30-12-24 17:46:31, Shervin Oloumi 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>

Christian is much more experienced in this area than me but let me share my
thoughts before he returns from vacation.

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

So this gets triggered for the legacy mount API path (mount(2) syscall).
For the new mount API, I can see open_tree() does not have any security
hook. Is that intented? Are you catching equivalent changes done through
the new mount API inside security_move_mount() hook?

Also what caught my eye is that the LSM doesn't care at all whether this is
a recursive bind mount (copying all mounts beneath the specified one) or
just a normal one (copying only a single mount). Maybe that's fine but it
seems a bit counter-intuitive to me as AFAIU it implicitly places a
requirement on the policy that if doing some bind mount is forbidden, then
doing bind mount of any predecessor must be forbidden as well (otherwise
the policy will be inconsistent).

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR

Powered by blists - more mailing lists