[PATCH v3 2/4] landlock: Fix handling of disconnected directories
Mickaël Salaün
mic at digikod.net
Wed Jul 23 21:01:42 UTC 2025
On Tue, Jul 22, 2025 at 07:04:02PM +0100, Tingmao Wang wrote:
> 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.
We can generalize this issue to not only the end file but any component
of the path: disconnected directories. In fact, the main issue is the
potential inconsistency of access checks over time (e.g. between two
renames). This could be exploited to bypass the security checks done
for FS_REFER.
I see two solutions:
1. *Always* walk down to the IS_ROOT directory, and then jump to the
mount point. This makes it possible to have consistent access checks
for renames and open/use. The first downside is that that would
change the current behavior for bind mounts that could get more
access rights (if the policy explicitly sets rights for the hidden
directories). The second downside is that we'll do more walk.
2. Return -EACCES (or -ENOENT) for actions involving disconnected
directories, or renames of disconnected opened files. This second
solution is simpler and safer but completely disables the use of
disconnected directories and the rename of disconnected files for
sandboxed processes.
It would be much better to be able to handle opened directories as
(object) capabilities, but that is not currently possible because of the
way paths are handled by the VFS and LSM hooks.
Tingmao, Günther, Jann, what do you think?
It looks like AppArmor also denies access to disconnected path in some
cases, but it tries to reconstruct the path for known internal
filesystems, and it seems to specifically handle the case of chroot. I
don't know when PATH_CONNECT_PATH is set though.
John, could you please clarify how disconnected directories and files
are handled by AppArmor?
>
> 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
>
More information about the Linux-security-module-archive
mailing list