Re: [PATCH v1 06/11] landlock: Add support for file reparenting with LANDLOCK_ACCESS_FS_REFER

[Paul Moore <paul@paul-moore.com>]

On Thu, Mar 17, 2022 at 8:03 AM Mickaël Salaün <mic@digikod.net> wrote:
> On 17/03/2022 02:26, Paul Moore wrote:
> > On Mon, Feb 21, 2022 at 4:15 PM 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).
> >>
> >> See the provided documentation for additional details.
> >>
> >> New tests are provided with a following commit.
> >>
> >> Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
> >> Link: https://lore.kernel.org/r/20220221212522.320243-7-mic@digikod.net
> >> ---
> >>   include/uapi/linux/landlock.h                |  27 +-
> >>   security/landlock/fs.c                       | 550 ++++++++++++++++---
> >>   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, 516 insertions(+), 70 deletions(-)

...

> >> +/*
> >> + * Returns true if there is at least one access right different than
> >> + * LANDLOCK_ACCESS_FS_REFER.
> >> + */
> >> +static inline bool is_eacces(
> >> +               const layer_mask_t (*const
> >> +                       layer_masks)[LANDLOCK_NUM_ACCESS_FS],
> >>                  const access_mask_t access_request)
> >>   {
> >
> > Granted, I don't have as deep of an understanding of Landlock as you
> > do, but the function name "is_eacces" seems a little odd given the
> > nature of the function.  Perhaps "is_fsrefer"?
>
> Hmm, this helper does multiple things which are necessary to know if we
> need to return -EACCES or -EXDEV. Renaming it to is_fsrefer() would
> require to inverse the logic and use boolean negations in the callers
> (because of ordering). Renaming to something like without_fs_refer()
> would not be completely correct because we also check if there is no
> layer_masks, which indicated that it doesn't contain an access right
> that should return -EACCES. This helper is named as such because the
> underlying semantic is to check for such error code, which is a tricky.
> I can rename it co contains_eacces() or something, but a longer name
> would require to cut the caller lines to fit 80 columns. :|

You know the Landlock code better than I do, if you like
"is_eacces()", then leave it as it is.

> >> -       layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {};
> >> -       bool allowed = false, has_access = false;
> >> +       unsigned long access_bit;
> >> +       /* LANDLOCK_ACCESS_FS_REFER alone must return -EXDEV. */
> >> +       const unsigned long access_check = access_request &
> >> +               ~LANDLOCK_ACCESS_FS_REFER;
> >> +
> >> +       if (!layer_masks)
> >> +               return false;
> >> +
> >> +       for_each_set_bit(access_bit, &access_check, ARRAY_SIZE(*layer_masks)) {
> >> +               if ((*layer_masks)[access_bit])
> >> +                       return true;
> >> +       }
> >
> > Is calling for_each_set_bit() overkill here?  @access_check should
> > only ever have at most one bit set (LANDLOCK_ACCESS_FS_REFER), yes?
>
> No, it is the contrary ...

Gotcha.  Thanks for the clarification, I must have missed that when I
was looking at it last night.

> >> @@ -287,22 +460,20 @@ static int check_access_path(const struct landlock_ruleset *const domain,
> >>          if (WARN_ON_ONCE(domain->num_layers < 1))
> >>                  return -EACCES;
> >>
> >> -       /* Saves all layers handling a subset of requested accesses. */
> >> -       for (i = 0; i < domain->num_layers; i++) {
> >> -               const unsigned long access_req = access_request;
> >> -               unsigned long access_bit;
> >> -
> >> -               for_each_set_bit(access_bit, &access_req,
> >> -                               ARRAY_SIZE(layer_masks)) {
> >> -                       if (domain->fs_access_masks[i] & BIT_ULL(access_bit)) {
> >> -                               layer_masks[access_bit] |= BIT_ULL(i);
> >> -                               has_access = true;
> >> -                       }
> >> -               }
> >> +       BUILD_BUG_ON(!layer_masks_dst_parent);
> >
> > I know the kbuild robot already flagged this, but checking function
> > parameters with BUILD_BUG_ON() does seem a bit ... unusual :)
>
> Yeah, I like such guarantee but it may not work without __always_inline.
> I moved this check in the previous WARN_ON_ONCE().

That sounds good to me.

> >> @@ -312,11 +483,50 @@ static int check_access_path(const struct landlock_ruleset *const domain,
> >>           */
> >>          while (true) {
> >>                  struct dentry *parent_dentry;
> >> +               const struct landlock_rule *rule;
> >> +
> >> +               /*
> >> +                * If at least all accesses allowed on the destination are
> >> +                * already allowed on the source, respectively if there is at
> >> +                * least as much as restrictions on the destination than on the
> >> +                * source, then we can safely refer files from the source to
> >> +                * the destination without risking a privilege escalation.
> >> +                * This is crucial for standalone multilayered security
> >> +                * policies.  Furthermore, this helps avoid policy writers to
> >> +                * shoot themselves in the foot.
> >> +                */
> >> +               if (is_dom_check && is_superset(child_is_directory,
> >> +                                       layer_masks_dst_parent,
> >> +                                       layer_masks_src_parent,
> >> +                                       layer_masks_child)) {
> >> +                       allowed_dst_parent =
> >> +                               scope_to_request(access_request_dst_parent,
> >> +                                               layer_masks_dst_parent);
> >> +                       allowed_src_parent =
> >> +                               scope_to_request(access_request_src_parent,
> >> +                                               layer_masks_src_parent);
> >> +
> >> +                       /* Stops when all accesses are granted. */
> >> +                       if (allowed_dst_parent && allowed_src_parent)
> >> +                               break;
> >> +
> >> +                       /*
> >> +                        * Downgrades checks from domain handled accesses to
> >> +                        * requested accesses.
> >> +                        */
> >> +                       is_dom_check = false;
> >> +                       access_masked_dst_parent = access_request_dst_parent;
> >> +                       access_masked_src_parent = access_request_src_parent;
> >> +               }
> >> +
> >> +               rule = find_rule(domain, walker_path.dentry);
> >> +               allowed_dst_parent = unmask_layers(rule, access_masked_dst_parent,
> >> +                               layer_masks_dst_parent);
> >> +               allowed_src_parent = unmask_layers(rule, access_masked_src_parent,
> >> +                               layer_masks_src_parent);
> >>
> >> -               allowed = unmask_layers(find_rule(domain, walker_path.dentry),
> >> -                               access_request, &layer_masks);
> >> -               if (allowed)
> >> -                       /* Stops when a rule from each layer grants access. */
> >> +               /* Stops when a rule from each layer grants access. */
> >> +               if (allowed_dst_parent && allowed_src_parent)
> >>                          break;
> >
> > If "(allowed_dst_parent && allowed_src_parent)" is true, you break out
> > of the while loop only to do a path_put(), check the two booleans once
> > more, and then return zero, yes?  Why not just do the path_put() and
> > return zero here?
>
> Correct, that would work, but I prefer not to duplicate the logic of
> granting access if it doesn't make the code more complex, which I think
> is not the case here, and I'm reluctant to duplicate path_get/put()
> calls. This loop break is a small optimization to avoid walking the path
> one more step, and writing it this way looks cleaner and less
> error-prone from my point of view.

I'm a big fan of maintainable code, and since you are the maintainer,
if you prefer this approach I say stick with what you have :)

-- 
paul-moore.com