Does Landlock not work with eCryptfs?

[Tyler Hicks (Microsoft)]

On 2023-03-23 18:05:38, Günther Noack wrote:
> +ecryptfs mailing list FYI
> 
> Just some additional data points on the Landlock/eCryptfs issues.
> 
> On Tue, Mar 21, 2023 at 07:16:28PM +0100, Mickaël Salaün wrote:
> > On 21/03/2023 18:24, Christian Brauner wrote:
> > > On Tue, Mar 21, 2023 at 05:36:19PM +0100, Mickaël Salaün wrote:
> > > > There is an inconsistency between ecryptfs_dir_open() and ecryptfs_open().
> > > > ecryptfs_dir_open() actually checks access right to the lower directory,
> > > > which is why landlocked processes may not access the upper directory when
> > > > reading its content. ecryptfs_open() uses a cache for upper files (which
> > > > could be a problem on its own). The execution flow is:
> > > > 
> > > > ecryptfs_open() -> ecryptfs_get_lower_file() -> ecryptfs_init_lower_file()
> > > > -> ecryptfs_privileged_open()
> > > > 
> > > > In ecryptfs_privileged_open(), the dentry_open() call failed if access to
> > > > the lower file is not allowed by Landlock (or other access-control systems).
> > > > Then wait_for_completion(&req.done) waits for a kernel's thread executing
> > > > ecryptfs_threadfn(), which uses the kernel's credential to access the lower
> > > > file.
> > > > 
> > > > I think there are two main solutions to fix this consistency issue:
> > > > - store the mounter credentials and uses them instead of the kernel's
> > > > credentials for lower file and directory access checks (ecryptfs_dir_open
> > > > and ecryptfs_threadfn changes);
> > > > - use the kernel's credentials for all lower file/dir access check,
> > > > especially in ecryptfs_dir_open().
> > > > 
> > > > I think using the mounter credentials makes more sense, is much safer, and
> > > > fits with overlayfs. It may not work in cases where the mounter doesn't have
> > > > access to the lower file hierarchy though.
> > > > 
> > > > File creation calls vfs_*() helpers (lower directory) and there is not path
> > > > nor file security hook calls for those, so it works unconditionally.
> > > > 
> > > >  From Landlock end users point of view, it makes more sense to grants access
> > > > to a file hierarchy (where access is already allowed) and be allowed to
> > > > access this file hierarchy, whatever it belongs to a specific filesystem
> > > > (and whatever the potential lower file hierarchy, which may be unknown to
> > > > users). This is how it works for overlayfs and I'd like to have the same
> > > > behavior for ecryptfs.
> > > 
> > > So given that ecryptfs is marked as "Odd Fixes" who is realistically
> > > going to do the work of switching it to a mounter's credentials model,
> > > making sure this doesn't regress anything, and dealing with any
> > > potential bugs caused by this. It might be potentially better to just
> > > refuse to combine Landlock with ecryptfs if that's possible.
> 
> There is now a patch to mark it orphaned (independent of this thread):
> https://lore.kernel.org/all/20230320182103.46350-1-frank.li@vivo.com/

I have little time to devote to eCryptfs these days. I'm not sure it
needs to be fully orphaned but I think deprecation and marking for
removal is the responsible thing to do.

> > If Tyler is OK with the proposed solutions, I'll get a closer look at it in
> > a few months. If anyone is interested to work on that, I'd be happy to
> > review and test (the Landlock part).
> 
> I wonder whether this problem of calling security hooks for the
> underlying directory might have been affecting AppArmor and SELinux as
> well?  There seem to be reports on the web, but it's possible that I
> am misinterpreting some of them:

Yes, this eCryptfs design problem is common for other LSMs, as well.

Tyler

> 
> https://wiki.ubuntu.com/SecurityTeam/Roadmap
>   mentions this "unscheduled wishlist item":
>   "eCryptfs + SELinux/AppArmor integration, to protect encrypted data from root"
> 
> https://askubuntu.com/a/1195430
>   reports that AppArmor does not work on an eCryptfs mount for their use case
>   "i tried adding the [eCryptfs] source folder as an alias in apparmor and it now works."
> 
> —Günther
> 
> --