[PATCH v3 2/4] landlock: Fix handling of disconnected directories

[Tingmao Wang]

On 7/19/25 11:42, Mickaël Salaün wrote:
> [...]
> @@ -784,12 +787,18 @@ static bool is_access_to_paths_allowed(
>  	if (WARN_ON_ONCE(!layer_masks_parent1))
>  		return false;
>  
> -	allowed_parent1 = is_layer_masks_allowed(layer_masks_parent1);
> -
>  	if (unlikely(layer_masks_parent2)) {
>  		if (WARN_ON_ONCE(!dentry_child1))
>  			return false;
>  
> +		/*
> +		 * Creates a backup of the initial layer masks to be able to restore
> +		 * them if we find out that we were walking a disconnected directory,
> +		 * which would make the collected access rights inconsistent (cf.
> +		 * reset_to_mount_root).
> +		 */

This comment is duplicate with the one below, is this intentional?

> [...]

On the other hand, I'm still a bit uncertain about the domain check
semantics.  While it would not cause a rename to be allowed if it is
otherwise not allowed by any rules on or above the mountpoint, this gets a
bit weird if we have a situation where renames are allowed on the
mountpoint or everywhere, but not read/writes, however read/writes are
allowed directly on a file, but the dir containing that file gets
disconnected so the sandboxed application can't read or write to it.
(Maybe someone would set up such a policy where renames are allowed,
expecting Landlock to always prevent renames where additional permissions
would be exposed?)

In the above situation, if the file is then moved to a connected
directory, it will become readable/writable again.

Here is an example test, using the layout1_bind fixture for flexibility
for now (and also because I needed to just go to bed yesterday lol) (but
this would probably be better written as an additional
layout5_disconnected_branch variant).

diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index 21dd95aaf5e4..2274f165d933 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -5100,6 +5100,118 @@ TEST_F_FORK(layout1_bind, path_disconnected_rename)
 	EXPECT_EQ(0, test_open(file1_s1d3, O_RDONLY));
 }
 
+static void
+path_disconnected_gain_back_rights_via_rename(struct __test_metadata *_metadata,
+					      bool has_read_rule_on_other_d)
+{
+	/*
+	 * This is a ruleset where rename/create/delete rights are allowed
+	 * anywhere under the mount, and so still applies after path gets
+	 * disconnected.  However the only read right is given to the file
+	 * directly, and therefore the file is no longer readable after the
+	 * path to it being disconnected.
+	 */
+	// clang-format off
+	struct rule layer1[] = {
+		{
+			.path = dir_s2d2,
+			.access = LANDLOCK_ACCESS_FS_REFER |
+					LANDLOCK_ACCESS_FS_MAKE_DIR |
+					LANDLOCK_ACCESS_FS_REMOVE_DIR |
+					LANDLOCK_ACCESS_FS_MAKE_REG |
+					LANDLOCK_ACCESS_FS_REMOVE_FILE
+		},
+		{
+			.path = file1_s1d3,
+			.access = LANDLOCK_ACCESS_FS_READ_FILE,
+		},
+		{
+			.path = TMP_DIR "/s1d1/s1d2/s1d3_2",
+			.access = LANDLOCK_ACCESS_FS_READ_FILE,
+		},
+		{}
+	};
+	// clang-format on
+
+	int ruleset_fd, bind_s1d3_fd, res;
+
+	if (!has_read_rule_on_other_d) {
+		layer1[2].path = NULL;
+		layer1[2].access = 0;
+	}
+
+	ASSERT_EQ(0, mkdir(dir_s4d1, 0755))
+	{
+		TH_LOG("Failed to create %s: %s", dir_s4d1, strerror(errno));
+	}
+
+	/* Directory used to move the file into, in order to try to regain read */
+	ASSERT_EQ(0, mkdir(TMP_DIR "/s1d1/s1d2/s1d3_2", 0755))
+	{
+		TH_LOG("Failed to create %s: %s", TMP_DIR "/s1d1/s1d2/s1d3_2",
+		       strerror(errno));
+	}
+
+	ruleset_fd = create_ruleset(_metadata, ACCESS_ALL, layer1);
+	ASSERT_LE(0, ruleset_fd);
+
+	bind_s1d3_fd = open(bind_dir_s1d3, O_PATH | O_CLOEXEC);
+	ASSERT_LE(0, bind_s1d3_fd);
+	EXPECT_EQ(0, test_open_rel(bind_s1d3_fd, file1_name, O_RDONLY));
+
+	/* Make disconnected */
+	ASSERT_EQ(0, rename(dir_s1d3, dir_s4d2))
+	{
+		TH_LOG("Failed to rename %s to %s: %s", dir_s1d3, dir_s4d2,
+		       strerror(errno));
+	}
+
+	enforce_ruleset(_metadata, ruleset_fd);
+	EXPECT_EQ(0, close(ruleset_fd));
+
+	/* We shouldn't be able to read file1 under disconnected path now */
+	EXPECT_EQ(EACCES, test_open_rel(bind_s1d3_fd, file1_name, O_RDONLY));
+
+	/*
+	 * But can we circumvent it by moving file1 to a connected path when
+	 * either we're allowed to read that move destination, or if we have
+	 * allow rules on the original file, then the move target doesn't even
+	 * need read rules on itself.
+	 *
+	 * This is possible even though the domain check should semantically
+	 * ensure that any path (?) we can't read can't become readable
+	 * (through that path) again by a rename?
+	 */
+	res = renameat(bind_s1d3_fd, file1_name, AT_FDCWD,
+		       TMP_DIR "/s2d1/s2d2/s1d3_2/f1");
+	if (res == 0) {
+		TH_LOG("Renamed file1 to %s, which should not have been allowed.",
+		       TMP_DIR "/s2d1/s2d2/s1d3_2/f1");
+		/* At this point the test has failed, but let's try reading it */
+		res = test_open(TMP_DIR "/s2d1/s2d2/s1d3_2/f1", O_RDONLY);
+		if (res != 0) {
+			TH_LOG("Failed to read file1 after rename: %s",
+			       strerror(res));
+		} else {
+			TH_LOG("file1 is readable after rename!");
+			ASSERT_TRUE(false);
+		}
+		ASSERT_TRUE(false);
+	}
+	ASSERT_EQ(-1, res);
+	EXPECT_EQ(EXDEV, errno);
+}
+
+TEST_F_FORK(layout1_bind, path_disconnected_gain_back_rights_1)
+{
+	path_disconnected_gain_back_rights_via_rename(_metadata, false);
+}
+
+TEST_F_FORK(layout1_bind, path_disconnected_gain_back_rights_2)
+{
+	path_disconnected_gain_back_rights_via_rename(_metadata, true);
+}
+
 /*
  * Test that linkat(2) with disconnected paths works under Landlock. This
  * test moves s1d3 to s4d1.

The behavior is as hypothesized above:

	root at b8f2ef644787 /t/landlock# ./fs_test -t path_disconnected_gain_back_rights_1 -t path_disconnected_gain_back_rights_2
	TAP version 13
	1..2
	# Starting 2 tests from 1 test cases.
	#  RUN           layout1_bind.path_disconnected_gain_back_rights_1 ...
	# fs_test.c:5188:path_disconnected_gain_back_rights_1:Renamed file1 to tmp/s2d1/s2d2/s1d3_2/f1, which should not have been allowed.
	# fs_test.c:5196:path_disconnected_gain_back_rights_1:file1 is readable after rename!
	# fs_test.c:5197:path_disconnected_gain_back_rights_1:Expected 0 (0) != false (0)
	# path_disconnected_gain_back_rights_1: Test terminated by assertion
	#          FAIL  layout1_bind.path_disconnected_gain_back_rights_1
	not ok 1 layout1_bind.path_disconnected_gain_back_rights_1
	#  RUN           layout1_bind.path_disconnected_gain_back_rights_2 ...
	# fs_test.c:5188:path_disconnected_gain_back_rights_2:Renamed file1 to tmp/s2d1/s2d2/s1d3_2/f1, which should not have been allowed.
	# fs_test.c:5196:path_disconnected_gain_back_rights_2:file1 is readable after rename!
	# fs_test.c:5197:path_disconnected_gain_back_rights_2:Expected 0 (0) != false (0)
	# path_disconnected_gain_back_rights_2: Test terminated by assertion
	#          FAIL  layout1_bind.path_disconnected_gain_back_rights_2
	not ok 2 layout1_bind.path_disconnected_gain_back_rights_2
	# FAILED: 0 / 2 tests passed.
	# Totals: pass:0 fail:2 xfail:0 xpass:0 skip:0 error:0

Would it be worth it to have the domain check take into account this edge
case?  (But on the other hand, one could argue that if rights are granted
directly to a file, then the policy author intended for access to be
allowed, but in which case shouldn't access, even if through disconnected
path, be allowed?)

Best,
Tingmao