| 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: <CAHC9VhTY06mOTD3LqTzhTsqt-VBJwezFyX8hwpJTz0VMC8KK7Q@mail.gmail.com>
Date: Thu, 17 Mar 2022 17:42:24 -0400
From: Paul Moore <paul@...l-moore.com>
To: Mickaël Salaün <mic@...ikod.net>
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 Thu, Mar 17, 2022 at 8:03 AM Mickaël Salaün <mic@...ikod.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@...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(-)
...
> >> +/*
> >> + * 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
Powered by blists - more mailing lists