lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Fri, 8 Apr 2022 18:07:17 +0200 From: Mickaël Salaün <mic@...ikod.net> To: Paul Moore <paul@...l-moore.com> Cc: James Morris <jmorris@...ei.org>, "Serge E . Hallyn" <serge@...lyn.com>, Al Viro <viro@...iv.linux.org.uk>, Jann Horn <jannh@...gle.com>, John Johansen <john.johansen@...onical.com>, Kees Cook <keescook@...omium.org>, Konstantin Meskhidze <konstantin.meskhidze@...wei.com>, Shuah Khan <shuah@...nel.org>, Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>, linux-doc@...r.kernel.org, linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org, linux-security-module@...r.kernel.org, Mickaël Salaün <mic@...ux.microsoft.com> Subject: Re: [PATCH v2 07/12] landlock: Add support for file reparenting with LANDLOCK_ACCESS_FS_REFER On 08/04/2022 03:42, Paul Moore wrote: > On Tue, Mar 29, 2022 at 8:51 AM Mickaël Salaün <mic@...ikod.net> wrote: >> >> From: Mickaël Salaün <mic@...ux.microsoft.com> >> >> Add a new LANDLOCK_ACCESS_FS_REFER access right to enable policy writers >> to allow sandboxed processes to link and rename files from and to a >> specific set of file hierarchies. This access right should be composed >> with LANDLOCK_ACCESS_FS_MAKE_* for the destination of a link or rename, >> and with LANDLOCK_ACCESS_FS_REMOVE_* for a source of a rename. This >> lift a Landlock limitation that always denied changing the parent of an >> inode. >> >> Renaming or linking to the same directory is still always allowed, >> whatever LANDLOCK_ACCESS_FS_REFER is used or not, because it is not >> considered a threat to user data. >> >> However, creating multiple links or renaming to a different parent >> directory may lead to privilege escalations if not handled properly. >> Indeed, we must be sure that the source doesn't gain more privileges by >> being accessible from the destination. This is handled by making sure >> that the source hierarchy (including the referenced file or directory >> itself) restricts at least as much the destination hierarchy. If it is >> not the case, an EXDEV error is returned, making it potentially possible >> for user space to copy the file hierarchy instead of moving or linking >> it. >> >> Instead of creating different access rights for the source and the >> destination, we choose to make it simple and consistent for users. >> Indeed, considering the previous constraint, it would be weird to >> require such destination access right to be also granted to the source >> (to make it a superset). 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 <paul@...l-moore.com> >> Signed-off-by: Mickaël Salaün <mic@...ux.microsoft.com> >> Link: https://lore.kernel.org/r/20220329125117.1393824-8-mic@digikod.net >> --- >> >> 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(-) > > I'm still not going to claim that I'm a Landlock expert, but this > looks sane to me. > > Reviewed-by: Paul Moore <paul@...l-moore.com> Thanks Paul! I'll send a small update shortly, with some typo fixes, some unlikely() calls, and rebased on the other Landlock patch series. > >> +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. */ > > ;) >
Powered by blists - more mailing lists