Re: Does Landlock not work with eCryptfs?

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.

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