lkml.org 
[lkml]   [2022]   [Apr]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v2 07/12] landlock: Add support for file reparenting with LANDLOCK_ACCESS_FS_REFER
On Fri, Apr 8, 2022 at 12:07 PM Mickaël Salaün <mic@digikod.net> wrote:
> On 08/04/2022 03:42, Paul Moore wrote:
> > On Tue, Mar 29, 2022 at 8:51 AM Mickaël Salaün <mic@digikod.net> wrote:
> >>
> >> From: Mickaël Salaün <mic@linux.microsoft.com>
> >>
> >> 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@paul-moore.com>
> >> Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
> >> Link: https://lore.kernel.org/r/20220329125117.1393824-8-mic@digikod.net
> >> ---
> >>
> >> 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@paul-moore.com>
>
> 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

\
 
 \ /
  Last update: 2022-04-08 19:14    [W:0.077 / U:0.544 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site