Does Landlock not work with eCryptfs?

Does Landlock not work with eCryptfs?

[Günther Noack]

Hello!

I have a machine where the home directory is encrypted with eCryptfs,
and it seems that Landlock is not working properly on eCryptfs files
(but the same program works as expected on other mounts)?


## Problem description

Steps to reproduce:

  * Create a directory "subdir" in the current directory
  * Enable Landlock but ask for "subdir" to be readable
  * os.ReadDir(dir)

Observed result:

* os.ReadDir function fails when trying to open the file (verified with strace)

Expected result:

* os.ReadDir should work, because we asked for it to work when enabling Landlock


## Reproduction code

I have uploaded a reproduction program in Go to Github,
which should be understandable also if you are primarily a C user:
https://github.com/gnoack/llecryptfsrepro/blob/main/repro.go

To build and run the reproduction code, run:

  git clone https://github.com/gnoack/llecryptfsrepro
  cd llecryptfsrepro
  go build
  ./llecryptfsrepro  # executes the three steps as above, check source code

You can invoke this binary in different file system types to see the difference.


I have admittedly only checked it with a distribution kernel on
Manjaro Linux: The Linux version is 6.2.2-1-MANJARO.

This looks like a bug to me?
Is this a known issue?

Thanks,
–Günther

Re: Does Landlock not work with eCryptfs?

[Mickaël Salaün]

Hi Günther,

Thanks for the report, I confirm there is indeed a bug. I tested with a 
Debian distro:

ecryptfs-setup-private --nopwcheck --noautomount
ecryptfs-mount-private
# And then with the kernel's sample/landlock/sandboxer:
LL_FS_RO="/usr" LL_FS_RW="${HOME}/Private" sandboxer ls ~/Private
ls: cannot open directory '/home/user/Private': Permission denied

Actions other than listing a directory (e.g. creating files/directories, 
reading/writing to files) are controlled as expected. The issue might be 
that directories' inodes are not the same when listing the content of a 
directory or when creating new files/directories (which is weird). My 
hypothesis is that Landlock would then deny directory reading because 
the directory's inode doesn't match any rule. It might be related to the 
overlay nature of ecryptfs.

Tyler, do you have some idea?

FYI, I sent a patch fixing hostfs's inode management: 
https://lore.kernel.org/all/20230309165455.175131-2-mic@digikod.net/

Regards,
  Mickaël


On 19/03/2023 16:56, Günther Noack wrote:
> Hello!
> 
> I have a machine where the home directory is encrypted with eCryptfs,
> and it seems that Landlock is not working properly on eCryptfs files
> (but the same program works as expected on other mounts)?
> 
> 
> ## Problem description
> 
> Steps to reproduce:
> 
>    * Create a directory "subdir" in the current directory
>    * Enable Landlock but ask for "subdir" to be readable
>    * os.ReadDir(dir)
> 
> Observed result:
> 
> * os.ReadDir function fails when trying to open the file (verified with strace)
> 
> Expected result:
> 
> * os.ReadDir should work, because we asked for it to work when enabling Landlock
> 
> 
> ## Reproduction code
> 
> I have uploaded a reproduction program in Go to Github,
> which should be understandable also if you are primarily a C user:
> https://github.com/gnoack/llecryptfsrepro/blob/main/repro.go
> 
> To build and run the reproduction code, run:
> 
>    git clone https://github.com/gnoack/llecryptfsrepro
>    cd llecryptfsrepro
>    go build
>    ./llecryptfsrepro  # executes the three steps as above, check source code
> 
> You can invoke this binary in different file system types to see the difference.
> 
> 
> I have admittedly only checked it with a distribution kernel on
> Manjaro Linux: The Linux version is 6.2.2-1-MANJARO.
> 
> This looks like a bug to me?
> Is this a known issue?
> 
> Thanks,
> –Günther
> 

Re: Does Landlock not work with eCryptfs?

