linux-kernel - Re: [PATCH v4 13/30] landlock: Optimize file path walks and prepare for audit support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250110.ahKa1heipoh9@digikod.net>
Date: Fri, 10 Jan 2025 12:24:39 +0100
From: Mickaël Salaün <mic@...ikod.net>
To: Eric Paris <eparis@...hat.com>, Paul Moore <paul@...l-moore.com>, 
	Günther Noack <gnoack@...gle.com>, "Serge E . Hallyn" <serge@...lyn.com>
Cc: Ben Scarlato <akhna@...gle.com>, 
	Casey Schaufler <casey@...aufler-ca.com>, Charles Zaffery <czaffery@...lox.com>, 
	Daniel Burgener <dburgener@...ux.microsoft.com>, Francis Laniel <flaniel@...ux.microsoft.com>, 
	James Morris <jmorris@...ei.org>, Jann Horn <jannh@...gle.com>, Jeff Xu <jeffxu@...gle.com>, 
	Jorge Lucangeli Obes <jorgelo@...gle.com>, Kees Cook <kees@...nel.org>, 
	Konstantin Meskhidze <konstantin.meskhidze@...wei.com>, Matt Bobrowski <mattbobrowski@...gle.com>, 
	Mikhail Ivanov <ivanov.mikhail1@...wei-partners.com>, Phil Sutter <phil@....cc>, 
	Praveen K Paladugu <prapal@...ux.microsoft.com>, Robert Salvet <robert.salvet@...lox.com>, 
	Shervin Oloumi <enlightened@...gle.com>, Song Liu <song@...nel.org>, 
	Tahera Fahimi <fahimitahera@...il.com>, Tyler Hicks <code@...icks.com>, audit@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-security-module@...r.kernel.org
Subject: Re: [PATCH v4 13/30] landlock: Optimize file path walks and prepare
 for audit support

On Wed, Jan 08, 2025 at 04:43:21PM +0100, Mickaël Salaün wrote:
> Always synchronize access_masked_parent* with access_request_parent*
> according to allowed_parent*.  This is required for audit support to be
> able to get back to the reason of denial.
> 
> In a rename/link action, instead of always checking a rule two times for
> the same parent directory of the source and the destination files, only
> check it when an action on a child was not already allowed.  This also
> enables us to keep consistent allowed_parent* status, which is required
> to get back to the reason of denial.
> 
> For internal mount points, only upgrade allowed_parent* to true but do
> not wrongfully set both of them to false otherwise.  This is also
> required to get back to the reason of denial.
> 
> This does not impact the current behavior but slightly optimize code and
> prepare for audit support that needs to know the exact reason why an
> access was denied.
> 
> Cc: Günther Noack <gnoack@...gle.com>
> Signed-off-by: Mickaël Salaün <mic@...ikod.net>
> Link: https://lore.kernel.org/r/20250108154338.1129069-14-mic@digikod.net

Pushed in my next tree to simplify next patch series.

> ---
> 
> Changes since v2:
> - New patch.
> ---
>  security/landlock/fs.c | 44 ++++++++++++++++++++++++++----------------
>  1 file changed, 27 insertions(+), 17 deletions(-)
> 
> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index ddadc465581e..01f9d5e78218 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
> @@ -854,15 +854,6 @@ static bool is_access_to_paths_allowed(
>  				     child1_is_directory, layer_masks_parent2,
>  				     layer_masks_child2,
>  				     child2_is_directory))) {
> -			allowed_parent1 = scope_to_request(
> -				access_request_parent1, layer_masks_parent1);
> -			allowed_parent2 = scope_to_request(
> -				access_request_parent2, layer_masks_parent2);
> -
> -			/* Stops when all accesses are granted. */
> -			if (allowed_parent1 && allowed_parent2)
> -				break;
> -
>  			/*
>  			 * Now, downgrades the remaining checks from domain
>  			 * handled accesses to requested accesses.
> @@ -870,15 +861,32 @@ static bool is_access_to_paths_allowed(
>  			is_dom_check = false;
>  			access_masked_parent1 = access_request_parent1;
>  			access_masked_parent2 = access_request_parent2;
> +
> +			allowed_parent1 =
> +				allowed_parent1 ||
> +				scope_to_request(access_masked_parent1,
> +						 layer_masks_parent1);
> +			allowed_parent2 =
> +				allowed_parent2 ||
> +				scope_to_request(access_masked_parent2,
> +						 layer_masks_parent2);
> +
> +			/* Stops when all accesses are granted. */
> +			if (allowed_parent1 && allowed_parent2)
> +				break;
>  		}
>  
>  		rule = find_rule(domain, walker_path.dentry);
> -		allowed_parent1 = landlock_unmask_layers(
> -			rule, access_masked_parent1, layer_masks_parent1,
> -			ARRAY_SIZE(*layer_masks_parent1));
> -		allowed_parent2 = landlock_unmask_layers(
> -			rule, access_masked_parent2, layer_masks_parent2,
> -			ARRAY_SIZE(*layer_masks_parent2));
> +		allowed_parent1 = allowed_parent1 ||
> +				  landlock_unmask_layers(
> +					  rule, access_masked_parent1,
> +					  layer_masks_parent1,
> +					  ARRAY_SIZE(*layer_masks_parent1));
> +		allowed_parent2 = allowed_parent2 ||
> +				  landlock_unmask_layers(
> +					  rule, access_masked_parent2,
> +					  layer_masks_parent2,
> +					  ARRAY_SIZE(*layer_masks_parent2));
>  
>  		/* Stops when a rule from each layer grants access. */
>  		if (allowed_parent1 && allowed_parent2)
> @@ -902,8 +910,10 @@ static bool is_access_to_paths_allowed(
>  			 * access to internal filesystems (e.g. nsfs, which is
>  			 * reachable through /proc/<pid>/ns/<namespace>).
>  			 */
> -			allowed_parent1 = allowed_parent2 =
> -				!!(walker_path.mnt->mnt_flags & MNT_INTERNAL);
> +			if (walker_path.mnt->mnt_flags & MNT_INTERNAL) {
> +				allowed_parent1 = true;
> +				allowed_parent2 = true;
> +			}
>  			break;
>  		}
>  		parent_dentry = dget_parent(walker_path.dentry);
> -- 
> 2.47.1
> 
>