Re: [PATCH v2 07/12] landlock: Add support for file reparenting with LANDLOCK_ACCESS_FS_REFER

[Mickaël Salaün]

On 08/04/2022 03:42, Paul Moore wrote:
> On Tue, Mar 29, 2022 at 8:51 AM Mickaël Salaün <mic@xxxxxxxxxxx> wrote:
> > From: Mickaël Salaün <mic@xxxxxxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxx>
> > Signed-off-by: Mickaël Salaün <mic@xxxxxxxxxxxxxxxxxxx>
> > Link: https://lore.kernel.org/r/20220329125117.1393824-8-mic@xxxxxxxxxxx
> > ---
> >
> > 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@xxxxxxxxxxxxxx>

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