[Günther Noack]

Hello!

On Sun, Mar 19, 2023 at 10:00:46PM +0100, Mickaël Salaün wrote:
> Hi Günther,
> 
> Thanks for the report, I confirm there is indeed a bug. I tested with a
> Debian distro:
> 
> ecryptfs-setup-private --nopwcheck --noautomount
> ecryptfs-mount-private
> # And then with the kernel's sample/landlock/sandboxer:
> LL_FS_RO="/usr" LL_FS_RW="${HOME}/Private" sandboxer ls ~/Private
> ls: cannot open directory '/home/user/Private': Permission denied
> 
> Actions other than listing a directory (e.g. creating files/directories,
> reading/writing to files) are controlled as expected. The issue might be
> that directories' inodes are not the same when listing the content of a
> directory or when creating new files/directories (which is weird). My
> hypothesis is that Landlock would then deny directory reading because the
> directory's inode doesn't match any rule. It might be related to the overlay
> nature of ecryptfs.
> 
> Tyler, do you have some idea?

I had a hunch, and found out that the example can be made to work by
granting the LANDLOCK_ACCESS_FS_READ_DIR right on the place where the
*encrypted* version of that home directory lives:

  err := landlock.V1.RestrictPaths(
          landlock.RODirs(dir),
          landlock.PathAccess(llsys.AccessFSReadDir, "/home/.ecryptfs/gnoack/.Private"),
  )

It does seem a bit like eCryptfs it calling security_file_open() under
the hood for the encrypted version of that file? Is that correct?

–Günther

Re: Does Landlock not work with eCryptfs?

[Mickaël Salaün]

On 20/03/2023 18:15, Günther Noack wrote:
> Hello!
> 
> On Sun, Mar 19, 2023 at 10:00:46PM +0100, Mickaël Salaün wrote:
>> Hi Günther,
>>
>> Thanks for the report, I confirm there is indeed a bug. I tested with a
>> Debian distro:
>>
>> ecryptfs-setup-private --nopwcheck --noautomount
>> ecryptfs-mount-private
>> # And then with the kernel's sample/landlock/sandboxer:
>> LL_FS_RO="/usr" LL_FS_RW="${HOME}/Private" sandboxer ls ~/Private
>> ls: cannot open directory '/home/user/Private': Permission denied
>>
>> Actions other than listing a directory (e.g. creating files/directories,
>> reading/writing to files) are controlled as expected. The issue might be
>> that directories' inodes are not the same when listing the content of a
>> directory or when creating new files/directories (which is weird). My
>> hypothesis is that Landlock would then deny directory reading because the
>> directory's inode doesn't match any rule. It might be related to the overlay
>> nature of ecryptfs.
>>
>> Tyler, do you have some idea?
> 
> I had a hunch, and found out that the example can be made to work by
> granting the LANDLOCK_ACCESS_FS_READ_DIR right on the place where the
> *encrypted* version of that home directory lives:
> 
>    err := landlock.V1.RestrictPaths(
>            landlock.RODirs(dir),
>            landlock.PathAccess(llsys.AccessFSReadDir, "/home/.ecryptfs/gnoack/.Private"),
>    )
> 
> It does seem a bit like eCryptfs it calling security_file_open() under
> the hood for the encrypted version of that file? Is that correct?

Yes, that's right, the lower directory is used to list the content of 
the ecryptfs directory: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/ecryptfs/file.c#n112 
iterate_dir(lower_file, …)

Re: Does Landlock not work with eCryptfs?

[Mickaël Salaün]

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.


