lkml.org 
[lkml]   [2022]   [Apr]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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@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.

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

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