[PATCH] selftests/landlock: Add tests for access through disconnected paths

[Mickaël Salaün]

On Tue, Jun 24, 2025 at 12:16:55AM +0100, Tingmao Wang wrote:
> On 6/23/25 20:40, Mickaël Salaün wrote:
> > On Sun, Jun 22, 2025 at 04:42:49PM +0100, Tingmao Wang wrote:
> >> On 6/19/25 12:38, Mickaël Salaün wrote:
> >>> On Sat, Jun 14, 2025 at 07:25:02PM +0100, Tingmao Wang wrote:
> >>>> [...]
> >>>>
> >>>> 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
> >>>> 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.)
> >>>
> >>> If one side is in a disconnected directory and not the other side, the
> >>> rename would be denied by the VFS,
> >>
> >> Not always, right? For example in the path_disconnected_rename test we did:
> > 
> > Correct, only the mount point matter.
> > 
> >>
> >> 5051.  ASSERT_EQ(0, renameat(bind_s1d3_fd, file2_name, AT_FDCWD, file1_s2d2))
> >>                              ^^^^^^^^^^^^^^^^^^^^^^^^  ^^^^^^^^^^^^^^^^^^^^
> >>                              Disconnected              Connected
> >>
> >> (and it also has the other way)
> >>
> >> So looks like as long as they are still reached from two fds with two
> >> paths that have the same mnt, it will be allowed.  It's just that when we
> >> do parent walk we end up missing the mount.  This also means that for this
> >> refer check, if after doing the two separate walks (with the disconnected
> >> side walking all the way to IS_ROOT), we then walk from mnt again, we
> >> would allow the rename if there is a rule on mnt (or its parents) allowing
> >> file creation and refers, even if the disconnected side technically now
> >> lives outside the file hierarchy under mnt and does not have a parent with
> >> a rule allowing file creation.
> >>
> >> (I'm not saying this is necessary wrong or needs fixing, but I think it's
> >> an interesting consequence of the current implementation.)
> > 
> > Hmm, that's indeed a very subtle side effect.  One issue with the
> > current implementation is that if a directory between the mount
> > point and the source has REFER, and another directory not part of the
> > source hierarchy but part of the disconnected directory's hierarchy has
> > REFER and no other directory has REFER, and either the source or the
> > destination hierarchy is disconnected between the mount point and the
> > directory with the REFER, then Landlock will still deny such
> > rename/link.  A directory with REFER initially between the mount point
> > and the disconnected directory would also be ignored.  There is also the
> > case where both the source and the destination are disconnected.
> 
> Sorry, I'm having trouble following this.  Can you maybe give a more
> specific example, perhaps with commands?

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 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.


> 
> By "mount point" do you mean the bind mount? If a path has became
> disconnected because the directory moved away from under the mountpoint,
> and is therefore not covered by any REFER (you said "either the source or
> the destination hierarchy is disconnected between the mount point and the
> directory with the REFER") wouldn't it make sense for the rename to be
> denied?

Yes if the new hierarchy (e.g. s3d1 in my example) doesn't have REFER.

> 
> > 
> > I didn't consider such cases with collect_domain_accesses().  I'm
> > wondering if this path walk gap should be fixed (instead of applying
> > https://lore.kernel.org/all/20250618134734.1673254-1-mic@digikod.net/ )
> > or not.  We should not rely on optimization side effects, but I'm not
> > sure which behavior would make more sense...  Any though?
> 
> I didn't quite understand your example above and how is it possible for us
> to end up denying something that should be allowed.  My understanding of
> the current implementation is, when either operands are disconnected, it
> will walk all the way to the current filesystem's root and stop there.
> However, it will then still do the walk from the original bind mount up to
> the real root (/), and if there is any REFER rules on that path, we will
> still allow the rename.  This means that if the rename still ends up being
> denied, then it wouldn't have been allowed in the first place, even if the
> path has not become disconnected.
> 
> 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.

> 
> /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?

> > 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?

> 
> 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.