[PATCH] selftests/landlock: Add tests for access through disconnected paths
Tingmao Wang
m at maowtm.org
Wed Jun 25 20:46:08 UTC 2025
On 6/25/25 15:52, Mickaël Salaün wrote:
> On Tue, Jun 24, 2025 at 12:16:55AM +0100, Tingmao Wang wrote:
>> [..]
>
> Let's say we initially have this hierarchy:
>
> root
> ├── s1d1
> │ └── s1d2 [REFER]
> │ └── s1d3
> │ └── s1d4
> │ └── f1
> ├── s2d1 [bind mount of s1d1]
> │ └── s1d2 [REFER]
> │ └── s1d3
> │ └── s1d4
> │ └── f1
> └── s3d1 [REFER]
>
> s1d3 has s1d2 as parent with the REFER right.
>
> We open [fd:s1d4].
>
> Now, s1d1/s1d2/s1d3 is moved to s3d1/s1d3, which makes [fd:s1d4]/..
> disconnected:
>
> root
> ├── s1d1
> │ └── s1d2 [REFER]
> ├── s2d1 [bind mount of s1d1]
> │ └── s1d2 [REFER]
> └── s3d1 [REFER]
> └── s1d3 [moved from s1d2]
> └── s1d4
> └── f1
>
> Now, s1d3 has s3d1 as parent with the REFER right.
>
> Moving [fd:s1d4]/f1 to s2d1/s1d2/f1 would be deny by Landlock
Maybe I'm misunderstanding your description, but this seems to work for
me?
diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
index d8f9259fffe4..5e550e6da49c 100644
--- a/tools/testing/selftests/landlock/fs_test.c
+++ b/tools/testing/selftests/landlock/fs_test.c
@@ -5201,6 +5201,72 @@ TEST_F_FORK(layout1_bind, path_disconnected_link)
}
}
+FIXTURE(layout_disconnected_special){};
+FIXTURE_SETUP(layout_disconnected_special)
+{
+ prepare_layout(_metadata);
+
+ create_file(_metadata, TMP_DIR "/s1d1/s1d2/s1d3/s1d4/f1");
+ create_directory(_metadata, TMP_DIR "/s2d1");
+ create_directory(_metadata, TMP_DIR "/s3d1");
+
+ set_cap(_metadata, CAP_SYS_ADMIN);
+ ASSERT_EQ(0,
+ mount(TMP_DIR "/s1d1", TMP_DIR "/s2d1", NULL, MS_BIND, NULL));
+ clear_cap(_metadata, CAP_SYS_ADMIN);
+}
+
+FIXTURE_TEARDOWN_PARENT(layout_disconnected_special)
+{
+ /* umount(TMP_DIR "/s2d1") is handled by namespace lifetime. */
+
+ remove_path(TMP_DIR "/s1d1/s1d2/s1d3/s1d4/f1");
+
+ cleanup_layout(_metadata);
+}
+
+TEST_F_FORK(layout_disconnected_special, disconnected_special)
+{
+ const __u64 access =
+ LANDLOCK_ACCESS_FS_REFER | LANDLOCK_ACCESS_FS_MAKE_REG |
+ LANDLOCK_ACCESS_FS_REMOVE_FILE | LANDLOCK_ACCESS_FS_READ_FILE;
+ const struct rule rules[] = {
+ {
+ .path = TMP_DIR "/s1d1/s1d2",
+ .access = access,
+ },
+ {
+ .path = TMP_DIR "/s3d1",
+ .access = access,
+ },
+ {},
+ };
+ int s1d4_bind_fd, ruleset_fd;
+
+ s1d4_bind_fd = open(TMP_DIR "/s2d1/s1d2/s1d3/s1d4", O_PATH | O_CLOEXEC);
+ EXPECT_LE(0, s1d4_bind_fd);
+
+ ruleset_fd = create_ruleset(_metadata, access, rules);
+ ASSERT_LE(0, ruleset_fd);
+
+ /* Make it disconnected. */
+ EXPECT_EQ(0, renameat(AT_FDCWD, TMP_DIR "/s1d1/s1d2/s1d3", AT_FDCWD,
+ TMP_DIR "/s3d1/s1d3"));
+
+ /* Check it's disconnected. */
+ ASSERT_EQ(ENOENT, test_open_rel(s1d4_bind_fd, "..", O_DIRECTORY));
+
+ enforce_ruleset(_metadata, ruleset_fd);
+ EXPECT_EQ(0, close(ruleset_fd));
+
+ EXPECT_EQ(0, renameat(s1d4_bind_fd, "f1", AT_FDCWD,
+ TMP_DIR "/s2d1/s1d2/f1"));
+ EXPECT_EQ(0, test_open(TMP_DIR "/s2d1/s1d2/f1", O_RDONLY));
+ EXPECT_EQ(0, renameat(AT_FDCWD, TMP_DIR "/s2d1/s1d2/f1", s1d4_bind_fd,
+ "f1"));
+ EXPECT_EQ(0, test_open(TMP_DIR "/s3d1/s1d3/s1d4/f1", O_RDONLY));
+}
+
#define LOWER_BASE TMP_DIR "/lower"
#define LOWER_DATA LOWER_BASE "/data"
static const char lower_fl1[] = LOWER_DATA "/fl1";
Output:
root at 5610c72ba8a0 /t/landlock# cp /linux/tools/testing/selftests/landlock/ /tmp -r
root at 5610c72ba8a0 /t/landlock# ./fs_test -t disconnected_special
TAP version 13
1..1
# Starting 1 tests from 1 test cases.
# RUN layout_disconnected_special.disconnected_special ...
# OK layout_disconnected_special.disconnected_special
ok 1 layout_disconnected_special.disconnected_special
# PASSED: 1 / 1 tests passed.
# Totals: pass:1 fail:0 xfail:0 xpass:0 skip:0 error:0
(I think this is similar to an existing test case, but if you think it's
worth having, feel free to add it to the commit (maybe it needs a better
name than disconnected_special))
I tested this manually initially which would have been on virtiofs instead
of tmpfs, and the behaviour is the same - rename was allowed.
> whereas
> the source and destination had and still have REFER in their
> hierarchies. This is because s3d1 and s1d2 are evaluated for
> [fd:s1d4]/f1. We could have a similar scenario for the destination and
> for both.
>
> [...]
>> An interesting concrete example I came up with:
>>
>> /# uname -a
>> Linux 5610c72ba8a0 6.16.0-rc2-dev #43 SMP ...
>> /# mkdir /a /b
>> /# mkdir /a/a1 /b/b1
>> /# mount -t tmpfs none /a/a1
>> /# mkdir /a/a1/a11
>> /# mount --bind /a/a1/a11 /b/b1
>> /# mkdir /a/a1/a11/a111
>> /# tree /a /b
>> /a
>> `-- a1
>> `-- a11
>> `-- a111
>> /b
>> `-- b1
>> `-- a111
>>
>> 7 directories, 0 files
>> /# cd /b/b1/a111/
>> /b/b1/a111# mv /a/a1/a11/a111 /a/a1/a12
>> /b/b1/a111# ls .. # we're disconnected now
>> ls: cannot access '..': No such file or directory
>> /b/b1/a111 [2]# touch /a/a1/a12/file
>>
>> /b/b1/a111# LL_FS_RO=/:/a/a1 LL_FS_RW=/:/b/b1 /sandboxer ls
>> Executing the sandboxed command...
>> file
>>
>> /b/b1/a111# LL_FS_RO=/:/a/a1 LL_FS_RW=/:/b/b1 /sandboxer mv -v file file2
>> Executing the sandboxed command...
>> mv: cannot move 'file' to 'file2': Permission denied
>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> # This fails because for same dir rename we just use is_access_to_path_allowed,
>> # which will stop at /a/a1 (and thus never reach either /b/b1 or /).
>
> Good example, and this rename should probably be allowed because the
> root directory allows REFER.
Well, it is disallowed for the same reason why a read to [disconnected
cwd]/file would be disallowed, even if root has an allow everything rule -
landlock never gets to the root because this is on a separate filesystem,
and so if the path is disconnected it can't get out of it.
>
>>
>> /b/b1/a111 [1]# mkdir subdir
>> /b/b1/a111# LL_FS_RO=/:/a/a1 LL_FS_RW=/b/b1 /sandboxer mv -v file subdir/file2
>> Executing the sandboxed command...
>> [..] WARNING: CPU: 1 PID: 656 at security/landlock/fs.c:1065 ...
>> renamed 'file' -> 'subdir/file2'
>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> # This works because now we restart walk from /b/b1 (the bind mnt)
>>
>> /b/b1/a111# mv subdir/file2 file
>> /b/b1/a111# LL_FS_RO=/:/a/a1 LL_FS_RW=/a /sandboxer mv -v file subdir/file2
>> Executing the sandboxed command...
>> mv: cannot move 'file' to 'subdir/file2': Permission denied
>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> # This is also not allowed, but that's OK since even though technically we're
>> # actually moving /a/a1/a12/file to /a/a1/a12/subdir/file2, we're not doing it
>> # through /a (we're walking into a12 via /b/b1, so rules on /a shouldn't
>> # apply anyway)
>
> Yes
>
>>
>> /b/b1/a111 [1]# LL_FS_RO=/:/a/a1 LL_FS_RW=/b /sandboxer mv -v file subdir/file2
>> Executing the sandboxed command...
>> renamed 'file' -> 'subdir/file2'
>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> # And this works because we walk from /b/b1 after doing collect_domain_accesses
>>
>> I think overall this is just a very strange edge case and people should
>> not rely on the exact behavior whether it's intentional or optimization
>> side-effect (as long as it deny access / renames when there is no rules at
>> any of the reasonable upper directories). Also, since as far as I can
>> tell this "optimization" only accidentally allows more access (i.e. rules
>> anywhere between the bind mountpoint to real root would apply, even if
>> technically the now disconnected directory belongs outside of the
>> mountpoint), I think it might be fine to leave it as-is, rather than
>> potentially complicating this code to deal with this quite unusual edge
>> case? (I mean, it's not exactly obvious to me whether it is more correct
>> to respect rules placed between the original bind mountpoint and root, or
>> more correct to ignore these rules (i.e. the behaviour of non-refer access
>> checks))
>
> I kind of agree, overall it's not really a security issue (if we
> consider the origin of files), but it may still be inconsistent for
> users in rare cases. Anyway, even if we don't want it, users could rely
> on this edge case (cf. Hyrum's law).
>
>>
>> It is a bit weird that `mv -v file file2` and `mv -v file subdir/file2`
>> behaves differently tho.
>
> Yes, `mv file file2` doesn't depends on REFER because it cannot impact a
> Landlock security policy. This behavior is a bit weird without kernel
> and Landlock knowledge though.
>
>>
>> If you would like to fix it, what do you think about my initial idea?:
>>> This might need more thinking, but maybe if one of the operands is
>>> disconnected, we can just let it walk until IS_ROOT(dentry), and also
>>> collect access for the other path until IS_ROOT(dentry), then call
>
> Until then, it would be unchanged, right?
Sorry, I'm not fully clear on what you meant by "until then", and what
would be unchanged, but on second thought I think this proposal is
problematic as it would mean that we won't follow_up on mountpoints even
for the other connected path (as collect_domain_accesses does not do
that).
>
>>> is_access_to_paths_allowed() passing in the root dentry we walked to? (In
>>> this case is_access_to_paths_allowed will not do any walking and just make
>>> an access decision.)
>
> Are you suggesting to not evaluate mnt_dir for disconnected paths? What
> about the case where both the source and the destination are
> disconnected?
Yes basically, but I think my proposal was problematic.
>
>>
>> This will basically make the refer checks behave the same as non-refer
>> checks on disconnected paths - walk until IS_ROOT, and stop there.
>
> I think it would make more sense indeed.
It would make sense if both sides are disconnected, but not if just one
side is. In that case we still want to walk the other connected path
normally.
More information about the Linux-security-module-archive
mailing list