From mboxrd@z Thu Jan 1 00:00:00 1970 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 B7AC6C433F5 for ; Fri, 7 Oct 2022 14:06:07 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229760AbiJGOGG (ORCPT ); Fri, 7 Oct 2022 10:06:06 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:33454 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229556AbiJGOGE (ORCPT ); Fri, 7 Oct 2022 10:06:04 -0400 Received: from ams.source.kernel.org (ams.source.kernel.org [IPv6:2604:1380:4601:e00::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id C7227114DF6; Fri, 7 Oct 2022 07:06:02 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id 685BBB8233B; Fri, 7 Oct 2022 14:06:01 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id CC963C433D6; Fri, 7 Oct 2022 14:05:57 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1665151560; bh=9F8LnVhypCVbAEjMY52MuA8FAoZt2lfR9cmWQLPDSQ4=; h=From:To:Cc:Subject:Date:From; b=hy6ua5Ef++vsD5aVa9YH8WOe5naV+655Risij8V6RXWX7mtrWeJF9SDZYlZySjmLe kr61iV+zTZtws4FiyG67BY4qPfLOGl3U/GTX0Xxz6a92F1VDIhu5ECQNVd+CIbU0Cl lv6bQ8kmc7FavX5ihaSYQ5H6/VMkvoRWyEYcQ2fV/OoTbCmGqI8ykeVnS7cINZNID/ c75r3MxL4EH8vmeZYM4RcEyFYKNpLUm4iQSE6STNUAxrN7zmMamzkm7dNBy626PetO vEfftQ/LrEqqZxQbJVepCgYzw/PNlR8PRqlxDGVVSXfICllpGiVVqfgDaqGIIDujjt WHZixYrKwpizQ== From: Christian Brauner To: Amir Goldstein , Miklos Szeredi , "Darrick J . Wong" Cc: "Christian Brauner (Microsoft)" , Al Viro , Christoph Hellwig , Seth Forshee , Yang Xu , Filipe Manana , linux-unionfs@vger.kernel.org, linux-fsdevel@vger.kernel.org Subject: [PATCH v2 0/5] fs: improve setgid stripping consistency even more Date: Fri, 7 Oct 2022 16:05:38 +0200 Message-Id: <20221007140543.1039983-1-brauner@kernel.org> X-Mailer: git-send-email 2.34.1 MIME-Version: 1.0 X-Developer-Signature: v=1; a=openpgp-sha256; l=5984; i=brauner@kernel.org; h=from:subject; bh=WKXNTmpCAwOaeD0nIX2om2wGZiw40YWrUTl35MBj7mg=; b=owGbwMvMwCU28Zj0gdSKO4sYT6slMSQ7GOmXrPeYZM4mkWIgFNg1L9lZ8lLWtmZ2N+7GWT16VQL8 t507SlkYxLgYZMUUWRzaTcLllvNUbDbK1ICZw8oEMoSBi1MAJnJ8JiPDvxUngnfkFtp/YOqxPsdeZi Lxm8nlXr20acmPUz6dNgviGBmaD97mnvZjYa/KczZfMX7BKSrulnw5xzh63N5EsNtmreMEAA== X-Developer-Key: i=brauner@kernel.org; a=openpgp; fpr=4880B8C9BD0E5106FC070F4F7B3C391EFEA93624 Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org From: "Christian Brauner (Microsoft)" Hey everyone, A long while ago I found a few setgid inheritance bugs in overlayfs in certain conditions. Amir recently picked this back up in https://lore.kernel.org/linux-fsdevel/20221003123040.900827-1-amir73il@gmail.com and I jumped on board to fix this more generally. This series should make setgid stripping more consistent and fix the related overlayfs bugs. Currently setgid stripping in file_remove_privs()'s should_remove_suid() helper is inconsistent with other parts of the vfs. Specifically, it only raises ATTR_KILL_SGID if the inode is S_ISGID and S_IXGRP but not if the inode isn't in the caller's groups and the caller isn't privileged over the inode although we require this already in setattr_prepare() and setattr_copy() and so all filesystem implement this requirement implicitly because they have to use setattr_{prepare,copy}() anyway. But the inconsistency shows up in setgid stripping bugs for overlayfs in xfstests (e.g., generic/673, generic/683, generic/685, generic/686, generic/687). For example, we test whether suid and setgid stripping works correctly when performing various write-like operations as an unprivileged user (fallocate, reflink, write, etc.): echo "Test 1 - qa_user, non-exec file $verb" setup_testfile chmod a+rws $junk_file commit_and_check "$qa_user" "$verb" 64k 64k The test basically creates a file with 6666 permissions. While the file has the S_ISUID and S_ISGID bits set it does not have the S_IXGRP set. On a regular filesystem like xfs what will happen is: sys_fallocate() -> vfs_fallocate() -> xfs_file_fallocate() -> file_modified() -> __file_remove_privs() -> dentry_needs_remove_privs() -> should_remove_suid() -> __remove_privs() newattrs.ia_valid = ATTR_FORCE | kill; -> notify_change() -> setattr_copy() In should_remove_suid() we can see that ATTR_KILL_SUID is raised unconditionally because the file in the test has S_ISUID set. But we also see that ATTR_KILL_SGID won't be set because while the file is S_ISGID it is not S_IXGRP (see above) which is a condition for ATTR_KILL_SGID being raised. So by the time we call notify_change() we have attr->ia_valid set to ATTR_KILL_SUID | ATTR_FORCE. Now notify_change() sees that ATTR_KILL_SUID is set and does: ia_valid = attr->ia_valid |= ATTR_MODE attr->ia_mode = (inode->i_mode & ~S_ISUID); which means that when we call setattr_copy() later we will definitely update inode->i_mode. Note that attr->ia_mode still contains S_ISGID. Now we call into the filesystem's ->setattr() inode operation which will end up calling setattr_copy(). Since ATTR_MODE is set we will hit: if (ia_valid & ATTR_MODE) { umode_t mode = attr->ia_mode; vfsgid_t vfsgid = i_gid_into_vfsgid(mnt_userns, inode); if (!vfsgid_in_group_p(vfsgid) && !capable_wrt_inode_uidgid(mnt_userns, inode, CAP_FSETID)) mode &= ~S_ISGID; inode->i_mode = mode; } and since the caller in the test is neither capable nor in the group of the inode the S_ISGID bit is stripped. But assume the file isn't suid then ATTR_KILL_SUID won't be raised which has the consequence that neither the setgid nor the suid bits are stripped even though it should be stripped because the inode isn't in the caller's groups and the caller isn't privileged over the inode. If overlayfs is in the mix things become a bit more complicated and the bug shows up more clearly. When e.g., ovl_setattr() is hit from ovl_fallocate()'s call to file_remove_privs() then ATTR_KILL_SUID and ATTR_KILL_SGID might be raised but because the check in notify_change() is questioning the ATTR_KILL_SGID flag again by requiring S_IXGRP for it to be stripped the S_ISGID bit isn't removed even though it should be stripped: sys_fallocate() -> vfs_fallocate() -> ovl_fallocate() -> file_remove_privs() -> dentry_needs_remove_privs() -> should_remove_suid() -> __remove_privs() newattrs.ia_valid = ATTR_FORCE | kill; -> notify_change() -> ovl_setattr() // TAKE ON MOUNTER'S CREDS -> ovl_do_notify_change() -> notify_change() // GIVE UP MOUNTER'S CREDS // TAKE ON MOUNTER'S CREDS -> vfs_fallocate() -> xfs_file_fallocate() -> file_modified() -> __file_remove_privs() -> dentry_needs_remove_privs() -> should_remove_suid() -> __remove_privs() newattrs.ia_valid = attr_force | kill; -> notify_change() The fix for all of this is to make file_remove_privs()'s should_remove_suid() helper to perform the same checks as we already require in setattr_prepare() and setattr_copy() and have notify_change() not pointlessly requiring S_IXGRP again. It doesn't make any sense in the first place because the caller must calculate the flags via should_remove_suid() anyway which would raise ATTR_KILL_SGID. Running xfstests with this doesn't report any regressions. We should really try and use consistent checks. Thanks! Christian Amir Goldstein (2): ovl: remove privs in ovl_copyfile() ovl: remove privs in ovl_fallocate() Christian Brauner (3): attr: add setattr_drop_sgid() attr: add should_remove_sgid() attr: use consistent sgid stripping checks fs/attr.c | 58 ++++++++++++++++++++++++++++++++++++++++----- fs/fuse/file.c | 2 +- fs/inode.c | 24 +++++++------------ fs/internal.h | 5 +++- fs/ocfs2/file.c | 4 ++-- fs/open.c | 8 +++---- fs/overlayfs/file.c | 28 +++++++++++++++++++--- include/linux/fs.h | 2 +- 8 files changed, 97 insertions(+), 34 deletions(-) -- 2.34.1