From 9f78dda3c3bb9f5e084495e4a24ccf265f1b5168 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micka=C3=ABl=20Sala=C3=BCn?= Date: Wed, 31 Aug 2022 22:38:40 +0200 Subject: landlock: Fix file reparenting without explicit LANDLOCK_ACCESS_FS_REFER MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit commit 55e55920bbe3ccf516022c51f5527e7d026b8f1d upstream. This change fixes a mis-handling of the LANDLOCK_ACCESS_FS_REFER right when multiple rulesets/domains are stacked. The expected behaviour was that an additional ruleset can only restrict the set of permitted operations, but in this particular case, it was potentially possible to re-gain the LANDLOCK_ACCESS_FS_REFER right. With the introduction of LANDLOCK_ACCESS_FS_REFER, we added the first globally denied-by-default access right. Indeed, this lifted an initial Landlock limitation to rename and link files, which was initially always denied when the source or the destination were different directories. This led to an inconsistent backward compatibility behavior which was only taken into account if no domain layer were using the new LANDLOCK_ACCESS_FS_REFER right. However, when restricting a thread with a new ruleset handling LANDLOCK_ACCESS_FS_REFER, all inherited parent rulesets/layers not explicitly handling LANDLOCK_ACCESS_FS_REFER would behave as if they were handling this access right and with all their rules allowing it. This means that renaming and linking files could became allowed by these parent layers, but all the other required accesses must also be granted: all layers must allow file removal or creation, and renaming and linking operations cannot lead to privilege escalation according to the Landlock policy. See detailed explanation in commit b91c3e4ea756 ("landlock: Add support for file reparenting with LANDLOCK_ACCESS_FS_REFER"). To say it another way, this bug may lift the renaming and linking limitations of the initial Landlock version, and a same ruleset can enforce different restrictions depending on previous or next enforced ruleset (i.e. inconsistent behavior). The LANDLOCK_ACCESS_FS_REFER right cannot give access to data not already allowed, but this doesn't follow the contract of the first Landlock ABI. This fix puts back the limitation for sandboxes that didn't opt-in for this additional right. For instance, if a first ruleset allows LANDLOCK_ACCESS_FS_MAKE_REG on /dst and LANDLOCK_ACCESS_FS_REMOVE_FILE on /src, renaming /src/file to /dst/file is denied. However, without this fix, stacking a new ruleset which allows LANDLOCK_ACCESS_FS_REFER on / would now permit the sandboxed thread to rename /src/file to /dst/file . This change fixes the (absolute) rule access rights, which now always forbid LANDLOCK_ACCESS_FS_REFER except when it is explicitly allowed when creating a rule. Making all domain handle LANDLOCK_ACCESS_FS_REFER was an initial approach but there is two downsides: * it makes the code more complex because we still want to check that a rule allowing LANDLOCK_ACCESS_FS_REFER is legitimate according to the ruleset's handled access rights (i.e. ABI v1 != ABI v2); * it would not allow to identify if the user created a ruleset explicitly handling LANDLOCK_ACCESS_FS_REFER or not, which will be an issue to audit Landlock. Instead, this change adds an ACCESS_INITIALLY_DENIED list of denied-by-default rights, which (only) contains LANDLOCK_ACCESS_FS_REFER. All domains are treated as if they are also handling this list, but without modifying their fs_access_masks field. A side effect is that the errno code returned by rename(2) or link(2) *may* be changed from EXDEV to EACCES according to the enforced restrictions. Indeed, we now have the mechanic to identify if an access is denied because of a required right (e.g. LANDLOCK_ACCESS_FS_MAKE_REG, LANDLOCK_ACCESS_FS_REMOVE_FILE) or if it is denied because of missing LANDLOCK_ACCESS_FS_REFER rights. This may result in different errno codes than for the initial Landlock version, but this approach is more consistent and better for rename/link compatibility reasons, and it wasn't possible before (hence no backport to ABI v1). The layout1.rename_file test reflects this change. Add 4 layout1.refer_denied_by_default* test suites to check that the behavior of a ruleset not handling LANDLOCK_ACCESS_FS_REFER (ABI v1) is unchanged even if another layer handles LANDLOCK_ACCESS_FS_REFER (i.e. ABI v1 precedence). Make sure rule's absolute access rights are correct by testing with and without a matching path. Add test_rename() and test_exchange() helpers. Extend layout1.inval tests to check that a denied-by-default access right is not necessarily part of a domain's handled access rights. Test coverage for security/landlock is 95.3% of 599 lines according to gcc/gcov-11. Fixes: b91c3e4ea756 ("landlock: Add support for file reparenting with LANDLOCK_ACCESS_FS_REFER") Reviewed-by: Paul Moore Reviewed-by: Günther Noack Link: https://lore.kernel.org/r/20220831203840.1370732-1-mic@digikod.net Cc: stable@vger.kernel.org [mic: Constify and slightly simplify test helpers] Signed-off-by: Mickaël Salaün Signed-off-by: Greg Kroah-Hartman --- security/landlock/fs.c | 48 +++++++++++++++++++++++++----------------------- 1 file changed, 25 insertions(+), 23 deletions(-) (limited to 'security') diff --git a/security/landlock/fs.c b/security/landlock/fs.c index ec5a6247cd3e..a9dbd99d9ee7 100644 --- a/security/landlock/fs.c +++ b/security/landlock/fs.c @@ -149,6 +149,16 @@ retry: LANDLOCK_ACCESS_FS_READ_FILE) /* clang-format on */ +/* + * All access rights that are denied by default whether they are handled or not + * by a ruleset/layer. This must be ORed with all ruleset->fs_access_masks[] + * entries when we need to get the absolute handled access masks. + */ +/* clang-format off */ +#define ACCESS_INITIALLY_DENIED ( \ + LANDLOCK_ACCESS_FS_REFER) +/* clang-format on */ + /* * @path: Should have been checked by get_path_from_fd(). */ @@ -167,7 +177,9 @@ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset, return -EINVAL; /* Transforms relative access rights to absolute ones. */ - access_rights |= LANDLOCK_MASK_ACCESS_FS & ~ruleset->fs_access_masks[0]; + access_rights |= + LANDLOCK_MASK_ACCESS_FS & + ~(ruleset->fs_access_masks[0] | ACCESS_INITIALLY_DENIED); object = get_inode_object(d_backing_inode(path->dentry)); if (IS_ERR(object)) return PTR_ERR(object); @@ -277,23 +289,12 @@ static inline bool is_nouser_or_private(const struct dentry *dentry) static inline access_mask_t get_handled_accesses(const struct landlock_ruleset *const domain) { - access_mask_t access_dom = 0; - unsigned long access_bit; - - for (access_bit = 0; access_bit < LANDLOCK_NUM_ACCESS_FS; - access_bit++) { - size_t layer_level; + access_mask_t access_dom = ACCESS_INITIALLY_DENIED; + size_t 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; + for (layer_level = 0; layer_level < domain->num_layers; layer_level++) + access_dom |= domain->fs_access_masks[layer_level]; + return access_dom & LANDLOCK_MASK_ACCESS_FS; } static inline access_mask_t @@ -316,8 +317,13 @@ init_layer_masks(const struct landlock_ruleset *const domain, for_each_set_bit(access_bit, &access_req, ARRAY_SIZE(*layer_masks)) { - if (domain->fs_access_masks[layer_level] & - BIT_ULL(access_bit)) { + /* + * Artificially handles all initially denied by default + * access rights. + */ + if (BIT_ULL(access_bit) & + (domain->fs_access_masks[layer_level] | + ACCESS_INITIALLY_DENIED)) { (*layer_masks)[access_bit] |= BIT_ULL(layer_level); handled_accesses |= BIT_ULL(access_bit); @@ -857,10 +863,6 @@ static int current_check_refer_path(struct dentry *const old_dentry, NULL, NULL); } - /* Backward compatibility: no reparenting support. */ - if (!(get_handled_accesses(dom) & LANDLOCK_ACCESS_FS_REFER)) - return -EXDEV; - access_request_parent1 |= LANDLOCK_ACCESS_FS_REFER; access_request_parent2 |= LANDLOCK_ACCESS_FS_REFER; -- cgit v1.2.3