Message-ID: <CAHC9VhQpZ12Chgd+xMibUxgvcPjTn9FMnCdMGYbLcWG3eTqDQg@mail.gmail.com>
Date:   Thu, 7 Apr 2022 21:42:01 -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 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>

> +static inline access_mask_t init_layer_masks(
> +               const struct landlock_ruleset *const domain,
> +               const access_mask_t access_request,
> +               layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS])
> +{
> +       access_mask_t handled_accesses = 0;
> +       size_t layer_level;
> +
> +       memset(layer_masks, 0, sizeof(*layer_masks));
> +       /* An empty access request can happen because of O_WRONLY | O_RDWR. */

 ;)

-- 
paul-moore.com

Powered by blists - more mailing lists