Hi all,
Commit 01ea173e103edd5ec41acec65b9261b87e123fc2 (upstream: xfs: fix up non-directory creation in SGID directories) fixes an issue where in xfs sometimes, a local user could create files with an unitended group permissions as an owner and execution where a directory is SGID and belongs to a certain group and is writable by a user who is not a member of this group and seems like a good candidate for the v5.10 stable tree given that 5.10 is used in versions of debian, ubuntu.
This patch applies cleanly. Let me know what you think
From: Christoph Hellwig hch@lst.de
XFS always inherits the SGID bit if it is set on the parent inode, while the generic inode_init_owner does not do this in a few cases where it can create a possible security problem, see commit 0fa3ecd87848 ("Fix up non-directory creation in SGID directories") for details.
Switch XFS to use the generic helper for the normal path to fix this, just keeping the simple field inheritance open coded for the case of the non-sgid case with the bsdgrpid mount option.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Reported-by: Christian Brauner christian.brauner@ubuntu.com Signed-off-by: Christoph Hellwig hch@lst.de Reviewed-by: Darrick J. Wong djwong@kernel.org Signed-off-by: Darrick J. Wong djwong@kernel.org --- fs/xfs/xfs_inode.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c index 8ebd9c64aa48..e2a1db4cee43 100644 --- a/fs/xfs/xfs_inode.c +++ b/fs/xfs/xfs_inode.c @@ -775,6 +775,7 @@ xfs_init_new_inode( prid_t prid, struct xfs_inode **ipp) { + struct inode *dir = pip ? VFS_I(pip) : NULL; struct xfs_mount *mp = tp->t_mountp; struct xfs_inode *ip; unsigned int flags; @@ -804,18 +805,17 @@ xfs_init_new_inode(
ASSERT(ip != NULL); inode = VFS_I(ip); - inode->i_mode = mode; set_nlink(inode, nlink); - inode->i_uid = current_fsuid(); inode->i_rdev = rdev; ip->i_d.di_projid = prid;
- if (pip && XFS_INHERIT_GID(pip)) { - inode->i_gid = VFS_I(pip)->i_gid; - if ((VFS_I(pip)->i_mode & S_ISGID) && S_ISDIR(mode)) - inode->i_mode |= S_ISGID; + if (dir && !(dir->i_mode & S_ISGID) && + (mp->m_flags & XFS_MOUNT_GRPID)) { + inode->i_uid = current_fsuid(); + inode->i_gid = dir->i_gid; + inode->i_mode = mode; } else { - inode->i_gid = current_fsgid(); + inode_init_owner(inode, dir, mode); }
/*
Hi,
Thanks for your patch.
FYI: kernel test robot notices the stable kernel rule is not satisfied.
Rule: 'Cc: stable@vger.kernel.org' or 'commit <sha1> upstream.' Subject: [PATCH] xfs: fix up non-directory creation in SGID directories Link: https://lore.kernel.org/stable/20220906183600.1926315-2-teratipally%40google...
The check is based on https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
On Tue, Sep 6, 2022 at 9:36 PM Varsha Teratipally teratipally@google.com wrote:
From: Christoph Hellwig hch@lst.de
XFS always inherits the SGID bit if it is set on the parent inode, while the generic inode_init_owner does not do this in a few cases where it can create a possible security problem, see commit 0fa3ecd87848 ("Fix up non-directory creation in SGID directories") for details.
Switch XFS to use the generic helper for the normal path to fix this, just keeping the simple field inheritance open coded for the case of the non-sgid case with the bsdgrpid mount option.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Reported-by: Christian Brauner christian.brauner@ubuntu.com Signed-off-by: Christoph Hellwig hch@lst.de Reviewed-by: Darrick J. Wong djwong@kernel.org Signed-off-by: Darrick J. Wong djwong@kernel.org
Hi Varsha,
For future reference, when posting an xfs patch for stable, please follow these guidelines:
1. Post it to xfs list for review BEFORE posting to stable 2. LKML is not a relevant list 3. Tag the patch with the target kernel [PATCH 5.10] 4. Include the upstream commit id 5. Add some description (after --- line) about how you tested
Regarding this specific patch for 5.10, I had already tested and posted it for review back in June [1].
Dave Chinner commented then that he was concerned about other security issues discovered later on related to the generic implementation of SGID stripping. At the time, the generic upstream fixes and tests were still WIP.
Christoph Hellwig, the author of the original patch replied to Dave's concern:
"To me backporting it seems good and useful, as it fixes a relatively big problem. The remaining issues seem minor compared to that."
Christiain Brauner who has been reviewing the generic upstream also agreed that:
"Imho, backporting this patch is useful. It fixes a basic issue."
So this specific fix patch from the v5.12 release, which is not relevant for 5.15.y has my blessing to go to 5.10.y.
Regardless, the last bits of the upstream work on the generic implementation by Yang Xu have landed in v6.0-rc1 [2] and the respective fstests have just recently landed in fstests v2022.09.04.
I already have all the patches backported to 5.10 [3] and will start testing them in the following weeks, but now I also depend on Leah to test them for 5.15.y before I can post to 5.10.y and that may take a while...
Thanks, Amir.
[1] https://lore.kernel.org/linux-xfs/CAOQ4uxg4=m9zEFbDAKXx7CP7HYiMwtsYSJvq076oK... [2] https://lore.kernel.org/linux-fsdevel/20220809103957.1851931-1-brauner@kerne... [3] https://github.com/amir73il/linux/commits/xfs-5.10.y-sgid-fixes
[Fix CC for brauner]
On Wed, Sep 7, 2022 at 10:40 AM Amir Goldstein amir73il@gmail.com wrote:
On Tue, Sep 6, 2022 at 9:36 PM Varsha Teratipally teratipally@google.com wrote:
From: Christoph Hellwig hch@lst.de
XFS always inherits the SGID bit if it is set on the parent inode, while the generic inode_init_owner does not do this in a few cases where it can create a possible security problem, see commit 0fa3ecd87848 ("Fix up non-directory creation in SGID directories") for details.
Switch XFS to use the generic helper for the normal path to fix this, just keeping the simple field inheritance open coded for the case of the non-sgid case with the bsdgrpid mount option.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Reported-by: Christian Brauner christian.brauner@ubuntu.com Signed-off-by: Christoph Hellwig hch@lst.de Reviewed-by: Darrick J. Wong djwong@kernel.org Signed-off-by: Darrick J. Wong djwong@kernel.org
Hi Varsha,
For future reference, when posting an xfs patch for stable, please follow these guidelines:
- Post it to xfs list for review BEFORE posting to stable
- LKML is not a relevant list
- Tag the patch with the target kernel [PATCH 5.10]
- Include the upstream commit id
- Add some description (after --- line) about how you tested
Regarding this specific patch for 5.10, I had already tested and posted it for review back in June [1].
Dave Chinner commented then that he was concerned about other security issues discovered later on related to the generic implementation of SGID stripping. At the time, the generic upstream fixes and tests were still WIP.
Christoph Hellwig, the author of the original patch replied to Dave's concern:
"To me backporting it seems good and useful, as it fixes a relatively big problem. The remaining issues seem minor compared to that."
Christiain Brauner who has been reviewing the generic upstream also agreed that:
"Imho, backporting this patch is useful. It fixes a basic issue."
So this specific fix patch from the v5.12 release, which is not relevant for 5.15.y has my blessing to go to 5.10.y.
Regardless, the last bits of the upstream work on the generic implementation by Yang Xu have landed in v6.0-rc1 [2] and the respective fstests have just recently landed in fstests v2022.09.04.
I already have all the patches backported to 5.10 [3] and will start testing them in the following weeks, but now I also depend on Leah to test them for 5.15.y before I can post to 5.10.y and that may take a while...
Thanks, Amir.
[1] https://lore.kernel.org/linux-xfs/CAOQ4uxg4=m9zEFbDAKXx7CP7HYiMwtsYSJvq076oK... [2] https://lore.kernel.org/linux-fsdevel/20220809103957.1851931-1-brauner@kerne... [3] https://github.com/amir73il/linux/commits/xfs-5.10.y-sgid-fixes
On Tue, Sep 06, 2022 at 06:36:00PM +0000, Varsha Teratipally wrote:
From: Christoph Hellwig hch@lst.de
XFS always inherits the SGID bit if it is set on the parent inode, while the generic inode_init_owner does not do this in a few cases where it can create a possible security problem, see commit 0fa3ecd87848 ("Fix up non-directory creation in SGID directories") for details.
Switch XFS to use the generic helper for the normal path to fix this, just keeping the simple field inheritance open coded for the case of the non-sgid case with the bsdgrpid mount option.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Reported-by: Christian Brauner christian.brauner@ubuntu.com Signed-off-by: Christoph Hellwig hch@lst.de Reviewed-by: Darrick J. Wong djwong@kernel.org Signed-off-by: Darrick J. Wong djwong@kernel.org
Why did you not sign off on this if you are forwarding it on?
Also, what is the git id of this commit in Linus's tree (we need that hint...)
Please fix both up and resend and get the ack of the stable xfs developers on it as well.
thanks,
greg k-h
On Thu, Sep 8, 2022 at 2:48 PM Greg KH gregkh@linuxfoundation.org wrote:
On Tue, Sep 06, 2022 at 06:36:00PM +0000, Varsha Teratipally wrote:
From: Christoph Hellwig hch@lst.de
XFS always inherits the SGID bit if it is set on the parent inode, while the generic inode_init_owner does not do this in a few cases where it can create a possible security problem, see commit 0fa3ecd87848 ("Fix up non-directory creation in SGID directories") for details.
Switch XFS to use the generic helper for the normal path to fix this, just keeping the simple field inheritance open coded for the case of the non-sgid case with the bsdgrpid mount option.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Reported-by: Christian Brauner christian.brauner@ubuntu.com Signed-off-by: Christoph Hellwig hch@lst.de Reviewed-by: Darrick J. Wong djwong@kernel.org Signed-off-by: Darrick J. Wong djwong@kernel.org
Why did you not sign off on this if you are forwarding it on?
Also, what is the git id of this commit in Linus's tree (we need that hint...)
Please fix both up and resend and get the ack of the stable xfs developers on it as well.
Varsha,
FWIW, I re-tested the patch on top of v5.10.141, so when re-posting [PATCH 5.10] you may add:
Tested-by: Amir Goldstein amir73il@gmail.com
Darrick or Christoph,
Can you please ACK this patch?
Thanks, Amir.
On Thu, Sep 08, 2022 at 03:02:41PM +0300, Amir Goldstein wrote:
On Thu, Sep 8, 2022 at 2:48 PM Greg KH gregkh@linuxfoundation.org wrote:
On Tue, Sep 06, 2022 at 06:36:00PM +0000, Varsha Teratipally wrote:
From: Christoph Hellwig hch@lst.de
XFS always inherits the SGID bit if it is set on the parent inode, while the generic inode_init_owner does not do this in a few cases where it can create a possible security problem, see commit 0fa3ecd87848 ("Fix up non-directory creation in SGID directories") for details.
Switch XFS to use the generic helper for the normal path to fix this, just keeping the simple field inheritance open coded for the case of the non-sgid case with the bsdgrpid mount option.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Reported-by: Christian Brauner christian.brauner@ubuntu.com Signed-off-by: Christoph Hellwig hch@lst.de Reviewed-by: Darrick J. Wong djwong@kernel.org Signed-off-by: Darrick J. Wong djwong@kernel.org
Why did you not sign off on this if you are forwarding it on?
Also, what is the git id of this commit in Linus's tree (we need that hint...)
Please fix both up and resend and get the ack of the stable xfs developers on it as well.
Varsha,
FWIW, I re-tested the patch on top of v5.10.141, so when re-posting [PATCH 5.10] you may add:
Tested-by: Amir Goldstein amir73il@gmail.com
Darrick or Christoph,
Can you please ACK this patch?
With all the bookkeepping bits corrected (and assuming that the VFS fixes have been or are about to be applied):
Acked-by: Darrick J. Wong djwong@kernel.org
--D
Thanks, Amir.
On Tue, Sep 6, 2022 at 9:36 PM Varsha Teratipally teratipally@google.com wrote:
Hi all,
Commit 01ea173e103edd5ec41acec65b9261b87e123fc2 (upstream: xfs: fix up non-directory creation in SGID directories) fixes an issue where in xfs sometimes, a local user could create files with an unitended group permissions as an owner and execution where a directory is SGID and belongs to a certain group and is writable by a user who is not a member of this group and seems like a good candidate for the v5.10 stable tree given that 5.10 is used in versions of debian, ubuntu.
This patch applies cleanly. Let me know what you think
Since you already posted the patch, I wrote what I think on the post:
https://lore.kernel.org/linux-xfs/CAOQ4uxi_Q8aXUg+FM0Q9__t=KqJSVqOgkS8j8kNC3...
Bottom line - I think that the patch should be applied to 5.10.y without further delay.
Thanks, Amir.
linux-stable-mirror@lists.linaro.org
-
Amir Goldstein -
Darrick J. Wong -
Greg KH -
kernel test robot -
Varsha Teratipally