* Re: Does Landlock not work with eCryptfs? [not found] <20230319.2139b35f996f@gnoack.org> @ 2023-03-19 21:00 ` Mickaël Salaün 2023-03-20 17:15 ` Günther Noack 0 siblings, 1 reply; 11+ messages in thread From: Mickaël Salaün @ 2023-03-19 21:00 UTC (permalink / raw) To: Günther Noack Cc: landlock, Tyler Hicks, linux-security-module, Linux-Fsdevel 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 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Does Landlock not work with eCryptfs? 2023-03-19 21:00 ` Does Landlock not work with eCryptfs? Mickaël Salaün @ 2023-03-20 17:15 ` Günther Noack 2023-03-20 17:21 ` Mickaël Salaün 0 siblings, 1 reply; 11+ messages in thread From: Günther Noack @ 2023-03-20 17:15 UTC (permalink / raw) To: Mickaël Salaün Cc: landlock, Tyler Hicks, linux-security-module, Linux-Fsdevel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Does Landlock not work with eCryptfs? 2023-03-20 17:15 ` Günther Noack @ 2023-03-20 17:21 ` Mickaël Salaün 2023-03-21 16:36 ` Mickaël Salaün 0 siblings, 1 reply; 11+ messages in thread From: Mickaël Salaün @ 2023-03-20 17:21 UTC (permalink / raw) To: Günther Noack Cc: landlock, Tyler Hicks, linux-security-module, Linux-Fsdevel 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, …) ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Does Landlock not work with eCryptfs? 2023-03-20 17:21 ` Mickaël Salaün @ 2023-03-21 16:36 ` Mickaël Salaün 2023-03-21 17:24 ` Christian Brauner 0 siblings, 1 reply; 11+ messages in thread From: Mickaël Salaün @ 2023-03-21 16:36 UTC (permalink / raw) To: Günther Noack Cc: landlock, Tyler Hicks, linux-security-module, Linux-Fsdevel, Christian Brauner, Al Viro 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, …) ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Does Landlock not work with eCryptfs? 2023-03-21 16:36 ` Mickaël Salaün @ 2023-03-21 17:24 ` Christian Brauner 2023-03-21 18:16 ` Mickaël Salaün 0 siblings, 1 reply; 11+ messages in thread From: Christian Brauner @ 2023-03-21 17:24 UTC (permalink / raw) To: Mickaël Salaün Cc: Günther Noack, landlock, Tyler Hicks, linux-security-module, Linux-Fsdevel, Al Viro 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Does Landlock not work with eCryptfs? 2023-03-21 17:24 ` Christian Brauner @ 2023-03-21 18:16 ` Mickaël Salaün 2023-03-23 17:05 ` Günther Noack 2023-03-24 22:53 ` Tyler Hicks 0 siblings, 2 replies; 11+ messages in thread From: Mickaël Salaün @ 2023-03-21 18:16 UTC (permalink / raw) To: Christian Brauner Cc: Günther Noack, landlock, Tyler Hicks, linux-security-module, Linux-Fsdevel, Al Viro 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). ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Does Landlock not work with eCryptfs? 2023-03-21 18:16 ` Mickaël Salaün @ 2023-03-23 17:05 ` Günther Noack 2023-03-24 22:45 ` Tyler Hicks (Microsoft) 2023-03-24 22:53 ` Tyler Hicks 1 sibling, 1 reply; 11+ messages in thread From: Günther Noack @ 2023-03-23 17:05 UTC (permalink / raw) To: Mickaël Salaün Cc: Christian Brauner, landlock, Tyler Hicks, linux-security-module, Linux-Fsdevel, Al Viro, eCryptfs +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 -- ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Does Landlock not work with eCryptfs? 2023-03-23 17:05 ` Günther Noack @ 2023-03-24 22:45 ` Tyler Hicks (Microsoft) 0 siblings, 0 replies; 11+ messages in thread From: Tyler Hicks (Microsoft) @ 2023-03-24 22:45 UTC (permalink / raw) To: Günther Noack Cc: Mickaël Salaün, Christian Brauner, landlock, linux-security-module, Linux-Fsdevel, Al Viro, eCryptfs 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 > > -- ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Does Landlock not work with eCryptfs? 2023-03-21 18:16 ` Mickaël Salaün 2023-03-23 17:05 ` Günther Noack @ 2023-03-24 22:53 ` Tyler Hicks 2023-03-26 21:19 ` Mickaël Salaün 1 sibling, 1 reply; 11+ messages in thread From: Tyler Hicks @ 2023-03-24 22:53 UTC (permalink / raw) To: Mickaël Salaün Cc: Christian Brauner, Günther Noack, landlock, linux-security-module, Linux-Fsdevel, Al Viro 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Does Landlock not work with eCryptfs? 2023-03-24 22:53 ` Tyler Hicks @ 2023-03-26 21:19 ` Mickaël Salaün 2023-03-27 21:01 ` Günther Noack 0 siblings, 1 reply; 11+ messages in thread From: Mickaël Salaün @ 2023-03-26 21:19 UTC (permalink / raw) To: Tyler Hicks Cc: Christian Brauner, Günther Noack, landlock, linux-security-module, Linux-Fsdevel, Al Viro 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Does Landlock not work with eCryptfs? 2023-03-26 21:19 ` Mickaël Salaün @ 2023-03-27 21:01 ` Günther Noack 0 siblings, 0 replies; 11+ messages in thread From: Günther Noack @ 2023-03-27 21:01 UTC (permalink / raw) To: Mickaël Salaün Cc: Tyler Hicks, Christian Brauner, landlock, linux-security-module, Linux-Fsdevel, Al Viro 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2023-03-27 21:01 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20230319.2139b35f996f@gnoack.org> 2023-03-19 21:00 ` Does Landlock not work with eCryptfs? Mickaël Salaün 2023-03-20 17:15 ` Günther Noack 2023-03-20 17:21 ` Mickaël Salaün 2023-03-21 16:36 ` Mickaël Salaün 2023-03-21 17:24 ` Christian Brauner 2023-03-21 18:16 ` Mickaël Salaün 2023-03-23 17:05 ` Günther Noack 2023-03-24 22:45 ` Tyler Hicks (Microsoft) 2023-03-24 22:53 ` Tyler Hicks 2023-03-26 21:19 ` Mickaël Salaün 2023-03-27 21:01 ` Günther Noack
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).