On Fri, Apr 8, 2022 at 12:07 PM Mickaël Salaün <mic@xxxxxxxxxxx> wrote: > On 08/04/2022 03:42, Paul Moore wrote: > > On Tue, Mar 29, 2022 at 8:51 AM Mickaël Salaün <mic@xxxxxxxxxxx> wrote: > >> > >> From: Mickaël Salaün <mic@xxxxxxxxxxxxxxxxxxx> > >> > >> Add a new LANDLOCK_ACCESS_FS_REFER access right to enable policy writers > >> to allow sandboxed processes to link and rename files from and to a > >> specific set of file hierarchies. This access right should be composed > >> with LANDLOCK_ACCESS_FS_MAKE_* for the destination of a link or rename, > >> and with LANDLOCK_ACCESS_FS_REMOVE_* for a source of a rename. This > >> lift a Landlock limitation that always denied changing the parent of an > >> inode. > >> > >> Renaming or linking to the same directory is still always allowed, > >> whatever LANDLOCK_ACCESS_FS_REFER is used or not, because it is not > >> considered a threat to user data. > >> > >> However, creating multiple links or renaming to a different parent > >> directory may lead to privilege escalations if not handled properly. > >> Indeed, we must be sure that the source doesn't gain more privileges by > >> being accessible from the destination. This is handled by making sure > >> that the source hierarchy (including the referenced file or directory > >> itself) restricts at least as much the destination hierarchy. If it is > >> not the case, an EXDEV error is returned, making it potentially possible > >> for user space to copy the file hierarchy instead of moving or linking > >> it. > >> > >> Instead of creating different access rights for the source and the > >> destination, we choose to make it simple and consistent for users. > >> Indeed, considering the previous constraint, it would be weird to > >> require such destination access right to be also granted to the source > >> (to make it a superset). Moreover, RENAME_EXCHANGE would also add to > >> the confusion because of paths being both a source and a destination. > >> > >> See the provided documentation for additional details. > >> > >> New tests are provided with a following commit. > >> > >> Cc: Paul Moore <paul@xxxxxxxxxxxxxx> > >> Signed-off-by: Mickaël Salaün <mic@xxxxxxxxxxxxxxxxxxx> > >> Link: https://lore.kernel.org/r/20220329125117.1393824-8-mic@xxxxxxxxxxx > >> --- > >> > >> Changes since v1: > >> * Update current_check_access_path() to efficiently handle > >> RENAME_EXCHANGE thanks to the updated LSM hook (see previous patch). > >> Only one path walk is performed per rename arguments until their > >> common mount point is reached. Superset of access rights is correctly > >> checked, including when exchanging a file with a directory. This > >> requires to store another matrix of layer masks. > >> * Reorder and rename check_access_path_dual() arguments in a more > >> generic way: switch from src/dst to 1/2. This makes it easier to > >> understand the RENAME_EXCHANGE cases alongs with the others. Update > >> and improve check_access_path_dual() documentation accordingly. > >> * Clean up the check_access_path_dual() loop: set both allowed_parent* > >> when reaching internal filesystems and remove a useless one. This > >> allows potential renames in internal filesystems (like for other > >> operations). > >> * Move the function arguments checks from BUILD_BUG_ON() to > >> WARN_ON_ONCE() to avoid clang build error. > >> * Rename is_superset() to no_more_access() and make it handle superset > >> checks of source and destination for simple and exchange cases. > >> * Move the layer_masks_child* creation from current_check_refer_path() > >> to check_access_path_dual(): this is simpler and less error-prone, > >> especially with RENAME_EXCHANGE. > >> * Remove one optimization in current_check_refer_path() to make the code > >> simpler, especially with the RENAME_EXCHANGE handling. > >> * Remove overzealous WARN_ON_ONCE() for !access_request check in > >> init_layer_masks(). > >> --- > >> include/uapi/linux/landlock.h | 27 +- > >> security/landlock/fs.c | 607 ++++++++++++++++--- > >> security/landlock/limits.h | 2 +- > >> security/landlock/syscalls.c | 2 +- > >> tools/testing/selftests/landlock/base_test.c | 2 +- > >> tools/testing/selftests/landlock/fs_test.c | 3 +- > >> 6 files changed, 566 insertions(+), 77 deletions(-) > > > > I'm still not going to claim that I'm a Landlock expert, but this > > looks sane to me. > > > > Reviewed-by: Paul Moore <paul@xxxxxxxxxxxxxx> > > Thanks Paul! I'll send a small update shortly, with some typo fixes, > some unlikely() calls, and rebased on the other Landlock patch series. Since it sounds like those are all pretty minor changes, feel free to preserve my 'Reviewed-by' on the respun patch. -- paul-moore.com
