| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <588e0fec-6a45-db81-e411-ae488b29e533@digikod.net>
Date: Thu, 17 Mar 2022 13:04:29 +0100
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>,
Kees Cook <keescook@...omium.org>,
Konstantin Meskhidze <konstantin.meskhidze@...wei.com>,
Shuah Khan <shuah@...nel.org>, 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 v1 06/11] landlock: Add support for file reparenting with
LANDLOCK_ACCESS_FS_REFER
On 17/03/2022 02:26, Paul Moore wrote:
> On Mon, Feb 21, 2022 at 4:15 PM 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).
>>
>> See the provided documentation for additional details.
>>
>> New tests are provided with a following commit.
>>
>> Signed-off-by: Mickaël Salaün <mic@...ux.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(-)
>
> ...
>
>> 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@...ikod.net>
>> * 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.
Right, I'll change that.
>
>> + 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 :)
We could switch to u8, but I prefer to stick to size_t for array indexes
which enable to reduce the cognitive workload related to the size of
such array. ;) I guess there is enough info for compilers to optimize
such code anyway.
>
>> + 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"?
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. :|
>
>> - 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, the bitmask is inverted and this loop check for
non-FS_REFER access rights that should then return -EACCES. For
instance, if a sandbox handles (and then restricts) MAKE_REG and REFER,
a request to link a regular file would contains both of these bits, and
the kernel should return -EACCES if MAKE_REG is not granted or -EXDEV if
the request is only denied because of REFER. The reparent_* tests check
the consistency of this behavior (with the exception of a
RENAME_EXCHANGE case, see [1]).
[1] https://lore.kernel.org/r/20220222175332.384545-1-mic@digikod.net
>
>> + 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 :)
Yeah, I like such guarantee but it may not work without __always_inline.
I moved this check in the previous WARN_ON_ONCE().
>
>> + 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?
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.
Powered by blists - more mailing lists