Message-ID: <CAHC9VhS0bYe9wOxuXoC2mw_K2g=Fw=LXiV+A_Z1vH_KqH-TBFA@mail.gmail.com>
Date:   Fri, 8 Apr 2022 13:13:59 -0400
From:   Paul Moore <paul@...l-moore.com>
To:     Mickaël Salaün <mic@...ikod.net>
Cc:     James Morris <jmorris@...ei.org>,
        "Serge E . Hallyn" <serge@...lyn.com>,
        Al Viro <viro@...iv.linux.org.uk>,
        Jann Horn <jannh@...gle.com>,
        John Johansen <john.johansen@...onical.com>,
        Kees Cook <keescook@...omium.org>,
        Konstantin Meskhidze <konstantin.meskhidze@...wei.com>,
        Shuah Khan <shuah@...nel.org>,
        Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>,
        linux-doc@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        linux-security-module@...r.kernel.org,
        Mickaël Salaün <mic@...ux.microsoft.com>
Subject: Re: [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@...ikod.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@...ikod.net> wrote:
> >>
> >> From: Mickaël Salaün <mic@...ux.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@...l-moore.com>
> >> Signed-off-by: Mickaël Salaün <mic@...ux.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@...l-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

Powered by blists - more mailing lists