On 20/03/2023 18:21, Mickaël Salaün wrote:
> 
> On 20/03/2023 18:15, Günther Noack wrote:
>> Hello!
>>
>> On Sun, Mar 19, 2023 at 10:00:46PM +0100, Mickaël Salaün wrote:
>>> Hi Günther,
>>>
>>> Thanks for the report, I confirm there is indeed a bug. I tested with a
>>> Debian distro:
>>>
>>> ecryptfs-setup-private --nopwcheck --noautomount
>>> ecryptfs-mount-private
>>> # And then with the kernel's sample/landlock/sandboxer:
>>> LL_FS_RO="/usr" LL_FS_RW="${HOME}/Private" sandboxer ls ~/Private
>>> ls: cannot open directory '/home/user/Private': Permission denied
>>>
>>> Actions other than listing a directory (e.g. creating files/directories,
>>> reading/writing to files) are controlled as expected. The issue might be
>>> that directories' inodes are not the same when listing the content of a
>>> directory or when creating new files/directories (which is weird). My
>>> hypothesis is that Landlock would then deny directory reading because the
>>> directory's inode doesn't match any rule. It might be related to the overlay
>>> nature of ecryptfs.
>>>
>>> Tyler, do you have some idea?
>>
>> I had a hunch, and found out that the example can be made to work by
>> granting the LANDLOCK_ACCESS_FS_READ_DIR right on the place where the
>> *encrypted* version of that home directory lives:
>>
>>     err := landlock.V1.RestrictPaths(
>>             landlock.RODirs(dir),
>>             landlock.PathAccess(llsys.AccessFSReadDir, "/home/.ecryptfs/gnoack/.Private"),
>>     )
>>
>> It does seem a bit like eCryptfs it calling security_file_open() under
>> the hood for the encrypted version of that file? Is that correct?
> 
> Yes, that's right, the lower directory is used to list the content of
> the ecryptfs directory:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/ecryptfs/file.c#n112
> iterate_dir(lower_file, …)

Re: Does Landlock not work with eCryptfs?

[Christian Brauner]

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.

Re: Does Landlock not work with eCryptfs?

[Mickaël Salaün]

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

Re: Does Landlock not work with eCryptfs?

[Günther Noack]

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

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

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

-- 

Re: 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
> 
> -- 

Re: Does Landlock not work with eCryptfs?

[Tyler Hicks]

On 2023-03-21 19:16:28, 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.
> 
> 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 am alright with using the mounter creds but eCryptfs is typically
mounted as root using a PAM module so the mounter is typically going to
be more privileged than the user accessing the eCryptfs mount (in the
common case of an encrypted home directory).

But, as Christian points out, I think it might be better to make
Landlock refuse to work with eCryptfs. Would you be at ease with that
decision if we marked eCryptfs as deprecated with a planned removal
date?

Tyler

Re: Does Landlock not work with eCryptfs?

[Mickaël Salaün]

On 24/03/2023 23:53, Tyler Hicks wrote:
> On 2023-03-21 19:16:28, 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.
>>
>> 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 am alright with using the mounter creds but eCryptfs is typically
> mounted as root using a PAM module so the mounter is typically going to
> be more privileged than the user accessing the eCryptfs mount (in the
> common case of an encrypted home directory).
> 
> But, as Christian points out, I think it might be better to make
> Landlock refuse to work with eCryptfs. Would you be at ease with that
> decision if we marked eCryptfs as deprecated with a planned removal
> date?

The only way to make Landlock "refuse" to work with eCryptfs would be to 
add exceptions according to the underlying filesystem when creating 
rules. Furthermore, to be consistent, this would need to be backported. 
I don't want to go in such direction to fix an eCryptfs issue.

Doing nothing would go against the principle of least astonishment 
because of unexpected and inconsistent behavior when using Landlock with 
eCryptfs. Indeed, Landlock users (e.g. app developers) may not know how, 
where, and on which kernel their sandboxed applications will run. This 
means that at best, developers will (potentially wrongly) check if 
eCryptfs is available/used and then disable sandboxing, and at worse the 
(opportunistically) sandboxed apps will break (because of denied 
access). In any case, it goes against user's interests.

