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

["Mickaël Salaün" <mic@digikod.net>]

On Thu, Jul 24, 2025 at 04:49:24PM +0200, Günther Noack wrote:
> On Wed, Jul 23, 2025 at 11:01:42PM +0200, Mickaël Salaün wrote:
> > On Tue, Jul 22, 2025 at 07:04:02PM +0100, Tingmao Wang wrote:
> > > 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?
> 
> I have to admit that so far, I still failed to wrap my head around the
> full patch set and its possible corner cases.  I hope I did not
> misunderstand things all too badly below:
> 
> As far as I understand the proposed patch, we are "checkpointing" the
> intermediate results of the path walk at every mount point boundary,
> and in the case where we run into a disconnected directory in one of
> the nested mount points, we restore from the intermediate result at
> the previous mount point directory and skip to the next mount point.

Correct

> 
> Visually speaking, if the layout is this (where ":" denotes a
> mountpoint boundary between the mountpoints MP1, MP2, MP3):
> 
>                           dirfd
>                             |
>           :                 V         :
> 	  :       ham <--- spam <--- eggs <--- x.txt
> 	  :    (disconn.)             :
>           :                           :
>   / <--- foo <--- bar <--- baz        :
>           :                           :
>     MP1                 MP2                  MP3
> 
> When a process holds a reference to the "spam" directory, which is now
> disconnected, and invokes openat(dirfd, "eggs/x.txt", ...), then we
> would:
> 
>   * traverse x.txt
>   * traverse eggs (checkpointing the intermediate result) <-.
>   * traverse spam                                           |
>   * traverse ham                                            |
>   * discover that ham is disconnected:                      |
>      * restore the intermediate result from "eggs" ---------'
>      * continue the walk at foo
>   * end up at the root
> 
> So effectively, since the results from "spam" and "ham" are discarded,
> we would traverse only the inodes in the outer and inner mountpoints
> MP1 and MP3, but effectively return a result that looks like we did
> not traverse MP2?

We'd still check MP2's inode, but otherwise yes.

> 
> Maybe (likely) I misread the code. :) It's not clear to me what the
> thinking behind this is.  Also, if there was another directory in
> between "spam" and "eggs" in MP2, wouldn't we be missing the access
> rights attached to this directory?

Yes, we would ignore this access right because we don't know that the
path was resolved from spam.

> 
> 
> Regarding the capability approach:
> 
> I agree that a "capability" approach would be the better solution, but
> it seems infeasible with the existing LSM hooks at the moment.  I
> would be in favor of it though.

Yes, it would be a new feature with potential important changes.

In the meantime, we still need a fix for disconnected directories, and
this fix needs to be backported.  That's why the capability approach is
not part of the two solutions. ;)

> 
> To spell it out a bit more explicitly what that would mean in my mind:
> 
> When a path is looked up relative to a dirfd, the path walk upwards
> would terminate at the dirfd and use previously calculated access
> rights stored in the associated struct file.  These access rights
> would be determined at the time of opening the dirfd, similar to how we
> are already storing the "truncate" access right today for regular
> files.
> 
> (Remark: There might still be corner cases where we have to solve it
> the hard way, if someone uses ".." together with a dirfd-relative
> lookup.)

Yep, real capabilities don't have ".." in their design.  On Linux (and
Landlock), we need to properly handle "..", which is challenging.

> 
> I also looked at what it would take to change the LSM hooks to pass
> the directory that the lookup was done relative to, but it seems that
> this would have to be passed through a bunch of VFS callbacks as well,
> which seems like a larger change.  I would be curious whether that
> would be deemed an acceptable change.
> 
> —Günther
> 
> 
> P.S. Related to relative directory lookups, there is some movement in
> the BSDs as well to use dirfds as capabilities, by adding a flag to
> open directories that enforces O_BENEATH on subsequent opens:
> 
>  * https://undeadly.org/cgi?action=article;sid=20250529080623
>  * https://reviews.freebsd.org/D50371
> 
> (both found via https://news.ycombinator.com/item?id=44575361)
> 
> If a dirfd had such a flag, that would get rid of the corner case
> above.

This would be nice but it would not solve the current issue because we
cannot force all processes to use this flag (which breaks some use
cases).

FYI, Capsicum is a more complete implementation:
https://man.freebsd.org/cgi/man.cgi?query=capsicum&sektion=4
See the vfs.lookup_cap_dotdot sysctl too.