Re: [RFC PATCH v2 10/14] landlock: Log file-related denials

[Francis Laniel <flaniel@linux.microsoft.com>]

Le mardi 22 octobre 2024, 18:10:05 CEST Mickaël Salaün a écrit :
> Add audit support for path_mkdir, path_mknod, path_symlink, path_unlink,
> path_rmdir, path_truncate, path_link, path_rename, and file_open hooks.
> 
> Audit record sample for a link action:
> 
>   DENY:     domain=4533720568 blockers=fs_refer path="/usr/bin" dev="vda2"
> ino=351 DOM_INFO: domain=4533720568 parent=0 pid=325 uid=0
> exe="/root/sandboxer" comm="sandboxer" DENY:     domain=4533720568
> blockers=fs_make_reg,fs_refer path="/usr/local" dev="vda2" ino=365 SYSCALL:
>  arch=c000003e syscall=265 success=no exit=-13 ...
> 
> Cc: Günther Noack <gnoack@google.com>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> Link: https://lore.kernel.org/r/20241022161009.982584-11-mic@digikod.net
> ---
> 
> Changes since v2:
> * Revamp logging and support the path_link and path_rename hooks.
> * Add KUnit tests.
> 
> Changes since v1:
> * Move audit code to the ptrace patch.
> ---
>  security/landlock/audit.c | 173 ++++++++++++++++++++++++++++++++++++--
>  security/landlock/audit.h |   9 ++
>  security/landlock/fs.c    |  64 +++++++++++---
>  3 files changed, 229 insertions(+), 17 deletions(-)
> 
> diff --git a/security/landlock/audit.c b/security/landlock/audit.c
> index 4cd9407459d2..9c8b6c246884 100644
> --- a/security/landlock/audit.c
> +++ b/security/landlock/audit.c
> @@ -7,23 +7,55 @@
> 
>  #include <kunit/test.h>
>  #include <linux/audit.h>
> +#include <linux/bitops.h>
>  #include <linux/lsm_audit.h>
>  #include <linux/pid.h>
>  #include <linux/uidgid.h>
> +#include <uapi/linux/landlock.h>
> 
>  #include "audit.h"
> +#include "common.h"
>  #include "cred.h"
>  #include "domain.h"
>  #include "ruleset.h"
> 
> -static const char *get_blocker(const enum landlock_request_type type)
> +static const char *const fs_access_strings[] = {
> +	[BIT_INDEX(LANDLOCK_ACCESS_FS_EXECUTE)] = "fs_execute",
> +	[BIT_INDEX(LANDLOCK_ACCESS_FS_WRITE_FILE)] = "fs_write_file",
> +	[BIT_INDEX(LANDLOCK_ACCESS_FS_READ_FILE)] = "fs_read_file",
> +	[BIT_INDEX(LANDLOCK_ACCESS_FS_READ_DIR)] = "fs_read_dir",
> +	[BIT_INDEX(LANDLOCK_ACCESS_FS_REMOVE_DIR)] = "fs_remove_dir",
> +	[BIT_INDEX(LANDLOCK_ACCESS_FS_REMOVE_FILE)] = "fs_remove_file",
> +	[BIT_INDEX(LANDLOCK_ACCESS_FS_MAKE_CHAR)] = "fs_make_char",
> +	[BIT_INDEX(LANDLOCK_ACCESS_FS_MAKE_DIR)] = "fs_make_dir",
> +	[BIT_INDEX(LANDLOCK_ACCESS_FS_MAKE_REG)] = "fs_make_reg",
> +	[BIT_INDEX(LANDLOCK_ACCESS_FS_MAKE_SOCK)] = "fs_make_sock",
> +	[BIT_INDEX(LANDLOCK_ACCESS_FS_MAKE_FIFO)] = "fs_make_fifo",
> +	[BIT_INDEX(LANDLOCK_ACCESS_FS_MAKE_BLOCK)] = "fs_make_block",
> +	[BIT_INDEX(LANDLOCK_ACCESS_FS_MAKE_SYM)] = "fs_make_sym",
> +	[BIT_INDEX(LANDLOCK_ACCESS_FS_REFER)] = "fs_refer",
> +	[BIT_INDEX(LANDLOCK_ACCESS_FS_TRUNCATE)] = "fs_truncate",
> +	[BIT_INDEX(LANDLOCK_ACCESS_FS_IOCTL_DEV)] = "fs_ioctl_dev",
> +};
> +static_assert(ARRAY_SIZE(fs_access_strings) == LANDLOCK_NUM_ACCESS_FS);
> +
> +static __attribute_const__ const char *
> +get_blocker(const enum landlock_request_type type,
> +	    const unsigned long access_bit)
>  {
>  	switch (type) {
>  	case LANDLOCK_REQUEST_PTRACE:
> +		WARN_ON_ONCE(access_bit != -1);
>  		return "ptrace";
> 
>  	case LANDLOCK_REQUEST_FS_CHANGE_LAYOUT:
> +		WARN_ON_ONCE(access_bit != -1);
>  		return "fs_change_layout";
> +
> +	case LANDLOCK_REQUEST_FS_ACCESS:
> +		if (WARN_ON_ONCE(access_bit >= ARRAY_SIZE(fs_access_strings)))
> +			return "unknown";
> +		return fs_access_strings[access_bit];
>  	}
> 
>  	WARN_ON_ONCE(1);
> @@ -31,9 +63,20 @@ static const char *get_blocker(const enum
> landlock_request_type type) }
> 
>  static void log_blockers(struct audit_buffer *const ab,
> -			 const enum landlock_request_type type)
> +			 const enum landlock_request_type type,
> +			 const access_mask_t access)
>  {
> -	audit_log_format(ab, "%s", get_blocker(type));
> +	const unsigned long access_mask = access;
> +	unsigned long access_bit;
> +	size_t i = 0;
> +
> +	for_each_set_bit(access_bit, &access_mask, BITS_PER_TYPE(access)) {
> +		audit_log_format(ab, "%s%s", (i == 0) ? "" : ",",
> +				 get_blocker(type, access_bit));
> +		i++;
> +	}
> +	if (i == 0)
> +		audit_log_format(ab, "%s", get_blocker(type, -1));
>  }
> 
>  static void log_node(struct landlock_hierarchy *const node)
> @@ -121,9 +164,110 @@ static void test_get_hierarchy(struct kunit *const
> test)
> 
>  #endif /* CONFIG_SECURITY_LANDLOCK_KUNIT_TEST */
> 
> +static size_t get_denied_layer(const struct landlock_ruleset *const domain,
> +			       access_mask_t *const access_request,
> +			       const layer_mask_t (*const layer_masks)[],
> +			       const size_t layer_masks_size)
> +{
> +	const unsigned long access_req = *access_request;

Nit: should access_request being checked for not being NULL?

> +	unsigned long access_bit;
> +	access_mask_t missing = 0;
> +	long youngest_layer = -1;
> +
> +	for_each_set_bit(access_bit, &access_req, layer_masks_size) {
> +		const access_mask_t mask = (*layer_masks)[access_bit];
> +		long layer;
> +
> +		if (!mask)
> +			continue;
> +
> +		/* __fls(1) == 0 */
> +		layer = __fls(mask);
> +		if (layer > youngest_layer) {
> +			youngest_layer = layer;
> +			missing = BIT(access_bit);
> +		} else if (layer == youngest_layer) {
> +			missing |= BIT(access_bit);
> +		}
> +	}
> +
> +	*access_request = missing;
> +	if (youngest_layer == -1)
> +		return domain->num_layers - 1;
> +
> +	return youngest_layer;
> +}
> +
> +#ifdef CONFIG_SECURITY_LANDLOCK_KUNIT_TEST
> +
> +static void test_get_denied_layer(struct kunit *const test)
> +{
> +	const struct landlock_ruleset dom = {
> +		.num_layers = 5,
> +	};
> +	const layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {
> +		[BIT_INDEX(LANDLOCK_ACCESS_FS_EXECUTE)] = BIT(0),
> +		[BIT_INDEX(LANDLOCK_ACCESS_FS_READ_FILE)] = BIT(1),
> +		[BIT_INDEX(LANDLOCK_ACCESS_FS_READ_DIR)] = BIT(1) | BIT(0),
> +		[BIT_INDEX(LANDLOCK_ACCESS_FS_REMOVE_DIR)] = BIT(2),
> +	};
> +	access_mask_t access;
> +
> +	access = LANDLOCK_ACCESS_FS_EXECUTE;
> +	KUNIT_EXPECT_EQ(test, 0,
> +			get_denied_layer(&dom, &access, &layer_masks,
> +					 sizeof(layer_masks)));
> +	KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_EXECUTE);
> +
> +	access = LANDLOCK_ACCESS_FS_READ_FILE;
> +	KUNIT_EXPECT_EQ(test, 1,
> +			get_denied_layer(&dom, &access, &layer_masks,
> +					 sizeof(layer_masks)));
> +	KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_READ_FILE);
> +
> +	access = LANDLOCK_ACCESS_FS_READ_DIR;
> +	KUNIT_EXPECT_EQ(test, 1,
> +			get_denied_layer(&dom, &access, &layer_masks,
> +					 sizeof(layer_masks)));
> +	KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_READ_DIR);
> +
> +	access = LANDLOCK_ACCESS_FS_READ_FILE | LANDLOCK_ACCESS_FS_READ_DIR;
> +	KUNIT_EXPECT_EQ(test, 1,
> +			get_denied_layer(&dom, &access, &layer_masks,
> +					 sizeof(layer_masks)));
> +	KUNIT_EXPECT_EQ(test, access,
> +			LANDLOCK_ACCESS_FS_READ_FILE |
> +				LANDLOCK_ACCESS_FS_READ_DIR);
> +
> +	access = LANDLOCK_ACCESS_FS_EXECUTE | LANDLOCK_ACCESS_FS_READ_DIR;
> +	KUNIT_EXPECT_EQ(test, 1,
> +			get_denied_layer(&dom, &access, &layer_masks,
> +					 sizeof(layer_masks)));
> +	KUNIT_EXPECT_EQ(test, access, LANDLOCK_ACCESS_FS_READ_DIR);
> +
> +	access = LANDLOCK_ACCESS_FS_WRITE_FILE;
> +	KUNIT_EXPECT_EQ(test, 4,
> +			get_denied_layer(&dom, &access, &layer_masks,
> +					 sizeof(layer_masks)));
> +	KUNIT_EXPECT_EQ(test, access, 0);
> +}
> +
> +#endif /* CONFIG_SECURITY_LANDLOCK_KUNIT_TEST */
> +
>  static bool is_valid_request(const struct landlock_request *const request)
>  {
> -	if (WARN_ON_ONCE(!request->layer_plus_one))
> +	if (WARN_ON_ONCE(!(!!request->layer_plus_one ^ !!request->access)))
> +		return false;
> +
> +	if (request->access) {
> +		if (WARN_ON_ONCE(!request->layer_masks))
> +			return false;
> +	} else {
> +		if (WARN_ON_ONCE(request->layer_masks))
> +			return false;
> +	}
> +
> +	if (WARN_ON_ONCE(!!request->layer_masks ^ !!request-
>layer_masks_size))
>  		return false;
> 
>  	return true;
> @@ -140,6 +284,7 @@ void landlock_log_denial(const struct landlock_ruleset
> *const domain, {
>  	struct audit_buffer *ab;
>  	struct landlock_hierarchy *youngest_denied;
> +	access_mask_t missing;
> 
>  	if (WARN_ON_ONCE(!domain || !domain->hierarchy || !request))
>  		return;
> @@ -155,9 +300,24 @@ void landlock_log_denial(const struct landlock_ruleset
> *const domain, if (!ab)
>  		return;
> 
> -	youngest_denied = get_hierarchy(domain, request->layer_plus_one - 1);
> +	missing = request->access;
> +	if (missing) {
> +		size_t youngest_layer;
> +
> +		/* Gets the nearest domain that denies the request. */
> +		if (request->layer_masks) {
> +			youngest_layer = get_denied_layer(
> +				domain, &missing, request->layer_masks,
> +				request->layer_masks_size);
> +		}
> +		youngest_denied = get_hierarchy(domain, youngest_layer);

If request->layer_masks is 0, it is possible to call get_hierarchy() with
uninitialized youngest_layer, is this wanted?

> +	} else {
> +		youngest_denied =
> +			get_hierarchy(domain, request->layer_plus_one - 1);
> +	}
> +
>  	audit_log_format(ab, "domain=%llu blockers=", youngest_denied->id);
> -	log_blockers(ab, request->type);
> +	log_blockers(ab, request->type, missing);
>  	audit_log_lsm_data(ab, &request->audit);
>  	audit_log_end(ab);
> 
> @@ -204,6 +364,7 @@ void landlock_log_drop_domain(const struct
> landlock_ruleset *const domain) static struct kunit_case test_cases[] = {
>  	/* clang-format off */
>  	KUNIT_CASE(test_get_hierarchy),
> +	KUNIT_CASE(test_get_denied_layer),
>  	{}
>  	/* clang-format on */
>  };
> diff --git a/security/landlock/audit.h b/security/landlock/audit.h
> index 6f5ad04b83c2..25fc8333cddc 100644
> --- a/security/landlock/audit.h
> +++ b/security/landlock/audit.h
> @@ -11,11 +11,13 @@
>  #include <linux/audit.h>
>  #include <linux/lsm_audit.h>
> 
> +#include "access.h"
>  #include "ruleset.h"
> 
>  enum landlock_request_type {
>  	LANDLOCK_REQUEST_PTRACE = 1,
>  	LANDLOCK_REQUEST_FS_CHANGE_LAYOUT,
> +	LANDLOCK_REQUEST_FS_ACCESS,
>  };
> 
>  /*
> @@ -33,6 +35,13 @@ struct landlock_request {
>  	 * extra one is useful to detect uninitialized field.
>  	 */
>  	size_t layer_plus_one;
> +
> +	/* Required field for configurable access control. */
> +	access_mask_t access;
> +
> +	/* Required fields for requests with layer masks. */
> +	const layer_mask_t (*layer_masks)[];
> +	size_t layer_masks_size;
>  };
> 
>  #ifdef CONFIG_AUDIT
> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index a099167d2347..7f69bed9e095 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
> @@ -730,6 +730,7 @@ static void test_is_eacces_with_write(struct kunit
> *const test) *     those identified by @access_request_parent1).  This
> matrix can *     initially refer to domain layer masks and, when the
> accesses for the *     destination and source are the same, to requested
> layer masks. + * @log_request_parent1: Audit request to fill if the related
> access is denied. * @dentry_child1: Dentry to the initial child of the
> parent1 path.  This *     pointer must be NULL for non-refer actions (i.e.
> not link nor rename). * @access_request_parent2: Similar to
> @access_request_parent1 but for a @@ -738,6 +739,7 @@ static void
> test_is_eacces_with_write(struct kunit *const test) *     the source.  Must
> be set to 0 when using a simple path request. * @layer_masks_parent2:
> Similar to @layer_masks_parent1 but for a refer *     action.  This must be
> NULL otherwise.
> + * @log_request_parent2: Audit request to fill if the related access is
> denied. * @dentry_child2: Dentry to the initial child of the parent2 path. 
> This *     pointer is only set for RENAME_EXCHANGE actions and must be NULL
> *     otherwise.
> @@ -757,10 +759,12 @@ static bool is_access_to_paths_allowed(
>  	const struct path *const path,
>  	const access_mask_t access_request_parent1,
>  	layer_mask_t (*const layer_masks_parent1)[LANDLOCK_NUM_ACCESS_FS],
> -	const struct dentry *const dentry_child1,
> +	struct landlock_request *const log_request_parent1,
> +	struct dentry *const dentry_child1,
>  	const access_mask_t access_request_parent2,
>  	layer_mask_t (*const layer_masks_parent2)[LANDLOCK_NUM_ACCESS_FS],
> -	const struct dentry *const dentry_child2)
> +	struct landlock_request *const log_request_parent2,
> +	struct dentry *const dentry_child2)
>  {
>  	bool allowed_parent1 = false, allowed_parent2 = false, is_dom_check,
>  	     child1_is_directory = true, child2_is_directory = true;
> @@ -907,6 +911,24 @@ static bool is_access_to_paths_allowed(
>  	}
>  	path_put(&walker_path);
> 
> +	if (!allowed_parent1 && log_request_parent1) {
> +		log_request_parent1->type = LANDLOCK_REQUEST_FS_ACCESS,
> +		log_request_parent1->audit.type = LSM_AUDIT_DATA_PATH,
> +		log_request_parent1->audit.u.path = *path;
> +		log_request_parent1->access = access_request_parent1;
> +		log_request_parent1->layer_masks = layer_masks_parent1;
> +		log_request_parent1->layer_masks_size =
> +			ARRAY_SIZE(*layer_masks_parent1);
> +	}
> +	if (!allowed_parent2 && log_request_parent2) {
> +		log_request_parent2->type = LANDLOCK_REQUEST_FS_ACCESS,
> +		log_request_parent2->audit.type = LSM_AUDIT_DATA_PATH,
> +		log_request_parent2->audit.u.path = *path;
> +		log_request_parent2->access = access_request_parent2;
> +		log_request_parent2->layer_masks = layer_masks_parent2;
> +		log_request_parent2->layer_masks_size =
> +			ARRAY_SIZE(*layer_masks_parent2);
> +	}

You may want to add a function to set these fields in log_request.

>  	return allowed_parent1 && allowed_parent2;
>  }
> 
> @@ -915,6 +937,7 @@ static int current_check_access_path(const struct path
> *const path, {
>  	const struct landlock_ruleset *const dom = get_current_fs_domain();
>  	layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {};
> +	struct landlock_request request = {};
> 
>  	if (!dom)
>  		return 0;
> @@ -922,9 +945,10 @@ static int current_check_access_path(const struct path
> *const path, access_request = landlock_init_layer_masks(
>  		dom, access_request, &layer_masks, LANDLOCK_KEY_INODE);
>  	if (is_access_to_paths_allowed(dom, path, access_request, 
&layer_masks,
> -				       NULL, 0, NULL, NULL))
> +				       &request, NULL, 0, NULL, NULL, NULL))
>  		return 0;
> 
> +	landlock_log_denial(dom, &request);
>  	return -EACCES;
>  }
> 
> @@ -1093,6 +1117,7 @@ static int current_check_refer_path(struct dentry
> *const old_dentry, struct dentry *old_parent;
>  	layer_mask_t layer_masks_parent1[LANDLOCK_NUM_ACCESS_FS] = {},
>  		     layer_masks_parent2[LANDLOCK_NUM_ACCESS_FS] = {};
> +	struct landlock_request request1 = {}, request2 = {};
> 
>  	if (!dom)
>  		return 0;
> @@ -1124,10 +1149,13 @@ static int current_check_refer_path(struct dentry
> *const old_dentry, access_request_parent1 = landlock_init_layer_masks(
>  			dom, access_request_parent1 | access_request_parent2,
>  			&layer_masks_parent1, LANDLOCK_KEY_INODE);
> -		if (is_access_to_paths_allowed(
> -			    dom, new_dir, access_request_parent1,
> -			    &layer_masks_parent1, NULL, 0, NULL, NULL))
> +		if (is_access_to_paths_allowed(dom, new_dir,
> +					       access_request_parent1,
> +					       &layer_masks_parent1, &request1,
> +					       NULL, 0, NULL, NULL, NULL))
>  			return 0;
> +
> +		landlock_log_denial(dom, &request1);
>  		return -EACCES;
>  	}
> 
> @@ -1162,12 +1190,22 @@ static int current_check_refer_path(struct dentry
> *const old_dentry, * parent access rights.  This will be useful to compare
> with the * destination parent access rights.
>  	 */
> -	if (is_access_to_paths_allowed(
> -		    dom, &mnt_dir, access_request_parent1, 
&layer_masks_parent1,
> -		    old_dentry, access_request_parent2, &layer_masks_parent2,
> -		    exchange ? new_dentry : NULL))
> +	if (is_access_to_paths_allowed(dom, &mnt_dir, access_request_parent1,
> +				       &layer_masks_parent1, &request1,
> +				       old_dentry, access_request_parent2,
> +				       &layer_masks_parent2, &request2,
> +				       exchange ? new_dentry : NULL))
>  		return 0;
> 
> +	if (request1.access) {
> +		request1.audit.u.path.dentry = old_parent;
> +		landlock_log_denial(dom, &request1);
> +	}
> +	if (request2.access) {
> +		request2.audit.u.path.dentry = new_dir->dentry;
> +		landlock_log_denial(dom, &request2);
> +	}
> +
>  	/*
>  	 * This prioritizes EACCES over EXDEV for all actions, including
>  	 * renames with RENAME_EXCHANGE.
> @@ -1546,6 +1584,7 @@ static int hook_file_open(struct file *const file)
>  		optional_access;
>  	const struct landlock_ruleset *const dom = landlock_match_ruleset(
>  		landlock_cred(file->f_cred)->domain, any_fs);
> +	struct landlock_request request = {};
> 
>  	if (!dom)
>  		return 0;
> @@ -1571,7 +1610,7 @@ static int hook_file_open(struct file *const file)
>  		    dom, &file->f_path,
>  		    landlock_init_layer_masks(dom, full_access_request,
>  					      &layer_masks, LANDLOCK_KEY_INODE),
> -		    &layer_masks, NULL, 0, NULL, NULL)) {
> +		    &layer_masks, &request, NULL, 0, NULL, NULL, NULL)) {
>  		allowed_access = full_access_request;
>  	} else {
>  		unsigned long access_bit;
> @@ -1601,6 +1640,9 @@ static int hook_file_open(struct file *const file)
>  	if ((open_access_request & allowed_access) == open_access_request)
>  		return 0;
> 
> +	/* Sets access to reflect the actual request. */
> +	request.access = open_access_request;
> +	landlock_log_denial(dom, &request);
>  	return -EACCES;
>  }