Message-ID: <3a5495b8-5d69-e327-1dfc-7a99257269ae@digikod.net>
Date:   Fri, 8 Apr 2022 18:07:17 +0200
From:   Mickaël Salaün <mic@...ikod.net>
To:     Paul Moore <paul@...l-moore.com>
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 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.

> 
>> +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. */
> 
>   ;)
> 

Powered by blists - more mailing lists