[PATCH v1] landlock: Fix file reparenting without explicit LANDLOCK_ACCESS_FS_REFER

[PATCH v1] landlock: Fix file reparenting without explicit LANDLOCK_ACCESS_FS_REFER

[Mickaël Salaün]

With the introduction of LANDLOCK_ACCESS_FS_REFER, we added the first
globally denied-by-default access right.  Indeed, this lifted an initial
Landlock limitation to rename and link files, which was initially always
denied when the source or the destination were different directories.

This led to an inconsistent backward compatibility behavior which was
only taken into account if no domain layer were using the new
LANDLOCK_ACCESS_FS_REFER right.  However, in a scenario where layers are
using the first and the second Landlock ABI (i.e.
LANDLOCK_ACCESS_FS_REFER or not), the access control behaves like if all
domains were handling LANDLOCK_ACCESS_FS_REFER with their rules
implicitely allowing such right.  Indeed, the not-handled access rights
were allowed, which should not be the case for LANDLOCK_ACCESS_FS_REFER.
It should be noted that this bug only allowed safe renames or links, but
no privilege escalation because the LANDLOCK_ACCESS_FS_REFER properties
were still enforced (e.g. only allowed to move a file according to all
other access rights, and if it doesn't give more Landlock accesses).

This change adds an ACCESS_INITIALLY_DENIED list of denied-by-default
rights, which (only) contains LANDLOCK_ACCESS_FS_REFER.  All domains are
treated as if they are also handling this list, but without modifying
their fs_access_masks field, which enables correct domain audit.

A side effect is that the errno code returned by rename(2) or link(2)
*may* be changed from EXDEV to EACCES according to the enforced
restrictions.  Indeed, we now have the mechanic to identify if an access
is denied because of a required right (e.g. LANDLOCK_ACCESS_FS_MAKE_REG,
LANDLOCK_ACCESS_FS_REMOVE_FILE) or if it is denied because of missing
LANDLOCK_ACCESS_FS_REFER rights.  This may result in different errno
codes than for the initial Landlock version, but this approach is better
for rename/link compatibility reasons, and it wasn't possible before
(hence no backport to ABI v1).  The layout1.rename_file test reflects
this change.

Add the layout1.refer_denied_by_default* tests to check that the
behavior of a ruleset not handling LANDLOCK_ACCESS_FS_REFER (ABI v1) is
unchanged even if another layer handles LANDLOCK_ACCESS_FS_REFER (i.e.
ABI v1 precedence).  Make sure rule's absolute access rights are correct
by testing with and without a matching path.

Extend layout1.inval tests to check that a denied-by-default access
right is not necessarily part of a domain's handled access rights.

Fixes: b91c3e4ea756 ("landlock: Add support for file reparenting with LANDLOCK_ACCESS_FS_REFER")
Cc: Günther Noack <gnoack3000@gmail.com>
Cc: Paul Moore <paul@paul-moore.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
Link: https://lore.kernel.org/r/20220823144123.633721-1-mic@digikod.net
---
 security/landlock/fs.c                     |  28 +++--
 tools/testing/selftests/landlock/fs_test.c | 133 +++++++++++++++++++--
 2 files changed, 143 insertions(+), 18 deletions(-)

diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index ec5a6247cd3e..0cf484e89f68 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -149,6 +149,15 @@ static struct landlock_object *get_inode_object(struct inode *const inode)
 	LANDLOCK_ACCESS_FS_READ_FILE)
 /* clang-format on */
 
+/*
+ * All access rights that are denied by default whether they are handled or not
+ * by a ruleset/layer.
+ */
+/* clang-format off */
+#define ACCESS_INITIALLY_DENIED ( \
+	LANDLOCK_ACCESS_FS_REFER)
+/* clang-format on */
+
 /*
  * @path: Should have been checked by get_path_from_fd().
  */
@@ -167,7 +176,9 @@ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset,
 		return -EINVAL;
 
 	/* Transforms relative access rights to absolute ones. */
-	access_rights |= LANDLOCK_MASK_ACCESS_FS & ~ruleset->fs_access_masks[0];
+	access_rights |=
+		LANDLOCK_MASK_ACCESS_FS &
+		~(ruleset->fs_access_masks[0] | ACCESS_INITIALLY_DENIED);
 	object = get_inode_object(d_backing_inode(path->dentry));
 	if (IS_ERR(object))
 		return PTR_ERR(object);
