From patchwork Tue Mar 29 12:51:06 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= X-Patchwork-Id: 12794804 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id E2149C4332F for ; Tue, 29 Mar 2022 12:51:31 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235508AbiC2MxJ (ORCPT ); Tue, 29 Mar 2022 08:53:09 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:34902 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237153AbiC2Mw7 (ORCPT ); Tue, 29 Mar 2022 08:52:59 -0400 Received: from smtp-42a8.mail.infomaniak.ch (smtp-42a8.mail.infomaniak.ch [IPv6:2001:1600:4:17::42a8]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id B458FE0A1 for ; Tue, 29 Mar 2022 05:51:00 -0700 (PDT) Received: from smtp-3-0001.mail.infomaniak.ch (unknown [10.4.36.108]) by smtp-3-3000.mail.infomaniak.ch (Postfix) with ESMTPS id 4KSTvQ73NDzMq176; Tue, 29 Mar 2022 14:50:58 +0200 (CEST) Received: from localhost (unknown [23.97.221.149]) by smtp-3-0001.mail.infomaniak.ch (Postfix) with ESMTPA id 4KSTvQ4FQnzlhMCC; Tue, 29 Mar 2022 14:50:58 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=digikod.net; s=20191114; t=1648558258; bh=V0QiE06mm1XiML32F73M5RFbgacxnLfvqx49xLlyRWs=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=zQ2QS3Rw86KnXSGeZJiq3E+ECvNzLyShIQA+Dxhx/8ckKJtg8j1CHD5IpNJ8Tny1D 05QDPxTz0//QwWO4nEA9ZuXOdzyF+pPE3ERFqvPAk9le2Giw8eAPzbDsLv1Y8OFY7/ QhAxi28qupbY5W3snsFmc2dQRI3ezLlSMLImma4U= From: =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= To: James Morris , "Serge E . Hallyn" Cc: =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= , Al Viro , Jann Horn , John Johansen , Kees Cook , Konstantin Meskhidze , Paul Moore , Shuah Khan , Tetsuo Handa , linux-doc@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, linux-security-module@vger.kernel.org, =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= Subject: [PATCH v2 01/12] landlock: Define access_mask_t to enforce a consistent access mask size Date: Tue, 29 Mar 2022 14:51:06 +0200 Message-Id: <20220329125117.1393824-2-mic@digikod.net> In-Reply-To: <20220329125117.1393824-1-mic@digikod.net> References: <20220329125117.1393824-1-mic@digikod.net> MIME-Version: 1.0 Precedence: bulk List-ID: From: Mickaël Salaün Create and use the access_mask_t typedef to enforce a consistent access mask size and uniformly use a 16-bits type. This will helps transition to a 32-bits value one day. Add a build check to make sure all (filesystem) access rights fit in. This will be extended with a following commit. Reviewed-by: Paul Moore Signed-off-by: Mickaël Salaün Link: https://lore.kernel.org/r/20220329125117.1393824-2-mic@digikod.net --- Changes since v1: * Add Reviewed-by: Paul Moore. --- security/landlock/fs.c | 19 ++++++++++--------- security/landlock/fs.h | 2 +- security/landlock/limits.h | 2 ++ security/landlock/ruleset.c | 6 ++++-- security/landlock/ruleset.h | 17 +++++++++++++---- 5 files changed, 30 insertions(+), 16 deletions(-) diff --git a/security/landlock/fs.c b/security/landlock/fs.c index 97b8e421f617..9de2a460a762 100644 --- a/security/landlock/fs.c +++ b/security/landlock/fs.c @@ -150,7 +150,7 @@ static struct landlock_object *get_inode_object(struct inode *const inode) * @path: Should have been checked by get_path_from_fd(). */ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset, - const struct path *const path, u32 access_rights) + const struct path *const path, access_mask_t access_rights) { int err; struct landlock_object *object; @@ -182,8 +182,8 @@ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset, static inline u64 unmask_layers( const struct landlock_ruleset *const domain, - const struct path *const path, const u32 access_request, - u64 layer_mask) + const struct path *const path, + const access_mask_t access_request, u64 layer_mask) { const struct landlock_rule *rule; const struct inode *inode; @@ -223,7 +223,8 @@ static inline u64 unmask_layers( } static int check_access_path(const struct landlock_ruleset *const domain, - const struct path *const path, u32 access_request) + const struct path *const path, + const access_mask_t access_request) { bool allowed = false; struct path walker_path; @@ -308,7 +309,7 @@ static int check_access_path(const struct landlock_ruleset *const domain, } static inline int current_check_access_path(const struct path *const path, - const u32 access_request) + const access_mask_t access_request) { const struct landlock_ruleset *const dom = landlock_get_current_domain(); @@ -511,7 +512,7 @@ static int hook_sb_pivotroot(const struct path *const old_path, /* Path hooks */ -static inline u32 get_mode_access(const umode_t mode) +static inline access_mask_t get_mode_access(const umode_t mode) { switch (mode & S_IFMT) { case S_IFLNK: @@ -563,7 +564,7 @@ static int hook_path_link(struct dentry *const old_dentry, get_mode_access(d_backing_inode(old_dentry)->i_mode)); } -static inline u32 maybe_remove(const struct dentry *const dentry) +static inline access_mask_t maybe_remove(const struct dentry *const dentry) { if (d_is_negative(dentry)) return 0; @@ -631,9 +632,9 @@ static int hook_path_rmdir(const struct path *const dir, /* File hooks */ -static inline u32 get_file_access(const struct file *const file) +static inline access_mask_t get_file_access(const struct file *const file) { - u32 access = 0; + access_mask_t access = 0; if (file->f_mode & FMODE_READ) { /* A directory can only be opened in read mode. */ diff --git a/security/landlock/fs.h b/security/landlock/fs.h index 187284b421c9..74be312aad96 100644 --- a/security/landlock/fs.h +++ b/security/landlock/fs.h @@ -65,6 +65,6 @@ static inline struct landlock_superblock_security *landlock_superblock( __init void landlock_add_fs_hooks(void); int landlock_append_fs_rule(struct landlock_ruleset *const ruleset, - const struct path *const path, u32 access_hierarchy); + const struct path *const path, access_mask_t access_hierarchy); #endif /* _SECURITY_LANDLOCK_FS_H */ diff --git a/security/landlock/limits.h b/security/landlock/limits.h index 2a0a1095ee27..458d1de32ed5 100644 --- a/security/landlock/limits.h +++ b/security/landlock/limits.h @@ -9,6 +9,7 @@ #ifndef _SECURITY_LANDLOCK_LIMITS_H #define _SECURITY_LANDLOCK_LIMITS_H +#include #include #include @@ -17,5 +18,6 @@ #define LANDLOCK_LAST_ACCESS_FS LANDLOCK_ACCESS_FS_MAKE_SYM #define LANDLOCK_MASK_ACCESS_FS ((LANDLOCK_LAST_ACCESS_FS << 1) - 1) +#define LANDLOCK_NUM_ACCESS_FS __const_hweight64(LANDLOCK_MASK_ACCESS_FS) #endif /* _SECURITY_LANDLOCK_LIMITS_H */ diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c index ec72b9262bf3..4e7aa8024fff 100644 --- a/security/landlock/ruleset.c +++ b/security/landlock/ruleset.c @@ -44,7 +44,8 @@ static struct landlock_ruleset *create_ruleset(const u32 num_layers) return new_ruleset; } -struct landlock_ruleset *landlock_create_ruleset(const u32 fs_access_mask) +struct landlock_ruleset *landlock_create_ruleset( + const access_mask_t fs_access_mask) { struct landlock_ruleset *new_ruleset; @@ -228,7 +229,8 @@ static void build_check_layer(void) /* @ruleset must be locked by the caller. */ int landlock_insert_rule(struct landlock_ruleset *const ruleset, - struct landlock_object *const object, const u32 access) + struct landlock_object *const object, + const access_mask_t access) { struct landlock_layer layers[] = {{ .access = access, diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h index 2d3ed7ec5a0a..7e7cac68e443 100644 --- a/security/landlock/ruleset.h +++ b/security/landlock/ruleset.h @@ -9,13 +9,20 @@ #ifndef _SECURITY_LANDLOCK_RULESET_H #define _SECURITY_LANDLOCK_RULESET_H +#include +#include #include #include #include #include +#include "limits.h" #include "object.h" +typedef u16 access_mask_t; +/* Makes sure all filesystem access rights can be stored. */ +static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_FS); + /** * struct landlock_layer - Access rights for a given layer */ @@ -28,7 +35,7 @@ struct landlock_layer { * @access: Bitfield of allowed actions on the kernel object. They are * relative to the object type (e.g. %LANDLOCK_ACTION_FS_READ). */ - u16 access; + access_mask_t access; }; /** @@ -135,18 +142,20 @@ struct landlock_ruleset { * layers are set once and never changed for the * lifetime of the ruleset. */ - u16 fs_access_masks[]; + access_mask_t fs_access_masks[]; }; }; }; -struct landlock_ruleset *landlock_create_ruleset(const u32 fs_access_mask); +struct landlock_ruleset *landlock_create_ruleset( + const access_mask_t fs_access_mask); void landlock_put_ruleset(struct landlock_ruleset *const ruleset); void landlock_put_ruleset_deferred(struct landlock_ruleset *const ruleset); int landlock_insert_rule(struct landlock_ruleset *const ruleset, - struct landlock_object *const object, const u32 access); + struct landlock_object *const object, + const access_mask_t access); struct landlock_ruleset *landlock_merge_ruleset( struct landlock_ruleset *const parent, From patchwork Tue Mar 29 12:51:07 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= X-Patchwork-Id: 12794807 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 19D9CC43217 for ; Tue, 29 Mar 2022 12:51:36 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236845AbiC2MxO (ORCPT ); Tue, 29 Mar 2022 08:53:14 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:36098 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237155AbiC2Mw7 (ORCPT ); Tue, 29 Mar 2022 08:52:59 -0400 Received: from smtp-42aa.mail.infomaniak.ch (smtp-42aa.mail.infomaniak.ch [84.16.66.170]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id EBDE1E0F3 for ; Tue, 29 Mar 2022 05:51:00 -0700 (PDT) Received: from smtp-3-0001.mail.infomaniak.ch (unknown [10.4.36.108]) by smtp-3-3000.mail.infomaniak.ch (Postfix) with ESMTPS id 4KSTvR4lCRzMprp1; Tue, 29 Mar 2022 14:50:59 +0200 (CEST) Received: from localhost (unknown [23.97.221.149]) by smtp-3-0001.mail.infomaniak.ch (Postfix) with ESMTPA id 4KSTvR2glxzlhMCK; Tue, 29 Mar 2022 14:50:59 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=digikod.net; s=20191114; t=1648558259; bh=cGKkI5dIAlnDd39MT7GxV5PbbTi1yoIW7hnT3vZmRfY=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=LX4efo/pLMuI6/U7ALHvLtfakZ2dPNyJF6F36JolZPTYGTB83Az99kayzwM2HZR2I VTazgEvGj4bR0mczQ6b+I8028BdNcRzblhsHk8DAN6dOZESAxNSIxtWUyAui7N58A6 hvTytlS+qnukZlFELNwBd0ZS9yoA/ykcigr+Z8NM= From: =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= To: James Morris , "Serge E . Hallyn" Cc: =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= , Al Viro , Jann Horn , John Johansen , Kees Cook , Konstantin Meskhidze , Paul Moore , Shuah Khan , Tetsuo Handa , linux-doc@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, linux-security-module@vger.kernel.org, =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= Subject: [PATCH v2 02/12] landlock: Reduce the maximum number of layers to 16 Date: Tue, 29 Mar 2022 14:51:07 +0200 Message-Id: <20220329125117.1393824-3-mic@digikod.net> In-Reply-To: <20220329125117.1393824-1-mic@digikod.net> References: <20220329125117.1393824-1-mic@digikod.net> MIME-Version: 1.0 Precedence: bulk List-ID: From: Mickaël Salaün The maximum number of nested Landlock domains is currently 64. Because of the following fix and to help reduce the stack size, let's reduce it to 16. This seems large enough for a lot of use cases (e.g. sandboxed init service, spawning a sandboxed SSH service, in nested sandboxed containers). Reducing the number of nested domains may also help to discover misuse of Landlock (e.g. creating a domain per rule). Add and use a dedicated layer_mask_t typedef to fit with the number of layers. This might be useful when changing it and to keep it consistent with the maximum number of layers. Reviewed-by: Paul Moore Signed-off-by: Mickaël Salaün Link: https://lore.kernel.org/r/20220329125117.1393824-3-mic@digikod.net --- Changes since v1: * Add Reviewed-by: Paul Moore. * Update documentation to reflect this change. --- Documentation/userspace-api/landlock.rst | 4 ++-- security/landlock/fs.c | 13 +++++-------- security/landlock/limits.h | 2 +- security/landlock/ruleset.h | 4 ++++ tools/testing/selftests/landlock/fs_test.c | 2 +- 5 files changed, 13 insertions(+), 12 deletions(-) diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst index f35552ff19ba..b68e7a51009f 100644 --- a/Documentation/userspace-api/landlock.rst +++ b/Documentation/userspace-api/landlock.rst @@ -267,8 +267,8 @@ restrict such paths with dedicated ruleset flags. Ruleset layers -------------- -There is a limit of 64 layers of stacked rulesets. This can be an issue for a -task willing to enforce a new ruleset in complement to its 64 inherited +There is a limit of 16 layers of stacked rulesets. This can be an issue for a +task willing to enforce a new ruleset in complement to its 16 inherited rulesets. Once this limit is reached, sys_landlock_restrict_self() returns E2BIG. It is then strongly suggested to carefully build rulesets once in the life of a thread, especially for applications able to launch other applications diff --git a/security/landlock/fs.c b/security/landlock/fs.c index 9de2a460a762..4048e3c04d75 100644 --- a/security/landlock/fs.c +++ b/security/landlock/fs.c @@ -180,10 +180,10 @@ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset, /* Access-control management */ -static inline u64 unmask_layers( +static inline layer_mask_t unmask_layers( const struct landlock_ruleset *const domain, const struct path *const path, - const access_mask_t access_request, u64 layer_mask) + const access_mask_t access_request, layer_mask_t layer_mask) { const struct landlock_rule *rule; const struct inode *inode; @@ -209,11 +209,11 @@ static inline u64 unmask_layers( */ for (i = 0; i < rule->num_layers; i++) { const struct landlock_layer *const layer = &rule->layers[i]; - const u64 layer_level = BIT_ULL(layer->level - 1); + const layer_mask_t layer_bit = BIT_ULL(layer->level - 1); /* Checks that the layer grants access to the full request. */ if ((layer->access & access_request) == access_request) { - layer_mask &= ~layer_level; + layer_mask &= ~layer_bit; if (layer_mask == 0) return layer_mask; @@ -228,12 +228,9 @@ static int check_access_path(const struct landlock_ruleset *const domain, { bool allowed = false; struct path walker_path; - u64 layer_mask; + layer_mask_t layer_mask; size_t i; - /* Make sure all layers can be checked. */ - BUILD_BUG_ON(BITS_PER_TYPE(layer_mask) < LANDLOCK_MAX_NUM_LAYERS); - if (!access_request) return 0; if (WARN_ON_ONCE(!domain || !path)) diff --git a/security/landlock/limits.h b/security/landlock/limits.h index 458d1de32ed5..126d1ec04d34 100644 --- a/security/landlock/limits.h +++ b/security/landlock/limits.h @@ -13,7 +13,7 @@ #include #include -#define LANDLOCK_MAX_NUM_LAYERS 64 +#define LANDLOCK_MAX_NUM_LAYERS 16 #define LANDLOCK_MAX_NUM_RULES U32_MAX #define LANDLOCK_LAST_ACCESS_FS LANDLOCK_ACCESS_FS_MAKE_SYM diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h index 7e7cac68e443..0128c56ee7ff 100644 --- a/security/landlock/ruleset.h +++ b/security/landlock/ruleset.h @@ -23,6 +23,10 @@ typedef u16 access_mask_t; /* Makes sure all filesystem access rights can be stored. */ static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_FS); +typedef u16 layer_mask_t; +/* Makes sure all layers can be checked. */ +static_assert(BITS_PER_TYPE(layer_mask_t) >= LANDLOCK_MAX_NUM_LAYERS); + /** * struct landlock_layer - Access rights for a given layer */ diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c index 10c9a1e4ebd9..99838cac970b 100644 --- a/tools/testing/selftests/landlock/fs_test.c +++ b/tools/testing/selftests/landlock/fs_test.c @@ -1080,7 +1080,7 @@ TEST_F_FORK(layout1, max_layers) const int ruleset_fd = create_ruleset(_metadata, ACCESS_RW, rules); ASSERT_LE(0, ruleset_fd); - for (i = 0; i < 64; i++) + for (i = 0; i < 16; i++) enforce_ruleset(_metadata, ruleset_fd); for (i = 0; i < 2; i++) { From patchwork Tue Mar 29 12:51:08 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= X-Patchwork-Id: 12794803 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4FF64C43219 for ; Tue, 29 Mar 2022 12:51:32 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236789AbiC2MxL (ORCPT ); Tue, 29 Mar 2022 08:53:11 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35752 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237157AbiC2Mw7 (ORCPT ); Tue, 29 Mar 2022 08:52:59 -0400 Received: from smtp-190e.mail.infomaniak.ch (smtp-190e.mail.infomaniak.ch [185.125.25.14]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 32E4C11155 for ; Tue, 29 Mar 2022 05:51:01 -0700 (PDT) Received: from smtp-3-0001.mail.infomaniak.ch (unknown [10.4.36.108]) by smtp-3-3000.mail.infomaniak.ch (Postfix) with ESMTPS id 4KSTvS2GXJzMq0vs; Tue, 29 Mar 2022 14:51:00 +0200 (CEST) Received: from localhost (unknown [23.97.221.149]) by smtp-3-0001.mail.infomaniak.ch (Postfix) with ESMTPA id 4KSTvS0TrszlhMCC; Tue, 29 Mar 2022 14:51:00 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=digikod.net; s=20191114; t=1648558260; bh=Z7Dv4Wuaux19f4A4TfPGhRhJCwQox2If8Ghi9nfOU5U=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=BPgLnbzLEtVqYdu82o/LrMMKpglq4vbp4cDMZDbbpGTCK/qXfX+hN979Kzg7RSemC mqTCwSSjVy9B0hddHLSPClVb4zdiAHu2ZeynsROXlVX83Lag+EgkmPC4VbYgKCKYR1 qOY4Q9SBuD0W92d9w8ftcJ+q+ScciBbdNzmxz/RA= From: =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= To: James Morris , "Serge E . Hallyn" Cc: =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= , Al Viro , Jann Horn , John Johansen , Kees Cook , Konstantin Meskhidze , Paul Moore , Shuah Khan , Tetsuo Handa , linux-doc@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, linux-security-module@vger.kernel.org, =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= Subject: [PATCH v2 03/12] landlock: Create find_rule() from unmask_layers() Date: Tue, 29 Mar 2022 14:51:08 +0200 Message-Id: <20220329125117.1393824-4-mic@digikod.net> In-Reply-To: <20220329125117.1393824-1-mic@digikod.net> References: <20220329125117.1393824-1-mic@digikod.net> MIME-Version: 1.0 Precedence: bulk List-ID: From: Mickaël Salaün This refactoring will be useful in a following commit. Reviewed-by: Paul Moore Signed-off-by: Mickaël Salaün Link: https://lore.kernel.org/r/20220329125117.1393824-4-mic@digikod.net --- Changes since v1: * Add Reviewed-by: Paul Moore. --- security/landlock/fs.c | 39 +++++++++++++++++++++++++++------------ 1 file changed, 27 insertions(+), 12 deletions(-) diff --git a/security/landlock/fs.c b/security/landlock/fs.c index 4048e3c04d75..0bcb27f2360a 100644 --- a/security/landlock/fs.c +++ b/security/landlock/fs.c @@ -180,23 +180,36 @@ int landlock_append_fs_rule(struct landlock_ruleset *const ruleset, /* Access-control management */ -static inline layer_mask_t unmask_layers( +/* + * The lifetime of the returned rule is tied to @domain. + * + * Returns NULL if no rule is found or if @dentry is negative. + */ +static inline const struct landlock_rule *find_rule( const struct landlock_ruleset *const domain, - const struct path *const path, - const access_mask_t access_request, layer_mask_t layer_mask) + const struct dentry *const dentry) { const struct landlock_rule *rule; const struct inode *inode; - size_t i; - if (d_is_negative(path->dentry)) - /* Ignore nonexistent leafs. */ - return layer_mask; - inode = d_backing_inode(path->dentry); + /* Ignores nonexistent leafs. */ + if (d_is_negative(dentry)) + return NULL; + + inode = d_backing_inode(dentry); rcu_read_lock(); rule = landlock_find_rule(domain, rcu_dereference(landlock_inode(inode)->object)); rcu_read_unlock(); + return rule; +} + +static inline layer_mask_t unmask_layers( + const struct landlock_rule *const rule, + const access_mask_t access_request, layer_mask_t layer_mask) +{ + size_t layer_level; + if (!rule) return layer_mask; @@ -207,8 +220,9 @@ static inline layer_mask_t unmask_layers( * the remaining layers for each inode, from the first added layer to * the last one. */ - for (i = 0; i < rule->num_layers; i++) { - const struct landlock_layer *const layer = &rule->layers[i]; + for (layer_level = 0; layer_level < rule->num_layers; layer_level++) { + const struct landlock_layer *const layer = + &rule->layers[layer_level]; const layer_mask_t layer_bit = BIT_ULL(layer->level - 1); /* Checks that the layer grants access to the full request. */ @@ -266,8 +280,9 @@ static int check_access_path(const struct landlock_ruleset *const domain, while (true) { struct dentry *parent_dentry; - layer_mask = unmask_layers(domain, &walker_path, - access_request, layer_mask); + layer_mask = unmask_layers(find_rule(domain, + walker_path.dentry), access_request, + layer_mask); if (layer_mask == 0) { /* Stops when a rule from each layer grants access. */ allowed = true; From patchwork Tue Mar 29 12:51:09 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= X-Patchwork-Id: 12794805 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9EA73C433EF for ; Tue, 29 Mar 2022 12:51:34 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236846AbiC2MxN (ORCPT ); Tue, 29 Mar 2022 08:53:13 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:36146 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237165AbiC2MxA (ORCPT ); Tue, 29 Mar 2022 08:53:00 -0400 Received: from smtp-bc08.mail.infomaniak.ch (smtp-bc08.mail.infomaniak.ch [IPv6:2001:1600:4:17::bc08]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 9EBE611C1A; Tue, 29 Mar 2022 05:51:02 -0700 (PDT) Received: from smtp-3-0001.mail.infomaniak.ch (unknown [10.4.36.108]) by smtp-3-3000.mail.infomaniak.ch (Postfix) with ESMTPS id 4KSTvS74JSzMq11v; Tue, 29 Mar 2022 14:51:00 +0200 (CEST) Received: from localhost (unknown [23.97.221.149]) by smtp-3-0001.mail.infomaniak.ch (Postfix) with ESMTPA id 4KSTvS4wrjzlhMCG; Tue, 29 Mar 2022 14:51:00 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=digikod.net; s=20191114; t=1648558260; bh=oOLJDQB5hwbJfFE+GVd5hlaYVgtsaPndzmaHv28AxNU=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=vM8iX1FSvvlrnUg1j7ZvgC3CYBD+A07wP5+e1dW+mZXNGV5KNZKYLMmRdlfglVdZs 1gFySiEWjNCTsK4KwrAYy8/KN7Ro7ungv0dOGnRT+P6evfsOkm4tswPbgCk09G3BCq ZfIEdhgZRGqWR2bOTMWMp5A/hi8qQyb3asjqhpdg= From: =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= To: James Morris , "Serge E . Hallyn" Cc: =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= , Al Viro , Jann Horn , John Johansen , Kees Cook , Konstantin Meskhidze , Paul Moore , Shuah Khan , Tetsuo Handa , linux-doc@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, linux-security-module@vger.kernel.org, =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= Subject: [PATCH v2 04/12] landlock: Fix same-layer rule unions Date: Tue, 29 Mar 2022 14:51:09 +0200 Message-Id: <20220329125117.1393824-5-mic@digikod.net> In-Reply-To: <20220329125117.1393824-1-mic@digikod.net> References: <20220329125117.1393824-1-mic@digikod.net> MIME-Version: 1.0 Precedence: bulk List-ID: From: Mickaël Salaün The original behavior was to check if the full set of requested accesses was allowed by at least a rule of every relevant layer. This didn't take into account requests for multiple accesses and same-layer rules allowing the union of these accesses in a complementary way. As a result, multiple accesses requested on a file hierarchy matching rules that, together, allowed these accesses, but without a unique rule allowing all of them, was illegitimately denied. This case should be rare in practice and it can only be triggered by the path_rename or file_open hook implementations. For instance, if, for the same layer, a rule allows execution beneath /a/b and another rule allows read beneath /a, requesting access to read and execute at the same time for /a/b should be allowed for this layer. This was an inconsistency because the union of same-layer rule accesses was already allowed if requested once at a time anyway. This fix changes the way allowed accesses are gathered over a path walk. To take into account all these rule accesses, we store in a matrix all layer granting the set of requested accesses, according to the handled accesses. To avoid heap allocation, we use an array on the stack which is 2*13 bytes. A following commit bringing the LANDLOCK_ACCESS_FS_REFER access right will increase this size to reach 84 bytes (2*14*3) in case of link or rename actions. Add a new layout1.layer_rule_unions test to check that accesses from different rules pertaining to the same layer are ORed in a file hierarchy. Also test that it is not the case for rules from different layers. Reviewed-by: Paul Moore Signed-off-by: Mickaël Salaün Link: https://lore.kernel.org/r/20220329125117.1393824-5-mic@digikod.net --- Changes since v1: * Add Reviewed-by: Paul Moore. * Small cosmetic comment cleanup. --- security/landlock/fs.c | 77 ++++++++++----- security/landlock/ruleset.h | 2 + tools/testing/selftests/landlock/fs_test.c | 107 +++++++++++++++++++++ 3 files changed, 160 insertions(+), 26 deletions(-) diff --git a/security/landlock/fs.c b/security/landlock/fs.c index 0bcb27f2360a..461751c01726 100644 --- a/security/landlock/fs.c +++ b/security/landlock/fs.c @@ -204,45 +204,66 @@ static inline const struct landlock_rule *find_rule( return rule; } -static inline layer_mask_t unmask_layers( - const struct landlock_rule *const rule, - const access_mask_t access_request, layer_mask_t layer_mask) +/* + * @layer_masks is read and may be updated according to the access request and + * the matching rule. + * + * Returns true if the request is allowed (i.e. relevant layer masks for the + * request are empty). + */ +static inline bool unmask_layers(const struct landlock_rule *const rule, + const access_mask_t access_request, + layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS]) { size_t layer_level; + if (!access_request || !layer_masks) + return true; if (!rule) - return layer_mask; + return false; /* * An access is granted if, for each policy layer, at least one rule - * encountered on the pathwalk grants the requested accesses, - * regardless of their position in the layer stack. We must then check + * encountered on the pathwalk grants the requested access, + * regardless of its position in the layer stack. We must then check * the remaining layers for each inode, from the first added layer to - * the last one. + * the last one. When there is multiple requested accesses, for each + * policy layer, the full set of requested accesses may not be granted + * by only one rule, but by the union (binary OR) of multiple rules. + * E.g. /a/b + /a => /a/b */ for (layer_level = 0; layer_level < rule->num_layers; layer_level++) { const struct landlock_layer *const layer = &rule->layers[layer_level]; const layer_mask_t layer_bit = BIT_ULL(layer->level - 1); + const unsigned long access_req = access_request; + unsigned long access_bit; + bool is_empty; - /* Checks that the layer grants access to the full request. */ - if ((layer->access & access_request) == access_request) { - layer_mask &= ~layer_bit; - - if (layer_mask == 0) - return layer_mask; + /* + * Records in @layer_masks which layer grants access to each + * requested access. + */ + is_empty = true; + for_each_set_bit(access_bit, &access_req, + ARRAY_SIZE(*layer_masks)) { + if (layer->access & BIT_ULL(access_bit)) + (*layer_masks)[access_bit] &= ~layer_bit; + is_empty = is_empty && !(*layer_masks)[access_bit]; } + if (is_empty) + return true; } - return layer_mask; + return false; } static int check_access_path(const struct landlock_ruleset *const domain, const struct path *const path, const access_mask_t access_request) { - bool allowed = false; + layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {}; + bool allowed = false, has_access = false; struct path walker_path; - layer_mask_t layer_mask; size_t i; if (!access_request) @@ -262,13 +283,20 @@ static int check_access_path(const struct landlock_ruleset *const domain, return -EACCES; /* Saves all layers handling a subset of requested accesses. */ - layer_mask = 0; for (i = 0; i < domain->num_layers; i++) { - if (domain->fs_access_masks[i] & access_request) - layer_mask |= BIT_ULL(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; + } + } } /* An access request not handled by the domain is allowed. */ - if (layer_mask == 0) + if (!has_access) return 0; walker_path = *path; @@ -280,14 +308,11 @@ static int check_access_path(const struct landlock_ruleset *const domain, while (true) { struct dentry *parent_dentry; - layer_mask = unmask_layers(find_rule(domain, - walker_path.dentry), access_request, - layer_mask); - if (layer_mask == 0) { + allowed = unmask_layers(find_rule(domain, walker_path.dentry), + access_request, &layer_masks); + if (allowed) /* Stops when a rule from each layer grants access. */ - allowed = true; break; - } jump_up: if (walker_path.dentry == walker_path.mnt->mnt_root) { diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h index 0128c56ee7ff..fa17cc1f82db 100644 --- a/security/landlock/ruleset.h +++ b/security/landlock/ruleset.h @@ -22,6 +22,8 @@ typedef u16 access_mask_t; /* Makes sure all filesystem access rights can be stored. */ static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_FS); +/* Makes sure for_each_set_bit() and for_each_clear_bit() calls are OK. */ +static_assert(sizeof(unsigned long) >= sizeof(access_mask_t)); typedef u16 layer_mask_t; /* Makes sure all layers can be checked. */ diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c index 99838cac970b..1ac41bfa7382 100644 --- a/tools/testing/selftests/landlock/fs_test.c +++ b/tools/testing/selftests/landlock/fs_test.c @@ -687,6 +687,113 @@ TEST_F_FORK(layout1, ruleset_overlap) ASSERT_EQ(0, test_open(dir_s1d3, O_RDONLY | O_DIRECTORY)); } +TEST_F_FORK(layout1, layer_rule_unions) +{ + const struct rule layer1[] = { + { + .path = dir_s1d2, + .access = LANDLOCK_ACCESS_FS_READ_FILE, + }, + /* dir_s1d3 should allow READ_FILE and WRITE_FILE (O_RDWR). */ + { + .path = dir_s1d3, + .access = LANDLOCK_ACCESS_FS_WRITE_FILE, + }, + {} + }; + const struct rule layer2[] = { + /* Doesn't change anything from layer1. */ + { + .path = dir_s1d2, + .access = LANDLOCK_ACCESS_FS_READ_FILE | + LANDLOCK_ACCESS_FS_WRITE_FILE, + }, + {} + }; + const struct rule layer3[] = { + /* Only allows write (but not read) to dir_s1d3. */ + { + .path = dir_s1d2, + .access = LANDLOCK_ACCESS_FS_WRITE_FILE, + }, + {} + }; + int ruleset_fd = create_ruleset(_metadata, ACCESS_RW, layer1); + + ASSERT_LE(0, ruleset_fd); + enforce_ruleset(_metadata, ruleset_fd); + ASSERT_EQ(0, close(ruleset_fd)); + + /* Checks s1d1 hierarchy with layer1. */ + ASSERT_EQ(EACCES, test_open(file1_s1d1, O_RDONLY)); + ASSERT_EQ(EACCES, test_open(file1_s1d1, O_WRONLY)); + ASSERT_EQ(EACCES, test_open(file1_s1d1, O_RDWR)); + ASSERT_EQ(EACCES, test_open(dir_s1d1, O_RDONLY | O_DIRECTORY)); + + /* Checks s1d2 hierarchy with layer1. */ + ASSERT_EQ(0, test_open(file1_s1d2, O_RDONLY)); + ASSERT_EQ(EACCES, test_open(file1_s1d2, O_WRONLY)); + ASSERT_EQ(EACCES, test_open(file1_s1d2, O_RDWR)); + ASSERT_EQ(EACCES, test_open(dir_s1d1, O_RDONLY | O_DIRECTORY)); + + /* Checks s1d3 hierarchy with layer1. */ + ASSERT_EQ(0, test_open(file1_s1d3, O_RDONLY)); + ASSERT_EQ(0, test_open(file1_s1d3, O_WRONLY)); + /* dir_s1d3 should allow READ_FILE and WRITE_FILE (O_RDWR). */ + ASSERT_EQ(0, test_open(file1_s1d3, O_RDWR)); + ASSERT_EQ(EACCES, test_open(dir_s1d1, O_RDONLY | O_DIRECTORY)); + + /* Doesn't change anything from layer1. */ + ruleset_fd = create_ruleset(_metadata, ACCESS_RW, layer2); + ASSERT_LE(0, ruleset_fd); + enforce_ruleset(_metadata, ruleset_fd); + ASSERT_EQ(0, close(ruleset_fd)); + + /* Checks s1d1 hierarchy with layer2. */ + ASSERT_EQ(EACCES, test_open(file1_s1d1, O_RDONLY)); + ASSERT_EQ(EACCES, test_open(file1_s1d1, O_WRONLY)); + ASSERT_EQ(EACCES, test_open(file1_s1d1, O_RDWR)); + ASSERT_EQ(EACCES, test_open(dir_s1d1, O_RDONLY | O_DIRECTORY)); + + /* Checks s1d2 hierarchy with layer2. */ + ASSERT_EQ(0, test_open(file1_s1d2, O_RDONLY)); + ASSERT_EQ(EACCES, test_open(file1_s1d2, O_WRONLY)); + ASSERT_EQ(EACCES, test_open(file1_s1d2, O_RDWR)); + ASSERT_EQ(EACCES, test_open(dir_s1d1, O_RDONLY | O_DIRECTORY)); + + /* Checks s1d3 hierarchy with layer2. */ + ASSERT_EQ(0, test_open(file1_s1d3, O_RDONLY)); + ASSERT_EQ(0, test_open(file1_s1d3, O_WRONLY)); + /* dir_s1d3 should allow READ_FILE and WRITE_FILE (O_RDWR). */ + ASSERT_EQ(0, test_open(file1_s1d3, O_RDWR)); + ASSERT_EQ(EACCES, test_open(dir_s1d1, O_RDONLY | O_DIRECTORY)); + + /* Only allows write (but not read) to dir_s1d3. */ + ruleset_fd = create_ruleset(_metadata, ACCESS_RW, layer3); + ASSERT_LE(0, ruleset_fd); + enforce_ruleset(_metadata, ruleset_fd); + ASSERT_EQ(0, close(ruleset_fd)); + + /* Checks s1d1 hierarchy with layer3. */ + ASSERT_EQ(EACCES, test_open(file1_s1d1, O_RDONLY)); + ASSERT_EQ(EACCES, test_open(file1_s1d1, O_WRONLY)); + ASSERT_EQ(EACCES, test_open(file1_s1d1, O_RDWR)); + ASSERT_EQ(EACCES, test_open(dir_s1d1, O_RDONLY | O_DIRECTORY)); + + /* Checks s1d2 hierarchy with layer3. */ + ASSERT_EQ(EACCES, test_open(file1_s1d2, O_RDONLY)); + ASSERT_EQ(EACCES, test_open(file1_s1d2, O_WRONLY)); + ASSERT_EQ(EACCES, test_open(file1_s1d2, O_RDWR)); + ASSERT_EQ(EACCES, test_open(dir_s1d1, O_RDONLY | O_DIRECTORY)); + + /* Checks s1d3 hierarchy with layer3. */ + ASSERT_EQ(EACCES, test_open(file1_s1d3, O_RDONLY)); + ASSERT_EQ(0, test_open(file1_s1d3, O_WRONLY)); + /* dir_s1d3 should now deny READ_FILE and WRITE_FILE (O_RDWR). */ + ASSERT_EQ(EACCES, test_open(file1_s1d3, O_RDWR)); + ASSERT_EQ(EACCES, test_open(dir_s1d1, O_RDONLY | O_DIRECTORY)); +} + TEST_F_FORK(layout1, non_overlapping_accesses) { const struct rule layer1[] = { From patchwork Tue Mar 29 12:51:10 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= X-Patchwork-Id: 12794806 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 857ABC433FE for ; Tue, 29 Mar 2022 12:51:35 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236809AbiC2MxM (ORCPT ); Tue, 29 Mar 2022 08:53:12 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35758 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237167AbiC2MxA (ORCPT ); Tue, 29 Mar 2022 08:53:00 -0400 Received: from smtp-190a.mail.infomaniak.ch (smtp-190a.mail.infomaniak.ch [IPv6:2001:1600:4:17::190a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id C778911C34 for ; Tue, 29 Mar 2022 05:51:02 -0700 (PDT) Received: from smtp-3-0001.mail.infomaniak.ch (unknown [10.4.36.108]) by smtp-3-3000.mail.infomaniak.ch (Postfix) with ESMTPS id 4KSTvT4R2wzMpqn6; Tue, 29 Mar 2022 14:51:01 +0200 (CEST) Received: from localhost (unknown [23.97.221.149]) by smtp-3-0001.mail.infomaniak.ch (Postfix) with ESMTPA id 4KSTvT2cKczlhMBy; Tue, 29 Mar 2022 14:51:01 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=digikod.net; s=20191114; t=1648558261; bh=WnfPpQmWAAzIHJDB9oGoL2aI5wnbEGO8XYf1VIXPCkk=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=V2wEcZ4qpC3UvBVmxslejauEnYFEBb23jRgeFzDz9J6bt4wX1b+OikqmVINcdKWFS uIS7k35B7oLqU6PqMTVZEmVIJMm5MNqnsvxe6aDq9dFolC46H0RyUitepACRN6F1Wc hmUkisZeYuPrB4EZ26mb6E6rFxCmUuDC9INfSmWU= From: =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= To: James Morris , "Serge E . Hallyn" Cc: =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= , Al Viro , Jann Horn , John Johansen , Kees Cook , Konstantin Meskhidze , Paul Moore , Shuah Khan , Tetsuo Handa , linux-doc@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, linux-security-module@vger.kernel.org, =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= Subject: [PATCH v2 05/12] landlock: Move filesystem helpers and add a new one Date: Tue, 29 Mar 2022 14:51:10 +0200 Message-Id: <20220329125117.1393824-6-mic@digikod.net> In-Reply-To: <20220329125117.1393824-1-mic@digikod.net> References: <20220329125117.1393824-1-mic@digikod.net> MIME-Version: 1.0 Precedence: bulk List-ID: From: Mickaël Salaün Move the SB_NOUSER and IS_PRIVATE dentry check to a standalone is_nouser_or_private() helper. This will be useful for a following commit. Move get_mode_access() and maybe_remove() to make them usable by new code provided by a following commit. Reviewed-by: Paul Moore Signed-off-by: Mickaël Salaün Link: https://lore.kernel.org/r/20220329125117.1393824-6-mic@digikod.net --- Changes since v1: * Move is_nouser_or_private() explanation up to a function header comment block as suggested by Paul Moore. * Add Reviewed-by: Paul Moore. --- security/landlock/fs.c | 87 ++++++++++++++++++++++-------------------- 1 file changed, 46 insertions(+), 41 deletions(-) diff --git a/security/landlock/fs.c b/security/landlock/fs.c index 461751c01726..57dc3fb0c557 100644 --- a/security/landlock/fs.c +++ b/security/landlock/fs.c @@ -257,6 +257,18 @@ static inline bool unmask_layers(const struct landlock_rule *const rule, return false; } +/* + * Allows access to pseudo filesystems that will never be mountable (e.g. + * sockfs, pipefs), but can still be reachable through + * /proc//fd/ + */ +static inline bool is_nouser_or_private(const struct dentry *dentry) +{ + return (dentry->d_sb->s_flags & SB_NOUSER) || + (d_is_positive(dentry) && + unlikely(IS_PRIVATE(d_backing_inode(dentry)))); +} + static int check_access_path(const struct landlock_ruleset *const domain, const struct path *const path, const access_mask_t access_request) @@ -270,14 +282,7 @@ static int check_access_path(const struct landlock_ruleset *const domain, return 0; if (WARN_ON_ONCE(!domain || !path)) return 0; - /* - * Allows access to pseudo filesystems that will never be mountable - * (e.g. sockfs, pipefs), but can still be reachable through - * /proc//fd/ . - */ - if ((path->dentry->d_sb->s_flags & SB_NOUSER) || - (d_is_positive(path->dentry) && - unlikely(IS_PRIVATE(d_backing_inode(path->dentry))))) + if (is_nouser_or_private(path->dentry)) return 0; if (WARN_ON_ONCE(domain->num_layers < 1)) return -EACCES; @@ -356,6 +361,39 @@ static inline int current_check_access_path(const struct path *const path, return check_access_path(dom, path, access_request); } +static inline access_mask_t get_mode_access(const umode_t mode) +{ + switch (mode & S_IFMT) { + case S_IFLNK: + return LANDLOCK_ACCESS_FS_MAKE_SYM; + case 0: + /* A zero mode translates to S_IFREG. */ + case S_IFREG: + return LANDLOCK_ACCESS_FS_MAKE_REG; + case S_IFDIR: + return LANDLOCK_ACCESS_FS_MAKE_DIR; + case S_IFCHR: + return LANDLOCK_ACCESS_FS_MAKE_CHAR; + case S_IFBLK: + return LANDLOCK_ACCESS_FS_MAKE_BLOCK; + case S_IFIFO: + return LANDLOCK_ACCESS_FS_MAKE_FIFO; + case S_IFSOCK: + return LANDLOCK_ACCESS_FS_MAKE_SOCK; + default: + WARN_ON_ONCE(1); + return 0; + } +} + +static inline access_mask_t maybe_remove(const struct dentry *const dentry) +{ + if (d_is_negative(dentry)) + return 0; + return d_is_dir(dentry) ? LANDLOCK_ACCESS_FS_REMOVE_DIR : + LANDLOCK_ACCESS_FS_REMOVE_FILE; +} + /* Inode hooks */ static void hook_inode_free_security(struct inode *const inode) @@ -549,31 +587,6 @@ static int hook_sb_pivotroot(const struct path *const old_path, /* Path hooks */ -static inline access_mask_t get_mode_access(const umode_t mode) -{ - switch (mode & S_IFMT) { - case S_IFLNK: - return LANDLOCK_ACCESS_FS_MAKE_SYM; - case 0: - /* A zero mode translates to S_IFREG. */ - case S_IFREG: - return LANDLOCK_ACCESS_FS_MAKE_REG; - case S_IFDIR: - return LANDLOCK_ACCESS_FS_MAKE_DIR; - case S_IFCHR: - return LANDLOCK_ACCESS_FS_MAKE_CHAR; - case S_IFBLK: - return LANDLOCK_ACCESS_FS_MAKE_BLOCK; - case S_IFIFO: - return LANDLOCK_ACCESS_FS_MAKE_FIFO; - case S_IFSOCK: - return LANDLOCK_ACCESS_FS_MAKE_SOCK; - default: - WARN_ON_ONCE(1); - return 0; - } -} - /* * Creating multiple links or renaming may lead to privilege escalations if not * handled properly. Indeed, we must be sure that the source doesn't gain more @@ -601,14 +614,6 @@ static int hook_path_link(struct dentry *const old_dentry, get_mode_access(d_backing_inode(old_dentry)->i_mode)); } -static inline access_mask_t maybe_remove(const struct dentry *const dentry) -{ - if (d_is_negative(dentry)) - return 0; - return d_is_dir(dentry) ? LANDLOCK_ACCESS_FS_REMOVE_DIR : - LANDLOCK_ACCESS_FS_REMOVE_FILE; -} - static int hook_path_rename(const struct path *const old_dir, struct dentry *const old_dentry, const struct path *const new_dir, From patchwork Tue Mar 29 12:51:11 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= X-Patchwork-Id: 12794814 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id CB3A8C433FE for ; Tue, 29 Mar 2022 12:51:47 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236844AbiC2Mx1 (ORCPT ); Tue, 29 Mar 2022 08:53:27 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35432 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237180AbiC2MxA (ORCPT ); Tue, 29 Mar 2022 08:53:00 -0400 Received: from smtp-bc0c.mail.infomaniak.ch (smtp-bc0c.mail.infomaniak.ch [45.157.188.12]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 64D3B15A09 for ; Tue, 29 Mar 2022 05:51:04 -0700 (PDT) Received: from smtp-3-0001.mail.infomaniak.ch (unknown [10.4.36.108]) by smtp-3-3000.mail.infomaniak.ch (Postfix) with ESMTPS id 4KSTvV64p7zMq17p; Tue, 29 Mar 2022 14:51:02 +0200 (CEST) Received: from localhost (unknown [23.97.221.149]) by smtp-3-0001.mail.infomaniak.ch (Postfix) with ESMTPA id 4KSTvV04V8zlhMCD; Tue, 29 Mar 2022 14:51:02 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=digikod.net; s=20191114; t=1648558262; bh=nD9Oyi8gxV+B05hDwh7Il4DQJP6sBS/TgNM+vfS7Y1w=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=SJz9xJy1mTLZyhh5q/Y0cuFvDpXd7N543mBzSV/zqGkBIQSe1gFMy43TM+axrlVuF KCAVW8tlavOpYHUxkCM7Vi0M6RVUOAWQfG3WHgSG2SY/nJ7l5S4WX7NLCKSuIuJIBb 3fkXylEC7knef6XSARV4m8NmaXoKcuy75sP0MmDQ= From: =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= To: James Morris , "Serge E . Hallyn" Cc: =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= , Al Viro , Jann Horn , John Johansen , Kees Cook , Konstantin Meskhidze , Paul Moore , Shuah Khan , Tetsuo Handa , linux-doc@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, linux-security-module@vger.kernel.org, Kentaro Takeda , =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= Subject: [PATCH v2 06/12] LSM: Remove double path_rename hook calls for RENAME_EXCHANGE Date: Tue, 29 Mar 2022 14:51:11 +0200 Message-Id: <20220329125117.1393824-7-mic@digikod.net> In-Reply-To: <20220329125117.1393824-1-mic@digikod.net> References: <20220329125117.1393824-1-mic@digikod.net> MIME-Version: 1.0 Precedence: bulk List-ID: From: Mickaël Salaün In order to be able to identify a file exchange with renameat2(2) and RENAME_EXCHANGE, which will be useful for Landlock [1], propagate the rename flags to LSMs. This may also improve performance because of the switch from two set of LSM hook calls to only one, and because LSMs using this hook may optimize the double check (e.g. only one lock, reduce the number of path walks). AppArmor, Landlock and Tomoyo are updated to leverage this change. This should not change the current behavior (same check order), except (different level of) speed boosts. [1] https://lore.kernel.org/r/20220221212522.320243-1-mic@digikod.net Cc: James Morris Cc: Kentaro Takeda Cc: Paul Moore Cc: Serge E. Hallyn Acked-by: John Johansen Acked-by: Tetsuo Handa Signed-off-by: Mickaël Salaün Link: https://lore.kernel.org/r/20220329125117.1393824-7-mic@digikod.net Reviewed-by: Paul Moore --- Changes since v1: * Import patch from https://lore.kernel.org/r/20220222175332.384545-1-mic@digikod.net * Add Acked-by: Tetsuo Handa. * Add Acked-by: John Johansen. --- include/linux/lsm_hook_defs.h | 2 +- include/linux/lsm_hooks.h | 1 + security/apparmor/lsm.c | 30 +++++++++++++++++++++++++----- security/landlock/fs.c | 12 ++++++++++-- security/security.c | 9 +-------- security/tomoyo/tomoyo.c | 11 ++++++++++- 6 files changed, 48 insertions(+), 17 deletions(-) diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h index 819ec92dc2a8..d8b49c9c3a8a 100644 --- a/include/linux/lsm_hook_defs.h +++ b/include/linux/lsm_hook_defs.h @@ -100,7 +100,7 @@ LSM_HOOK(int, 0, path_link, struct dentry *old_dentry, const struct path *new_dir, struct dentry *new_dentry) LSM_HOOK(int, 0, path_rename, const struct path *old_dir, struct dentry *old_dentry, const struct path *new_dir, - struct dentry *new_dentry) + struct dentry *new_dentry, unsigned int flags) LSM_HOOK(int, 0, path_chmod, const struct path *path, umode_t mode) LSM_HOOK(int, 0, path_chown, const struct path *path, kuid_t uid, kgid_t gid) LSM_HOOK(int, 0, path_chroot, const struct path *path) diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index 3bf5c658bc44..32cd2a7fe9fc 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -358,6 +358,7 @@ * @old_dentry contains the dentry structure of the old link. * @new_dir contains the path structure for parent of the new link. * @new_dentry contains the dentry structure of the new link. + * @flags may contain rename options such as RENAME_EXCHANGE. * Return 0 if permission is granted. * @path_chmod: * Check for permission to change a mode of the file @path. The new diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c index 4f0eecb67dde..900bc540656a 100644 --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c @@ -354,13 +354,16 @@ static int apparmor_path_link(struct dentry *old_dentry, const struct path *new_ } static int apparmor_path_rename(const struct path *old_dir, struct dentry *old_dentry, - const struct path *new_dir, struct dentry *new_dentry) + const struct path *new_dir, struct dentry *new_dentry, + const unsigned int flags) { struct aa_label *label; int error = 0; if (!path_mediated_fs(old_dentry)) return 0; + if ((flags & RENAME_EXCHANGE) && !path_mediated_fs(new_dentry)) + return 0; label = begin_current_label_crit_section(); if (!unconfined(label)) { @@ -374,10 +377,27 @@ static int apparmor_path_rename(const struct path *old_dir, struct dentry *old_d d_backing_inode(old_dentry)->i_mode }; - error = aa_path_perm(OP_RENAME_SRC, label, &old_path, 0, - MAY_READ | AA_MAY_GETATTR | MAY_WRITE | - AA_MAY_SETATTR | AA_MAY_DELETE, - &cond); + if (flags & RENAME_EXCHANGE) { + struct path_cond cond_exchange = { + i_uid_into_mnt(mnt_userns, d_backing_inode(new_dentry)), + d_backing_inode(new_dentry)->i_mode + }; + + error = aa_path_perm(OP_RENAME_SRC, label, &new_path, 0, + MAY_READ | AA_MAY_GETATTR | MAY_WRITE | + AA_MAY_SETATTR | AA_MAY_DELETE, + &cond_exchange); + if (!error) + error = aa_path_perm(OP_RENAME_DEST, label, &old_path, + 0, MAY_WRITE | AA_MAY_SETATTR | + AA_MAY_CREATE, &cond_exchange); + } + + if (!error) + error = aa_path_perm(OP_RENAME_SRC, label, &old_path, 0, + MAY_READ | AA_MAY_GETATTR | MAY_WRITE | + AA_MAY_SETATTR | AA_MAY_DELETE, + &cond); if (!error) error = aa_path_perm(OP_RENAME_DEST, label, &new_path, 0, MAY_WRITE | AA_MAY_SETATTR | diff --git a/security/landlock/fs.c b/security/landlock/fs.c index 57dc3fb0c557..4422789814ed 100644 --- a/security/landlock/fs.c +++ b/security/landlock/fs.c @@ -617,10 +617,12 @@ static int hook_path_link(struct dentry *const old_dentry, static int hook_path_rename(const struct path *const old_dir, struct dentry *const old_dentry, const struct path *const new_dir, - struct dentry *const new_dentry) + struct dentry *const new_dentry, + const unsigned int flags) { const struct landlock_ruleset *const dom = landlock_get_current_domain(); + u32 exchange_access = 0; if (!dom) return 0; @@ -628,11 +630,17 @@ static int hook_path_rename(const struct path *const old_dir, if (old_dir->dentry != new_dir->dentry) /* Gracefully forbids reparenting. */ return -EXDEV; + if (flags & RENAME_EXCHANGE) { + if (unlikely(d_is_negative(new_dentry))) + return -ENOENT; + exchange_access = + get_mode_access(d_backing_inode(new_dentry)->i_mode); + } if (unlikely(d_is_negative(old_dentry))) return -ENOENT; /* RENAME_EXCHANGE is handled because directories are the same. */ return check_access_path(dom, old_dir, maybe_remove(old_dentry) | - maybe_remove(new_dentry) | + maybe_remove(new_dentry) | exchange_access | get_mode_access(d_backing_inode(old_dentry)->i_mode)); } diff --git a/security/security.c b/security/security.c index 22261d79f333..8634da4cfd46 100644 --- a/security/security.c +++ b/security/security.c @@ -1184,15 +1184,8 @@ int security_path_rename(const struct path *old_dir, struct dentry *old_dentry, (d_is_positive(new_dentry) && IS_PRIVATE(d_backing_inode(new_dentry))))) return 0; - if (flags & RENAME_EXCHANGE) { - int err = call_int_hook(path_rename, 0, new_dir, new_dentry, - old_dir, old_dentry); - if (err) - return err; - } - return call_int_hook(path_rename, 0, old_dir, old_dentry, new_dir, - new_dentry); + new_dentry, flags); } EXPORT_SYMBOL(security_path_rename); diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c index b6a31901f289..71e82d855ebf 100644 --- a/security/tomoyo/tomoyo.c +++ b/security/tomoyo/tomoyo.c @@ -264,17 +264,26 @@ static int tomoyo_path_link(struct dentry *old_dentry, const struct path *new_di * @old_dentry: Pointer to "struct dentry". * @new_parent: Pointer to "struct path". * @new_dentry: Pointer to "struct dentry". + * @flags: Rename options. * * Returns 0 on success, negative value otherwise. */ static int tomoyo_path_rename(const struct path *old_parent, struct dentry *old_dentry, const struct path *new_parent, - struct dentry *new_dentry) + struct dentry *new_dentry, + const unsigned int flags) { struct path path1 = { .mnt = old_parent->mnt, .dentry = old_dentry }; struct path path2 = { .mnt = new_parent->mnt, .dentry = new_dentry }; + if (flags & RENAME_EXCHANGE) { + const int err = tomoyo_path2_perm(TOMOYO_TYPE_RENAME, &path2, + &path1); + + if (err) + return err; + } return tomoyo_path2_perm(TOMOYO_TYPE_RENAME, &path1, &path2); } From patchwork Tue Mar 29 12:51:12 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= X-Patchwork-Id: 12794808 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id E1180C4321E for ; Tue, 29 Mar 2022 12:51:36 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236866AbiC2MxR (ORCPT ); Tue, 29 Mar 2022 08:53:17 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:36282 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237186AbiC2MxB (ORCPT ); Tue, 29 Mar 2022 08:53:01 -0400 Received: from smtp-190e.mail.infomaniak.ch (smtp-190e.mail.infomaniak.ch [IPv6:2001:1600:4:17::190e]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 2921F15FFD for ; Tue, 29 Mar 2022 05:51:05 -0700 (PDT) Received: from smtp-3-0001.mail.infomaniak.ch (unknown [10.4.36.108]) by smtp-3-3000.mail.infomaniak.ch (Postfix) with ESMTPS id 4KSTvW3X8GzMq12C; Tue, 29 Mar 2022 14:51:03 +0200 (CEST) Received: from localhost (unknown [23.97.221.149]) by smtp-3-0001.mail.infomaniak.ch (Postfix) with ESMTPA id 4KSTvW1dnSzlhZvx; Tue, 29 Mar 2022 14:51:03 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=digikod.net; s=20191114; t=1648558263; bh=yTtjEyCUu+6h5oQTtaPQ7OslhzznMZtbhrm4eGSmE9o=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=BrFDhYyBQpNXkTgzTyHOb95rLsrXoxQoPWg0RbD1CHEwyBtLwtAtm9rn8QeOAMUfs GVK0zjsk6lEJmESd3xMnKc+HEdHh3H406aGaTIMHudeltaACnIM4uL4qmDtsjK7HYC Dd2eDWgFGWGyopoHPaiDCWs7aSwyuY1GxYufH8f0= From: =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= To: James Morris , "Serge E . Hallyn" Cc: =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= , Al Viro , Jann Horn , John Johansen , Kees Cook , Konstantin Meskhidze , Paul Moore , Shuah Khan , Tetsuo Handa , linux-doc@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, linux-security-module@vger.kernel.org, =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= Subject: [PATCH v2 07/12] landlock: Add support for file reparenting with LANDLOCK_ACCESS_FS_REFER Date: Tue, 29 Mar 2022 14:51:12 +0200 Message-Id: <20220329125117.1393824-8-mic@digikod.net> In-Reply-To: <20220329125117.1393824-1-mic@digikod.net> References: <20220329125117.1393824-1-mic@digikod.net> MIME-Version: 1.0 Precedence: bulk List-ID: From: Mickaël Salaün 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). Moreover, RENAME_EXCHANGE would also add to the confusion because of paths being both a source and a destination. See the provided documentation for additional details. New tests are provided with a following commit. Cc: Paul Moore Signed-off-by: Mickaël Salaün Link: https://lore.kernel.org/r/20220329125117.1393824-8-mic@digikod.net Reviewed-by: Paul Moore --- Changes since v1: * Update current_check_access_path() to efficiently handle RENAME_EXCHANGE thanks to the updated LSM hook (see previous patch). Only one path walk is performed per rename arguments until their common mount point is reached. Superset of access rights is correctly checked, including when exchanging a file with a directory. This requires to store another matrix of layer masks. * Reorder and rename check_access_path_dual() arguments in a more generic way: switch from src/dst to 1/2. This makes it easier to understand the RENAME_EXCHANGE cases alongs with the others. Update and improve check_access_path_dual() documentation accordingly. * Clean up the check_access_path_dual() loop: set both allowed_parent* when reaching internal filesystems and remove a useless one. This allows potential renames in internal filesystems (like for other operations). * Move the function arguments checks from BUILD_BUG_ON() to WARN_ON_ONCE() to avoid clang build error. * Rename is_superset() to no_more_access() and make it handle superset checks of source and destination for simple and exchange cases. * Move the layer_masks_child* creation from current_check_refer_path() to check_access_path_dual(): this is simpler and less error-prone, especially with RENAME_EXCHANGE. * Remove one optimization in current_check_refer_path() to make the code simpler, especially with the RENAME_EXCHANGE handling. * Remove overzealous WARN_ON_ONCE() for !access_request check in init_layer_masks(). --- include/uapi/linux/landlock.h | 27 +- security/landlock/fs.c | 607 ++++++++++++++++--- 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, 566 insertions(+), 77 deletions(-) diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h index b3d952067f59..f433d58a58f2 100644 --- a/include/uapi/linux/landlock.h +++ b/include/uapi/linux/landlock.h @@ -21,8 +21,14 @@ struct landlock_ruleset_attr { /** * @handled_access_fs: Bitmask of actions (cf. `Filesystem flags`_) * that is handled by this ruleset and should then be forbidden if no - * rule explicitly allow them. This is needed for backward - * compatibility reasons. + * rule explicitly allow them: it is a deny-by-default list that should + * contain as much Landlock access rights as possible. Indeed, all + * Landlock filesystem access rights that are not part of + * handled_access_fs are allowed. This is needed for backward + * compatibility reasons. One exception is the + * LANDLOCK_ACCESS_FS_REFER access right, which is always implicitly + * handled, but must still be explicitly handled to add new rules with + * this access right. */ __u64 handled_access_fs; }; @@ -109,6 +115,22 @@ struct landlock_path_beneath_attr { * - %LANDLOCK_ACCESS_FS_MAKE_FIFO: Create (or rename or link) a named pipe. * - %LANDLOCK_ACCESS_FS_MAKE_BLOCK: Create (or rename or link) a block device. * - %LANDLOCK_ACCESS_FS_MAKE_SYM: Create (or rename or link) a symbolic link. + * - %LANDLOCK_ACCESS_FS_REFER: Link or rename a file from or to a different + * directory (i.e. reparent a file hierarchy). This access right is + * available since the second version of the Landlock ABI. This is also the + * only access right which is always considered handled by any ruleset in + * such a way that reparenting a file hierarchy is always denied by default. + * To avoid privilege escalation, it is not enough to add a rule with this + * access right. When linking or renaming a file, the destination directory + * hierarchy must also always have the same or a superset of restrictions of + * the source hierarchy. If it is not the case, or if the domain doesn't + * handle this access right, such actions are denied by default with errno + * set to EXDEV. Linking also requires a LANDLOCK_ACCESS_FS_MAKE_* access + * right on the destination directory, and renaming also requires a + * LANDLOCK_ACCESS_FS_REMOVE_* access right on the source's (file or + * directory) parent. Otherwise, such actions are denied with errno set to + * EACCES. The EACCES errno prevails over EXDEV to let user space + * efficiently deal with an unrecoverable error. * * .. warning:: * @@ -133,5 +155,6 @@ struct landlock_path_beneath_attr { #define LANDLOCK_ACCESS_FS_MAKE_FIFO (1ULL << 10) #define LANDLOCK_ACCESS_FS_MAKE_BLOCK (1ULL << 11) #define LANDLOCK_ACCESS_FS_MAKE_SYM (1ULL << 12) +#define LANDLOCK_ACCESS_FS_REFER (1ULL << 13) #endif /* _UAPI_LINUX_LANDLOCK_H */ diff --git a/security/landlock/fs.c b/security/landlock/fs.c index 4422789814ed..a8c028f4a17e 100644 --- a/security/landlock/fs.c +++ b/security/landlock/fs.c @@ -4,6 +4,7 @@ * * Copyright © 2016-2020 Mickaël Salaün * Copyright © 2018-2020 ANSSI + * Copyright © 2021-2022 Microsoft Corporation */ #include @@ -269,40 +270,269 @@ 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, - const access_mask_t access_request) +static inline access_mask_t get_handled_accesses( + const struct landlock_ruleset *const domain) { - layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {}; - bool allowed = false, has_access = false; - struct path walker_path; - size_t i; + 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; + + 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)); + /* An empty access request can happen because of O_WRONLY | O_RDWR. */ if (!access_request) return 0; - if (WARN_ON_ONCE(!domain || !path)) - return 0; - if (is_nouser_or_private(path->dentry)) - return 0; - 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++) { + /* 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[i] & BIT_ULL(access_bit)) { - layer_masks[access_bit] |= BIT_ULL(i); - has_access = true; + 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); } } } - /* An access request not handled by the domain is allowed. */ - if (!has_access) + 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. + * + * @layer_masks_child2: Optional child masks. + */ +static inline bool no_more_access( + const layer_mask_t (*const + layer_masks_parent1)[LANDLOCK_NUM_ACCESS_FS], + const layer_mask_t (*const + layer_masks_child1)[LANDLOCK_NUM_ACCESS_FS], + const bool child1_is_directory, + const layer_mask_t (*const + layer_masks_parent2)[LANDLOCK_NUM_ACCESS_FS], + const layer_mask_t (*const + layer_masks_child2)[LANDLOCK_NUM_ACCESS_FS], + const bool child2_is_directory) +{ + unsigned long access_bit; + + for (access_bit = 0; access_bit < ARRAY_SIZE(*layer_masks_parent2); + access_bit++) { + /* Ignores accesses that only make sense for directories. */ + const bool is_file_access = !!(BIT_ULL(access_bit) & ACCESS_FILE); + + if (child1_is_directory || is_file_access) { + /* + * Checks if the destination restrictions are a + * superset of the source ones (i.e. inherited access + * rights without child exceptions): + * restrictions(parent2) >= restrictions(child1) + */ + if ((((*layer_masks_parent1)[access_bit] & + (*layer_masks_child1)[access_bit]) | + (*layer_masks_parent2)[access_bit]) != + (*layer_masks_parent2)[access_bit]) + return false; + } + + if (!layer_masks_child2) + continue; + if (child2_is_directory || is_file_access) { + /* + * Checks inverted restrictions for RENAME_EXCHANGE: + * restrictions(parent1) >= restrictions(child2) + */ + if ((((*layer_masks_parent2)[access_bit] & + (*layer_masks_child2)[access_bit]) | + (*layer_masks_parent1)[access_bit]) != + (*layer_masks_parent1)[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) +{ + 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; + } + return false; +} + +/** + * check_access_path_dual - Check accesses for requests with a common path + * + * @domain: Domain to check against. + * @path: File hierarchy to walk through. + * @access_request_parent1: Accesses to check, once @layer_masks_parent1 is + * equal to @layer_masks_parent2 (if any). This is tied to the unique + * requested path for most actions, or the source in case of a refer action + * (i.e. rename or link), or the source and destination in case of + * RENAME_EXCHANGE. + * @layer_masks_parent1: 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_parent1). This matrix can + * initially refer to domain layer masks and, when the accesses for the + * destination and source are the same, to requested layer masks. + * @dentry_child1: Dentry to the initial child of the parent1 path. This + * pointer must be NULL for non-refer actions (i.e. not link nor rename). + * @access_request_parent2: Similar to @access_request_parent1 but for a + * request involving a source and a destination. This refers to the + * destination, except in case of RENAME_EXCHANGE where it also refers to + * the source. Must be set to 0 when using a simple path request. + * @layer_masks_parent2: Similar to @layer_masks_parent1 but for a refer + * action. This must be NULL otherwise. + * @dentry_child2: Dentry to the initial child of the parent2 path. This + * pointer is only set for RENAME_EXCHANGE actions and must be NULL + * otherwise. + * + * This helper first checks that the destination has a superset of restrictions + * compared to the source (if any) for a common path. Because of + * RENAME_EXCHANGE actions, source and destinations may be swapped. 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, + const access_mask_t access_request_parent1, + layer_mask_t (*const + layer_masks_parent1)[LANDLOCK_NUM_ACCESS_FS], + const struct dentry *const dentry_child1, + const access_mask_t access_request_parent2, + layer_mask_t (*const + layer_masks_parent2)[LANDLOCK_NUM_ACCESS_FS], + const struct dentry *const dentry_child2) +{ + bool allowed_parent1 = false, allowed_parent2 = false, is_dom_check, + child1_is_directory = true, child2_is_directory = true; + struct path walker_path; + access_mask_t access_masked_parent1, access_masked_parent2; + layer_mask_t _layer_masks_child1[LANDLOCK_NUM_ACCESS_FS], + _layer_masks_child2[LANDLOCK_NUM_ACCESS_FS]; + layer_mask_t (*layer_masks_child1)[LANDLOCK_NUM_ACCESS_FS] = NULL, + (*layer_masks_child2)[LANDLOCK_NUM_ACCESS_FS] = NULL; + + if (!access_request_parent1 && !access_request_parent2) + return 0; + if (WARN_ON_ONCE(!domain || !path)) + return 0; + if (is_nouser_or_private(path->dentry)) return 0; + if (WARN_ON_ONCE(domain->num_layers < 1 || !layer_masks_parent1)) + return -EACCES; + + if (layer_masks_parent2) { + if (WARN_ON_ONCE(!dentry_child1)) + return -EACCES; + /* + * For a double request, first check for potential privilege + * escalation by looking at domain handled accesses (which are + * a superset of the meaningful requested accesses). + */ + access_masked_parent1 = access_masked_parent2 = + get_handled_accesses(domain); + is_dom_check = true; + } else { + if (WARN_ON_ONCE(dentry_child1 || dentry_child2)) + return -EACCES; + /* For a simple request, only check for requested accesses. */ + access_masked_parent1 = access_request_parent1; + access_masked_parent2 = access_request_parent2; + is_dom_check = false; + } + + if (dentry_child1) { + unmask_layers(find_rule(domain, dentry_child1), + init_layer_masks(domain, + LANDLOCK_MASK_ACCESS_FS, + &_layer_masks_child1), + &_layer_masks_child1); + layer_masks_child1 = &_layer_masks_child1; + child1_is_directory = d_is_dir(dentry_child1); + } + if (dentry_child2) { + unmask_layers(find_rule(domain, dentry_child2), + init_layer_masks(domain, + LANDLOCK_MASK_ACCESS_FS, + &_layer_masks_child2), + &_layer_masks_child2); + layer_masks_child2 = &_layer_masks_child2; + child2_is_directory = d_is_dir(dentry_child2); + } walker_path = *path; path_get(&walker_path); @@ -312,11 +542,55 @@ 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 also applies in the case of RENAME_EXCHANGE, which + * implies checks on both direction. This is crucial for + * standalone multilayered security policies. Furthermore, + * this helps avoid policy writers to shoot themselves in the + * foot. + */ + if (is_dom_check && no_more_access( + layer_masks_parent1, + layer_masks_child1, + child1_is_directory, + layer_masks_parent2, + layer_masks_child2, + child2_is_directory)) { + allowed_parent1 = + scope_to_request(access_request_parent1, + layer_masks_parent1); + allowed_parent2 = + scope_to_request(access_request_parent2, + layer_masks_parent2); + + /* Stops when all accesses are granted. */ + if (allowed_parent1 && allowed_parent2) + break; - allowed = unmask_layers(find_rule(domain, walker_path.dentry), - access_request, &layer_masks); - if (allowed) - /* Stops when a rule from each layer grants access. */ + /* + * Now, downgrades the remaining checks from domain + * handled accesses to requested accesses. + */ + is_dom_check = false; + access_masked_parent1 = access_request_parent1; + access_masked_parent2 = access_request_parent2; + } + + rule = find_rule(domain, walker_path.dentry); + allowed_parent1 = unmask_layers(rule, access_masked_parent1, + layer_masks_parent1); + allowed_parent2 = unmask_layers(rule, access_masked_parent2, + layer_masks_parent2); + + /* Stops when a rule from each layer grants access. */ + if (allowed_parent1 && allowed_parent2) break; jump_up: @@ -329,7 +603,6 @@ static int check_access_path(const struct landlock_ruleset *const domain, * Stops at the real root. Denies access * because not all layers have granted access. */ - allowed = false; break; } } @@ -339,7 +612,8 @@ static int check_access_path(const struct landlock_ruleset *const domain, * access to internal filesystems (e.g. nsfs, which is * reachable through /proc//ns/). */ - allowed = !!(walker_path.mnt->mnt_flags & MNT_INTERNAL); + allowed_parent1 = allowed_parent2 = + !!(walker_path.mnt->mnt_flags & MNT_INTERNAL); break; } parent_dentry = dget_parent(walker_path.dentry); @@ -347,7 +621,38 @@ static int check_access_path(const struct landlock_ruleset *const domain, walker_path.dentry = parent_dentry; } path_put(&walker_path); - return allowed ? 0 : -EACCES; + + if (allowed_parent1 && allowed_parent2) + return 0; + + /* + * This prioritizes EACCES over EXDEV for all actions, including + * renames with RENAME_EXCHANGE. + */ + if (likely(is_eacces(layer_masks_parent1, access_request_parent1) + || is_eacces(layer_masks_parent2, + access_request_parent2))) + return -EACCES; + + /* + * Gracefully forbids reparenting if the destination directory + * hierarchy is not a superset of restrictions of the source directory + * hierarchy, or if LANDLOCK_ACCESS_FS_REFER is not allowed by the + * source or the destination. + */ + return -EXDEV; +} + +static inline int check_access_path(const struct landlock_ruleset *const domain, + const struct path *const path, + access_mask_t access_request) +{ + layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {}; + + access_request = init_layer_masks(domain, access_request, + &layer_masks); + return check_access_path_dual(domain, path, access_request, + &layer_masks, NULL, 0, NULL, NULL); } static inline int current_check_access_path(const struct path *const path, @@ -394,6 +699,204 @@ static inline access_mask_t maybe_remove(const struct dentry *const dentry) LANDLOCK_ACCESS_FS_REMOVE_FILE; } +/** + * collect_domain_accesses - Walk through a file path and collect accesses + * + * @domain: Domain to check against. + * @mnt_root: Last directory to check. + * @dir: Directory to start the walk from. + * @layer_masks_dom: Where to store the collected accesses. + * + * This helper is useful to begin a path walk from the @dir directory to a + * @mnt_root directory used as a mount point. This mount point is the common + * ancestor between the source and the destination of a renamed and linked + * file. While walking from @dir to @mnt_root, we record all the domain's + * allowed accesses in @layer_masks_dom. + * + * This is similar to check_access_path_dual() but much simpler because it only + * handles walking on the same mount point and only check one set of accesses. + * + * Returns: + * - true if all the domain access rights are allowed for @dir; + * - false if the walk reached @mnt_root. + */ +static bool collect_domain_accesses( + const struct landlock_ruleset *const domain, + const struct dentry *const mnt_root, struct dentry *dir, + layer_mask_t (*const layer_masks_dom)[LANDLOCK_NUM_ACCESS_FS]) +{ + unsigned long access_dom; + bool ret = false; + + if (WARN_ON_ONCE(!domain || !mnt_root || !dir || !layer_masks_dom)) + return true; + if (is_nouser_or_private(dir)) + return true; + + access_dom = init_layer_masks(domain, LANDLOCK_MASK_ACCESS_FS, + layer_masks_dom); + + dget(dir); + while (true) { + struct dentry *parent_dentry; + + /* Gets all layers allowing all domain accesses. */ + if (unmask_layers(find_rule(domain, dir), access_dom, + layer_masks_dom)) { + /* + * Stops when all handled accesses are allowed by at + * least one rule in each layer. + */ + ret = true; + break; + } + + /* We should not reach a root other than @mnt_root. */ + if (dir == mnt_root || WARN_ON_ONCE(IS_ROOT(dir))) { + ret = false; + break; + } + + parent_dentry = dget_parent(dir); + dput(dir); + dir = parent_dentry; + } + dput(dir); + return ret; +} + +/** + * current_check_refer_path - Check if a rename or link action is allowed + * + * @old_dentry: File or directory requested to be moved or linked. + * @new_dir: Destination parent directory. + * @new_dentry: Destination file or directory. + * @removable: Sets to true if it is a rename operation. + * @exchange: Sets to true if it is a rename operation with RENAME_EXCHANGE. + * + * Because of its unprivileged constraints, Landlock relies on file hierarchies + * (and not only inodes) to tie access rights to files. Being able to link or + * rename a file hierarchy brings some challenges. Indeed, moving or linking a + * file (i.e. creating a new reference to an inode) can have an impact on the + * actions allowed for a set of files if it would change its parent directory + * (i.e. reparenting). + * + * To avoid trivial access right bypasses, Landlock first checks if the file or + * directory requested to be moved would gain new access rights inherited from + * its new hierarchy. Before returning any error, Landlock then checks that + * the parent source hierarchy and the destination hierarchy would allow the + * link or rename action. If it is not the case, an error with EACCES is + * returned to inform user space that there is no way to remove or create the + * requested source file type. If it should be allowed but the new inherited + * access rights would be greater than the source access rights, then the + * kernel returns an error with EXDEV. Prioritizing EACCES over EXDEV enables + * user space to abort the whole operation if there is no way to do it, or to + * manually copy the source to the destination if this remains allowed, e.g. + * because file creation is allowed on the destination directory but not direct + * linking. + * + * To achieve this goal, the kernel needs to compare two file hierarchies: the + * one identifying the source file or directory (including itself), and the + * destination one. This can be seen as a multilayer partial ordering problem. + * The kernel walks through these paths and collect in a matrix the access + * rights that are denied per layer. These matrices are then compared to see + * if the destination one has more (or the same) restrictions as the source + * one. If this is the case, the requested action will not return EXDEV, which + * doesn't mean the action is allowed. The parent hierarchy of the source + * (i.e. parent directory), and the destination hierarchy must also be checked + * to verify that they explicitly allow such action (i.e. referencing, + * creation and potentially removal rights). The kernel implementation is then + * required to rely on three matrices of access rights: one for the source file + * or directory (i.e. the child), one for the source parent hierarchy and one + * for the destination hierarchy. These ephemeral matrices take some space on + * the stack, which limits the number of layers to a deemed reasonable number: + * 16. + * + * Returns: + * - 0 if access is allowed; + * - -EXDEV if @old_dentry would inherit new access rights from @new_dir; + * - -EACCES if file removal or creation is denied. + */ +static int current_check_refer_path(struct dentry *const old_dentry, + const struct path *const new_dir, + struct dentry *const new_dentry, + const bool removable, const bool exchange) +{ + const struct landlock_ruleset *const dom = + landlock_get_current_domain(); + bool allow_parent1, allow_parent2; + access_mask_t access_request_parent1, access_request_parent2; + struct path mnt_dir; + layer_mask_t layer_masks_parent1[LANDLOCK_NUM_ACCESS_FS], + layer_masks_parent2[LANDLOCK_NUM_ACCESS_FS]; + + if (!dom) + return 0; + if (WARN_ON_ONCE(dom->num_layers < 1)) + return -EACCES; + if (unlikely(d_is_negative(old_dentry))) + return -ENOENT; + if (exchange) { + if (unlikely(d_is_negative(new_dentry))) + return -ENOENT; + access_request_parent1 = + get_mode_access(d_backing_inode(new_dentry)->i_mode); + } else { + access_request_parent1 = 0; + } + access_request_parent2 = + get_mode_access(d_backing_inode(old_dentry)->i_mode); + if (removable) { + access_request_parent1 |= maybe_remove(old_dentry); + access_request_parent2 |= maybe_remove(new_dentry); + } + + /* The mount points are the same for old and new paths, cf. EXDEV. */ + if (old_dentry->d_parent == new_dir->dentry) { + /* + * The LANDLOCK_ACCESS_FS_REFER access right is not required + * for same-directory referer (i.e. no reparenting). + */ + access_request_parent1 = init_layer_masks(dom, + access_request_parent1 | access_request_parent2, + &layer_masks_parent1); + return check_access_path_dual(dom, new_dir, + access_request_parent1, &layer_masks_parent1, + NULL, 0, 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; + + /* Saves the common mount point. */ + mnt_dir.mnt = new_dir->mnt; + mnt_dir.dentry = new_dir->mnt->mnt_root; + + /* new_dir->dentry is equal to new_dentry->d_parent */ + allow_parent1 = collect_domain_accesses(dom, mnt_dir.dentry, + old_dentry->d_parent, &layer_masks_parent1); + allow_parent2 = collect_domain_accesses(dom, mnt_dir.dentry, + new_dir->dentry, &layer_masks_parent2); + + if (allow_parent1 && allow_parent2) + return 0; + + /* + * To be able to compare source and destination domain access rights, + * take into account the @old_dentry access rights aggregated with its + * parent access rights. This will be useful to compare with the + * destination parent access rights. + */ + return check_access_path_dual(dom, &mnt_dir, access_request_parent1, + &layer_masks_parent1, old_dentry, + access_request_parent2, &layer_masks_parent2, + exchange ? new_dentry : NULL); +} + /* Inode hooks */ static void hook_inode_free_security(struct inode *const inode) @@ -587,31 +1090,12 @@ static int hook_sb_pivotroot(const struct path *const old_path, /* Path hooks */ -/* - * Creating multiple links or renaming 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 getting more - * complex when dealing with multiple layers. The whole picture can be seen as - * a multilayer partial ordering problem. A future version of Landlock will - * deal with that. - */ static int hook_path_link(struct dentry *const old_dentry, const struct path *const new_dir, struct dentry *const new_dentry) { - const struct landlock_ruleset *const dom = - landlock_get_current_domain(); - - if (!dom) - return 0; - /* The mount points are the same for old and new paths, cf. EXDEV. */ - if (old_dentry->d_parent != new_dir->dentry) - /* Gracefully forbids reparenting. */ - return -EXDEV; - if (unlikely(d_is_negative(old_dentry))) - return -ENOENT; - return check_access_path(dom, new_dir, - get_mode_access(d_backing_inode(old_dentry)->i_mode)); + return current_check_refer_path(old_dentry, new_dir, new_dentry, false, + false); } static int hook_path_rename(const struct path *const old_dir, @@ -620,28 +1104,9 @@ static int hook_path_rename(const struct path *const old_dir, struct dentry *const new_dentry, const unsigned int flags) { - const struct landlock_ruleset *const dom = - landlock_get_current_domain(); - u32 exchange_access = 0; - - if (!dom) - return 0; - /* The mount points are the same for old and new paths, cf. EXDEV. */ - if (old_dir->dentry != new_dir->dentry) - /* Gracefully forbids reparenting. */ - return -EXDEV; - if (flags & RENAME_EXCHANGE) { - if (unlikely(d_is_negative(new_dentry))) - return -ENOENT; - exchange_access = - get_mode_access(d_backing_inode(new_dentry)->i_mode); - } - if (unlikely(d_is_negative(old_dentry))) - return -ENOENT; - /* RENAME_EXCHANGE is handled because directories are the same. */ - return check_access_path(dom, old_dir, maybe_remove(old_dentry) | - maybe_remove(new_dentry) | exchange_access | - get_mode_access(d_backing_inode(old_dentry)->i_mode)); + /* old_dir refers to old_dentry->d_parent and new_dir->mnt */ + return current_check_refer_path(old_dentry, new_dir, new_dentry, true, + !!(flags & RENAME_EXCHANGE)); } static int hook_path_mkdir(const struct path *const dir, diff --git a/security/landlock/limits.h b/security/landlock/limits.h index 126d1ec04d34..26c8166d0265 100644 --- a/security/landlock/limits.h +++ b/security/landlock/limits.h @@ -16,7 +16,7 @@ #define LANDLOCK_MAX_NUM_LAYERS 16 #define LANDLOCK_MAX_NUM_RULES U32_MAX -#define LANDLOCK_LAST_ACCESS_FS LANDLOCK_ACCESS_FS_MAKE_SYM +#define LANDLOCK_LAST_ACCESS_FS LANDLOCK_ACCESS_FS_REFER #define LANDLOCK_MASK_ACCESS_FS ((LANDLOCK_LAST_ACCESS_FS << 1) - 1) #define LANDLOCK_NUM_ACCESS_FS __const_hweight64(LANDLOCK_MASK_ACCESS_FS) diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c index 32396962f04d..fa14f09b6bf4 100644 --- a/security/landlock/syscalls.c +++ b/security/landlock/syscalls.c @@ -128,7 +128,7 @@ static const struct file_operations ruleset_fops = { .write = fop_dummy_write, }; -#define LANDLOCK_ABI_VERSION 1 +#define LANDLOCK_ABI_VERSION 2 /** * sys_landlock_create_ruleset - Create a new ruleset diff --git a/tools/testing/selftests/landlock/base_test.c b/tools/testing/selftests/landlock/base_test.c index ca40abe9daa8..99aab93d50e1 100644 --- a/tools/testing/selftests/landlock/base_test.c +++ b/tools/testing/selftests/landlock/base_test.c @@ -67,7 +67,7 @@ TEST(abi_version) { const struct landlock_ruleset_attr ruleset_attr = { .handled_access_fs = LANDLOCK_ACCESS_FS_READ_FILE, }; - ASSERT_EQ(1, landlock_create_ruleset(NULL, 0, + ASSERT_EQ(2, landlock_create_ruleset(NULL, 0, LANDLOCK_CREATE_RULESET_VERSION)); ASSERT_EQ(-1, landlock_create_ruleset(&ruleset_attr, 0, diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c index 1ac41bfa7382..0568d1193492 100644 --- a/tools/testing/selftests/landlock/fs_test.c +++ b/tools/testing/selftests/landlock/fs_test.c @@ -381,7 +381,7 @@ TEST_F_FORK(layout1, inval) LANDLOCK_ACCESS_FS_WRITE_FILE | \ LANDLOCK_ACCESS_FS_READ_FILE) -#define ACCESS_LAST LANDLOCK_ACCESS_FS_MAKE_SYM +#define ACCESS_LAST LANDLOCK_ACCESS_FS_REFER #define ACCESS_ALL ( \ ACCESS_FILE | \ @@ -394,6 +394,7 @@ TEST_F_FORK(layout1, inval) LANDLOCK_ACCESS_FS_MAKE_SOCK | \ LANDLOCK_ACCESS_FS_MAKE_FIFO | \ LANDLOCK_ACCESS_FS_MAKE_BLOCK | \ + LANDLOCK_ACCESS_FS_MAKE_SYM | \ ACCESS_LAST) TEST_F_FORK(layout1, file_access_rights) From patchwork Tue Mar 29 12:51:13 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= X-Patchwork-Id: 12794813 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id C43E5C433F5 for ; Tue, 29 Mar 2022 12:51:43 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236926AbiC2MxX (ORCPT ); Tue, 29 Mar 2022 08:53:23 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:34736 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237188AbiC2MxB (ORCPT ); Tue, 29 Mar 2022 08:53:01 -0400 Received: from smtp-bc0e.mail.infomaniak.ch (smtp-bc0e.mail.infomaniak.ch [45.157.188.14]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id A2E4717A8D for ; Tue, 29 Mar 2022 05:51:05 -0700 (PDT) Received: from smtp-3-0001.mail.infomaniak.ch (unknown [10.4.36.108]) by smtp-3-3000.mail.infomaniak.ch (Postfix) with ESMTPS id 4KSTvX1CxpzMq0wY; Tue, 29 Mar 2022 14:51:04 +0200 (CEST) Received: from localhost (unknown [23.97.221.149]) by smtp-3-0001.mail.infomaniak.ch (Postfix) with ESMTPA id 4KSTvW6BrKzlhMCM; Tue, 29 Mar 2022 14:51:03 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=digikod.net; s=20191114; t=1648558264; bh=Rf4F0uPWmqZNWrYhAB8+K6gcochzOrhdgfLa8M2PPNg=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=l+wc3iCMqYRmBijyDhGjwI7hBB+NKQNwdatQHOIoOY4vbdEzZ89IsR+ujzAc5QgWn ysnTMuUZ5YNRLmoZvpMADW7Etnk/K7ZslLqA4NTdPdZn3XVKI6wOGwpHroV4HyePrg ADaFWzXVmCIIY7DPsUAEVcKNcWHDcgEBi6wVSty4= From: =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= To: James Morris , "Serge E . Hallyn" Cc: =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= , Al Viro , Jann Horn , John Johansen , Kees Cook , Konstantin Meskhidze , Paul Moore , Shuah Khan , Tetsuo Handa , linux-doc@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, linux-security-module@vger.kernel.org, =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= Subject: [PATCH v2 08/12] selftest/landlock: Add 11 new test suites dedicated to file reparenting Date: Tue, 29 Mar 2022 14:51:13 +0200 Message-Id: <20220329125117.1393824-9-mic@digikod.net> In-Reply-To: <20220329125117.1393824-1-mic@digikod.net> References: <20220329125117.1393824-1-mic@digikod.net> MIME-Version: 1.0 Precedence: bulk List-ID: From: Mickaël Salaün These test suites try to check all edge cases for directory and file renaming or linking involving a new parent directory, with and without LANDLOCK_ACCESS_FS_REFER and other access rights. layout1: * reparent_refer: Tests simple FS_REFER usage. * reparent_link: Tests a mix of FS_MAKE_REG and FS_REFER with links. * reparent_rename: Tests a mix of FS_MAKE_REG and FS_REFER with renames and RENAME_EXCHANGE. * reparent_exdev_layers_rename1/2: Tests renames with two layers. * reparent_exdev_layers_exchange1/2/3: Tests exchanges with two layers. * reparent_remove: Tests file and directory removal with rename. * reparent_dom_superset: Tests access partial ordering. layout1_bind: * reparent_cross_mount: Tests FS_REFER propagation across mount points. This brings coverage for security/landlock/ from 93.5% of lines to 94.6%. Cc: Paul Moore Signed-off-by: Mickaël Salaün Link: https://lore.kernel.org/r/20220329125117.1393824-9-mic@digikod.net --- Changes since v1: * Add tests specific to RENAME_EXCHANGE and split layout1.reparent_exdev_layers to avoid inconsistent layout modifications and make these tests easier to understand. * Add full error predominance tests with RENAME_EXCHANGE to layout1.reparent_exdev_layers* . This now work as expected thanks to the rename hook change. * Add layout1.reparent_remove tests to check file and directory removal with rename. * Improve the remove_path() helper to handle unlinking paths with non-existing components. --- tools/testing/selftests/landlock/fs_test.c | 754 ++++++++++++++++++++- 1 file changed, 753 insertions(+), 1 deletion(-) diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c index 0568d1193492..cc0466b29c2d 100644 --- a/tools/testing/selftests/landlock/fs_test.c +++ b/tools/testing/selftests/landlock/fs_test.c @@ -130,7 +130,7 @@ static int remove_path(const char *const path) goto out; } if (unlink(path) && rmdir(path)) { - if (errno != ENOENT) + if (errno != ENOENT && errno != ENOTDIR) err = errno; goto out; } @@ -1851,6 +1851,721 @@ TEST_F_FORK(layout1, rename_dir) ASSERT_EQ(0, rmdir(dir_s1d3)); } +TEST_F_FORK(layout1, reparent_refer) +{ + const struct rule layer1[] = { + { + .path = dir_s1d2, + .access = LANDLOCK_ACCESS_FS_REFER, + }, + { + .path = dir_s2d2, + .access = LANDLOCK_ACCESS_FS_REFER, + }, + {} + }; + int ruleset_fd = create_ruleset(_metadata, LANDLOCK_ACCESS_FS_REFER, + layer1); + + ASSERT_LE(0, ruleset_fd); + enforce_ruleset(_metadata, ruleset_fd); + ASSERT_EQ(0, close(ruleset_fd)); + + ASSERT_EQ(-1, rename(dir_s1d2, dir_s2d1)); + ASSERT_EQ(EXDEV, errno); + ASSERT_EQ(-1, rename(dir_s1d2, dir_s2d2)); + ASSERT_EQ(EXDEV, errno); + ASSERT_EQ(-1, rename(dir_s1d2, dir_s2d3)); + ASSERT_EQ(EXDEV, errno); + + ASSERT_EQ(-1, rename(dir_s1d3, dir_s2d1)); + ASSERT_EQ(EXDEV, errno); + ASSERT_EQ(-1, rename(dir_s1d3, dir_s2d2)); + ASSERT_EQ(EXDEV, errno); + /* + * Moving should only be allowed when the source and the destination + * parent directory have REFER. + */ + ASSERT_EQ(-1, rename(dir_s1d3, dir_s2d3)); + ASSERT_EQ(ENOTEMPTY, errno); + ASSERT_EQ(0, unlink(file1_s2d3)); + ASSERT_EQ(0, unlink(file2_s2d3)); + ASSERT_EQ(0, rename(dir_s1d3, dir_s2d3)); +} + +TEST_F_FORK(layout1, reparent_link) +{ + const struct rule layer1[] = { + { + .path = dir_s1d2, + .access = LANDLOCK_ACCESS_FS_MAKE_REG, + }, + { + .path = dir_s1d3, + .access = LANDLOCK_ACCESS_FS_REFER, + }, + { + .path = dir_s2d2, + .access = LANDLOCK_ACCESS_FS_REFER, + }, + { + .path = dir_s2d3, + .access = LANDLOCK_ACCESS_FS_MAKE_REG, + }, + {} + }; + const int ruleset_fd = create_ruleset(_metadata, + LANDLOCK_ACCESS_FS_MAKE_REG | LANDLOCK_ACCESS_FS_REFER, + layer1); + + ASSERT_LE(0, ruleset_fd); + enforce_ruleset(_metadata, ruleset_fd); + ASSERT_EQ(0, close(ruleset_fd)); + + ASSERT_EQ(0, unlink(file1_s1d1)); + ASSERT_EQ(0, unlink(file1_s1d2)); + ASSERT_EQ(0, unlink(file1_s1d3)); + + /* Denies linking because of missing MAKE_REG. */ + ASSERT_EQ(-1, link(file2_s1d1, file1_s1d1)); + ASSERT_EQ(EACCES, errno); + /* Denies linking because of missing source and destination REFER. */ + ASSERT_EQ(-1, link(file1_s2d1, file1_s1d2)); + ASSERT_EQ(EXDEV, errno); + /* Denies linking because of missing source REFER. */ + ASSERT_EQ(-1, link(file1_s2d1, file1_s1d3)); + ASSERT_EQ(EXDEV, errno); + + /* Denies linking because of missing MAKE_REG. */ + ASSERT_EQ(-1, link(file1_s2d2, file1_s1d1)); + ASSERT_EQ(EACCES, errno); + /* Denies linking because of missing destination REFER. */ + ASSERT_EQ(-1, link(file1_s2d2, file1_s1d2)); + ASSERT_EQ(EXDEV, errno); + + /* Allows linking because of REFER and MAKE_REG. */ + ASSERT_EQ(0, link(file1_s2d2, file1_s1d3)); + ASSERT_EQ(0, unlink(file1_s2d2)); + /* Reverse linking denied because of missing MAKE_REG. */ + ASSERT_EQ(-1, link(file1_s1d3, file1_s2d2)); + ASSERT_EQ(EACCES, errno); + ASSERT_EQ(0, unlink(file1_s2d3)); + /* Checks reverse linking. */ + ASSERT_EQ(0, link(file1_s1d3, file1_s2d3)); + ASSERT_EQ(0, unlink(file1_s1d3)); + + /* + * This is OK for a file link, but it should not be allowed for a + * directory rename (because of the superset of access rights. + */ + ASSERT_EQ(0, link(file1_s2d3, file1_s1d3)); + ASSERT_EQ(0, unlink(file1_s1d3)); + + ASSERT_EQ(-1, link(file2_s1d2, file1_s1d3)); + ASSERT_EQ(EXDEV, errno); + ASSERT_EQ(-1, link(file2_s1d3, file1_s1d2)); + ASSERT_EQ(EXDEV, errno); + + ASSERT_EQ(0, link(file2_s1d2, file1_s1d2)); + ASSERT_EQ(0, link(file2_s1d3, file1_s1d3)); +} + +TEST_F_FORK(layout1, reparent_rename) +{ + /* Same rules as for reparent_link. */ + const struct rule layer1[] = { + { + .path = dir_s1d2, + .access = LANDLOCK_ACCESS_FS_MAKE_REG, + }, + { + .path = dir_s1d3, + .access = LANDLOCK_ACCESS_FS_REFER, + }, + { + .path = dir_s2d2, + .access = LANDLOCK_ACCESS_FS_REFER, + }, + { + .path = dir_s2d3, + .access = LANDLOCK_ACCESS_FS_MAKE_REG, + }, + {} + }; + const int ruleset_fd = create_ruleset(_metadata, + LANDLOCK_ACCESS_FS_MAKE_REG | LANDLOCK_ACCESS_FS_REFER, + layer1); + + ASSERT_LE(0, ruleset_fd); + enforce_ruleset(_metadata, ruleset_fd); + ASSERT_EQ(0, close(ruleset_fd)); + + ASSERT_EQ(0, unlink(file1_s1d2)); + ASSERT_EQ(0, unlink(file1_s1d3)); + + /* Denies renaming because of missing MAKE_REG. */ + ASSERT_EQ(-1, renameat2(AT_FDCWD, file2_s1d1, AT_FDCWD, file1_s1d1, + RENAME_EXCHANGE)); + ASSERT_EQ(EACCES, errno); + ASSERT_EQ(-1, renameat2(AT_FDCWD, file1_s1d1, AT_FDCWD, file2_s1d1, + RENAME_EXCHANGE)); + ASSERT_EQ(EACCES, errno); + ASSERT_EQ(0, unlink(file1_s1d1)); + ASSERT_EQ(-1, rename(file2_s1d1, file1_s1d1)); + ASSERT_EQ(EACCES, errno); + /* Even denies same file exchange. */ + ASSERT_EQ(-1, renameat2(AT_FDCWD, file2_s1d1, AT_FDCWD, file2_s1d1, + RENAME_EXCHANGE)); + ASSERT_EQ(EACCES, errno); + + /* Denies renaming because of missing source and destination REFER. */ + ASSERT_EQ(-1, rename(file1_s2d1, file1_s1d2)); + ASSERT_EQ(EXDEV, errno); + /* + * Denies renaming because of missing MAKE_REG, source and destination + * REFER. + */ + ASSERT_EQ(-1, renameat2(AT_FDCWD, file1_s2d1, AT_FDCWD, file2_s1d1, + RENAME_EXCHANGE)); + ASSERT_EQ(EACCES, errno); + ASSERT_EQ(-1, renameat2(AT_FDCWD, file2_s1d1, AT_FDCWD, file1_s2d1, + RENAME_EXCHANGE)); + ASSERT_EQ(EACCES, errno); + + /* Denies renaming because of missing source REFER. */ + ASSERT_EQ(-1, rename(file1_s2d1, file1_s1d3)); + ASSERT_EQ(EXDEV, errno); + /* Denies renaming because of missing MAKE_REG. */ + ASSERT_EQ(-1, renameat2(AT_FDCWD, file1_s2d1, AT_FDCWD, file2_s1d3, + RENAME_EXCHANGE)); + ASSERT_EQ(EACCES, errno); + + /* Denies renaming because of missing MAKE_REG. */ + ASSERT_EQ(-1, rename(file1_s2d2, file1_s1d1)); + ASSERT_EQ(EACCES, errno); + /* Denies renaming because of missing destination REFER*/ + ASSERT_EQ(-1, rename(file1_s2d2, file1_s1d2)); + ASSERT_EQ(EXDEV, errno); + + /* Denies exchange because of one missing MAKE_REG. */ + ASSERT_EQ(-1, renameat2(AT_FDCWD, file1_s2d2, AT_FDCWD, file2_s1d3, + RENAME_EXCHANGE)); + ASSERT_EQ(EACCES, errno); + /* Allows renaming because of REFER and MAKE_REG. */ + ASSERT_EQ(0, rename(file1_s2d2, file1_s1d3)); + + /* Reverse renaming denied because of missing MAKE_REG. */ + ASSERT_EQ(-1, rename(file1_s1d3, file1_s2d2)); + ASSERT_EQ(EACCES, errno); + ASSERT_EQ(0, unlink(file1_s2d3)); + ASSERT_EQ(0, rename(file1_s1d3, file1_s2d3)); + + /* Tests reverse renaming. */ + ASSERT_EQ(0, rename(file1_s2d3, file1_s1d3)); + ASSERT_EQ(0, renameat2(AT_FDCWD, file2_s2d3, AT_FDCWD, file1_s1d3, + RENAME_EXCHANGE)); + ASSERT_EQ(0, rename(file1_s1d3, file1_s2d3)); + + /* + * This is OK for a file rename, but it should not be allowed for a + * directory rename (because of the superset of access rights). + */ + ASSERT_EQ(0, rename(file1_s2d3, file1_s1d3)); + ASSERT_EQ(0, rename(file1_s1d3, file1_s2d3)); + + /* + * Tests superset restrictions applied to directories. Not only the + * dir_s2d3's parent (dir_s2d2) should be taken into account but also + * access rights tied to dir_s2d3. dir_s2d2 is missing one access right + * compared to dir_s1d3/file1_s1d3 (MAKE_REG) but it is provided + * directly by the moved dir_s2d3. + */ + ASSERT_EQ(0, rename(dir_s2d3, file1_s1d3)); + ASSERT_EQ(0, rename(file1_s1d3, dir_s2d3)); + /* + * The first rename is allowed but not the exchange because dir_s1d3's + * parent (dir_s1d2) doesn't have REFER. + */ + ASSERT_EQ(-1, renameat2(AT_FDCWD, file1_s2d3, AT_FDCWD, dir_s1d3, + RENAME_EXCHANGE)); + ASSERT_EQ(EXDEV, errno); + ASSERT_EQ(-1, renameat2(AT_FDCWD, dir_s1d3, AT_FDCWD, file1_s2d3, + RENAME_EXCHANGE)); + ASSERT_EQ(EXDEV, errno); + ASSERT_EQ(-1, rename(file1_s2d3, dir_s1d3)); + ASSERT_EQ(EXDEV, errno); + + ASSERT_EQ(-1, rename(file2_s1d2, file1_s1d3)); + ASSERT_EQ(EXDEV, errno); + ASSERT_EQ(-1, rename(file2_s1d3, file1_s1d2)); + ASSERT_EQ(EXDEV, errno); + + /* Renaming in the same directory is always allowed. */ + ASSERT_EQ(0, rename(file2_s1d2, file1_s1d2)); + ASSERT_EQ(0, rename(file2_s1d3, file1_s1d3)); + + ASSERT_EQ(0, unlink(file1_s1d2)); + /* Denies because of missing source MAKE_REG and destination REFER. */ + ASSERT_EQ(-1, rename(dir_s2d3, file1_s1d2)); + ASSERT_EQ(EXDEV, errno); + + ASSERT_EQ(0, unlink(file1_s1d3)); + /* Denies because of missing source MAKE_REG and REFER. */ + ASSERT_EQ(-1, rename(dir_s2d2, file1_s1d3)); + ASSERT_EQ(EXDEV, errno); +} + + +static void reparent_exdev_layers_enforce1( + struct __test_metadata *const _metadata) +{ + const struct rule layer1[] = { + { + .path = dir_s1d2, + .access = LANDLOCK_ACCESS_FS_REFER, + }, + { + /* Interesting for the layer2 tests. */ + .path = dir_s1d3, + .access = LANDLOCK_ACCESS_FS_MAKE_REG, + }, + { + .path = dir_s2d2, + .access = LANDLOCK_ACCESS_FS_REFER, + }, + { + .path = dir_s2d3, + .access = LANDLOCK_ACCESS_FS_MAKE_REG, + }, + {} + }; + const int ruleset_fd = create_ruleset(_metadata, + LANDLOCK_ACCESS_FS_MAKE_REG | LANDLOCK_ACCESS_FS_REFER, + layer1); + + ASSERT_LE(0, ruleset_fd); + enforce_ruleset(_metadata, ruleset_fd); + ASSERT_EQ(0, close(ruleset_fd)); +} + +static void reparent_exdev_layers_enforce2( + struct __test_metadata *const _metadata) +{ + const struct rule layer2[] = { + { + .path = dir_s2d3, + .access = LANDLOCK_ACCESS_FS_MAKE_DIR, + }, + {} + }; + /* + * Same checks as before but with a second layer and a new MAKE_DIR + * rule (and no explicit handling of REFER). + */ + const int ruleset_fd = create_ruleset(_metadata, + LANDLOCK_ACCESS_FS_MAKE_DIR, layer2); + + ASSERT_LE(0, ruleset_fd); + enforce_ruleset(_metadata, ruleset_fd); + ASSERT_EQ(0, close(ruleset_fd)); +} + +TEST_F_FORK(layout1, reparent_exdev_layers_rename1) +{ + ASSERT_EQ(0, unlink(file1_s2d2)); + ASSERT_EQ(0, unlink(file1_s2d3)); + + reparent_exdev_layers_enforce1(_metadata); + + /* + * Moving the dir_s1d3 directory below dir_s2d2 is allowed by Landlock + * because it doesn't inherit new access rights. + */ + ASSERT_EQ(0, rename(dir_s1d3, file1_s2d2)); + ASSERT_EQ(0, rename(file1_s2d2, dir_s1d3)); + + /* + * Moving the dir_s1d3 directory below dir_s2d3 is allowed, even if it + * gets a new inherited access rights (MAKE_REG), because MAKE_REG is + * already allowed for dir_s1d3. + */ + ASSERT_EQ(0, rename(dir_s1d3, file1_s2d3)); + ASSERT_EQ(0, rename(file1_s2d3, dir_s1d3)); + + /* + * However, moving the file1_s1d3 file below dir_s2d3 is allowed + * because it cannot inherit MAKE_REG right (which is dedicated to + * directories). + */ + ASSERT_EQ(0, rename(file1_s1d3, file1_s2d3)); + + reparent_exdev_layers_enforce2(_metadata); + + /* + * Moving the dir_s1d3 directory below dir_s2d2 is now denied because + * MAKE_DIR is not tied to dir_s2d2. + */ + ASSERT_EQ(-1, rename(dir_s1d3, file1_s2d2)); + ASSERT_EQ(EACCES, errno); + + /* + * Moving the dir_s1d3 directory below dir_s2d3 is forbidden because it + * would grants MAKE_REG and MAKE_DIR rights to it. + */ + ASSERT_EQ(-1, rename(dir_s1d3, file1_s2d3)); + ASSERT_EQ(EXDEV, errno); + + /* + * However, moving the file2_s1d3 file below dir_s2d3 is allowed + * because it cannot inherit MAKE_REG nor MAKE_DIR rights (which are + * dedicated to directories). + */ + ASSERT_EQ(0, rename(file2_s1d3, file1_s2d3)); +} + +TEST_F_FORK(layout1, reparent_exdev_layers_rename2) +{ + reparent_exdev_layers_enforce1(_metadata); + + /* Checks EACCES predominance over EXDEV. */ + ASSERT_EQ(-1, rename(file1_s1d1, file1_s2d2)); + ASSERT_EQ(EACCES, errno); + ASSERT_EQ(-1, rename(file1_s1d2, file1_s2d2)); + ASSERT_EQ(EACCES, errno); + ASSERT_EQ(-1, rename(file1_s1d1, file1_s2d3)); + ASSERT_EQ(EXDEV, errno); + /* Modify layout! */ + ASSERT_EQ(0, rename(file1_s1d2, file1_s2d3)); + + /* Without REFER source. */ + ASSERT_EQ(-1, rename(dir_s1d1, file1_s2d2)); + ASSERT_EQ(EXDEV, errno); + ASSERT_EQ(-1, rename(dir_s1d2, file1_s2d2)); + ASSERT_EQ(EXDEV, errno); + + reparent_exdev_layers_enforce2(_metadata); + + /* Checks EACCES predominance over EXDEV. */ + ASSERT_EQ(-1, rename(file1_s1d1, file1_s2d2)); + ASSERT_EQ(EACCES, errno); + /* Checks with actual file2_s1d2. */ + ASSERT_EQ(-1, rename(file2_s1d2, file1_s2d2)); + ASSERT_EQ(EACCES, errno); + ASSERT_EQ(-1, rename(file1_s1d1, file1_s2d3)); + ASSERT_EQ(EXDEV, errno); + /* Modify layout! */ + ASSERT_EQ(0, rename(file2_s1d2, file1_s2d3)); + + /* Without REFER source, EACCES wins over EXDEV. */ + ASSERT_EQ(-1, rename(dir_s1d1, file1_s2d2)); + ASSERT_EQ(EACCES, errno); + ASSERT_EQ(-1, rename(dir_s1d2, file1_s2d2)); + ASSERT_EQ(EACCES, errno); +} + +TEST_F_FORK(layout1, reparent_exdev_layers_exchange1) +{ + const char *const dir_file1_s1d2 = file1_s1d2, + *const dir_file2_s2d3 = file2_s2d3; + + ASSERT_EQ(0, unlink(file1_s1d2)); + ASSERT_EQ(0, mkdir(file1_s1d2, 0700)); + ASSERT_EQ(0, unlink(file2_s2d3)); + ASSERT_EQ(0, mkdir(file2_s2d3, 0700)); + + reparent_exdev_layers_enforce1(_metadata); + + /* Error predominance with file exchange: returns EXDEV and EACCES. */ + ASSERT_EQ(-1, renameat2(AT_FDCWD, file1_s1d1, AT_FDCWD, file1_s2d3, + RENAME_EXCHANGE)); + ASSERT_EQ(EACCES, errno); + ASSERT_EQ(-1, renameat2(AT_FDCWD, file1_s2d3, AT_FDCWD, file1_s1d1, + RENAME_EXCHANGE)); + ASSERT_EQ(EACCES, errno); + + /* + * Checks with directories which creation could be allowed, but denied + * because of access rights that would be inherited. + */ + ASSERT_EQ(-1, renameat2(AT_FDCWD, dir_file1_s1d2, AT_FDCWD, + dir_file2_s2d3, RENAME_EXCHANGE)); + ASSERT_EQ(EXDEV, errno); + ASSERT_EQ(-1, renameat2(AT_FDCWD, dir_file2_s2d3, AT_FDCWD, + dir_file1_s1d2, RENAME_EXCHANGE)); + ASSERT_EQ(EXDEV, errno); + + /* Checks with same access rights. */ + ASSERT_EQ(0, renameat2(AT_FDCWD, dir_s1d3, AT_FDCWD, dir_s2d3, + RENAME_EXCHANGE)); + ASSERT_EQ(0, renameat2(AT_FDCWD, dir_s2d3, AT_FDCWD, dir_s1d3, + RENAME_EXCHANGE)); + + /* Checks with different (child-only) access rights. */ + ASSERT_EQ(0, renameat2(AT_FDCWD, dir_s2d3, AT_FDCWD, dir_file1_s1d2, + RENAME_EXCHANGE)); + ASSERT_EQ(0, renameat2(AT_FDCWD, dir_file1_s1d2, AT_FDCWD, dir_s2d3, + RENAME_EXCHANGE)); + + /* + * Checks that exchange between file and directory are consistent. + * + * Moving a file (file1_s2d2) to a directory which only grants more + * directory-related access rights is allowed, and at the same time + * moving a directory (dir_file2_s2d3) to another directory which + * grants less access rights is allowed too. + * + * See layout1.reparent_exdev_layers_exchange3 for inverted arguments. + */ + ASSERT_EQ(0, renameat2(AT_FDCWD, file1_s2d2, AT_FDCWD, dir_file2_s2d3, + RENAME_EXCHANGE)); + /* + * However, moving back the directory is denied because it would get + * more access rights than the current state and because file creation + * is forbidden (in dir_s2d2). + */ + ASSERT_EQ(-1, renameat2(AT_FDCWD, dir_file2_s2d3, AT_FDCWD, file1_s2d2, + RENAME_EXCHANGE)); + ASSERT_EQ(EACCES, errno); + ASSERT_EQ(-1, renameat2(AT_FDCWD, file1_s2d2, AT_FDCWD, dir_file2_s2d3, + RENAME_EXCHANGE)); + ASSERT_EQ(EACCES, errno); + + reparent_exdev_layers_enforce2(_metadata); + + /* Error predominance with file exchange: returns EXDEV and EACCES. */ + ASSERT_EQ(-1, renameat2(AT_FDCWD, file1_s1d1, AT_FDCWD, file1_s2d3, + RENAME_EXCHANGE)); + ASSERT_EQ(EACCES, errno); + ASSERT_EQ(-1, renameat2(AT_FDCWD, file1_s2d3, AT_FDCWD, file1_s1d1, + RENAME_EXCHANGE)); + ASSERT_EQ(EACCES, errno); + + /* Checks with directories which creation is now denied. */ + ASSERT_EQ(-1, renameat2(AT_FDCWD, dir_file1_s1d2, AT_FDCWD, + dir_file2_s2d3, RENAME_EXCHANGE)); + ASSERT_EQ(EACCES, errno); + ASSERT_EQ(-1, renameat2(AT_FDCWD, dir_file2_s2d3, AT_FDCWD, + dir_file1_s1d2, RENAME_EXCHANGE)); + ASSERT_EQ(EACCES, errno); + + /* Checks with different (child-only) access rights. */ + ASSERT_EQ(-1, renameat2(AT_FDCWD, dir_s1d3, AT_FDCWD, dir_s2d3, + RENAME_EXCHANGE)); + /* Denied because of MAKE_DIR. */ + ASSERT_EQ(EACCES, errno); + ASSERT_EQ(-1, renameat2(AT_FDCWD, dir_s2d3, AT_FDCWD, dir_s1d3, + RENAME_EXCHANGE)); + ASSERT_EQ(EACCES, errno); + + /* Checks with different (child-only) access rights. */ + ASSERT_EQ(-1, renameat2(AT_FDCWD, dir_s2d3, AT_FDCWD, dir_file1_s1d2, + RENAME_EXCHANGE)); + /* Denied because of MAKE_DIR. */ + ASSERT_EQ(EACCES, errno); + ASSERT_EQ(-1, renameat2(AT_FDCWD, dir_file1_s1d2, AT_FDCWD, dir_s2d3, + RENAME_EXCHANGE)); + ASSERT_EQ(EACCES, errno); + + /* See layout1.reparent_exdev_layers_exchange2 for complement. */ +} + +TEST_F_FORK(layout1, reparent_exdev_layers_exchange2) +{ + const char *const dir_file2_s2d3 = file2_s2d3; + + ASSERT_EQ(0, unlink(file2_s2d3)); + ASSERT_EQ(0, mkdir(file2_s2d3, 0700)); + + reparent_exdev_layers_enforce1(_metadata); + reparent_exdev_layers_enforce2(_metadata); + + /* Checks that exchange between file and directory are consistent. */ + ASSERT_EQ(-1, renameat2(AT_FDCWD, file1_s2d2, AT_FDCWD, dir_file2_s2d3, + RENAME_EXCHANGE)); + ASSERT_EQ(EACCES, errno); + ASSERT_EQ(-1, renameat2(AT_FDCWD, dir_file2_s2d3, AT_FDCWD, file1_s2d2, + RENAME_EXCHANGE)); + ASSERT_EQ(EACCES, errno); +} + +TEST_F_FORK(layout1, reparent_exdev_layers_exchange3) +{ + const char *const dir_file2_s2d3 = file2_s2d3; + + ASSERT_EQ(0, unlink(file2_s2d3)); + ASSERT_EQ(0, mkdir(file2_s2d3, 0700)); + + reparent_exdev_layers_enforce1(_metadata); + + /* + * Checks that exchange between file and directory are consistent, + * including with inverted arguments (see + * layout1.reparent_exdev_layers_exchange1). + */ + ASSERT_EQ(0, renameat2(AT_FDCWD, dir_file2_s2d3, AT_FDCWD, file1_s2d2, + RENAME_EXCHANGE)); + ASSERT_EQ(-1, renameat2(AT_FDCWD, file1_s2d2, AT_FDCWD, dir_file2_s2d3, + RENAME_EXCHANGE)); + ASSERT_EQ(EACCES, errno); + ASSERT_EQ(-1, renameat2(AT_FDCWD, dir_file2_s2d3, AT_FDCWD, file1_s2d2, + RENAME_EXCHANGE)); + ASSERT_EQ(EACCES, errno); +} + +TEST_F_FORK(layout1, reparent_remove) +{ + const struct rule layer1[] = { + { + .path = dir_s1d1, + .access = LANDLOCK_ACCESS_FS_REFER | + LANDLOCK_ACCESS_FS_REMOVE_DIR, + }, + { + .path = dir_s1d2, + .access = LANDLOCK_ACCESS_FS_REMOVE_FILE, + }, + { + .path = dir_s2d1, + .access = LANDLOCK_ACCESS_FS_REFER | + LANDLOCK_ACCESS_FS_REMOVE_FILE, + }, + {} + }; + const int ruleset_fd = create_ruleset(_metadata, + LANDLOCK_ACCESS_FS_REFER | + LANDLOCK_ACCESS_FS_REMOVE_DIR | + LANDLOCK_ACCESS_FS_REMOVE_FILE, layer1); + + ASSERT_LE(0, ruleset_fd); + enforce_ruleset(_metadata, ruleset_fd); + ASSERT_EQ(0, close(ruleset_fd)); + + /* Access denied because of wrong/swapped remove file/dir. */ + ASSERT_EQ(-1, rename(file1_s1d1, dir_s2d2)); + ASSERT_EQ(EACCES, errno); + ASSERT_EQ(-1, rename(dir_s2d2, file1_s1d1)); + ASSERT_EQ(EACCES, errno); + ASSERT_EQ(-1, renameat2(AT_FDCWD, file1_s1d1, AT_FDCWD, dir_s2d2, + RENAME_EXCHANGE)); + ASSERT_EQ(EACCES, errno); + ASSERT_EQ(-1, renameat2(AT_FDCWD, file1_s1d1, AT_FDCWD, dir_s2d3, + RENAME_EXCHANGE)); + ASSERT_EQ(EACCES, errno); + + /* Access allowed thanks to the matching rights. */ + ASSERT_EQ(-1, rename(file1_s2d1, dir_s1d2)); + ASSERT_EQ(EISDIR, errno); + ASSERT_EQ(-1, rename(dir_s1d2, file1_s2d1)); + ASSERT_EQ(ENOTDIR, errno); + ASSERT_EQ(-1, rename(dir_s1d3, file1_s2d1)); + ASSERT_EQ(ENOTDIR, errno); + ASSERT_EQ(0, unlink(file1_s2d1)); + ASSERT_EQ(0, unlink(file1_s1d3)); + ASSERT_EQ(0, unlink(file2_s1d3)); + ASSERT_EQ(0, rename(dir_s1d3, file1_s2d1)); + + /* Effectively removes a file and a directory by exchanging them. */ + ASSERT_EQ(0, mkdir(dir_s1d3, 0700)); + ASSERT_EQ(0, renameat2(AT_FDCWD, file1_s2d2, AT_FDCWD, dir_s1d3, + RENAME_EXCHANGE)); + ASSERT_EQ(-1, renameat2(AT_FDCWD, file1_s2d2, AT_FDCWD, dir_s1d3, + RENAME_EXCHANGE)); + ASSERT_EQ(EACCES, errno); +} + +TEST_F_FORK(layout1, reparent_dom_superset) +{ + const struct rule layer1[] = { + { + .path = dir_s1d2, + .access = LANDLOCK_ACCESS_FS_REFER, + }, + { + .path = file1_s1d2, + .access = LANDLOCK_ACCESS_FS_EXECUTE, + }, + { + .path = dir_s1d3, + .access = LANDLOCK_ACCESS_FS_MAKE_SOCK | + LANDLOCK_ACCESS_FS_EXECUTE, + }, + { + .path = dir_s2d2, + .access = LANDLOCK_ACCESS_FS_REFER | + LANDLOCK_ACCESS_FS_EXECUTE | + LANDLOCK_ACCESS_FS_MAKE_SOCK, + }, + { + .path = dir_s2d3, + .access = LANDLOCK_ACCESS_FS_READ_FILE | + LANDLOCK_ACCESS_FS_MAKE_FIFO, + }, + {} + }; + int ruleset_fd = create_ruleset(_metadata, + LANDLOCK_ACCESS_FS_REFER | + LANDLOCK_ACCESS_FS_EXECUTE | + LANDLOCK_ACCESS_FS_MAKE_SOCK | + LANDLOCK_ACCESS_FS_READ_FILE | + LANDLOCK_ACCESS_FS_MAKE_FIFO, + layer1); + + ASSERT_LE(0, ruleset_fd); + enforce_ruleset(_metadata, ruleset_fd); + ASSERT_EQ(0, close(ruleset_fd)); + + ASSERT_EQ(-1, rename(file1_s1d2, file1_s2d1)); + ASSERT_EQ(EXDEV, errno); + /* + * Moving file1_s1d2 beneath dir_s2d3 would grant it the READ_FILE + * access right. + */ + ASSERT_EQ(-1, rename(file1_s1d2, file1_s2d3)); + ASSERT_EQ(EXDEV, errno); + /* + * Moving file1_s1d2 should be allowed even if dir_s2d2 grants a + * superset of access rights compared to dir_s1d2, because file1_s1d2 + * already has these access rights anyway. + */ + ASSERT_EQ(0, rename(file1_s1d2, file1_s2d2)); + ASSERT_EQ(0, rename(file1_s2d2, file1_s1d2)); + + ASSERT_EQ(-1, rename(dir_s1d3, file1_s2d1)); + ASSERT_EQ(EXDEV, errno); + /* + * Moving dir_s1d3 beneath dir_s2d3 would grant it the MAKE_FIFO access + * right. + */ + ASSERT_EQ(-1, rename(dir_s1d3, file1_s2d3)); + ASSERT_EQ(EXDEV, errno); + /* + * Moving dir_s1d3 should be allowed even if dir_s2d2 grants a superset + * of access rights compared to dir_s1d2, because dir_s1d3 already has + * these access rights anyway. + */ + ASSERT_EQ(0, rename(dir_s1d3, file1_s2d2)); + ASSERT_EQ(0, rename(file1_s2d2, dir_s1d3)); + + /* + * Moving file1_s2d3 beneath dir_s1d2 is allowed, but moving it back + * will be denied because the new inherited access rights from dir_s1d2 + * will be less than the destination (original) dir_s2d3. This is a + * sinkhole scenario where we cannot move back files or directories. + */ + ASSERT_EQ(0, rename(file1_s2d3, file2_s1d2)); + ASSERT_EQ(-1, rename(file2_s1d2, file1_s2d3)); + ASSERT_EQ(EXDEV, errno); + ASSERT_EQ(0, unlink(file2_s1d2)); + ASSERT_EQ(0, unlink(file2_s2d3)); + /* + * Checks similar directory one-way move: dir_s2d3 loses EXECUTE and + * MAKE_SOCK which were inherited from dir_s1d3. + */ + ASSERT_EQ(0, rename(dir_s2d3, file2_s1d2)); + ASSERT_EQ(-1, rename(file2_s1d2, dir_s2d3)); + ASSERT_EQ(EXDEV, errno); +} + TEST_F_FORK(layout1, remove_dir) { const struct rule rules[] = { @@ -2390,6 +3105,43 @@ TEST_F_FORK(layout1_bind, same_content_same_file) ASSERT_EQ(EACCES, test_open(bind_file1_s1d3, O_WRONLY)); } +TEST_F_FORK(layout1_bind, reparent_cross_mount) +{ + const struct rule layer1[] = { + { + /* dir_s2d1 is beneath the dir_s2d2 mount point. */ + .path = dir_s2d1, + .access = LANDLOCK_ACCESS_FS_REFER, + }, + { + .path = bind_dir_s1d3, + .access = LANDLOCK_ACCESS_FS_EXECUTE, + }, + {} + }; + int ruleset_fd = create_ruleset(_metadata, LANDLOCK_ACCESS_FS_REFER | + LANDLOCK_ACCESS_FS_EXECUTE, layer1); + + ASSERT_LE(0, ruleset_fd); + enforce_ruleset(_metadata, ruleset_fd); + ASSERT_EQ(0, close(ruleset_fd)); + + /* Checks basic denied move. */ + ASSERT_EQ(-1, rename(file1_s1d1, file1_s1d2)); + ASSERT_EQ(EXDEV, errno); + + /* Checks real cross-mount move (Landlock is not involved). */ + ASSERT_EQ(-1, rename(file1_s2d1, file1_s2d2)); + ASSERT_EQ(EXDEV, errno); + + /* Checks move that will give more accesses. */ + ASSERT_EQ(-1, rename(file1_s2d2, bind_file1_s1d3)); + ASSERT_EQ(EXDEV, errno); + + /* Checks legitimate downgrade move. */ + ASSERT_EQ(0, rename(bind_file1_s1d3, file1_s2d2)); +} + #define LOWER_BASE TMP_DIR "/lower" #define LOWER_DATA LOWER_BASE "/data" static const char lower_fl1[] = LOWER_DATA "/fl1"; From patchwork Tue Mar 29 12:51:14 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= X-Patchwork-Id: 12794812 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id B9F15C43219 for ; Tue, 29 Mar 2022 12:51:42 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236915AbiC2MxW (ORCPT ); Tue, 29 Mar 2022 08:53:22 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:36144 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237198AbiC2MxB (ORCPT ); Tue, 29 Mar 2022 08:53:01 -0400 Received: from smtp-42ac.mail.infomaniak.ch (smtp-42ac.mail.infomaniak.ch [84.16.66.172]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 02826297 for ; Tue, 29 Mar 2022 05:51:07 -0700 (PDT) Received: from smtp-2-0000.mail.infomaniak.ch (unknown [10.5.36.107]) by smtp-3-3000.mail.infomaniak.ch (Postfix) with ESMTPS id 4KSTvZ2H5jzMq17v; Tue, 29 Mar 2022 14:51:06 +0200 (CEST) Received: from localhost (unknown [23.97.221.149]) by smtp-2-0000.mail.infomaniak.ch (Postfix) with ESMTPA id 4KSTvX5pqLzlhSMW; Tue, 29 Mar 2022 14:51:04 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=digikod.net; s=20191114; t=1648558266; bh=RKNGYAFAo3yZ4fLKZPqQ7CCdLKDqIyDN4+wRHaJ7huU=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=M5+zWNcyu5ps2IXlHGYpizZgwfwnTYNgX+8C7zqPUGxnn5D+JQ8h/NnzVI0FcReCb s1v79ni/TQzaCCfPsd93YiEGuf/DTrbPcLhq4QHvqZJ2gwzewHGT7jTryskZswRr1b +bENc+XOrrsYxUuL73XhyJndoVZLCab32oXt312U= From: =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= To: James Morris , "Serge E . Hallyn" Cc: =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= , Al Viro , Jann Horn , John Johansen , Kees Cook , Konstantin Meskhidze , Paul Moore , Shuah Khan , Tetsuo Handa , linux-doc@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, linux-security-module@vger.kernel.org, =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= Subject: [PATCH v2 09/12] samples/landlock: Add support for file reparenting Date: Tue, 29 Mar 2022 14:51:14 +0200 Message-Id: <20220329125117.1393824-10-mic@digikod.net> In-Reply-To: <20220329125117.1393824-1-mic@digikod.net> References: <20220329125117.1393824-1-mic@digikod.net> MIME-Version: 1.0 Precedence: bulk List-ID: From: Mickaël Salaün Add LANDLOCK_ACCESS_FS_REFER to the "roughly write" access rights and leverage the Landlock ABI version to only try to enforce it if it is supported by the running kernel. Reviewed-by: Paul Moore Signed-off-by: Mickaël Salaün Link: https://lore.kernel.org/r/20220329125117.1393824-10-mic@digikod.net --- Changes since v1: * Add Reviewed-by: Paul Moore. --- samples/landlock/sandboxer.c | 37 +++++++++++++++++++++++++----------- 1 file changed, 26 insertions(+), 11 deletions(-) diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c index 7a15910d2171..8509543fcbbb 100644 --- a/samples/landlock/sandboxer.c +++ b/samples/landlock/sandboxer.c @@ -153,16 +153,21 @@ static int populate_ruleset( LANDLOCK_ACCESS_FS_MAKE_SOCK | \ LANDLOCK_ACCESS_FS_MAKE_FIFO | \ LANDLOCK_ACCESS_FS_MAKE_BLOCK | \ - LANDLOCK_ACCESS_FS_MAKE_SYM) + LANDLOCK_ACCESS_FS_MAKE_SYM | \ + LANDLOCK_ACCESS_FS_REFER) + +#define ACCESS_ABI_2 ( \ + LANDLOCK_ACCESS_FS_REFER) int main(const int argc, char *const argv[], char *const *const envp) { const char *cmd_path; char *const *cmd_argv; - int ruleset_fd; + int ruleset_fd, abi; + __u64 access_fs_ro = ACCESS_FS_ROUGHLY_READ, + access_fs_rw = ACCESS_FS_ROUGHLY_READ | ACCESS_FS_ROUGHLY_WRITE; struct landlock_ruleset_attr ruleset_attr = { - .handled_access_fs = ACCESS_FS_ROUGHLY_READ | - ACCESS_FS_ROUGHLY_WRITE, + .handled_access_fs = access_fs_rw, }; if (argc < 2) { @@ -183,11 +188,11 @@ int main(const int argc, char *const argv[], char *const *const envp) return 1; } - ruleset_fd = landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0); - if (ruleset_fd < 0) { + abi = landlock_create_ruleset(NULL, 0, LANDLOCK_CREATE_RULESET_VERSION); + if (abi < 0) { const int err = errno; - perror("Failed to create a ruleset"); + perror("Failed to check Landlock compatibility"); switch (err) { case ENOSYS: fprintf(stderr, "Hint: Landlock is not supported by the current kernel. " @@ -205,12 +210,22 @@ int main(const int argc, char *const argv[], char *const *const envp) } return 1; } - if (populate_ruleset(ENV_FS_RO_NAME, ruleset_fd, - ACCESS_FS_ROUGHLY_READ)) { + /* Best-effort security. */ + if (abi < 2) { + ruleset_attr.handled_access_fs &= ~ACCESS_ABI_2; + access_fs_ro &= ~ACCESS_ABI_2; + access_fs_rw &= ~ACCESS_ABI_2; + } + + ruleset_fd = landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0); + if (ruleset_fd < 0) { + perror("Failed to create a ruleset"); + return 1; + } + if (populate_ruleset(ENV_FS_RO_NAME, ruleset_fd, access_fs_ro)) { goto err_close_ruleset; } - if (populate_ruleset(ENV_FS_RW_NAME, ruleset_fd, - ACCESS_FS_ROUGHLY_READ | ACCESS_FS_ROUGHLY_WRITE)) { + if (populate_ruleset(ENV_FS_RW_NAME, ruleset_fd, access_fs_rw)) { goto err_close_ruleset; } if (prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)) { From patchwork Tue Mar 29 12:51:15 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= X-Patchwork-Id: 12794809 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id DA059C433FE for ; Tue, 29 Mar 2022 12:51:38 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236873AbiC2MxS (ORCPT ); Tue, 29 Mar 2022 08:53:18 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:36286 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237215AbiC2MxD (ORCPT ); Tue, 29 Mar 2022 08:53:03 -0400 Received: from smtp-190e.mail.infomaniak.ch (smtp-190e.mail.infomaniak.ch [IPv6:2001:1600:4:17::190e]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 3C9931A38C for ; Tue, 29 Mar 2022 05:51:08 -0700 (PDT) Received: from smtp-2-0000.mail.infomaniak.ch (unknown [10.5.36.107]) by smtp-3-3000.mail.infomaniak.ch (Postfix) with ESMTPS id 4KSTvZ6wwCzMq17r; Tue, 29 Mar 2022 14:51:06 +0200 (CEST) Received: from localhost (unknown [23.97.221.149]) by smtp-2-0000.mail.infomaniak.ch (Postfix) with ESMTPA id 4KSTvZ4yVbzlhSMV; Tue, 29 Mar 2022 14:51:06 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=digikod.net; s=20191114; t=1648558266; bh=suOPlW+pglZP6qARGB/EHlCvTPnI9/Xg1+9RB+mCXZs=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=XqRZP9SU+4WE3wNnTQpEoWXcaSPBKTW4Aaq+5+TAwALwTe5JHgdtR3kRj44ror0cd DoSiQkDvtLUMNzqOLwVABqbpd3g6XE2LdxP7cWgkT2oDpg+0sLbIS/TR+olUR5iSaX o2/D/PbQlX2CObgcQnVXFghXFNKOf54VxE5usnSQ= From: =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= To: James Morris , "Serge E . Hallyn" Cc: =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= , Al Viro , Jann Horn , John Johansen , Kees Cook , Konstantin Meskhidze , Paul Moore , Shuah Khan , Tetsuo Handa , linux-doc@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, linux-security-module@vger.kernel.org, =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= Subject: [PATCH v2 10/12] landlock: Document LANDLOCK_ACCESS_FS_REFER and ABI versioning Date: Tue, 29 Mar 2022 14:51:15 +0200 Message-Id: <20220329125117.1393824-11-mic@digikod.net> In-Reply-To: <20220329125117.1393824-1-mic@digikod.net> References: <20220329125117.1393824-1-mic@digikod.net> MIME-Version: 1.0 Precedence: bulk List-ID: From: Mickaël Salaün Add LANDLOCK_ACCESS_FS_REFER in the example and properly check to only use it if the current kernel support it thanks to the Landlock ABI version. Move the file renaming and linking limitation to a new "Previous limitations" section. Improve documentation about the backward and forward compatibility, including the rational for ruleset's handled_access_fs. Reviewed-by: Paul Moore Signed-off-by: Mickaël Salaün Link: https://lore.kernel.org/r/20220329125117.1393824-11-mic@digikod.net --- Changes since v1: * Add Reviewed-by: Paul Moore. * Update date. --- Documentation/userspace-api/landlock.rst | 124 +++++++++++++++++++---- 1 file changed, 104 insertions(+), 20 deletions(-) diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst index b68e7a51009f..b066d281f9f2 100644 --- a/Documentation/userspace-api/landlock.rst +++ b/Documentation/userspace-api/landlock.rst @@ -8,7 +8,7 @@ Landlock: unprivileged access control ===================================== :Author: Mickaël Salaün -:Date: March 2021 +:Date: March 2022 The goal of Landlock is to enable to restrict ambient rights (e.g. global filesystem access) for a set of processes. Because Landlock is a stackable @@ -29,14 +29,15 @@ the thread enforcing it, and its future children. Defining and enforcing a security policy ---------------------------------------- -We first need to create the ruleset that will contain our rules. For this +We first need to define the ruleset that will contain our rules. For this example, the ruleset will contain rules that only allow read actions, but write actions will be denied. The ruleset then needs to handle both of these kind of -actions. +actions. This is required for backward and forward compatibility (i.e. the +kernel and user space may not know each other's supported restrictions), hence +the need to be explicit about the denied-by-default access rights. .. code-block:: c - int ruleset_fd; struct landlock_ruleset_attr ruleset_attr = { .handled_access_fs = LANDLOCK_ACCESS_FS_EXECUTE | @@ -51,9 +52,34 @@ actions. LANDLOCK_ACCESS_FS_MAKE_SOCK | LANDLOCK_ACCESS_FS_MAKE_FIFO | LANDLOCK_ACCESS_FS_MAKE_BLOCK | - LANDLOCK_ACCESS_FS_MAKE_SYM, + LANDLOCK_ACCESS_FS_MAKE_SYM | + LANDLOCK_ACCESS_FS_REFER, }; +Because we may not know on which kernel version an application will be +executed, it is safer to follow a best-effort security approach. Indeed, we +should try to protect users as much as possible whatever the kernel they are +using. To avoid binary enforcement (i.e. either all security features or +none), we can leverage a dedicated Landlock command to get the current version +of the Landlock ABI and adapt the handled accesses. Let's check if we should +remove the `LANDLOCK_ACCESS_FS_REFER` access right which is only supported +starting with the second version of the ABI. + +.. code-block:: c + + int abi; + + abi = landlock_create_ruleset(NULL, 0, LANDLOCK_CREATE_RULESET_VERSION); + if (abi < 2) { + ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_REFER; + } + +This enables to create an inclusive ruleset that will contain our rules. + +.. code-block:: c + + int ruleset_fd; + ruleset_fd = landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0); if (ruleset_fd < 0) { perror("Failed to create a ruleset"); @@ -92,6 +118,11 @@ descriptor. return 1; } +It may also be required to create rules following the same logic as explained +for the ruleset creation, by filtering access rights according to the Landlock +ABI version. In this example, this is not required because +`LANDLOCK_ACCESS_FS_REFER` is not allowed by any rule. + We now have a ruleset with one rule allowing read access to ``/usr`` while denying all other handled accesses for the filesystem. The next step is to restrict the current thread from gaining more privileges (e.g. thanks to a SUID @@ -192,6 +223,56 @@ To be allowed to use :manpage:`ptrace(2)` and related syscalls on a target process, a sandboxed process should have a subset of the target process rules, which means the tracee must be in a sub-domain of the tracer. +Compatibility +============= + +Backward and forward compatibility +---------------------------------- + +Landlock is designed to be compatible with past and future versions of the +kernel. This is achieved thanks to the system call attributes and the +associated bitflags, particularly the ruleset's `handled_access_fs`. Making +handled access right explicit enables the kernel and user space to have a clear +contract with each other. This is required to make sure sandboxing will not +get stricter with a system update, which could break applications. + +Developers can subscribe to the `Landlock mailing list +`_ to knowingly update and +test their applications with the latest available features. In the interest of +users, and because they may use different kernel versions, it is strongly +encouraged to follow a best-effort security approach by checking the Landlock +ABI version at runtime and only enforcing the supported features. + +Landlock ABI versions +--------------------- + +The Landlock ABI version can be read with the sys_landlock_create_ruleset() +system call: + +.. code-block:: c + + int abi; + + abi = landlock_create_ruleset(NULL, 0, LANDLOCK_CREATE_RULESET_VERSION); + if (abi < 0) { + switch (errno) { + case ENOSYS: + printf("Landlock is not supported by the current kernel.\n"); + break; + case EOPNOTSUPP: + printf("Landlock is currently disabled.\n"); + break; + } + return 0; + } + if (abi >= 2) { + printf("Landlock supports LANDLOCK_ACCESS_FS_REFER.\n"); + } + +The following kernel interfaces are implicitly supported by the first ABI +version. Features only supported from a specific version are explicitly marked +as such. + Kernel interface ================ @@ -228,21 +309,6 @@ Enforcing a ruleset Current limitations =================== -File renaming and linking -------------------------- - -Because Landlock targets unprivileged access controls, it is needed to properly -handle composition of rules. Such property also implies rules nesting. -Properly handling multiple layers of ruleset, each one of them able to restrict -access to files, also implies to inherit the ruleset restrictions from a parent -to its hierarchy. Because files are identified and restricted by their -hierarchy, moving or linking a file from one directory to another implies to -propagate the hierarchy constraints. To protect against privilege escalations -through renaming or linking, and for the sake of simplicity, Landlock currently -limits linking and renaming to the same directory. Future Landlock evolutions -will enable more flexibility for renaming and linking, with dedicated ruleset -flags. - Filesystem topology modification -------------------------------- @@ -281,6 +347,24 @@ Memory usage Kernel memory allocated to create rulesets is accounted and can be restricted by the Documentation/admin-guide/cgroup-v1/memory.rst. +Previous limitations +==================== + +File renaming and linking (ABI 1) +--------------------------------- + +Because Landlock targets unprivileged access controls, it is needed to properly +handle composition of rules. Such property also implies rules nesting. +Properly handling multiple layers of ruleset, each one of them able to restrict +access to files, also implies to inherit the ruleset restrictions from a parent +to its hierarchy. Because files are identified and restricted by their +hierarchy, moving or linking a file from one directory to another implies to +propagate the hierarchy constraints. To protect against privilege escalations +through renaming or linking, and for the sake of simplicity, Landlock previously +limited linking and renaming to the same directory. Starting with the Landlock +ABI version 2, it is now possible to securely control renaming and linking +thanks to the new `LANDLOCK_ACCESS_FS_REFER` access right. + Questions and answers ===================== From patchwork Tue Mar 29 12:51:16 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= X-Patchwork-Id: 12794810 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id A1FFBC433F5 for ; Tue, 29 Mar 2022 12:51:41 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236889AbiC2MxT (ORCPT ); Tue, 29 Mar 2022 08:53:19 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:36224 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237205AbiC2MxC (ORCPT ); Tue, 29 Mar 2022 08:53:02 -0400 Received: from smtp-8fa8.mail.infomaniak.ch (smtp-8fa8.mail.infomaniak.ch [83.166.143.168]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 4775A1CB06 for ; Tue, 29 Mar 2022 05:51:08 -0700 (PDT) Received: from smtp-2-0000.mail.infomaniak.ch (unknown [10.5.36.107]) by smtp-3-3000.mail.infomaniak.ch (Postfix) with ESMTPS id 4KSTvb4K4XzMpylX; Tue, 29 Mar 2022 14:51:07 +0200 (CEST) Received: from localhost (unknown [23.97.221.149]) by smtp-2-0000.mail.infomaniak.ch (Postfix) with ESMTPA id 4KSTvb2X1mzlhSMV; Tue, 29 Mar 2022 14:51:07 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=digikod.net; s=20191114; t=1648558267; bh=Myv+feEqRMvVi5vshp42PeNetgkHZQ6w/RvawsOsgJ4=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=BPARrTXoCBwk6sr/GS0DLrE1KC/kkizLhv/i3QRLmTTo0b1uBLDU/Ra1OqXy17jDV takscT6ICLibCT6uhpFy0hOxVdA2AQjn1XyaLWBBX+vJg/VaL/XHATt5DJNcHqRCsr wMHwzK0Vo3yQI28HNmmTdU5ffh3GZaCRqQamrww4= From: =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= To: James Morris , "Serge E . Hallyn" Cc: =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= , Al Viro , Jann Horn , John Johansen , Kees Cook , Konstantin Meskhidze , Paul Moore , Shuah Khan , Tetsuo Handa , linux-doc@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, linux-security-module@vger.kernel.org, =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= Subject: [PATCH v2 11/12] landlock: Document good practices about filesystem policies Date: Tue, 29 Mar 2022 14:51:16 +0200 Message-Id: <20220329125117.1393824-12-mic@digikod.net> In-Reply-To: <20220329125117.1393824-1-mic@digikod.net> References: <20220329125117.1393824-1-mic@digikod.net> MIME-Version: 1.0 Precedence: bulk List-ID: From: Mickaël Salaün Reviewed-by: Paul Moore Signed-off-by: Mickaël Salaün Link: https://lore.kernel.org/r/20220329125117.1393824-12-mic@digikod.net --- Changes since v1: * Add Reviewed-by: Paul Moore. --- Documentation/userspace-api/landlock.rst | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/Documentation/userspace-api/landlock.rst b/Documentation/userspace-api/landlock.rst index b066d281f9f2..4b22c8cd76b4 100644 --- a/Documentation/userspace-api/landlock.rst +++ b/Documentation/userspace-api/landlock.rst @@ -156,6 +156,27 @@ ruleset. Full working code can be found in `samples/landlock/sandboxer.c`_. +Good practices +-------------- + +It is recommended setting access rights to file hierarchy leaves as much as +possible. For instance, it is better to be able to have ``~/doc/`` as a +read-only hierarchy and ``~/tmp/`` as a read-write hierarchy, compared to +``~/`` as a read-only hierarchy and ``~/tmp/`` as a read-write hierarchy. +Following this good practice leads to self-sufficient hierarchies that don't +depend on their location (i.e. parent directories). This is particularly +relevant when we want to allow linking or renaming. Indeed, having consistent +access rights per directory enables to change the location of such directory +without relying on the destination directory access rights (except those that +are required for this operation, see `LANDLOCK_ACCESS_FS_REFER` documentation). +Having self-sufficient hierarchies also helps to tighten the required access +rights to the minimal set of data. This also helps avoid sinkhole directories, +i.e. directories where data can be linked to but not linked from. However, +this depends on data organization, which might not be controlled by developers. +In this case, granting read-write access to ``~/tmp/``, instead of write-only +access, would potentially allow to move ``~/tmp/`` to a non-readable directory +and still keep the ability to list the content of ``~/tmp/``. + Layers of file path access rights --------------------------------- From patchwork Tue Mar 29 12:51:17 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= X-Patchwork-Id: 12794811 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id D87A2C433EF for ; Tue, 29 Mar 2022 12:51:41 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236910AbiC2MxU (ORCPT ); Tue, 29 Mar 2022 08:53:20 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35844 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237203AbiC2MxC (ORCPT ); Tue, 29 Mar 2022 08:53:02 -0400 Received: from smtp-bc0a.mail.infomaniak.ch (smtp-bc0a.mail.infomaniak.ch [IPv6:2001:1600:4:17::bc0a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 479531CFE4 for ; Tue, 29 Mar 2022 05:51:09 -0700 (PDT) Received: from smtp-3-0001.mail.infomaniak.ch (unknown [10.4.36.108]) by smtp-3-3000.mail.infomaniak.ch (Postfix) with ESMTPS id 4KSTvc1kLVzMq173; Tue, 29 Mar 2022 14:51:08 +0200 (CEST) Received: from localhost (unknown [23.97.221.149]) by smtp-3-0001.mail.infomaniak.ch (Postfix) with ESMTPA id 4KSTvb754VzlhMbh; Tue, 29 Mar 2022 14:51:07 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=digikod.net; s=20191114; t=1648558268; bh=smcHkSzsHA9UZrPxO4w4wuj0alDdabBqgJAayQUQddQ=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=0Spx8mfvteAOpN5mHxVWCFqncvkCGz4WPO4np6Z2DNZsi685wIXBij4Q3qpCU9Ld2 FYuOBVwBQZf04SFcEHQwAnZS+4Nwf8pOPhvrFsZI9Vl11zKUyHZvi0UwUZpKkk05T9 e4rIBNgAen+ssbWfvNqAQUta5mFhqHmg5KMuu870= From: =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= To: James Morris , "Serge E . Hallyn" Cc: =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= , Al Viro , Jann Horn , John Johansen , Kees Cook , Konstantin Meskhidze , Paul Moore , Shuah Khan , Tetsuo Handa , linux-doc@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, linux-security-module@vger.kernel.org, =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= Subject: [PATCH v2 12/12] landlock: Add design choices documentation for filesystem access rights Date: Tue, 29 Mar 2022 14:51:17 +0200 Message-Id: <20220329125117.1393824-13-mic@digikod.net> In-Reply-To: <20220329125117.1393824-1-mic@digikod.net> References: <20220329125117.1393824-1-mic@digikod.net> MIME-Version: 1.0 Precedence: bulk List-ID: From: Mickaël Salaün Reviewed-by: Paul Moore Signed-off-by: Mickaël Salaün Link: https://lore.kernel.org/r/20220329125117.1393824-13-mic@digikod.net --- Changes since v1: * Add Reviewed-by: Paul Moore. * Update date. --- Documentation/security/landlock.rst | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/Documentation/security/landlock.rst b/Documentation/security/landlock.rst index 3df68cb1d10f..eb4905993a59 100644 --- a/Documentation/security/landlock.rst +++ b/Documentation/security/landlock.rst @@ -7,7 +7,7 @@ Landlock LSM: kernel documentation ================================== :Author: Mickaël Salaün -:Date: March 2021 +:Date: March 2022 Landlock's goal is to create scoped access-control (i.e. sandboxing). To harden a whole system, this feature should be available to any process, @@ -42,6 +42,21 @@ Guiding principles for safe access controls * Computation related to Landlock operations (e.g. enforcing a ruleset) shall only impact the processes requesting them. +Design choices +============== + +Filesystem access rights +------------------------ + +All access rights are tied to an inode and what can be accessed through it. +Reading the content of a directory doesn't imply to be allowed to read the +content of a listed inode. Indeed, a file name is local to its parent +directory, and an inode can be referenced by multiple file names thanks to +(hard) links. Being able to unlink a file only has a direct impact on the +directory, not the unlinked inode. This is the reason why +`LANDLOCK_ACCESS_FS_REMOVE_FILE` or `LANDLOCK_ACCESS_FS_REFER` are not allowed +to be tied to files but only to directories. + Tests =====