Even if eCryptfs is marked as deprecated, it will be available for years 
(a lot for LTS) and (legitimate) bug reports will keep coming.

Instead, I'd like to fix the eCryptfs inconsistent behavior (highlighted 
by Landlock and other LSMs). I think it's worth trying the first 
proposed solution, which might not be too difficult to implement, and 
will get eCryptfs closer to the overlayfs semantic.

Re: Does Landlock not work with eCryptfs?

[Günther Noack]

Hello!

On Sun, Mar 26, 2023 at 11:19:19PM +0200, Mickaël Salaün wrote:
> On 24/03/2023 23:53, Tyler Hicks wrote:
> > On 2023-03-21 19:16:28, Mickaël Salaün wrote:
> > > 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 am alright with using the mounter creds but eCryptfs is typically
> > mounted as root using a PAM module so the mounter is typically going to
> > be more privileged than the user accessing the eCryptfs mount (in the
> > common case of an encrypted home directory).
> > 
> > But, as Christian points out, I think it might be better to make
> > Landlock refuse to work with eCryptfs. Would you be at ease with that
> > decision if we marked eCryptfs as deprecated with a planned removal
> > date?
> 
> The only way to make Landlock "refuse" to work with eCryptfs would be to add
> exceptions according to the underlying filesystem when creating rules.
> Furthermore, to be consistent, this would need to be backported. I don't
> want to go in such direction to fix an eCryptfs issue.

Here is an example where the Landlock LSM can't detect eCryptfs:

  mkdir -p /foo/bar/baz /foo/secret
  landlock-restrict -ro / -rw /foo/bar -- /bin/bash
  
  # finally, in a different terminal:
  mount.ecryptfs /foo/secret /foo/bar/baz

The shell is supposed to have access to /foo/bar and everything below
it, but at the time of creating the Landlock ruleset, it can't tell
yet that this directory will contain an eCryptfs mount later.

Admittedly, the example is obscure, but it's strictly speaking
supposed to work according to the Landlock documentation.  (Only the
landlocked process can't use mount(2) any more, but other processes
still can.)

Note on the side: Even when mount.ecryptfs happens before the Landlock
restriction, the Landlock LSM would have to traverse the existing
mounts to see if there is an eCryptfs mount *below* /foo/bar.

> Doing nothing would go against the principle of least astonishment because
> of unexpected and inconsistent behavior when using Landlock with eCryptfs.
> Indeed, Landlock users (e.g. app developers) may not know how, where, and on
> which kernel their sandboxed applications will run. This means that at best,
> developers will (potentially wrongly) check if eCryptfs is available/used
> and then disable sandboxing, and at worse the (opportunistically) sandboxed
> apps will break (because of denied access). In any case, it goes against
> user's interests.

I agree, I also believe that a kernel-side fix is needed.

I don't think this is feasible to do in a good way in userspace, even
if we want to resort to "falling back to doing nothing" in best effort
mode if eCryptfs file hierarchies are affected.

I have pondered these userspace approaches how to detect eCryptfs, but
they are both lacking:

* Looking for eCryptfs in /proc/mounts might not work
  if we are layering multiple Landlock rulesets.

* stat(2) also does not give away whether a file is on eCryptfs
  (the device number is just a generic kernel internal one)

Finally, it all falls apart anyway if we want to support the case
where eCryptfs is mounted after enabling the sandbox.  (Obscure, but
possible.)

> Even if eCryptfs is marked as deprecated, it will be available for years (a
> lot for LTS) and (legitimate) bug reports will keep coming.
> 
> Instead, I'd like to fix the eCryptfs inconsistent behavior (highlighted by
> Landlock and other LSMs). I think it's worth trying the first proposed
> solution, which might not be too difficult to implement, and will get
> eCryptfs closer to the overlayfs semantic.

+1. As you also said: I think it's important to fix it in the LTS
releases, so that we can tell people to use Landlock without having to
think about these corner cases.

–Günther