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

[Paul Moore]

On Mon, Feb 21, 2022 at 4:15 PM 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).
>
> See the provided documentation for additional details.
>
> New tests are provided with a following commit.
>
> Signed-off-by: Mickaël Salaün <mic@xxxxxxxxxxxxxxxxxxx>
> Link: https://lore.kernel.org/r/20220221212522.320243-7-mic@xxxxxxxxxxx
> ---
>  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(-)

...

> diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> index 3886f9ad1a60..c7c7ce4e7cd5 100644
> --- a/security/landlock/fs.c
> +++ b/security/landlock/fs.c
> @@ -4,6 +4,7 @@
>   *
>   * Copyright © 2016-2020 Mickaël Salaün <mic@xxxxxxxxxxx>
>   * Copyright © 2018-2020 ANSSI
> + * Copyright © 2021-2022 Microsoft Corporation
>   */
>
>  #include <linux/atomic.h>
> @@ -269,16 +270,188 @@ static inline bool is_nouser_or_private(const struct dentry *dentry)
>                          unlikely(IS_PRIVATE(d_backing_inode(dentry))));
>  }
>
> -static int check_access_path(const struct landlock_ruleset *const domain,
> -               const struct path *const path,
> +static inline access_mask_t get_handled_accesses(
> +               const struct landlock_ruleset *const domain)
> +{
> +       access_mask_t access_dom = 0;
> +       unsigned long access_bit;

Would it be better to declare @access_bit as an access_mask_t type?
You're not using any macros like for_each_set_bit() in this function
so I believe it should be safe.

> +       for (access_bit = 0; access_bit < LANDLOCK_NUM_ACCESS_FS;
> +                       access_bit++) {
> +               size_t layer_level;

Considering the number of layers has dropped down to 16, it seems like
a normal unsigned int might be big enough for @layer_level :)

> +               for (layer_level = 0; layer_level < domain->num_layers;
> +                               layer_level++) {
> +                       if (domain->fs_access_masks[layer_level] &
> +                                       BIT_ULL(access_bit)) {
> +                               access_dom |= BIT_ULL(access_bit);
> +                               break;
> +                       }
> +               }
> +       }
> +       return access_dom;
> +}
> +
> +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));
> +       if (WARN_ON_ONCE(!access_request))
> +               return 0;
> +
> +       /* Saves all handled accesses per layer. */
> +       for (layer_level = 0; layer_level < domain->num_layers;
> +                       layer_level++) {
> +               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[layer_level] &
> +                                       BIT_ULL(access_bit)) {
> +                               (*layer_masks)[access_bit] |=
> +                                       BIT_ULL(layer_level);
> +                               handled_accesses |= BIT_ULL(access_bit);
> +                       }
> +               }
> +       }
> +       return handled_accesses;
> +}
> +
> +/*
> + * Check that a destination file hierarchy has more restrictions than a source
> + * file hierarchy.  This is only used for link and rename actions.
> + */
> +static inline bool is_superset(bool child_is_directory,
> +               const layer_mask_t (*const
> +                       layer_masks_dst_parent)[LANDLOCK_NUM_ACCESS_FS],
> +               const layer_mask_t (*const
> +                       layer_masks_src_parent)[LANDLOCK_NUM_ACCESS_FS],
> +               const layer_mask_t (*const
> +                       layer_masks_child)[LANDLOCK_NUM_ACCESS_FS])
> +{
> +       unsigned long access_bit;
> +
> +       for (access_bit = 0; access_bit < ARRAY_SIZE(*layer_masks_dst_parent);
> +                       access_bit++) {
> +               /* Ignores accesses that only make sense for directories. */
> +               if (!child_is_directory && !(BIT_ULL(access_bit) & ACCESS_FILE))
> +                       continue;
> +
> +               /*
> +                * Checks if the destination restrictions are a superset of the
> +                * source ones (i.e. inherited access rights without child
> +                * exceptions).
> +                */
> +               if ((((*layer_masks_src_parent)[access_bit] & (*layer_masks_child)[access_bit]) |
> +                                       (*layer_masks_dst_parent)[access_bit]) !=
> +                               (*layer_masks_dst_parent)[access_bit])
> +                       return false;
> +       }
> +       return true;
> +}
> +
> +/*
> + * Removes @layer_masks accesses that are not requested.
> + *
> + * Returns true if the request is allowed, false otherwise.
> + */
> +static inline bool scope_to_request(const access_mask_t access_request,
> +               layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS])
> +{
> +       const unsigned long access_req = access_request;
> +       unsigned long access_bit;
> +
> +       if (WARN_ON_ONCE(!layer_masks))
> +               return true;
> +
> +       for_each_clear_bit(access_bit, &access_req, ARRAY_SIZE(*layer_masks))
> +               (*layer_masks)[access_bit] = 0;
> +       return !memchr_inv(layer_masks, 0, sizeof(*layer_masks));
> +}
> +
> +/*
> + * 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"?

> -       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?

> +       return false;
> +}
> +
> +/**
> + * check_access_path_dual - Check a source and a destination accesses
> + *
> + * @domain: Domain to check against.
> + * @path: File hierarchy to walk through.
> + * @child_is_directory: Must be set to true if the (original) leaf is a
> + *     directory, false otherwise.
> + * @access_request_dst_parent: Accesses to check, once @layer_masks_dst_parent
> + *     is equal to @layer_masks_src_parent (if any).
> + * @layer_masks_dst_parent: Pointer to a matrix of layer masks per access
> + *     masks, identifying the layers that forbid a specific access.  Bits from
> + *     this matrix can be unset according to the @path walk.  An empty matrix
> + *     means that @domain allows all possible Landlock accesses (i.e. not only
> + *     those identified by @access_request_dst_parent).  This matrix can
> + *     initially refer to domain layer masks and, when the accesses for the
> + *     destination and source are the same, to request layer masks.
> + * @access_request_src_parent: Similar to @access_request_dst_parent but for an
> + *     initial source path request.  Only taken into account if
> + *     @layer_masks_src_parent is not NULL.
> + * @layer_masks_src_parent: Similar to @layer_masks_dst_parent but for an
> + *     initial source path walk.  This can be NULL if only dealing with a
> + *     destination access request (i.e. not a rename nor a link action).
> + * @layer_masks_child: Similar to @layer_masks_src_parent but only for the
> + *     linked or renamed inode (without hierarchy).  This is only used if
> + *     @layer_masks_src_parent is not NULL.
> + *
> + * This helper first checks that the destination has a superset of restrictions
> + * compared to the source (if any) for a common path.  It then checks that the
> + * collected accesses and the remaining ones are enough to allow the request.
> + *
> + * Returns:
> + * - 0 if the access request is granted;
> + * - -EACCES if it is denied because of access right other than
> + *   LANDLOCK_ACCESS_FS_REFER;
> + * - -EXDEV if the renaming or linking would be a privileged escalation
> + *   (according to each layered policies), or if LANDLOCK_ACCESS_FS_REFER is
> + *   not allowed by the source or the destination.
> + */
> +static int check_access_path_dual(const struct landlock_ruleset *const domain,
> +               const struct path *const path,
> +               bool child_is_directory,
> +               const access_mask_t access_request_dst_parent,
> +               layer_mask_t (*const
> +                       layer_masks_dst_parent)[LANDLOCK_NUM_ACCESS_FS],
> +               const access_mask_t access_request_src_parent,
> +               layer_mask_t (*layer_masks_src_parent)[LANDLOCK_NUM_ACCESS_FS],
> +               layer_mask_t (*layer_masks_child)[LANDLOCK_NUM_ACCESS_FS])
> +{
> +       bool allowed_dst_parent = false, allowed_src_parent = false, is_dom_check;
>         struct path walker_path;
> -       size_t i;
> +       access_mask_t access_masked_dst_parent, access_masked_src_parent;
>
> -       if (!access_request)
> +       if (!access_request_dst_parent && !access_request_src_parent)
>                 return 0;
>         if (WARN_ON_ONCE(!domain || !path))
>                 return 0;
> @@ -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 :)

> +       if (layer_masks_src_parent) {
> +               if (WARN_ON_ONCE(!layer_masks_child))
> +                       return -EACCES;
> +               access_masked_dst_parent = access_masked_src_parent =
> +                       get_handled_accesses(domain);
> +               is_dom_check = true;
> +       } else {
> +               if (WARN_ON_ONCE(layer_masks_child))
> +                       return -EACCES;
> +               access_masked_dst_parent = access_request_dst_parent;
> +               access_masked_src_parent = access_request_src_parent;
> +               is_dom_check = false;
>         }
> -       /* An access request not handled by the domain is allowed. */
> -       if (!has_access)
> -               return 0;
>
>         walker_path = *path;
>         path_get(&walker_path);
> @@ -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?


--
paul-moore.com