@@ -277,7 +288,7 @@ static inline bool is_nouser_or_private(const struct dentry *dentry)
 static inline access_mask_t
 get_handled_accesses(const struct landlock_ruleset *const domain)
 {
-	access_mask_t access_dom = 0;
+	access_mask_t access_dom = ACCESS_INITIALLY_DENIED;
 	unsigned long access_bit;
 
 	for (access_bit = 0; access_bit < LANDLOCK_NUM_ACCESS_FS;
@@ -316,8 +327,13 @@ init_layer_masks(const struct landlock_ruleset *const domain,
 
 		for_each_set_bit(access_bit, &access_req,
 				 ARRAY_SIZE(*layer_masks)) {
-			if (domain->fs_access_masks[layer_level] &
-			    BIT_ULL(access_bit)) {
+			/*
+			 * Artificially handles all initially denied by default
+			 * access rights.
+			 */
+			if (BIT_ULL(access_bit) &
+			    (domain->fs_access_masks[layer_level] |
+			     ACCESS_INITIALLY_DENIED)) {
 				(*layer_masks)[access_bit] |=
 					BIT_ULL(layer_level);
 				handled_accesses |= BIT_ULL(access_bit);
@@ -857,10 +873,6 @@ static int current_check_refer_path(struct dentry *const old_dentry,
 					      NULL, NULL);
 	}
 
-	/* Backward compatibility: no reparenting support. */
-	if (!(get_handled_accesses(dom) & LANDLOCK_ACCESS_FS_REFER))
-		return -EXDEV;
-
 	access_request_parent1 |= LANDLOCK_ACCESS_FS_REFER;
 	access_request_parent2 |= LANDLOCK_ACCESS_FS_REFER;
 
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 21a2ce8fa739..2c134a9202a1 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -4,7 +4,7 @@
  *
  * Copyright © 2017-2020 Mickaël Salaün <mic@digikod.net>
  * Copyright © 2020 ANSSI
- * Copyright © 2020-2021 Microsoft Corporation
+ * Copyright © 2020-2022 Microsoft Corporation
  */
 
 #define _GNU_SOURCE
@@ -371,6 +371,13 @@ TEST_F_FORK(layout1, inval)
 	ASSERT_EQ(EINVAL, errno);
 	path_beneath.allowed_access &= ~LANDLOCK_ACCESS_FS_EXECUTE;
 
+	/* Tests with denied-by-default access right. */
+	path_beneath.allowed_access |= LANDLOCK_ACCESS_FS_REFER;
+	ASSERT_EQ(-1, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_PATH_BENEATH,
+					&path_beneath, 0));
+	ASSERT_EQ(EINVAL, errno);
+	path_beneath.allowed_access &= ~LANDLOCK_ACCESS_FS_REFER;
+
 	/* Test with unknown (64-bits) value. */
 	path_beneath.allowed_access |= (1ULL << 60);
 	ASSERT_EQ(-1, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_PATH_BENEATH,
@@ -1867,10 +1874,10 @@ TEST_F_FORK(layout1, rename_file)
 	 * to a different directory (which allows file removal).
 	 */
 	ASSERT_EQ(-1, rename(file1_s2d1, file1_s1d3));
-	ASSERT_EQ(EXDEV, errno);
+	ASSERT_EQ(EACCES, errno);
 	ASSERT_EQ(-1, renameat2(AT_FDCWD, file1_s2d1, AT_FDCWD, file1_s1d3,
 				RENAME_EXCHANGE));
-	ASSERT_EQ(EXDEV, errno);
+	ASSERT_EQ(EACCES, errno);
 	ASSERT_EQ(-1, renameat2(AT_FDCWD, dir_s2d2, AT_FDCWD, file1_s1d3,
 				RENAME_EXCHANGE));
 	ASSERT_EQ(EXDEV, errno);
@@ -1894,7 +1901,7 @@ TEST_F_FORK(layout1, rename_file)
 	ASSERT_EQ(EXDEV, errno);
 	ASSERT_EQ(0, unlink(file1_s1d3));
 	ASSERT_EQ(-1, rename(file1_s2d1, file1_s1d3));
-	ASSERT_EQ(EXDEV, errno);
+	ASSERT_EQ(EACCES, errno);
 
 	/* Exchanges and renames files with same parent. */
 	ASSERT_EQ(0, renameat2(AT_FDCWD, file2_s2d3, AT_FDCWD, file1_s2d3,
@@ -2014,6 +2021,107 @@ TEST_F_FORK(layout1, reparent_refer)
 	ASSERT_EQ(0, rename(dir_s1d3, dir_s2d3));
 }
 
+/* Checks renames beneath dir_s1d1. */
+static void refer_denied_by_default(struct __test_metadata *const _metadata,
+				    const struct rule layer1_abi_v2[],
+				    const struct rule layer2_abi_v1[])
+{
+	int ruleset_fd;
+
+	ASSERT_EQ(0, unlink(file1_s1d2));
+
+	/*
+	 * First layer, which handles LANDLOCK_ACCESS_FS_REFER, can allow some
+	 * different-parent renames and links.
+	 */
+	ruleset_fd = create_ruleset(_metadata, layer1_abi_v2[0].access,
+				    layer1_abi_v2);
+	ASSERT_LE(0, ruleset_fd);
+	enforce_ruleset(_metadata, ruleset_fd);
+	ASSERT_EQ(0, close(ruleset_fd));
+
+	/* Checks that legitimate renames are allowed. */
+	ASSERT_EQ(0, rename(file1_s1d1, file1_s1d2));
+	ASSERT_EQ(0, rename(file1_s1d2, file1_s1d1));
+	ASSERT_EQ(0, renameat2(AT_FDCWD, file2_s1d1, AT_FDCWD, file2_s1d2,
+			       RENAME_EXCHANGE));
+	ASSERT_EQ(0, renameat2(AT_FDCWD, file2_s1d2, AT_FDCWD, file2_s1d1,
+			       RENAME_EXCHANGE));
+
+	/*
+	 * Second layer, which does not handle LANDLOCK_ACCESS_FS_REFER, denies
+	 * any different-parent renames and links, thus making the first layer
+	 * null and void.
+	 */
+	ruleset_fd = create_ruleset(_metadata, layer2_abi_v1[0].access,
+				    layer2_abi_v1);
+	ASSERT_LE(0, ruleset_fd);
+	enforce_ruleset(_metadata, ruleset_fd);
+	ASSERT_EQ(0, close(ruleset_fd));
+
+	/* Checks that all renames are now denied. */
+	ASSERT_EQ(-1, rename(file1_s1d1, file1_s1d2));
+	ASSERT_EQ(EXDEV, errno);
+	ASSERT_EQ(-1, renameat2(AT_FDCWD, file2_s1d1, AT_FDCWD, file2_s1d2,
+				RENAME_EXCHANGE));
+	ASSERT_EQ(EXDEV, errno);
+	ASSERT_EQ(-1, renameat2(AT_FDCWD, file2_s1d2, AT_FDCWD, file2_s1d1,
+				RENAME_EXCHANGE));
+	ASSERT_EQ(EXDEV, errno);
+}
+
+/*
+ * Tests precedence over renames: denied by default for different parent
+ * directories, *with* a rule matching a parent directory, but not directly
+ * denying access (with MAKE_REG nor REMOVE).
+ */
+TEST_F_FORK(layout1, refer_denied_by_default1)
+{
+	const struct rule layer1_abi_v2[] = {
+		{
+			.path = dir_s1d1,
+			.access = LANDLOCK_ACCESS_FS_REFER,
+		},
+		{},
+	};
+	const struct rule layer2_abi_v1[] = {
+		{
+			/* Matches a parent directory. */
+			.path = dir_s1d1,
+			.access = LANDLOCK_ACCESS_FS_EXECUTE,
+		},
+		{},
+	};
+
+	refer_denied_by_default(_metadata, layer1_abi_v2, layer2_abi_v1);
+}
+
+/*
+ * Tests precedence over renames: denied by default for different parent
+ * directories, *without* a rule matching a parent directory, but not directly
+ * denying access (with MAKE_REG nor REMOVE).
+ */
+TEST_F_FORK(layout1, refer_denied_by_default2)
+{
+	const struct rule layer1_abi_v2[] = {
+		{
+			.path = dir_s1d1,
+			.access = LANDLOCK_ACCESS_FS_REFER,
+		},
+		{},
+	};
+	const struct rule layer2_abi_v1[] = {
+		{
+			/* Does not match a parent directory. */
+			.path = dir_s2d1,
+			.access = LANDLOCK_ACCESS_FS_EXECUTE,
+		},
+		{},
+	};
+
+	refer_denied_by_default(_metadata, layer1_abi_v2, layer2_abi_v1);
+}
+
 TEST_F_FORK(layout1, reparent_link)
 {
 	const struct rule layer1[] = {
@@ -2336,11 +2444,12 @@ TEST_F_FORK(layout1, reparent_exdev_layers_rename1)
 	ASSERT_EQ(EXDEV, errno);
 
 	/*
-	 * However, moving the file2_s1d3 file below dir_s2d3 is allowed
-	 * because it cannot inherit MAKE_REG nor MAKE_DIR rights (which are
-	 * dedicated to directories).
+	 * Moving the file2_s1d3 file below dir_s2d3 is denied because the
+	 * second layer does not handle REFER, which is always denied by
+	 * default.
 	 */
-	ASSERT_EQ(0, rename(file2_s1d3, file1_s2d3));
+	ASSERT_EQ(-1, rename(file2_s1d3, file1_s2d3));
+	ASSERT_EQ(EXDEV, errno);
 }
 
 TEST_F_FORK(layout1, reparent_exdev_layers_rename2)
@@ -2373,8 +2482,12 @@ TEST_F_FORK(layout1, reparent_exdev_layers_rename2)
 	ASSERT_EQ(EACCES, errno);
 	ASSERT_EQ(-1, rename(file1_s1d1, file1_s2d3));
 	ASSERT_EQ(EXDEV, errno);
-	/* Modify layout! */
-	ASSERT_EQ(0, rename(file2_s1d2, file1_s2d3));
+	/*
+	 * Modifying the layout is now denied because the second layer does not
+	 * handle REFER, which is always denied by default.
+	 */
+	ASSERT_EQ(-1, rename(file2_s1d2, file1_s2d3));
+	ASSERT_EQ(EXDEV, errno);
 
 	/* Without REFER source, EACCES wins over EXDEV. */
 	ASSERT_EQ(-1, rename(dir_s1d1, file1_s2d2));

base-commit: 3d7cb6b04c3f3115719235cc6866b10326de34cd
-- 
2.37.2

Re: [PATCH v1] landlock: Fix file reparenting without explicit LANDLOCK_ACCESS_FS_REFER

[Günther Noack]

On Tue, Aug 23, 2022 at 04:41:23PM +0200, Mickaël Salaün wrote:
> With the introduction of LANDLOCK_ACCESS_FS_REFER, we added the first
> globally denied-by-default access right.  Indeed, this lifted an initial
> Landlock limitation to rename and link files, which was initially always
> denied when the source or the destination were different directories.
>
> This led to an inconsistent backward compatibility behavior which was
> only taken into account if no domain layer were using the new
> LANDLOCK_ACCESS_FS_REFER right.  However, in a scenario where layers are
> using the first and the second Landlock ABI (i.e.
> LANDLOCK_ACCESS_FS_REFER or not), the access control behaves like if all
> domains were handling LANDLOCK_ACCESS_FS_REFER with their rules
> implicitely allowing such right.  Indeed, the not-handled access rights
> were allowed, which should not be the case for LANDLOCK_ACCESS_FS_REFER.
> It should be noted that this bug only allowed safe renames or links, but
> no privilege escalation because the LANDLOCK_ACCESS_FS_REFER properties
> were still enforced (e.g. only allowed to move a file according to all
> other access rights, and if it doesn't give more Landlock accesses).

I had trouble understanding this paragraph - but I tried it out and I believe
I understand what you mean; let me try to paraphrase to see whether we are on
the same page:

This is a bug where chaining an additional ruleset on top of another one can
increase the number of possible operations, rather than just reduce it.

Steps to reproduce:
 - Enforce ruleset 1 *without* handled_access_fs containing "refer"
 - Enforce ruleset 2 with handled_access_fs containing "refer"

Expected behaviour:
 - refer-operations (rename, link...) should not be permitted because
   ruleset 1 uses ABI v1 and refer-operations are always forbidden there.

Observed behaviour:
 - refer-operations (rename, link...) work as if LANDLOCK_ACCESS_FS_REFER had
   been handled in both rulesets and all rules within

Do I understand this correctly?


I believe I can reproduce it with the Go sandboxing tool (go install
github.com/landlock-lsm/go-landlock/cmd/landlock-restrict@latest) like this:

Starting with these directory contents:

  $ find .
  .
  ./dst
  ./src
  ./src/hello

Then it is not possible to use refer (linking) with an ABIv1 ruleset (expected):

  $ $HOME/go/bin/landlock-restrict -1 -ro / -rw . -- \
    /bin/ln src/hello dst/hello
  /bin/ln: failed to create hard link 'dst/hello' => 'src/hello': Invalid cross-device link

But when chaining that ruleset with a ABIv2 ruleset, it suddenly does become
possible to do "refer" operations:

  $ $HOME/go/bin/landlock-restrict -1 -ro / -rw . -- \
    $HOME/go/bin/landlock-restrict -2 -ro / -rw +refer . -- \
    /bin/ln src/hello dst/hello
  $ find .
  .
  ./dst
  ./dst/hello
  ./src
  ./src/hello

I need to still understand the underlying code better to reason about it,
but this is the issue that this patch is fixing, right?

—Günther

> This change adds an ACCESS_INITIALLY_DENIED list of denied-by-default
> rights, which (only) contains LANDLOCK_ACCESS_FS_REFER.  All domains are
> treated as if they are also handling this list, but without modifying
> their fs_access_masks field, which enables correct domain audit.
>
> A side effect is that the errno code returned by rename(2) or link(2)
> *may* be changed from EXDEV to EACCES according to the enforced
> restrictions.  Indeed, we now have the mechanic to identify if an access
> is denied because of a required right (e.g. LANDLOCK_ACCESS_FS_MAKE_REG,
> LANDLOCK_ACCESS_FS_REMOVE_FILE) or if it is denied because of missing
> LANDLOCK_ACCESS_FS_REFER rights.  This may result in different errno
> codes than for the initial Landlock version, but this approach is better
> for rename/link compatibility reasons, and it wasn't possible before
> (hence no backport to ABI v1).  The layout1.rename_file test reflects
> this change.
>
> Add the layout1.refer_denied_by_default* tests to check that the
> behavior of a ruleset not handling LANDLOCK_ACCESS_FS_REFER (ABI v1) is
> unchanged even if another layer handles LANDLOCK_ACCESS_FS_REFER (i.e.
> ABI v1 precedence).  Make sure rule's absolute access rights are correct
> by testing with and without a matching path.
>
> Extend layout1.inval tests to check that a denied-by-default access
> right is not necessarily part of a domain's handled access rights.
>
> Fixes: b91c3e4ea756 ("landlock: Add support for file reparenting with LANDLOCK_ACCESS_FS_REFER")
> Cc: Günther Noack <gnoack3000@gmail.com>
> Cc: Paul Moore <paul@paul-moore.com>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> Link: https://lore.kernel.org/r/20220823144123.633721-1-mic@digikod.net
> ---
>  security/landlock/fs.c                     |  28 +++--
>  tools/testing/selftests/landlock/fs_test.c | 133 +++++++++++++++++++--
>  2 files changed, 143 insertions(+), 18 deletions(-)
>
> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index ec5a6247cd3e..0cf484e89f68 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
> @@ -149,6 +149,15 @@ static struct landlock_object *get_inode_object(struct inode *const inode)
>  	LANDLOCK_ACCESS_FS_READ_FILE)
>  /* clang-format on */
>
> +/*
> + * All access rights that are denied by default whether they are handled or not
> + * by a ruleset/layer.
> + */
> +/* clang-format off */
> +#define ACCESS_INITIALLY_DENIED ( \
> +	LANDLOCK_ACCESS_FS_REFER)
> +/* clang-format on */
> +
>  /*
>   * @path: Should have been checked by get_path_from_fd().
>   */
> @@ -167,7 +176,9 @@ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset,
>  		return -EINVAL;
>
>  	/* Transforms relative access rights to absolute ones. */
> -	access_rights |= LANDLOCK_MASK_ACCESS_FS & ~ruleset->fs_access_masks[0];
> +	access_rights |=
> +		LANDLOCK_MASK_ACCESS_FS &
> +		~(ruleset->fs_access_masks[0] | ACCESS_INITIALLY_DENIED);
>  	object = get_inode_object(d_backing_inode(path->dentry));
>  	if (IS_ERR(object))
>  		return PTR_ERR(object);
> @@ -277,7 +288,7 @@ static inline bool is_nouser_or_private(const struct dentry *dentry)
>  static inline access_mask_t
>  get_handled_accesses(const struct landlock_ruleset *const domain)
>  {
> -	access_mask_t access_dom = 0;
> +	access_mask_t access_dom = ACCESS_INITIALLY_DENIED;
>  	unsigned long access_bit;
>
>  	for (access_bit = 0; access_bit < LANDLOCK_NUM_ACCESS_FS;
> @@ -316,8 +327,13 @@ init_layer_masks(const struct landlock_ruleset *const domain,
>
>  		for_each_set_bit(access_bit, &access_req,
>  				 ARRAY_SIZE(*layer_masks)) {
> -			if (domain->fs_access_masks[layer_level] &
> -			    BIT_ULL(access_bit)) {
> +			/*
> +			 * Artificially handles all initially denied by default
> +			 * access rights.
> +			 */
> +			if (BIT_ULL(access_bit) &
> +			    (domain->fs_access_masks[layer_level] |
> +			     ACCESS_INITIALLY_DENIED)) {
>  				(*layer_masks)[access_bit] |=
>  					BIT_ULL(layer_level);
>  				handled_accesses |= BIT_ULL(access_bit);
> @@ -857,10 +873,6 @@ static int current_check_refer_path(struct dentry *const old_dentry,
>  					      NULL, NULL);
>  	}
>
> -	/* Backward compatibility: no reparenting support. */
> -	if (!(get_handled_accesses(dom) & LANDLOCK_ACCESS_FS_REFER))
> -		return -EXDEV;
> -
>  	access_request_parent1 |= LANDLOCK_ACCESS_FS_REFER;
>  	access_request_parent2 |= LANDLOCK_ACCESS_FS_REFER;
>
> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> index 21a2ce8fa739..2c134a9202a1 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c
> @@ -4,7 +4,7 @@
>   *
>   * Copyright © 2017-2020 Mickaël Salaün <mic@digikod.net>
>   * Copyright © 2020 ANSSI
> - * Copyright © 2020-2021 Microsoft Corporation
> + * Copyright © 2020-2022 Microsoft Corporation
>   */
>
>  #define _GNU_SOURCE
> @@ -371,6 +371,13 @@ TEST_F_FORK(layout1, inval)
>  	ASSERT_EQ(EINVAL, errno);
>  	path_beneath.allowed_access &= ~LANDLOCK_ACCESS_FS_EXECUTE;
>
> +	/* Tests with denied-by-default access right. */
> +	path_beneath.allowed_access |= LANDLOCK_ACCESS_FS_REFER;
> +	ASSERT_EQ(-1, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_PATH_BENEATH,
> +					&path_beneath, 0));
> +	ASSERT_EQ(EINVAL, errno);
> +	path_beneath.allowed_access &= ~LANDLOCK_ACCESS_FS_REFER;
> +
>  	/* Test with unknown (64-bits) value. */
>  	path_beneath.allowed_access |= (1ULL << 60);
>  	ASSERT_EQ(-1, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_PATH_BENEATH,
> @@ -1867,10 +1874,10 @@ TEST_F_FORK(layout1, rename_file)
>  	 * to a different directory (which allows file removal).
>  	 */
>  	ASSERT_EQ(-1, rename(file1_s2d1, file1_s1d3));
> -	ASSERT_EQ(EXDEV, errno);
> +	ASSERT_EQ(EACCES, errno);
>  	ASSERT_EQ(-1, renameat2(AT_FDCWD, file1_s2d1, AT_FDCWD, file1_s1d3,
>  				RENAME_EXCHANGE));
> -	ASSERT_EQ(EXDEV, errno);
> +	ASSERT_EQ(EACCES, errno);
>  	ASSERT_EQ(-1, renameat2(AT_FDCWD, dir_s2d2, AT_FDCWD, file1_s1d3,
>  				RENAME_EXCHANGE));
>  	ASSERT_EQ(EXDEV, errno);
> @@ -1894,7 +1901,7 @@ TEST_F_FORK(layout1, rename_file)
>  	ASSERT_EQ(EXDEV, errno);
>  	ASSERT_EQ(0, unlink(file1_s1d3));
>  	ASSERT_EQ(-1, rename(file1_s2d1, file1_s1d3));
> -	ASSERT_EQ(EXDEV, errno);
> +	ASSERT_EQ(EACCES, errno);
>
>  	/* Exchanges and renames files with same parent. */
>  	ASSERT_EQ(0, renameat2(AT_FDCWD, file2_s2d3, AT_FDCWD, file1_s2d3,
> @@ -2014,6 +2021,107 @@ TEST_F_FORK(layout1, reparent_refer)
>  	ASSERT_EQ(0, rename(dir_s1d3, dir_s2d3));
>  }
>
> +/* Checks renames beneath dir_s1d1. */
> +static void refer_denied_by_default(struct __test_metadata *const _metadata,
> +				    const struct rule layer1_abi_v2[],
> +				    const struct rule layer2_abi_v1[])
> +{
> +	int ruleset_fd;
> +
> +	ASSERT_EQ(0, unlink(file1_s1d2));
> +
> +	/*
> +	 * First layer, which handles LANDLOCK_ACCESS_FS_REFER, can allow some
> +	 * different-parent renames and links.
> +	 */
> +	ruleset_fd = create_ruleset(_metadata, layer1_abi_v2[0].access,
> +				    layer1_abi_v2);
> +	ASSERT_LE(0, ruleset_fd);
> +	enforce_ruleset(_metadata, ruleset_fd);
> +	ASSERT_EQ(0, close(ruleset_fd));
> +
> +	/* Checks that legitimate renames are allowed. */
> +	ASSERT_EQ(0, rename(file1_s1d1, file1_s1d2));
> +	ASSERT_EQ(0, rename(file1_s1d2, file1_s1d1));
> +	ASSERT_EQ(0, renameat2(AT_FDCWD, file2_s1d1, AT_FDCWD, file2_s1d2,
> +			       RENAME_EXCHANGE));
> +	ASSERT_EQ(0, renameat2(AT_FDCWD, file2_s1d2, AT_FDCWD, file2_s1d1,
> +			       RENAME_EXCHANGE));
> +
> +	/*
> +	 * Second layer, which does not handle LANDLOCK_ACCESS_FS_REFER, denies
> +	 * any different-parent renames and links, thus making the first layer
> +	 * null and void.
> +	 */
> +	ruleset_fd = create_ruleset(_metadata, layer2_abi_v1[0].access,
> +				    layer2_abi_v1);
> +	ASSERT_LE(0, ruleset_fd);
> +	enforce_ruleset(_metadata, ruleset_fd);
> +	ASSERT_EQ(0, close(ruleset_fd));
> +
> +	/* Checks that all renames are now denied. */
> +	ASSERT_EQ(-1, rename(file1_s1d1, file1_s1d2));
> +	ASSERT_EQ(EXDEV, errno);
> +	ASSERT_EQ(-1, renameat2(AT_FDCWD, file2_s1d1, AT_FDCWD, file2_s1d2,
> +				RENAME_EXCHANGE));
> +	ASSERT_EQ(EXDEV, errno);
> +	ASSERT_EQ(-1, renameat2(AT_FDCWD, file2_s1d2, AT_FDCWD, file2_s1d1,
> +				RENAME_EXCHANGE));
> +	ASSERT_EQ(EXDEV, errno);
> +}
> +
> +/*
> + * Tests precedence over renames: denied by default for different parent
> + * directories, *with* a rule matching a parent directory, but not directly
> + * denying access (with MAKE_REG nor REMOVE).
> + */
> +TEST_F_FORK(layout1, refer_denied_by_default1)
> +{
> +	const struct rule layer1_abi_v2[] = {
> +		{
> +			.path = dir_s1d1,
> +			.access = LANDLOCK_ACCESS_FS_REFER,
> +		},
> +		{},
> +	};
> +	const struct rule layer2_abi_v1[] = {
> +		{
> +			/* Matches a parent directory. */
> +			.path = dir_s1d1,
> +			.access = LANDLOCK_ACCESS_FS_EXECUTE,
> +		},
> +		{},
> +	};
> +
> +	refer_denied_by_default(_metadata, layer1_abi_v2, layer2_abi_v1);
> +}
> +
> +/*
> + * Tests precedence over renames: denied by default for different parent
> + * directories, *without* a rule matching a parent directory, but not directly
> + * denying access (with MAKE_REG nor REMOVE).
> + */
> +TEST_F_FORK(layout1, refer_denied_by_default2)
> +{
> +	const struct rule layer1_abi_v2[] = {
> +		{
> +			.path = dir_s1d1,
> +			.access = LANDLOCK_ACCESS_FS_REFER,
> +		},
> +		{},
> +	};
> +	const struct rule layer2_abi_v1[] = {
> +		{
> +			/* Does not match a parent directory. */
> +			.path = dir_s2d1,
> +			.access = LANDLOCK_ACCESS_FS_EXECUTE,
> +		},
> +		{},
> +	};
> +
> +	refer_denied_by_default(_metadata, layer1_abi_v2, layer2_abi_v1);
> +}
> +
>  TEST_F_FORK(layout1, reparent_link)
>  {
>  	const struct rule layer1[] = {
> @@ -2336,11 +2444,12 @@ TEST_F_FORK(layout1, reparent_exdev_layers_rename1)
>  	ASSERT_EQ(EXDEV, errno);
>
>  	/*
> -	 * However, moving the file2_s1d3 file below dir_s2d3 is allowed
> -	 * because it cannot inherit MAKE_REG nor MAKE_DIR rights (which are
> -	 * dedicated to directories).
> +	 * Moving the file2_s1d3 file below dir_s2d3 is denied because the
> +	 * second layer does not handle REFER, which is always denied by
> +	 * default.
>  	 */
> -	ASSERT_EQ(0, rename(file2_s1d3, file1_s2d3));
> +	ASSERT_EQ(-1, rename(file2_s1d3, file1_s2d3));
> +	ASSERT_EQ(EXDEV, errno);
>  }
>
>  TEST_F_FORK(layout1, reparent_exdev_layers_rename2)
> @@ -2373,8 +2482,12 @@ TEST_F_FORK(layout1, reparent_exdev_layers_rename2)
>  	ASSERT_EQ(EACCES, errno);
>  	ASSERT_EQ(-1, rename(file1_s1d1, file1_s2d3));
>  	ASSERT_EQ(EXDEV, errno);
> -	/* Modify layout! */
> -	ASSERT_EQ(0, rename(file2_s1d2, file1_s2d3));
> +	/*
> +	 * Modifying the layout is now denied because the second layer does not
> +	 * handle REFER, which is always denied by default.
> +	 */
> +	ASSERT_EQ(-1, rename(file2_s1d2, file1_s2d3));
> +	ASSERT_EQ(EXDEV, errno);
>
>  	/* Without REFER source, EACCES wins over EXDEV. */
>  	ASSERT_EQ(-1, rename(dir_s1d1, file1_s2d2));
>
> base-commit: 3d7cb6b04c3f3115719235cc6866b10326de34cd
> --
> 2.37.2
>

--

Re: [PATCH v1] landlock: Fix file reparenting without explicit LANDLOCK_ACCESS_FS_REFER

[Mickaël Salaün]

On 23/08/2022 22:07, Günther Noack wrote:
> On Tue, Aug 23, 2022 at 04:41:23PM +0200, Mickaël Salaün wrote:
>> With the introduction of LANDLOCK_ACCESS_FS_REFER, we added the first
>> globally denied-by-default access right.  Indeed, this lifted an initial
>> Landlock limitation to rename and link files, which was initially always
>> denied when the source or the destination were different directories.
>>
>> This led to an inconsistent backward compatibility behavior which was
>> only taken into account if no domain layer were using the new
>> LANDLOCK_ACCESS_FS_REFER right.  However, in a scenario where layers are
>> using the first and the second Landlock ABI (i.e.
>> LANDLOCK_ACCESS_FS_REFER or not), the access control behaves like if all
>> domains were handling LANDLOCK_ACCESS_FS_REFER with their rules
>> implicitely allowing such right.

"the access control behaves like if domains not handling 
LANDLOCK_ACCESS_FS_REFER are in fact handling it and with their rules 
implicitely allowing such right."

Is this better?


>>  Indeed, the not-handled access rights
>> were allowed, which should not be the case for LANDLOCK_ACCESS_FS_REFER.
>> It should be noted that this bug only allowed safe renames or links, but
>> no privilege escalation because the LANDLOCK_ACCESS_FS_REFER properties
>> were still enforced (e.g. only allowed to move a file according to all
>> other access rights, and if it doesn't give more Landlock accesses).
> 
> I had trouble understanding this paragraph - but I tried it out and I believe
> I understand what you mean; let me try to paraphrase to see whether we are on
> the same page:
> 
> This is a bug where chaining an additional ruleset on top of another one can
> increase the number of possible operations, rather than just reduce it.
> 
> Steps to reproduce:
>   - Enforce ruleset 1 *without* handled_access_fs containing "refer"
>   - Enforce ruleset 2 with handled_access_fs containing "refer"
> 
> Expected behaviour:
>   - refer-operations (rename, link...) should not be permitted because
>     ruleset 1 uses ABI v1 and refer-operations are always forbidden there.
> 
> Observed behaviour:
>   - refer-operations (rename, link...) work as if LANDLOCK_ACCESS_FS_REFER had
>     been handled in both rulesets and all rules within

"All rules within the ruleset/layer not (theoritically) handling refer 
operations". Layers handling refer-operations behave as expected and are 
not affected by this bug.

> 
> Do I understand this correctly?

Yes, except for the rules correctly handling refer operations.


> 
> 
> I believe I can reproduce it with the Go sandboxing tool (go install
> github.com/landlock-lsm/go-landlock/cmd/landlock-restrict@latest) like this:
> 
> Starting with these directory contents:
> 
>    $ find .
>    .
>    ./dst
>    ./src
>    ./src/hello
> 
> Then it is not possible to use refer (linking) with an ABIv1 ruleset (expected):
> 
>    $ $HOME/go/bin/landlock-restrict -1 -ro / -rw . -- \
>      /bin/ln src/hello dst/hello
>    /bin/ln: failed to create hard link 'dst/hello' => 'src/hello': Invalid cross-device link
> 
> But when chaining that ruleset with a ABIv2 ruleset, it suddenly does become
> possible to do "refer" operations:
> 
>    $ $HOME/go/bin/landlock-restrict -1 -ro / -rw . -- \
>      $HOME/go/bin/landlock-restrict -2 -ro / -rw +refer . -- \
>      /bin/ln src/hello dst/hello
>    $ find .
>    .
>    ./dst
>    ./dst/hello
>    ./src
>    ./src/hello

This is correct. In this case, the first landlock-restrict command 
behaves as the second command when the second is executed (i.e. a new 
refer-aware layer is added).


> 
> I need to still understand the underlying code better to reason about it,
> but this is the issue that this patch is fixing, right?
> 
> —Günther
> 
>> This change adds an ACCESS_INITIALLY_DENIED list of denied-by-default
>> rights, which (only) contains LANDLOCK_ACCESS_FS_REFER.  All domains are
>> treated as if they are also handling this list, but without modifying
>> their fs_access_masks field, which enables correct domain audit.
>>
>> A side effect is that the errno code returned by rename(2) or link(2)
>> *may* be changed from EXDEV to EACCES according to the enforced
>> restrictions.  Indeed, we now have the mechanic to identify if an access
>> is denied because of a required right (e.g. LANDLOCK_ACCESS_FS_MAKE_REG,
>> LANDLOCK_ACCESS_FS_REMOVE_FILE) or if it is denied because of missing
>> LANDLOCK_ACCESS_FS_REFER rights.  This may result in different errno
>> codes than for the initial Landlock version, but this approach is better
>> for rename/link compatibility reasons, and it wasn't possible before
>> (hence no backport to ABI v1).  The layout1.rename_file test reflects
>> this change.
>>
>> Add the layout1.refer_denied_by_default* tests to check that the
>> behavior of a ruleset not handling LANDLOCK_ACCESS_FS_REFER (ABI v1) is
>> unchanged even if another layer handles LANDLOCK_ACCESS_FS_REFER (i.e.
>> ABI v1 precedence).  Make sure rule's absolute access rights are correct
>> by testing with and without a matching path.
>>
>> Extend layout1.inval tests to check that a denied-by-default access
>> right is not necessarily part of a domain's handled access rights.
>>
>> Fixes: b91c3e4ea756 ("landlock: Add support for file reparenting with LANDLOCK_ACCESS_FS_REFER")
>> Cc: Günther Noack <gnoack3000@gmail.com>
>> Cc: Paul Moore <paul@paul-moore.com>
>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
>> Link: https://lore.kernel.org/r/20220823144123.633721-1-mic@digikod.net

Re: [PATCH v1] landlock: Fix file reparenting without explicit LANDLOCK_ACCESS_FS_REFER

[Paul Moore]

On Wed, Aug 24, 2022 at 5:04 AM Mickaël Salaün <mic@digikod.net> wrote:
> On 23/08/2022 22:07, Günther Noack wrote:
> > On Tue, Aug 23, 2022 at 04:41:23PM +0200, Mickaël Salaün wrote:
> >> With the introduction of LANDLOCK_ACCESS_FS_REFER, we added the first
> >> globally denied-by-default access right.  Indeed, this lifted an initial
> >> Landlock limitation to rename and link files, which was initially always
> >> denied when the source or the destination were different directories.
> >>
> >> This led to an inconsistent backward compatibility behavior which was
> >> only taken into account if no domain layer were using the new
> >> LANDLOCK_ACCESS_FS_REFER right.  However, in a scenario where layers are
> >> using the first and the second Landlock ABI (i.e.
> >> LANDLOCK_ACCESS_FS_REFER or not), the access control behaves like if all
> >> domains were handling LANDLOCK_ACCESS_FS_REFER with their rules
> >> implicitely allowing such right.
>
> "the access control behaves like if domains not handling
> LANDLOCK_ACCESS_FS_REFER are in fact handling it and with their rules
> implicitely allowing such right."
>
> Is this better?

I'm still looking at the actual code changes, but I had similar
problems as Günther while I was reading the description.  While the
new text above is different, I'm not sure it is significantly easier
to understand.  I might suggest adding a short example to the commit
description showing what happens now and what will change with this
patch; similar to what Günther did in his reply.

-- 
paul-moore.com

Re: [PATCH v1] landlock: Fix file reparenting without explicit LANDLOCK_ACCESS_FS_REFER

[Paul Moore]

On Tue, Aug 23, 2022 at 10:41 AM Mickaël Salaün <mic@digikod.net> wrote:
>
> With the introduction of LANDLOCK_ACCESS_FS_REFER, we added the first
> globally denied-by-default access right.  Indeed, this lifted an initial
> Landlock limitation to rename and link files, which was initially always
> denied when the source or the destination were different directories.
>
> This led to an inconsistent backward compatibility behavior which was
> only taken into account if no domain layer were using the new
> LANDLOCK_ACCESS_FS_REFER right.  However, in a scenario where layers are
> using the first and the second Landlock ABI (i.e.
> LANDLOCK_ACCESS_FS_REFER or not), the access control behaves like if all
> domains were handling LANDLOCK_ACCESS_FS_REFER with their rules
> implicitely allowing such right.  Indeed, the not-handled access rights
> were allowed, which should not be the case for LANDLOCK_ACCESS_FS_REFER.
> It should be noted that this bug only allowed safe renames or links, but
> no privilege escalation because the LANDLOCK_ACCESS_FS_REFER properties
> were still enforced (e.g. only allowed to move a file according to all
> other access rights, and if it doesn't give more Landlock accesses).
>
> This change adds an ACCESS_INITIALLY_DENIED list of denied-by-default
> rights, which (only) contains LANDLOCK_ACCESS_FS_REFER.  All domains are
> treated as if they are also handling this list, but without modifying
> their fs_access_masks field, which enables correct domain audit.
>
> A side effect is that the errno code returned by rename(2) or link(2)
> *may* be changed from EXDEV to EACCES according to the enforced
> restrictions.  Indeed, we now have the mechanic to identify if an access
> is denied because of a required right (e.g. LANDLOCK_ACCESS_FS_MAKE_REG,
> LANDLOCK_ACCESS_FS_REMOVE_FILE) or if it is denied because of missing
> LANDLOCK_ACCESS_FS_REFER rights.  This may result in different errno
> codes than for the initial Landlock version, but this approach is better
> for rename/link compatibility reasons, and it wasn't possible before
> (hence no backport to ABI v1).  The layout1.rename_file test reflects
> this change.
>
> Add the layout1.refer_denied_by_default* tests to check that the
> behavior of a ruleset not handling LANDLOCK_ACCESS_FS_REFER (ABI v1) is
> unchanged even if another layer handles LANDLOCK_ACCESS_FS_REFER (i.e.
> ABI v1 precedence).  Make sure rule's absolute access rights are correct
> by testing with and without a matching path.
>
> Extend layout1.inval tests to check that a denied-by-default access
> right is not necessarily part of a domain's handled access rights.
>
> Fixes: b91c3e4ea756 ("landlock: Add support for file reparenting with LANDLOCK_ACCESS_FS_REFER")
> Cc: Günther Noack <gnoack3000@gmail.com>
> Cc: Paul Moore <paul@paul-moore.com>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> Link: https://lore.kernel.org/r/20220823144123.633721-1-mic@digikod.net
> ---
>  security/landlock/fs.c                     |  28 +++--
>  tools/testing/selftests/landlock/fs_test.c | 133 +++++++++++++++++++--
>  2 files changed, 143 insertions(+), 18 deletions(-)

I'm still working on getting more confident of my Landlock
understanding, but the kernel code looks sane to me and I like how
this removes the compatibility hack in current_check_refer_path().

Reviewed-by: Paul Moore <paul@paul-moore.com>

-- 
paul-moore.com

Re: [PATCH v1] landlock: Fix file reparenting without explicit LANDLOCK_ACCESS_FS_REFER

[Mickaël Salaün]

On 25/08/2022 22:16, Paul Moore wrote:
> On Wed, Aug 24, 2022 at 5:04 AM Mickaël Salaün <mic@digikod.net> wrote:
>> On 23/08/2022 22:07, Günther Noack wrote:
>>> On Tue, Aug 23, 2022 at 04:41:23PM +0200, Mickaël Salaün wrote:
>>>> With the introduction of LANDLOCK_ACCESS_FS_REFER, we added the first
>>>> globally denied-by-default access right.  Indeed, this lifted an initial
>>>> Landlock limitation to rename and link files, which was initially always
>>>> denied when the source or the destination were different directories.
>>>>
>>>> This led to an inconsistent backward compatibility behavior which was
>>>> only taken into account if no domain layer were using the new
>>>> LANDLOCK_ACCESS_FS_REFER right.  However, in a scenario where layers are
>>>> using the first and the second Landlock ABI (i.e.
>>>> LANDLOCK_ACCESS_FS_REFER or not), the access control behaves like if all
>>>> domains were handling LANDLOCK_ACCESS_FS_REFER with their rules
>>>> implicitely allowing such right.
>>
>> "the access control behaves like if domains not handling
>> LANDLOCK_ACCESS_FS_REFER are in fact handling it and with their rules
>> implicitely allowing such right."
>>
>> Is this better?
> 
> I'm still looking at the actual code changes, but I had similar
> problems as Günther while I was reading the description.  While the
> new text above is different, I'm not sure it is significantly easier
> to understand.  I might suggest adding a short example to the commit
> description showing what happens now and what will change with this
> patch; similar to what Günther did in his reply.

OK, what about these new paragraphs replacing the second one?

This led to an inconsistent backward compatibility behavior which was
only taken into account if no domain layer were using the new
LANDLOCK_ACCESS_FS_REFER right. However, when restricting a thread with 
a new ruleset handling LANDLOCK_ACCESS_FS_REFER, all inherited parent 
ruleset/layers not explicitly handling LANDLOCK_ACCESS_FS_REFER would 
behave as if they were handling this access right and with all their 
rules allowing it. This means that renaming and linking files could 
became allowed by these parent layers, but all the other required 
accesses must also be granted: all layers must allow file removal or 
creation, and renaming and linking operations must not lead to privilege 
escalation according to the Landlock policy (see detailed explanation in 
commit b91c3e4ea756).

To say it another way, this bug may lifts the renaming and linking 
limitations of the initial Landlock version, and a same ruleset can 
restrict differently depending on previous or next enforced ruleset 
(i.e. inconsistent behavior). The LANDLOCK_ACCESS_FS_REFER right cannot 
give access to data not already allowed, but this doesn't follow the 
contract of the first Landlock ABI. This fix puts back the limitation 
for sandboxes that didn't opt-in for this additional right.

For instance, if a first ruleset allows LANDLOCK_ACCESS_FS_MAKE_REG on 
/dst and LANDLOCK_ACCESS_FS_REMOVE_FILE on /src, renaming /src/file to 
/dst/file is denied. However, without this fix, stacking a new ruleset 
which allows LANDLOCK_ACCESS_FS_REFER on / would now permit the 
sandboxed thread to rename /src/file to /dst/file .

Re: [PATCH v1] landlock: Fix file reparenting without explicit LANDLOCK_ACCESS_FS_REFER

[Günther Noack]

On Tue, Aug 23, 2022 at 04:41:23PM +0200, Mickaël Salaün wrote:
> With the introduction of LANDLOCK_ACCESS_FS_REFER, we added the first
> globally denied-by-default access right.  Indeed, this lifted an initial
> Landlock limitation to rename and link files, which was initially always
> denied when the source or the destination were different directories.
>
> This led to an inconsistent backward compatibility behavior which was
> only taken into account if no domain layer were using the new
> LANDLOCK_ACCESS_FS_REFER right.  However, in a scenario where layers are
> using the first and the second Landlock ABI (i.e.
> LANDLOCK_ACCESS_FS_REFER or not), the access control behaves like if all
> domains were handling LANDLOCK_ACCESS_FS_REFER with their rules
> implicitely allowing such right.  Indeed, the not-handled access rights
> were allowed, which should not be the case for LANDLOCK_ACCESS_FS_REFER.
> It should be noted that this bug only allowed safe renames or links, but
> no privilege escalation because the LANDLOCK_ACCESS_FS_REFER properties
> were still enforced (e.g. only allowed to move a file according to all
> other access rights, and if it doesn't give more Landlock accesses).
>
> This change adds an ACCESS_INITIALLY_DENIED list of denied-by-default
> rights, which (only) contains LANDLOCK_ACCESS_FS_REFER.  All domains are
> treated as if they are also handling this list, but without modifying
> their fs_access_masks field, which enables correct domain audit.
>
> A side effect is that the errno code returned by rename(2) or link(2)
> *may* be changed from EXDEV to EACCES according to the enforced
> restrictions.  Indeed, we now have the mechanic to identify if an access
> is denied because of a required right (e.g. LANDLOCK_ACCESS_FS_MAKE_REG,
> LANDLOCK_ACCESS_FS_REMOVE_FILE) or if it is denied because of missing
> LANDLOCK_ACCESS_FS_REFER rights.  This may result in different errno
> codes than for the initial Landlock version, but this approach is better
> for rename/link compatibility reasons, and it wasn't possible before
> (hence no backport to ABI v1).  The layout1.rename_file test reflects
> this change.
>
> Add the layout1.refer_denied_by_default* tests to check that the
> behavior of a ruleset not handling LANDLOCK_ACCESS_FS_REFER (ABI v1) is
> unchanged even if another layer handles LANDLOCK_ACCESS_FS_REFER (i.e.
> ABI v1 precedence).  Make sure rule's absolute access rights are correct
> by testing with and without a matching path.
>
> Extend layout1.inval tests to check that a denied-by-default access
> right is not necessarily part of a domain's handled access rights.
>
> Fixes: b91c3e4ea756 ("landlock: Add support for file reparenting with LANDLOCK_ACCESS_FS_REFER")
> Cc: Günther Noack <gnoack3000@gmail.com>
> Cc: Paul Moore <paul@paul-moore.com>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> Link: https://lore.kernel.org/r/20220823144123.633721-1-mic@digikod.net
> ---
>  security/landlock/fs.c                     |  28 +++--
>  tools/testing/selftests/landlock/fs_test.c | 133 +++++++++++++++++++--
>  2 files changed, 143 insertions(+), 18 deletions(-)
>
> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index ec5a6247cd3e..0cf484e89f68 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
> @@ -149,6 +149,15 @@ static struct landlock_object *get_inode_object(struct inode *const inode)
>  	LANDLOCK_ACCESS_FS_READ_FILE)
>  /* clang-format on */
>
> +/*
> + * All access rights that are denied by default whether they are handled or not
> + * by a ruleset/layer.
> + */
> +/* clang-format off */
> +#define ACCESS_INITIALLY_DENIED ( \
> +	LANDLOCK_ACCESS_FS_REFER)
> +/* clang-format on */
> +
>  /*
>   * @path: Should have been checked by get_path_from_fd().
>   */
> @@ -167,7 +176,9 @@ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset,
>  		return -EINVAL;
>
>  	/* Transforms relative access rights to absolute ones. */
> -	access_rights |= LANDLOCK_MASK_ACCESS_FS & ~ruleset->fs_access_masks[0];
> +	access_rights |=
> +		LANDLOCK_MASK_ACCESS_FS &
> +		~(ruleset->fs_access_masks[0] | ACCESS_INITIALLY_DENIED);
>
>  	object = get_inode_object(d_backing_inode(path->dentry));
>  	if (IS_ERR(object))
>  		return PTR_ERR(object);
> @@ -277,7 +288,7 @@ static inline bool is_nouser_or_private(const struct dentry *dentry)
>  static inline access_mask_t
>  get_handled_accesses(const struct landlock_ruleset *const domain)
>  {
> -	access_mask_t access_dom = 0;
> +	access_mask_t access_dom = ACCESS_INITIALLY_DENIED;
>  	unsigned long access_bit;
>
>  	for (access_bit = 0; access_bit < LANDLOCK_NUM_ACCESS_FS;
> @@ -316,8 +327,13 @@ init_layer_masks(const struct landlock_ruleset *const domain,
>
>  		for_each_set_bit(access_bit, &access_req,
>  				 ARRAY_SIZE(*layer_masks)) {
> -			if (domain->fs_access_masks[layer_level] &
> -			    BIT_ULL(access_bit)) {
> +			/*
> +			 * Artificially handles all initially denied by default
> +			 * access rights.
> +			 */
> +			if (BIT_ULL(access_bit) &
> +			    (domain->fs_access_masks[layer_level] |
> +			     ACCESS_INITIALLY_DENIED)) {
>  				(*layer_masks)[access_bit] |=
>  					BIT_ULL(layer_level);
>  				handled_accesses |= BIT_ULL(access_bit);
> @@ -857,10 +873,6 @@ static int current_check_refer_path(struct dentry *const old_dentry,
>  					      NULL, NULL);
>  	}
>
> -	/* Backward compatibility: no reparenting support. */
> -	if (!(get_handled_accesses(dom) & LANDLOCK_ACCESS_FS_REFER))
> -		return -EXDEV;
> -
>  	access_request_parent1 |= LANDLOCK_ACCESS_FS_REFER;
>  	access_request_parent2 |= LANDLOCK_ACCESS_FS_REFER;
>

In this change, you are adding "| ACCESS_INITIALLY_DENIED" to a bunch
of places in fs.c where a domain->fs_access_masks[...] is read.

In the documentation for the refer right, it is described with:

  This is also the only access right which is always considered
  handled by any ruleset in such a way that reparenting a file
  hierarchy is always denied by default.

Naive question: Is anything keeping us from also implementing it in
that way? We could pretend early during ruleset creation that the
caller had asked to also handle the "refer" right. Then we would just
have the "refer" bit set on every domain->fs_access_masks[...] entry.

Wouldn't that do the same, without further complicating the access
check logic in fs.c?

FWIW, one thing that would happen in this approach would be that users
could add rules asking to grant the "refer" right on files, on
rulesets where they have not previously declared it as a "handled"
right. It's debatable whether that is good API design, but it would
not do much damage either, and could also be fixed by keeping a copy
of the originally requested handled_access_fs.

—Günther

> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> index 21a2ce8fa739..2c134a9202a1 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c
> @@ -4,7 +4,7 @@
>   *
>   * Copyright © 2017-2020 Mickaël Salaün <mic@digikod.net>
>   * Copyright © 2020 ANSSI
> - * Copyright © 2020-2021 Microsoft Corporation
> + * Copyright © 2020-2022 Microsoft Corporation
>   */
>
>  #define _GNU_SOURCE
> @@ -371,6 +371,13 @@ TEST_F_FORK(layout1, inval)
>  	ASSERT_EQ(EINVAL, errno);
>  	path_beneath.allowed_access &= ~LANDLOCK_ACCESS_FS_EXECUTE;
>
> +	/* Tests with denied-by-default access right. */
> +	path_beneath.allowed_access |= LANDLOCK_ACCESS_FS_REFER;
> +	ASSERT_EQ(-1, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_PATH_BENEATH,
> +					&path_beneath, 0));
> +	ASSERT_EQ(EINVAL, errno);
> +	path_beneath.allowed_access &= ~LANDLOCK_ACCESS_FS_REFER;
> +
>  	/* Test with unknown (64-bits) value. */
>  	path_beneath.allowed_access |= (1ULL << 60);
>  	ASSERT_EQ(-1, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_PATH_BENEATH,
> @@ -1867,10 +1874,10 @@ TEST_F_FORK(layout1, rename_file)
>  	 * to a different directory (which allows file removal).
>  	 */
>  	ASSERT_EQ(-1, rename(file1_s2d1, file1_s1d3));
> -	ASSERT_EQ(EXDEV, errno);
> +	ASSERT_EQ(EACCES, errno);
>  	ASSERT_EQ(-1, renameat2(AT_FDCWD, file1_s2d1, AT_FDCWD, file1_s1d3,
>  				RENAME_EXCHANGE));
> -	ASSERT_EQ(EXDEV, errno);
> +	ASSERT_EQ(EACCES, errno);
>  	ASSERT_EQ(-1, renameat2(AT_FDCWD, dir_s2d2, AT_FDCWD, file1_s1d3,
>  				RENAME_EXCHANGE));
>  	ASSERT_EQ(EXDEV, errno);
> @@ -1894,7 +1901,7 @@ TEST_F_FORK(layout1, rename_file)
>  	ASSERT_EQ(EXDEV, errno);
>  	ASSERT_EQ(0, unlink(file1_s1d3));
>  	ASSERT_EQ(-1, rename(file1_s2d1, file1_s1d3));
> -	ASSERT_EQ(EXDEV, errno);
> +	ASSERT_EQ(EACCES, errno);
>
>  	/* Exchanges and renames files with same parent. */
>  	ASSERT_EQ(0, renameat2(AT_FDCWD, file2_s2d3, AT_FDCWD, file1_s2d3,
> @@ -2014,6 +2021,107 @@ TEST_F_FORK(layout1, reparent_refer)
>  	ASSERT_EQ(0, rename(dir_s1d3, dir_s2d3));
>  }
>
> +/* Checks renames beneath dir_s1d1. */
> +static void refer_denied_by_default(struct __test_metadata *const _metadata,
> +				    const struct rule layer1_abi_v2[],
> +				    const struct rule layer2_abi_v1[])
> +{
> +	int ruleset_fd;
> +
> +	ASSERT_EQ(0, unlink(file1_s1d2));
> +
> +	/*
> +	 * First layer, which handles LANDLOCK_ACCESS_FS_REFER, can allow some
> +	 * different-parent renames and links.
> +	 */
> +	ruleset_fd = create_ruleset(_metadata, layer1_abi_v2[0].access,
> +				    layer1_abi_v2);
> +	ASSERT_LE(0, ruleset_fd);
> +	enforce_ruleset(_metadata, ruleset_fd);
> +	ASSERT_EQ(0, close(ruleset_fd));
> +
> +	/* Checks that legitimate renames are allowed. */
> +	ASSERT_EQ(0, rename(file1_s1d1, file1_s1d2));
> +	ASSERT_EQ(0, rename(file1_s1d2, file1_s1d1));
> +	ASSERT_EQ(0, renameat2(AT_FDCWD, file2_s1d1, AT_FDCWD, file2_s1d2,
> +			       RENAME_EXCHANGE));
> +	ASSERT_EQ(0, renameat2(AT_FDCWD, file2_s1d2, AT_FDCWD, file2_s1d1,
> +			       RENAME_EXCHANGE));
> +
> +	/*
> +	 * Second layer, which does not handle LANDLOCK_ACCESS_FS_REFER, denies
> +	 * any different-parent renames and links, thus making the first layer
> +	 * null and void.
> +	 */
> +	ruleset_fd = create_ruleset(_metadata, layer2_abi_v1[0].access,
> +				    layer2_abi_v1);
> +	ASSERT_LE(0, ruleset_fd);
> +	enforce_ruleset(_metadata, ruleset_fd);
> +	ASSERT_EQ(0, close(ruleset_fd));
> +
> +	/* Checks that all renames are now denied. */
> +	ASSERT_EQ(-1, rename(file1_s1d1, file1_s1d2));
> +	ASSERT_EQ(EXDEV, errno);
> +	ASSERT_EQ(-1, renameat2(AT_FDCWD, file2_s1d1, AT_FDCWD, file2_s1d2,
> +				RENAME_EXCHANGE));
> +	ASSERT_EQ(EXDEV, errno);
> +	ASSERT_EQ(-1, renameat2(AT_FDCWD, file2_s1d2, AT_FDCWD, file2_s1d1,
> +				RENAME_EXCHANGE));
> +	ASSERT_EQ(EXDEV, errno);
> +}
> +
> +/*
> + * Tests precedence over renames: denied by default for different parent
> + * directories, *with* a rule matching a parent directory, but not directly
> + * denying access (with MAKE_REG nor REMOVE).
> + */
> +TEST_F_FORK(layout1, refer_denied_by_default1)
> +{
> +	const struct rule layer1_abi_v2[] = {
> +		{
> +			.path = dir_s1d1,
> +			.access = LANDLOCK_ACCESS_FS_REFER,
> +		},
> +		{},
> +	};
> +	const struct rule layer2_abi_v1[] = {
> +		{
> +			/* Matches a parent directory. */
> +			.path = dir_s1d1,
> +			.access = LANDLOCK_ACCESS_FS_EXECUTE,
> +		},
> +		{},
> +	};
> +
> +	refer_denied_by_default(_metadata, layer1_abi_v2, layer2_abi_v1);
> +}
> +
> +/*
> + * Tests precedence over renames: denied by default for different parent
> + * directories, *without* a rule matching a parent directory, but not directly
> + * denying access (with MAKE_REG nor REMOVE).
> + */
> +TEST_F_FORK(layout1, refer_denied_by_default2)
> +{
> +	const struct rule layer1_abi_v2[] = {
> +		{
> +			.path = dir_s1d1,
> +			.access = LANDLOCK_ACCESS_FS_REFER,
> +		},
> +		{},
> +	};
> +	const struct rule layer2_abi_v1[] = {
> +		{
> +			/* Does not match a parent directory. */
> +			.path = dir_s2d1,
> +			.access = LANDLOCK_ACCESS_FS_EXECUTE,
> +		},
> +		{},
> +	};
> +
> +	refer_denied_by_default(_metadata, layer1_abi_v2, layer2_abi_v1);
> +}
> +
>  TEST_F_FORK(layout1, reparent_link)
>  {
>  	const struct rule layer1[] = {
> @@ -2336,11 +2444,12 @@ TEST_F_FORK(layout1, reparent_exdev_layers_rename1)
>  	ASSERT_EQ(EXDEV, errno);
>
>  	/*
> -	 * However, moving the file2_s1d3 file below dir_s2d3 is allowed
> -	 * because it cannot inherit MAKE_REG nor MAKE_DIR rights (which are
> -	 * dedicated to directories).
> +	 * Moving the file2_s1d3 file below dir_s2d3 is denied because the
> +	 * second layer does not handle REFER, which is always denied by
> +	 * default.
>  	 */
> -	ASSERT_EQ(0, rename(file2_s1d3, file1_s2d3));
> +	ASSERT_EQ(-1, rename(file2_s1d3, file1_s2d3));
> +	ASSERT_EQ(EXDEV, errno);
>  }
>
>  TEST_F_FORK(layout1, reparent_exdev_layers_rename2)
> @@ -2373,8 +2482,12 @@ TEST_F_FORK(layout1, reparent_exdev_layers_rename2)
>  	ASSERT_EQ(EACCES, errno);
>  	ASSERT_EQ(-1, rename(file1_s1d1, file1_s2d3));
>  	ASSERT_EQ(EXDEV, errno);
> -	/* Modify layout! */
> -	ASSERT_EQ(0, rename(file2_s1d2, file1_s2d3));
> +	/*
> +	 * Modifying the layout is now denied because the second layer does not
> +	 * handle REFER, which is always denied by default.
> +	 */
> +	ASSERT_EQ(-1, rename(file2_s1d2, file1_s2d3));
> +	ASSERT_EQ(EXDEV, errno);
>
>  	/* Without REFER source, EACCES wins over EXDEV. */
>  	ASSERT_EQ(-1, rename(dir_s1d1, file1_s2d2));
>
> base-commit: 3d7cb6b04c3f3115719235cc6866b10326de34cd
> --
> 2.37.2
>

--

Re: [PATCH v1] landlock: Fix file reparenting without explicit LANDLOCK_ACCESS_FS_REFER

[Mickaël Salaün]

On 26/08/2022 00:16, Günther Noack wrote:
> On Tue, Aug 23, 2022 at 04:41:23PM +0200, Mickaël Salaün wrote:
>> With the introduction of LANDLOCK_ACCESS_FS_REFER, we added the first
>> globally denied-by-default access right.  Indeed, this lifted an initial
>> Landlock limitation to rename and link files, which was initially always
>> denied when the source or the destination were different directories.
>>
>> This led to an inconsistent backward compatibility behavior which was
>> only taken into account if no domain layer were using the new
>> LANDLOCK_ACCESS_FS_REFER right.  However, in a scenario where layers are
>> using the first and the second Landlock ABI (i.e.
>> LANDLOCK_ACCESS_FS_REFER or not), the access control behaves like if all
>> domains were handling LANDLOCK_ACCESS_FS_REFER with their rules
>> implicitely allowing such right.  Indeed, the not-handled access rights
>> were allowed, which should not be the case for LANDLOCK_ACCESS_FS_REFER.
>> It should be noted that this bug only allowed safe renames or links, but
>> no privilege escalation because the LANDLOCK_ACCESS_FS_REFER properties
>> were still enforced (e.g. only allowed to move a file according to all
>> other access rights, and if it doesn't give more Landlock accesses).
>>
>> This change adds an ACCESS_INITIALLY_DENIED list of denied-by-default
>> rights, which (only) contains LANDLOCK_ACCESS_FS_REFER.  All domains are
>> treated as if they are also handling this list, but without modifying
>> their fs_access_masks field, which enables correct domain audit.
>>
>> A side effect is that the errno code returned by rename(2) or link(2)
>> *may* be changed from EXDEV to EACCES according to the enforced
>> restrictions.  Indeed, we now have the mechanic to identify if an access
>> is denied because of a required right (e.g. LANDLOCK_ACCESS_FS_MAKE_REG,
>> LANDLOCK_ACCESS_FS_REMOVE_FILE) or if it is denied because of missing
>> LANDLOCK_ACCESS_FS_REFER rights.  This may result in different errno
>> codes than for the initial Landlock version, but this approach is better
>> for rename/link compatibility reasons, and it wasn't possible before
>> (hence no backport to ABI v1).  The layout1.rename_file test reflects
>> this change.
>>
>> Add the layout1.refer_denied_by_default* tests to check that the
>> behavior of a ruleset not handling LANDLOCK_ACCESS_FS_REFER (ABI v1) is
>> unchanged even if another layer handles LANDLOCK_ACCESS_FS_REFER (i.e.
>> ABI v1 precedence).  Make sure rule's absolute access rights are correct
>> by testing with and without a matching path.
>>
>> Extend layout1.inval tests to check that a denied-by-default access
>> right is not necessarily part of a domain's handled access rights.
>>
>> Fixes: b91c3e4ea756 ("landlock: Add support for file reparenting with LANDLOCK_ACCESS_FS_REFER")
>> Cc: Günther Noack <gnoack3000@gmail.com>
>> Cc: Paul Moore <paul@paul-moore.com>
>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
>> Link: https://lore.kernel.org/r/20220823144123.633721-1-mic@digikod.net
>> ---
>>   security/landlock/fs.c                     |  28 +++--
>>   tools/testing/selftests/landlock/fs_test.c | 133 +++++++++++++++++++--
>>   2 files changed, 143 insertions(+), 18 deletions(-)
>>
>> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
>> index ec5a6247cd3e..0cf484e89f68 100644
>> --- a/security/landlock/fs.c
>> +++ b/security/landlock/fs.c
>> @@ -149,6 +149,15 @@ static struct landlock_object *get_inode_object(struct inode *const inode)
>>   	LANDLOCK_ACCESS_FS_READ_FILE)
>>   /* clang-format on */
>>
>> +/*
>> + * All access rights that are denied by default whether they are handled or not
>> + * by a ruleset/layer.
>> + */
>> +/* clang-format off */
>> +#define ACCESS_INITIALLY_DENIED ( \
>> +	LANDLOCK_ACCESS_FS_REFER)
>> +/* clang-format on */
>> +
>>   /*
>>    * @path: Should have been checked by get_path_from_fd().
>>    */
>> @@ -167,7 +176,9 @@ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset,
>>   		return -EINVAL;
>>
>>   	/* Transforms relative access rights to absolute ones. */
>> -	access_rights |= LANDLOCK_MASK_ACCESS_FS & ~ruleset->fs_access_masks[0];
>> +	access_rights |=
>> +		LANDLOCK_MASK_ACCESS_FS &
>> +		~(ruleset->fs_access_masks[0] | ACCESS_INITIALLY_DENIED);
>>
>>   	object = get_inode_object(d_backing_inode(path->dentry));
>>   	if (IS_ERR(object))
>>   		return PTR_ERR(object);
>> @@ -277,7 +288,7 @@ static inline bool is_nouser_or_private(const struct dentry *dentry)
>>   static inline access_mask_t
>>   get_handled_accesses(const struct landlock_ruleset *const domain)
>>   {
>> -	access_mask_t access_dom = 0;
>> +	access_mask_t access_dom = ACCESS_INITIALLY_DENIED;
>>   	unsigned long access_bit;
>>
>>   	for (access_bit = 0; access_bit < LANDLOCK_NUM_ACCESS_FS;
>> @@ -316,8 +327,13 @@ init_layer_masks(const struct landlock_ruleset *const domain,
>>
>>   		for_each_set_bit(access_bit, &access_req,
>>   				 ARRAY_SIZE(*layer_masks)) {
>> -			if (domain->fs_access_masks[layer_level] &
>> -			    BIT_ULL(access_bit)) {
>> +			/*
>> +			 * Artificially handles all initially denied by default
>> +			 * access rights.
>> +			 */
>> +			if (BIT_ULL(access_bit) &
>> +			    (domain->fs_access_masks[layer_level] |
>> +			     ACCESS_INITIALLY_DENIED)) {
>>   				(*layer_masks)[access_bit] |=
>>   					BIT_ULL(layer_level);
>>   				handled_accesses |= BIT_ULL(access_bit);
>> @@ -857,10 +873,6 @@ static int current_check_refer_path(struct dentry *const old_dentry,
>>   					      NULL, NULL);
>>   	}
>>
>> -	/* Backward compatibility: no reparenting support. */
>> -	if (!(get_handled_accesses(dom) & LANDLOCK_ACCESS_FS_REFER))
>> -		return -EXDEV;
>> -
>>   	access_request_parent1 |= LANDLOCK_ACCESS_FS_REFER;
>>   	access_request_parent2 |= LANDLOCK_ACCESS_FS_REFER;
>>
> 
> In this change, you are adding "| ACCESS_INITIALLY_DENIED" to a bunch
> of places in fs.c where a domain->fs_access_masks[...] is read.
> 
> In the documentation for the refer right, it is described with:
> 
>    This is also the only access right which is always considered
>    handled by any ruleset in such a way that reparenting a file
>    hierarchy is always denied by default.
> 
> Naive question: Is anything keeping us from also implementing it in
> that way? We could pretend early during ruleset creation that the
> caller had asked to also handle the "refer" right. Then we would just
> have the "refer" bit set on every domain->fs_access_masks[...] entry.
> 
> Wouldn't that do the same, without further complicating the access
> check logic in fs.c?

This patch fixes the (absolute) rule access rights, which now always 
forbid LANDLOCK_ACCESS_FS_REFER except when it is explicitely allowed 
when creating a rule. Making all domain handle LANDLOCK_ACCESS_FS_REFER 
was may initial approach but there is two downsides:
- it makes the code more complex because we still want to check that a 
rule allowing LANDLOCK_ACCESS_FS_REFER is legitimate according to the 
ruleset's handled access rights (i.e. ABI v1 != ABI v2);
- it would not allow to identify if the user created a ruleset 
explicitely handling LANDLOCK_ACCESS_FS_REFER or not, which will be an 
issue to audit Landlock (not really possible right now but soon ;) ).

The difference with the current approach is only in the 
get_handled_accesses() and init_layer_masks(), instead of patching the 
ruleset enforcement.


> 
> FWIW, one thing that would happen in this approach would be that users
> could add rules asking to grant the "refer" right on files, on
> rulesets where they have not previously declared it as a "handled"
> right. It's debatable whether that is good API design, but it would
> not do much damage either, and could also be fixed by keeping a copy
> of the originally requested handled_access_fs.

Ah! From my point of view it is not a good API because it is a moving 
one (without notice). Keeping a copy of the originally requested 
handled_access_fs works but consumes data memory for something that can 
be done with the code, hence this solution.

Re: [PATCH v1] landlock: Fix file reparenting without explicit LANDLOCK_ACCESS_FS_REFER

[Paul Moore]

On Thu, Aug 25, 2022 at 6:27 PM Mickaël Salaün <mic@digikod.net> wrote:
> This patch fixes the (absolute) rule access rights, which now always
> forbid LANDLOCK_ACCESS_FS_REFER except when it is explicitely allowed
> when creating a rule. Making all domain handle LANDLOCK_ACCESS_FS_REFER
> was may initial approach but there is two downsides:
> - it makes the code more complex because we still want to check that a
> rule allowing LANDLOCK_ACCESS_FS_REFER is legitimate according to the
> ruleset's handled access rights (i.e. ABI v1 != ABI v2);
> - it would not allow to identify if the user created a ruleset
> explicitely handling LANDLOCK_ACCESS_FS_REFER or not, which will be an
> issue to audit Landlock (not really possible right now but soon ;) ).

I like this explanation much better!

-- 
paul-moore.com

Re: [PATCH v1] landlock: Fix file reparenting without explicit LANDLOCK_ACCESS_FS_REFER

[Günther Noack]

On Fri, Aug 26, 2022 at 10:39:56AM -0400, Paul Moore wrote:
> On Thu, Aug 25, 2022 at 6:27 PM Mickaël Salaün <mic@digikod.net> wrote:
> > This patch fixes the (absolute) rule access rights, which now always
> > forbid LANDLOCK_ACCESS_FS_REFER except when it is explicitely allowed
> > when creating a rule. Making all domain handle LANDLOCK_ACCESS_FS_REFER
> > was may initial approach but there is two downsides:
> > - it makes the code more complex because we still want to check that a
> > rule allowing LANDLOCK_ACCESS_FS_REFER is legitimate according to the
> > ruleset's handled access rights (i.e. ABI v1 != ABI v2);
> > - it would not allow to identify if the user created a ruleset
> > explicitely handling LANDLOCK_ACCESS_FS_REFER or not, which will be an
> > issue to audit Landlock (not really possible right now but soon ;) ).
>
> I like this explanation much better!

+1 I agree.

Phrasing wise, I'd also recommend to put the summary first, for example:

This patch fixes a mis-handling of the refer right when multiple
rulesets are layered. The expected behaviour was that an additional
ruleset can only restrict the set of permitted operations, but in this
particular case, it was possible to re-gain the "refer" right.

Does that sound like a reasonable summary?


--

Re: [PATCH v1] landlock: Fix file reparenting without explicit LANDLOCK_ACCESS_FS_REFER

[Günther Noack]

This patch looks good to me.


On Tue, Aug 23, 2022 at 04:41:23PM +0200, Mickaël Salaün wrote:
> With the introduction of LANDLOCK_ACCESS_FS_REFER, we added the first
> globally denied-by-default access right.  Indeed, this lifted an initial
> Landlock limitation to rename and link files, which was initially always
> denied when the source or the destination were different directories.
>
> This led to an inconsistent backward compatibility behavior which was
> only taken into account if no domain layer were using the new
> LANDLOCK_ACCESS_FS_REFER right.  However, in a scenario where layers are
> using the first and the second Landlock ABI (i.e.
> LANDLOCK_ACCESS_FS_REFER or not), the access control behaves like if all
> domains were handling LANDLOCK_ACCESS_FS_REFER with their rules
> implicitely allowing such right.  Indeed, the not-handled access rights
> were allowed, which should not be the case for LANDLOCK_ACCESS_FS_REFER.
> It should be noted that this bug only allowed safe renames or links, but
> no privilege escalation because the LANDLOCK_ACCESS_FS_REFER properties
> were still enforced (e.g. only allowed to move a file according to all
> other access rights, and if it doesn't give more Landlock accesses).
>
> This change adds an ACCESS_INITIALLY_DENIED list of denied-by-default
> rights, which (only) contains LANDLOCK_ACCESS_FS_REFER.  All domains are
> treated as if they are also handling this list, but without modifying
> their fs_access_masks field, which enables correct domain audit.
>
> A side effect is that the errno code returned by rename(2) or link(2)
> *may* be changed from EXDEV to EACCES according to the enforced
> restrictions.  Indeed, we now have the mechanic to identify if an access
> is denied because of a required right (e.g. LANDLOCK_ACCESS_FS_MAKE_REG,
> LANDLOCK_ACCESS_FS_REMOVE_FILE) or if it is denied because of missing
> LANDLOCK_ACCESS_FS_REFER rights.  This may result in different errno
> codes than for the initial Landlock version, but this approach is better
> for rename/link compatibility reasons, and it wasn't possible before
> (hence no backport to ABI v1).  The layout1.rename_file test reflects
> this change.
>
> Add the layout1.refer_denied_by_default* tests to check that the
> behavior of a ruleset not handling LANDLOCK_ACCESS_FS_REFER (ABI v1) is
> unchanged even if another layer handles LANDLOCK_ACCESS_FS_REFER (i.e.
> ABI v1 precedence).  Make sure rule's absolute access rights are correct
> by testing with and without a matching path.
>
> Extend layout1.inval tests to check that a denied-by-default access
> right is not necessarily part of a domain's handled access rights.
>
> Fixes: b91c3e4ea756 ("landlock: Add support for file reparenting with LANDLOCK_ACCESS_FS_REFER")
> Cc: Günther Noack <gnoack3000@gmail.com>
> Cc: Paul Moore <paul@paul-moore.com>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> Link: https://lore.kernel.org/r/20220823144123.633721-1-mic@digikod.net
> ---
>  security/landlock/fs.c                     |  28 +++--
>  tools/testing/selftests/landlock/fs_test.c | 133 +++++++++++++++++++--
>  2 files changed, 143 insertions(+), 18 deletions(-)
>
> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index ec5a6247cd3e..0cf484e89f68 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
> @@ -149,6 +149,15 @@ static struct landlock_object *get_inode_object(struct inode *const inode)
>  	LANDLOCK_ACCESS_FS_READ_FILE)
>  /* clang-format on */
>
> +/*
> + * All access rights that are denied by default whether they are handled or not
> + * by a ruleset/layer.

I feel it might be a good idea to explain why and how this constant
must be used. In my understanding, ACCESS_INITIALLY_DENIED must be
bitwise-or'd to ruleset->fs_access_masks[...] everytime that it is
being read? (That is at least the case everywhere in fs.c.)

(I still suspect that the layering might be easier to understand if we
already populated fs_access_masks with the 'refer' flag set, but I'm
not feeling that strongly about it if you want to keep it as is.)

> + */
> +/* clang-format off */
> +#define ACCESS_INITIALLY_DENIED ( \
> +	LANDLOCK_ACCESS_FS_REFER)
> +/* clang-format on */
> +
>  /*
>   * @path: Should have been checked by get_path_from_fd().
>   */
> @@ -167,7 +176,9 @@ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset,
>  		return -EINVAL;
>
>  	/* Transforms relative access rights to absolute ones. */
> -	access_rights |= LANDLOCK_MASK_ACCESS_FS & ~ruleset->fs_access_masks[0];
> +	access_rights |=
> +		LANDLOCK_MASK_ACCESS_FS &
> +		~(ruleset->fs_access_masks[0] | ACCESS_INITIALLY_DENIED);
>  	object = get_inode_object(d_backing_inode(path->dentry));
>  	if (IS_ERR(object))
>  		return PTR_ERR(object);
> @@ -277,7 +288,7 @@ static inline bool is_nouser_or_private(const struct dentry *dentry)
>  static inline access_mask_t
>  get_handled_accesses(const struct landlock_ruleset *const domain)
>  {
> -	access_mask_t access_dom = 0;
> +	access_mask_t access_dom = ACCESS_INITIALLY_DENIED;
>  	unsigned long access_bit;
>
>  	for (access_bit = 0; access_bit < LANDLOCK_NUM_ACCESS_FS;
> @@ -316,8 +327,13 @@ init_layer_masks(const struct landlock_ruleset *const domain,
>
>  		for_each_set_bit(access_bit, &access_req,
>  				 ARRAY_SIZE(*layer_masks)) {
> -			if (domain->fs_access_masks[layer_level] &
> -			    BIT_ULL(access_bit)) {
> +			/*
> +			 * Artificially handles all initially denied by default
> +			 * access rights.
> +			 */
> +			if (BIT_ULL(access_bit) &
> +			    (domain->fs_access_masks[layer_level] |
> +			     ACCESS_INITIALLY_DENIED)) {
>  				(*layer_masks)[access_bit] |=
>  					BIT_ULL(layer_level);
>  				handled_accesses |= BIT_ULL(access_bit);

(Putting this in parenthesis, because it's maybe out of scope for this
code review:

It seems to me that the two nested loops in get_handled_accesses() are
equivalent to doing:

   access_mask_t access_dom = ACCESS_INITIALLY_DENIED;
   size_t i;
   for (i = 0; i < domain->num_layers; i++) {
       access_dom |= domain->fs_access_masks[i];
   }
   return access_dom;

I have my doubts that the early break in the inner loop makes any
difference performance wise when we can replace one of the loops with
bitwise operations?

Even more unrelated thoughts:

In a similar fashion, a lot of the fs.c bit logic could be simplified
by inverting the layer_masks to be a max-layers-sized array of access
masks instead of a max-access-right-sized array of layer masks. I
notice that you have been counting the individual bytes for this
structure in the "refer" patch set though :-/ I wish this was
simpler...

I imagine that walking the path once per layer to unmask the
access_req for each layer was also discussed at the time, but then
discarded? (Is this to protect against concurrent modifications to the
file hierarchy?)

(These remarks are definitely out of scope for this change, they just
came up because I tried to understand all the surrounding code. I
guess I'm still catching up...))

> @@ -857,10 +873,6 @@ static int current_check_refer_path(struct dentry *const old_dentry,
>  					      NULL, NULL);
>  	}
>
> -	/* Backward compatibility: no reparenting support. */
> -	if (!(get_handled_accesses(dom) & LANDLOCK_ACCESS_FS_REFER))
> -		return -EXDEV;

If I understand correctly, this was the place where the logic bug was
- is that correct?

* with one ruleset where handled_access_fs *does not* have refer,
  it returns EXDEV (OK)
* with one ruleset where handled_access_fs *does* have refer,
  it continues (OK)
* with two ruleset where one does and the other does not have refer,
  it acts like refer is handled and continues without EXDEV
  but it should have returned EXDEV because that was the strict behaviour of
  the "v1"-style ruleset.

I think it's better to handle the V1 and V2 calls in a more unified
way, good to remove this backwards compatibility check.

>  	access_request_parent1 |= LANDLOCK_ACCESS_FS_REFER;
>  	access_request_parent2 |= LANDLOCK_ACCESS_FS_REFER;
>
> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> index 21a2ce8fa739..2c134a9202a1 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c
> @@ -4,7 +4,7 @@
>   *
>   * Copyright © 2017-2020 Mickaël Salaün <mic@digikod.net>
>   * Copyright © 2020 ANSSI
> - * Copyright © 2020-2021 Microsoft Corporation
> + * Copyright © 2020-2022 Microsoft Corporation
>   */
>
>  #define _GNU_SOURCE
> @@ -371,6 +371,13 @@ TEST_F_FORK(layout1, inval)
>  	ASSERT_EQ(EINVAL, errno);
>  	path_beneath.allowed_access &= ~LANDLOCK_ACCESS_FS_EXECUTE;
>
> +	/* Tests with denied-by-default access right. */
> +	path_beneath.allowed_access |= LANDLOCK_ACCESS_FS_REFER;
> +	ASSERT_EQ(-1, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_PATH_BENEATH,
> +					&path_beneath, 0));
> +	ASSERT_EQ(EINVAL, errno);
> +	path_beneath.allowed_access &= ~LANDLOCK_ACCESS_FS_REFER;
> +
>  	/* Test with unknown (64-bits) value. */
>  	path_beneath.allowed_access |= (1ULL << 60);
>  	ASSERT_EQ(-1, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_PATH_BENEATH,
> @@ -1867,10 +1874,10 @@ TEST_F_FORK(layout1, rename_file)
>  	 * to a different directory (which allows file removal).
>  	 */
>  	ASSERT_EQ(-1, rename(file1_s2d1, file1_s1d3));
> -	ASSERT_EQ(EXDEV, errno);
> +	ASSERT_EQ(EACCES, errno);
>  	ASSERT_EQ(-1, renameat2(AT_FDCWD, file1_s2d1, AT_FDCWD, file1_s1d3,
>  				RENAME_EXCHANGE));
> -	ASSERT_EQ(EXDEV, errno);
> +	ASSERT_EQ(EACCES, errno);
>  	ASSERT_EQ(-1, renameat2(AT_FDCWD, dir_s2d2, AT_FDCWD, file1_s1d3,
>  				RENAME_EXCHANGE));
>  	ASSERT_EQ(EXDEV, errno);
> @@ -1894,7 +1901,7 @@ TEST_F_FORK(layout1, rename_file)
>  	ASSERT_EQ(EXDEV, errno);
>  	ASSERT_EQ(0, unlink(file1_s1d3));
>  	ASSERT_EQ(-1, rename(file1_s2d1, file1_s1d3));
> -	ASSERT_EQ(EXDEV, errno);
> +	ASSERT_EQ(EACCES, errno);

Do I understand correctly that these changes are the cases where we
were previously hitting the early -EXDEV return (the one which I
commented on above)? So now a V1-style Landlock ruleset (without refer
in handled_access_fs) will result in errors that are in line with what
would happen if the refer right had been part of handled_access_fs in
the same ruleset?

This is a slight API change, but only in error codes and hopefully
within reason. I hope the backwards compatibility policy is a bit
forgiving in such cases? (Landlock does not have that many users yet,
after all.)

>
>  	/* Exchanges and renames files with same parent. */
>  	ASSERT_EQ(0, renameat2(AT_FDCWD, file2_s2d3, AT_FDCWD, file1_s2d3,
> @@ -2014,6 +2021,107 @@ TEST_F_FORK(layout1, reparent_refer)
>  	ASSERT_EQ(0, rename(dir_s1d3, dir_s2d3));
>  }
>
> +/* Checks renames beneath dir_s1d1. */
> +static void refer_denied_by_default(struct __test_metadata *const _metadata,
> +				    const struct rule layer1_abi_v2[],
> +				    const struct rule layer2_abi_v1[])
> +{
> +	int ruleset_fd;
> +
> +	ASSERT_EQ(0, unlink(file1_s1d2));
> +
> +	/*
> +	 * First layer, which handles LANDLOCK_ACCESS_FS_REFER, can allow some
> +	 * different-parent renames and links.
> +	 */
> +	ruleset_fd = create_ruleset(_metadata, layer1_abi_v2[0].access,
> +				    layer1_abi_v2);
> +	ASSERT_LE(0, ruleset_fd);
> +	enforce_ruleset(_metadata, ruleset_fd);
> +	ASSERT_EQ(0, close(ruleset_fd));
> +
> +	/* Checks that legitimate renames are allowed. */
> +	ASSERT_EQ(0, rename(file1_s1d1, file1_s1d2));
> +	ASSERT_EQ(0, rename(file1_s1d2, file1_s1d1));
> +	ASSERT_EQ(0, renameat2(AT_FDCWD, file2_s1d1, AT_FDCWD, file2_s1d2,
> +			       RENAME_EXCHANGE));
> +	ASSERT_EQ(0, renameat2(AT_FDCWD, file2_s1d2, AT_FDCWD, file2_s1d1,
> +			       RENAME_EXCHANGE));

*optional* We could use EXPECT_EQ for the rename{,at2} checks.

> +
> +	/*
> +	 * Second layer, which does not handle LANDLOCK_ACCESS_FS_REFER, denies
> +	 * any different-parent renames and links, thus making the first layer
> +	 * null and void.
> +	 */
> +	ruleset_fd = create_ruleset(_metadata, layer2_abi_v1[0].access,
> +				    layer2_abi_v1);
> +	ASSERT_LE(0, ruleset_fd);
> +	enforce_ruleset(_metadata, ruleset_fd);
> +	ASSERT_EQ(0, close(ruleset_fd));
> +
> +	/* Checks that all renames are now denied. */
> +	ASSERT_EQ(-1, rename(file1_s1d1, file1_s1d2));
> +	ASSERT_EQ(EXDEV, errno);
> +	ASSERT_EQ(-1, renameat2(AT_FDCWD, file2_s1d1, AT_FDCWD, file2_s1d2,
> +				RENAME_EXCHANGE));
> +	ASSERT_EQ(EXDEV, errno);
> +	ASSERT_EQ(-1, renameat2(AT_FDCWD, file2_s1d2, AT_FDCWD, file2_s1d1,
> +				RENAME_EXCHANGE));
> +	ASSERT_EQ(EXDEV, errno);
> +}
> +
> +/*
> + * Tests precedence over renames: denied by default for different parent
> + * directories, *with* a rule matching a parent directory, but not directly
> + * denying access (with MAKE_REG nor REMOVE).
> + */
> +TEST_F_FORK(layout1, refer_denied_by_default1)
> +{
> +	const struct rule layer1_abi_v2[] = {
> +		{
> +			.path = dir_s1d1,
> +			.access = LANDLOCK_ACCESS_FS_REFER,
> +		},
> +		{},
> +	};
> +	const struct rule layer2_abi_v1[] = {
> +		{
> +			/* Matches a parent directory. */
> +			.path = dir_s1d1,
> +			.access = LANDLOCK_ACCESS_FS_EXECUTE,
> +		},
> +		{},
> +	};
> +
> +	refer_denied_by_default(_metadata, layer1_abi_v2, layer2_abi_v1);
> +}
> +
> +/*
> + * Tests precedence over renames: denied by default for different parent
> + * directories, *without* a rule matching a parent directory, but not directly
> + * denying access (with MAKE_REG nor REMOVE).
> + */
> +TEST_F_FORK(layout1, refer_denied_by_default2)
> +{
> +	const struct rule layer1_abi_v2[] = {
> +		{
> +			.path = dir_s1d1,
> +			.access = LANDLOCK_ACCESS_FS_REFER,
> +		},
> +		{},
> +	};
> +	const struct rule layer2_abi_v1[] = {
> +		{
> +			/* Does not match a parent directory. */
> +			.path = dir_s2d1,
> +			.access = LANDLOCK_ACCESS_FS_EXECUTE,
> +		},
> +		{},
> +	};
> +
> +	refer_denied_by_default(_metadata, layer1_abi_v2, layer2_abi_v1);
> +}

These are just test style comments, so non-blockers in my mind; I find
the interface to the refer_denied_by_default helper function difficult
and it required a bit of jumping back and forth to make sense of it. I
find it often useful to use an approach where each test helper only
spans one of the test phases (set up / exercise / assert) - that helps
to keep the overall test structure in that set up / exercise / assert
form.

I imaginge that these tests could in principle look roughly like this:

TEST_F_FORK(layout1, refer_denied_by_default1)
{
	ASSERT_EQ(0, landlock_restrict_fs(LANDLOCK_ACCESS_FS_REFER, (struct rule[]){
        	{
                	.path = "superdir";
                        .access = LANDLOCK_ACCESS_FS_REFER,
                },
                {},
        }));
        EXPECT_EQ(0, test_rename("superdir/file1", "superdir/subdir/file1"));

	ASSERT_EQ(0, landlock_restrict_fs(LANDLOCK_ACCESS_FS_EXECUTE, (struct rule[]){
        	{
                	.path = "superdir";
                        .access = LANDLOCK_ACCESS_FS_EXECUTE,
                },
                {},
        }));
        EXPECT_EQ(EXDEV, test_rename("superdir/file2", "superdir/subdir/file2"));
}

Small optional remark on the side: I find the test scenario has a
slightly better punchline when turning around the order in which the
two layers are installed - because that also worked prior to this fix,
and it's more clearly wrong, as it was possible to do *more*
operations after installing the second layer.

> +
>  TEST_F_FORK(layout1, reparent_link)
>  {
>  	const struct rule layer1[] = {
> @@ -2336,11 +2444,12 @@ TEST_F_FORK(layout1, reparent_exdev_layers_rename1)
>  	ASSERT_EQ(EXDEV, errno);
>
>  	/*
> -	 * However, moving the file2_s1d3 file below dir_s2d3 is allowed
> -	 * because it cannot inherit MAKE_REG nor MAKE_DIR rights (which are
> -	 * dedicated to directories).
> +	 * Moving the file2_s1d3 file below dir_s2d3 is denied because the
> +	 * second layer does not handle REFER, which is always denied by
> +	 * default.
>  	 */
> -	ASSERT_EQ(0, rename(file2_s1d3, file1_s2d3));
> +	ASSERT_EQ(-1, rename(file2_s1d3, file1_s2d3));
> +	ASSERT_EQ(EXDEV, errno);
>  }
>
>  TEST_F_FORK(layout1, reparent_exdev_layers_rename2)
> @@ -2373,8 +2482,12 @@ TEST_F_FORK(layout1, reparent_exdev_layers_rename2)
>  	ASSERT_EQ(EACCES, errno);
>  	ASSERT_EQ(-1, rename(file1_s1d1, file1_s2d3));
>  	ASSERT_EQ(EXDEV, errno);
> -	/* Modify layout! */
> -	ASSERT_EQ(0, rename(file2_s1d2, file1_s2d3));
> +	/*
> +	 * Modifying the layout is now denied because the second layer does not
> +	 * handle REFER, which is always denied by default.
> +	 */
> +	ASSERT_EQ(-1, rename(file2_s1d2, file1_s2d3));
> +	ASSERT_EQ(EXDEV, errno);
>
>  	/* Without REFER source, EACCES wins over EXDEV. */
>  	ASSERT_EQ(-1, rename(dir_s1d1, file1_s2d2));
>
> base-commit: 3d7cb6b04c3f3115719235cc6866b10326de34cd
> --
> 2.37.2
>

--

Re: [PATCH v1] landlock: Fix file reparenting without explicit LANDLOCK_ACCESS_FS_REFER

[Mickaël Salaün]

On 30/08/2022 20:25, Günther Noack wrote:
> On Fri, Aug 26, 2022 at 10:39:56AM -0400, Paul Moore wrote:
>> On Thu, Aug 25, 2022 at 6:27 PM Mickaël Salaün <mic@digikod.net> wrote:
>>> This patch fixes the (absolute) rule access rights, which now always
>>> forbid LANDLOCK_ACCESS_FS_REFER except when it is explicitely allowed
>>> when creating a rule. Making all domain handle LANDLOCK_ACCESS_FS_REFER
>>> was may initial approach but there is two downsides:
>>> - it makes the code more complex because we still want to check that a
>>> rule allowing LANDLOCK_ACCESS_FS_REFER is legitimate according to the
>>> ruleset's handled access rights (i.e. ABI v1 != ABI v2);
>>> - it would not allow to identify if the user created a ruleset
>>> explicitely handling LANDLOCK_ACCESS_FS_REFER or not, which will be an
>>> issue to audit Landlock (not really possible right now but soon ;) ).
>>
>> I like this explanation much better!
> 
> +1 I agree.
> 
> Phrasing wise, I'd also recommend to put the summary first, for example:
> 
> This patch fixes a mis-handling of the refer right when multiple
> rulesets are layered. The expected behaviour was that an additional
> ruleset can only restrict the set of permitted operations, but in this
> particular case, it was possible to re-gain the "refer" right.
> 
> Does that sound like a reasonable summary?

Hmm, it's not exactly to regain the "refer" right because there is no 
issue when it is handled by the ruleset/layer.

I pushed this patch with some rephrasing in -next Monday: 
https://git.kernel.org/pub/scm/linux/kernel/git/mic/linux.git/commit/?h=next&id=7e4c602a992a7cb635ae0c87f5ec2e49136f620c
I can still improve the description though. I plan to send it to Linus 
very soon.

Re: [PATCH v1] landlock: Fix file reparenting without explicit LANDLOCK_ACCESS_FS_REFER

[Mickaël Salaün]

On 30/08/2022 22:01, Günther Noack wrote:
> This patch looks good to me.
> 
> 
> On Tue, Aug 23, 2022 at 04:41:23PM +0200, Mickaël Salaün wrote:
>> With the introduction of LANDLOCK_ACCESS_FS_REFER, we added the first
>> globally denied-by-default access right.  Indeed, this lifted an initial
>> Landlock limitation to rename and link files, which was initially always
>> denied when the source or the destination were different directories.
>>
>> This led to an inconsistent backward compatibility behavior which was
>> only taken into account if no domain layer were using the new
>> LANDLOCK_ACCESS_FS_REFER right.  However, in a scenario where layers are
>> using the first and the second Landlock ABI (i.e.
>> LANDLOCK_ACCESS_FS_REFER or not), the access control behaves like if all
>> domains were handling LANDLOCK_ACCESS_FS_REFER with their rules
>> implicitely allowing such right.  Indeed, the not-handled access rights
>> were allowed, which should not be the case for LANDLOCK_ACCESS_FS_REFER.
>> It should be noted that this bug only allowed safe renames or links, but
>> no privilege escalation because the LANDLOCK_ACCESS_FS_REFER properties
>> were still enforced (e.g. only allowed to move a file according to all
>> other access rights, and if it doesn't give more Landlock accesses).
>>
>> This change adds an ACCESS_INITIALLY_DENIED list of denied-by-default
>> rights, which (only) contains LANDLOCK_ACCESS_FS_REFER.  All domains are
>> treated as if they are also handling this list, but without modifying
>> their fs_access_masks field, which enables correct domain audit.
>>
>> A side effect is that the errno code returned by rename(2) or link(2)
>> *may* be changed from EXDEV to EACCES according to the enforced
>> restrictions.  Indeed, we now have the mechanic to identify if an access
>> is denied because of a required right (e.g. LANDLOCK_ACCESS_FS_MAKE_REG,
>> LANDLOCK_ACCESS_FS_REMOVE_FILE) or if it is denied because of missing
>> LANDLOCK_ACCESS_FS_REFER rights.  This may result in different errno
>> codes than for the initial Landlock version, but this approach is better
>> for rename/link compatibility reasons, and it wasn't possible before
>> (hence no backport to ABI v1).  The layout1.rename_file test reflects
>> this change.
>>
>> Add the layout1.refer_denied_by_default* tests to check that the
>> behavior of a ruleset not handling LANDLOCK_ACCESS_FS_REFER (ABI v1) is
>> unchanged even if another layer handles LANDLOCK_ACCESS_FS_REFER (i.e.
>> ABI v1 precedence).  Make sure rule's absolute access rights are correct
>> by testing with and without a matching path.
>>
>> Extend layout1.inval tests to check that a denied-by-default access
>> right is not necessarily part of a domain's handled access rights.
>>
>> Fixes: b91c3e4ea756 ("landlock: Add support for file reparenting with LANDLOCK_ACCESS_FS_REFER")
>> Cc: Günther Noack <gnoack3000@gmail.com>
>> Cc: Paul Moore <paul@paul-moore.com>
>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
>> Link: https://lore.kernel.org/r/20220823144123.633721-1-mic@digikod.net
>> ---
>>   security/landlock/fs.c                     |  28 +++--
>>   tools/testing/selftests/landlock/fs_test.c | 133 +++++++++++++++++++--
>>   2 files changed, 143 insertions(+), 18 deletions(-)
>>
>> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
>> index ec5a6247cd3e..0cf484e89f68 100644
>> --- a/security/landlock/fs.c
>> +++ b/security/landlock/fs.c
>> @@ -149,6 +149,15 @@ static struct landlock_object *get_inode_object(struct inode *const inode)
>>   	LANDLOCK_ACCESS_FS_READ_FILE)
>>   /* clang-format on */
>>
>> +/*
>> + * All access rights that are denied by default whether they are handled or not
>> + * by a ruleset/layer.
> 
> I feel it might be a good idea to explain why and how this constant
> must be used. In my understanding, ACCESS_INITIALLY_DENIED must be
> bitwise-or'd to ruleset->fs_access_masks[...] everytime that it is
> being read? (That is at least the case everywhere in fs.c.)

Correct, but this is also used by landlock_append_fs_rule().


> 
> (I still suspect that the layering might be easier to understand if we
> already populated fs_access_masks with the 'refer' flag set, but I'm
> not feeling that strongly about it if you want to keep it as is.)

For now yes, I prefer to keep it as is.


> 
>> + */
>> +/* clang-format off */
>> +#define ACCESS_INITIALLY_DENIED ( \
>> +	LANDLOCK_ACCESS_FS_REFER)
>> +/* clang-format on */
>> +
>>   /*
>>    * @path: Should have been checked by get_path_from_fd().
>>    */
>> @@ -167,7 +176,9 @@ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset,
>>   		return -EINVAL;
>>
>>   	/* Transforms relative access rights to absolute ones. */
>> -	access_rights |= LANDLOCK_MASK_ACCESS_FS & ~ruleset->fs_access_masks[0];
>> +	access_rights |=
>> +		LANDLOCK_MASK_ACCESS_FS &
>> +		~(ruleset->fs_access_masks[0] | ACCESS_INITIALLY_DENIED);
>>   	object = get_inode_object(d_backing_inode(path->dentry));
>>   	if (IS_ERR(object))
>>   		return PTR_ERR(object);
>> @@ -277,7 +288,7 @@ static inline bool is_nouser_or_private(const struct dentry *dentry)
>>   static inline access_mask_t
>>   get_handled_accesses(const struct landlock_ruleset *const domain)
>>   {
>> -	access_mask_t access_dom = 0;
>> +	access_mask_t access_dom = ACCESS_INITIALLY_DENIED;
>>   	unsigned long access_bit;
>>
>>   	for (access_bit = 0; access_bit < LANDLOCK_NUM_ACCESS_FS;
>> @@ -316,8 +327,13 @@ init_layer_masks(const struct landlock_ruleset *const domain,
>>
>>   		for_each_set_bit(access_bit, &access_req,
>>   				 ARRAY_SIZE(*layer_masks)) {
>> -			if (domain->fs_access_masks[layer_level] &
>> -			    BIT_ULL(access_bit)) {
>> +			/*
>> +			 * Artificially handles all initially denied by default
>> +			 * access rights.
>> +			 */
>> +			if (BIT_ULL(access_bit) &
>> +			    (domain->fs_access_masks[layer_level] |
>> +			     ACCESS_INITIALLY_DENIED)) {
>>   				(*layer_masks)[access_bit] |=
>>   					BIT_ULL(layer_level);
>>   				handled_accesses |= BIT_ULL(access_bit);
> 
> (Putting this in parenthesis, because it's maybe out of scope for this
> code review:
> 
> It seems to me that the two nested loops in get_handled_accesses() are
> equivalent to doing:
> 
>     access_mask_t access_dom = ACCESS_INITIALLY_DENIED;
>     size_t i;
>     for (i = 0; i < domain->num_layers; i++) {
>         access_dom |= domain->fs_access_masks[i];
>     }
>     return access_dom;
> 
> I have my doubts that the early break in the inner loop makes any
> difference performance wise when we can replace one of the loops with
> bitwise operations?

This is better indeed, I'll do that.


> 
> Even more unrelated thoughts:
> 
> In a similar fashion, a lot of the fs.c bit logic could be simplified
> by inverting the layer_masks to be a max-layers-sized array of access
> masks instead of a max-access-right-sized array of layer masks. I
> notice that you have been counting the individual bytes for this
> structure in the "refer" patch set though :-/ I wish this was
> simpler...
> 
> I imagine that walking the path once per layer to unmask the
> access_req for each layer was also discussed at the time, but then
> discarded? (Is this to protect against concurrent modifications to the
> file hierarchy?)

I designed it this way to only traverse a path once (instead of 
potentially 16 times), whatever the number of layers.


> 
> (These remarks are definitely out of scope for this change, they just
> came up because I tried to understand all the surrounding code. I
> guess I'm still catching up...))

It's still useful to discuss about the current code. :)


> 
>> @@ -857,10 +873,6 @@ static int current_check_refer_path(struct dentry *const old_dentry,
>>   					      NULL, NULL);
>>   	}
>>
>> -	/* Backward compatibility: no reparenting support. */
>> -	if (!(get_handled_accesses(dom) & LANDLOCK_ACCESS_FS_REFER))
>> -		return -EXDEV;
> 
> If I understand correctly, this was the place where the logic bug was
> - is that correct?
> 
> * with one ruleset where handled_access_fs *does not* have refer,
>    it returns EXDEV (OK)
> * with one ruleset where handled_access_fs *does* have refer,
>    it continues (OK)
> * with two ruleset where one does and the other does not have refer,
>    it acts like refer is handled and continues without EXDEV
>    but it should have returned EXDEV because that was the strict behaviour of
>    the "v1"-style ruleset.

Correct

> 
> I think it's better to handle the V1 and V2 calls in a more unified
> way, good to remove this backwards compatibility check.

Yes


> 
>>   	access_request_parent1 |= LANDLOCK_ACCESS_FS_REFER;
>>   	access_request_parent2 |= LANDLOCK_ACCESS_FS_REFER;
>>
>> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
>> index 21a2ce8fa739..2c134a9202a1 100644
>> --- a/tools/testing/selftests/landlock/fs_test.c
>> +++ b/tools/testing/selftests/landlock/fs_test.c
>> @@ -4,7 +4,7 @@
>>    *
>>    * Copyright © 2017-2020 Mickaël Salaün <mic@digikod.net>
>>    * Copyright © 2020 ANSSI
>> - * Copyright © 2020-2021 Microsoft Corporation
>> + * Copyright © 2020-2022 Microsoft Corporation
>>    */
>>
>>   #define _GNU_SOURCE
>> @@ -371,6 +371,13 @@ TEST_F_FORK(layout1, inval)
>>   	ASSERT_EQ(EINVAL, errno);
>>   	path_beneath.allowed_access &= ~LANDLOCK_ACCESS_FS_EXECUTE;
>>
>> +	/* Tests with denied-by-default access right. */
>> +	path_beneath.allowed_access |= LANDLOCK_ACCESS_FS_REFER;
>> +	ASSERT_EQ(-1, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_PATH_BENEATH,
>> +					&path_beneath, 0));
>> +	ASSERT_EQ(EINVAL, errno);
>> +	path_beneath.allowed_access &= ~LANDLOCK_ACCESS_FS_REFER;
>> +
>>   	/* Test with unknown (64-bits) value. */
>>   	path_beneath.allowed_access |= (1ULL << 60);
>>   	ASSERT_EQ(-1, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_PATH_BENEATH,
>> @@ -1867,10 +1874,10 @@ TEST_F_FORK(layout1, rename_file)
>>   	 * to a different directory (which allows file removal).
>>   	 */
>>   	ASSERT_EQ(-1, rename(file1_s2d1, file1_s1d3));
>> -	ASSERT_EQ(EXDEV, errno);
>> +	ASSERT_EQ(EACCES, errno);
>>   	ASSERT_EQ(-1, renameat2(AT_FDCWD, file1_s2d1, AT_FDCWD, file1_s1d3,
>>   				RENAME_EXCHANGE));
>> -	ASSERT_EQ(EXDEV, errno);
>> +	ASSERT_EQ(EACCES, errno);
>>   	ASSERT_EQ(-1, renameat2(AT_FDCWD, dir_s2d2, AT_FDCWD, file1_s1d3,
>>   				RENAME_EXCHANGE));
>>   	ASSERT_EQ(EXDEV, errno);
>> @@ -1894,7 +1901,7 @@ TEST_F_FORK(layout1, rename_file)
>>   	ASSERT_EQ(EXDEV, errno);
>>   	ASSERT_EQ(0, unlink(file1_s1d3));
>>   	ASSERT_EQ(-1, rename(file1_s2d1, file1_s1d3));
>> -	ASSERT_EQ(EXDEV, errno);
>> +	ASSERT_EQ(EACCES, errno);
> 
> Do I understand correctly that these changes are the cases where we
> were previously hitting the early -EXDEV return (the one which I
> commented on above)? So now a V1-style Landlock ruleset (without refer
> in handled_access_fs) will result in errors that are in line with what
> would happen if the refer right had been part of handled_access_fs in
> the same ruleset?

Correct

> 
> This is a slight API change, but only in error codes and hopefully
> within reason. I hope the backwards compatibility policy is a bit
> forgiving in such cases? (Landlock does not have that many users yet,
> after all.)

It is kind of a slight *ABI* change because of the potential errno code 
change, but this is the kind of things that may change over time with 
the kernel because it doesn't change the way user space interacts with it.

This fix may change the errno but in a better way because it only 
returns EXDEV when a file copy is possible. This may be seen as a fix too.


> 
>>
>>   	/* Exchanges and renames files with same parent. */
>>   	ASSERT_EQ(0, renameat2(AT_FDCWD, file2_s2d3, AT_FDCWD, file1_s2d3,
>> @@ -2014,6 +2021,107 @@ TEST_F_FORK(layout1, reparent_refer)
>>   	ASSERT_EQ(0, rename(dir_s1d3, dir_s2d3));
>>   }
>>
>> +/* Checks renames beneath dir_s1d1. */
>> +static void refer_denied_by_default(struct __test_metadata *const _metadata,
>> +				    const struct rule layer1_abi_v2[],
>> +				    const struct rule layer2_abi_v1[])
>> +{
>> +	int ruleset_fd;
>> +
>> +	ASSERT_EQ(0, unlink(file1_s1d2));
>> +
>> +	/*
>> +	 * First layer, which handles LANDLOCK_ACCESS_FS_REFER, can allow some
>> +	 * different-parent renames and links.
>> +	 */
>> +	ruleset_fd = create_ruleset(_metadata, layer1_abi_v2[0].access,
>> +				    layer1_abi_v2);
>> +	ASSERT_LE(0, ruleset_fd);
>> +	enforce_ruleset(_metadata, ruleset_fd);
>> +	ASSERT_EQ(0, close(ruleset_fd));
>> +
>> +	/* Checks that legitimate renames are allowed. */
>> +	ASSERT_EQ(0, rename(file1_s1d1, file1_s1d2));
>> +	ASSERT_EQ(0, rename(file1_s1d2, file1_s1d1));
>> +	ASSERT_EQ(0, renameat2(AT_FDCWD, file2_s1d1, AT_FDCWD, file2_s1d2,
>> +			       RENAME_EXCHANGE));
>> +	ASSERT_EQ(0, renameat2(AT_FDCWD, file2_s1d2, AT_FDCWD, file2_s1d1,
>> +			       RENAME_EXCHANGE));
> 
> *optional* We could use EXPECT_EQ for the rename{,at2} checks.

Yes, as long as these files are not part of a ruleset.


> 
>> +
>> +	/*
>> +	 * Second layer, which does not handle LANDLOCK_ACCESS_FS_REFER, denies
>> +	 * any different-parent renames and links, thus making the first layer
>> +	 * null and void.
>> +	 */
>> +	ruleset_fd = create_ruleset(_metadata, layer2_abi_v1[0].access,
>> +				    layer2_abi_v1);
>> +	ASSERT_LE(0, ruleset_fd);
>> +	enforce_ruleset(_metadata, ruleset_fd);
>> +	ASSERT_EQ(0, close(ruleset_fd));
>> +
>> +	/* Checks that all renames are now denied. */
>> +	ASSERT_EQ(-1, rename(file1_s1d1, file1_s1d2));
>> +	ASSERT_EQ(EXDEV, errno);
>> +	ASSERT_EQ(-1, renameat2(AT_FDCWD, file2_s1d1, AT_FDCWD, file2_s1d2,
>> +				RENAME_EXCHANGE));
>> +	ASSERT_EQ(EXDEV, errno);
>> +	ASSERT_EQ(-1, renameat2(AT_FDCWD, file2_s1d2, AT_FDCWD, file2_s1d1,
>> +				RENAME_EXCHANGE));
>> +	ASSERT_EQ(EXDEV, errno);
>> +}
>> +
>> +/*
>> + * Tests precedence over renames: denied by default for different parent
>> + * directories, *with* a rule matching a parent directory, but not directly
>> + * denying access (with MAKE_REG nor REMOVE).
>> + */
>> +TEST_F_FORK(layout1, refer_denied_by_default1)
>> +{
>> +	const struct rule layer1_abi_v2[] = {
>> +		{
>> +			.path = dir_s1d1,
>> +			.access = LANDLOCK_ACCESS_FS_REFER,
>> +		},
>> +		{},
>> +	};
>> +	const struct rule layer2_abi_v1[] = {
>> +		{
>> +			/* Matches a parent directory. */
>> +			.path = dir_s1d1,
>> +			.access = LANDLOCK_ACCESS_FS_EXECUTE,
>> +		},
>> +		{},
>> +	};
>> +
>> +	refer_denied_by_default(_metadata, layer1_abi_v2, layer2_abi_v1);
>> +}
>> +
>> +/*
>> + * Tests precedence over renames: denied by default for different parent
>> + * directories, *without* a rule matching a parent directory, but not directly
>> + * denying access (with MAKE_REG nor REMOVE).
>> + */
>> +TEST_F_FORK(layout1, refer_denied_by_default2)
>> +{
>> +	const struct rule layer1_abi_v2[] = {
>> +		{
>> +			.path = dir_s1d1,
>> +			.access = LANDLOCK_ACCESS_FS_REFER,
>> +		},
>> +		{},
>> +	};
>> +	const struct rule layer2_abi_v1[] = {
>> +		{
>> +			/* Does not match a parent directory. */
>> +			.path = dir_s2d1,
>> +			.access = LANDLOCK_ACCESS_FS_EXECUTE,
>> +		},
>> +		{},
>> +	};
>> +
>> +	refer_denied_by_default(_metadata, layer1_abi_v2, layer2_abi_v1);
>> +}
> 
> These are just test style comments, so non-blockers in my mind; I find
> the interface to the refer_denied_by_default helper function difficult
> and it required a bit of jumping back and forth to make sense of it. I
> find it often useful to use an approach where each test helper only
> spans one of the test phases (set up / exercise / assert) - that helps
> to keep the overall test structure in that set up / exercise / assert
> form.
> 
> I imaginge that these tests could in principle look roughly like this:
> 
> TEST_F_FORK(layout1, refer_denied_by_default1)
> {
> 	ASSERT_EQ(0, landlock_restrict_fs(LANDLOCK_ACCESS_FS_REFER, (struct rule[]){
>          	{
>                  	.path = "superdir";
>                          .access = LANDLOCK_ACCESS_FS_REFER,
>                  },
>                  {},
>          }));
>          EXPECT_EQ(0, test_rename("superdir/file1", "superdir/subdir/file1"));
> 
> 	ASSERT_EQ(0, landlock_restrict_fs(LANDLOCK_ACCESS_FS_EXECUTE, (struct rule[]){
>          	{
>                  	.path = "superdir";
>                          .access = LANDLOCK_ACCESS_FS_EXECUTE,
>                  },
>                  {},
>          }));
>          EXPECT_EQ(EXDEV, test_rename("superdir/file2", "superdir/subdir/file2"));
> }

I tend to agree but the rename checks would be buried in 
landlock_restrict_fs() and the test output would be more difficult to 
understand.


> 
> Small optional remark on the side: I find the test scenario has a
> slightly better punchline when turning around the order in which the
> two layers are installed - because that also worked prior to this fix,
> and it's more clearly wrong, as it was possible to do *more*
> operations after installing the second layer.

These tests enable to check that a rename is first allowed and then 
denied (because of the missing FS_REFER). I agree that turning around 
the order makes the tests different scenarios. I'll send a v2 with the 
four test cases.


> 
>> +
>>   TEST_F_FORK(layout1, reparent_link)
>>   {
>>   	const struct rule layer1[] = {
>> @@ -2336,11 +2444,12 @@ TEST_F_FORK(layout1, reparent_exdev_layers_rename1)
>>   	ASSERT_EQ(EXDEV, errno);
>>
>>   	/*
>> -	 * However, moving the file2_s1d3 file below dir_s2d3 is allowed
>> -	 * because it cannot inherit MAKE_REG nor MAKE_DIR rights (which are
>> -	 * dedicated to directories).
>> +	 * Moving the file2_s1d3 file below dir_s2d3 is denied because the
>> +	 * second layer does not handle REFER, which is always denied by
>> +	 * default.
>>   	 */
>> -	ASSERT_EQ(0, rename(file2_s1d3, file1_s2d3));
>> +	ASSERT_EQ(-1, rename(file2_s1d3, file1_s2d3));
>> +	ASSERT_EQ(EXDEV, errno);
>>   }
>>
>>   TEST_F_FORK(layout1, reparent_exdev_layers_rename2)
>> @@ -2373,8 +2482,12 @@ TEST_F_FORK(layout1, reparent_exdev_layers_rename2)
>>   	ASSERT_EQ(EACCES, errno);
>>   	ASSERT_EQ(-1, rename(file1_s1d1, file1_s2d3));
>>   	ASSERT_EQ(EXDEV, errno);
>> -	/* Modify layout! */
>> -	ASSERT_EQ(0, rename(file2_s1d2, file1_s2d3));
>> +	/*
>> +	 * Modifying the layout is now denied because the second layer does not
>> +	 * handle REFER, which is always denied by default.
>> +	 */
>> +	ASSERT_EQ(-1, rename(file2_s1d2, file1_s2d3));
>> +	ASSERT_EQ(EXDEV, errno);
>>
>>   	/* Without REFER source, EACCES wins over EXDEV. */
>>   	ASSERT_EQ(-1, rename(dir_s1d1, file1_s2d2));
>>
>> base-commit: 3d7cb6b04c3f3115719235cc6866b10326de34cd
>> --
>> 2.37.2
>>
> 
> --