Re: [PATCH v1 06/11] landlock: Add support for file reparenting with LANDLOCK_ACCESS_FS_REFER
From: Paul Moore <paul@paul-moore.com>
Date: 2022-03-17 01:27:23
Also in:
linux-fsdevel, linux-security-module, lkml
On Mon, Feb 21, 2022 at 4:15 PM Mickaël Salaün [off-list ref] wrote:
From: Mickaël Salaün <redacted> 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 <redacted> Link: https://lore.kernel.org/r/20220221212522.320243-7-mic@digikod.net (local) --- 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(-)
...
quoted hunk ↗ jump to hunk
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@digikod.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.
+ 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?
quoted hunk ↗ jump to hunk
+ 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 :)
quoted hunk ↗ jump to hunk
+ 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