[PATCH v5.10] ceph: fix up non-directory creation in SGID directories

[PATCH v5.10] ceph: fix up non-directory creation in SGID directories

[Christian Brauner]

From: Christian Brauner <christian.brauner@ubuntu.com>

Ceph 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 (cf. [1]).

Update ceph to strip the SGID bit just as inode_init_owner would.

This bug was detected by the mapped mount testsuite in [3]. The
testsuite tests all core VFS functionality and semantics with and
without mapped mounts. That is to say it functions as a generic VFS
testsuite in addition to a mapped mount testsuite. While working on
mapped mount support for ceph, SIGD inheritance was the only failing
test for ceph after the port.

The same bug was detected by the mapped mount testsuite in XFS in
January 2021 (cf. [2]).

[1]: commit 0fa3ecd87848 ("Fix up non-directory creation in SGID directories")
[2]: commit 01ea173e103e ("xfs: fix up non-directory creation in SGID directories")
[3]: https://git.kernel.org/fs/xfs/xfstests-dev.git

Cc: stable@vger.kernel.org (adapted to v5.10)
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
---
 fs/ceph/file.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/fs/ceph/file.c b/fs/ceph/file.c
index 8e6855e7ed83..8ed881fd7440 100644
--- a/fs/ceph/file.c
+++ b/fs/ceph/file.c
@@ -603,13 +603,25 @@ static int ceph_finish_async_create(struct inode *dir, struct dentry *dentry,
 	in.cap.realm = cpu_to_le64(ci->i_snap_realm->ino);
 	in.cap.flags = CEPH_CAP_FLAG_AUTH;
 	in.ctime = in.mtime = in.atime = iinfo.btime;
-	in.mode = cpu_to_le32((u32)mode);
 	in.truncate_seq = cpu_to_le32(1);
 	in.truncate_size = cpu_to_le64(-1ULL);
 	in.xattr_version = cpu_to_le64(1);
 	in.uid = cpu_to_le32(from_kuid(&init_user_ns, current_fsuid()));
-	in.gid = cpu_to_le32(from_kgid(&init_user_ns, dir->i_mode & S_ISGID ?
-				dir->i_gid : current_fsgid()));
+	if (dir->i_mode & S_ISGID) {
+		in.gid = cpu_to_le32(from_kgid(&init_user_ns, dir->i_gid));
+
+		/* Directories always inherit the setgid bit. */
+		if (S_ISDIR(mode))
+			mode |= S_ISGID;
+		else if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP) &&
+			 !in_group_p(dir->i_gid) &&
+			 !capable_wrt_inode_uidgid(dir, CAP_FSETID))
+			mode &= ~S_ISGID;
+	} else {
+		in.gid = cpu_to_le32(from_kgid(&init_user_ns, current_fsgid()));
+	}
+	in.mode = cpu_to_le32((u32)mode);
+
 	in.nlink = cpu_to_le32(1);
 	in.max_size = cpu_to_le64(lo->stripe_unit);
 
-- 
2.30.2

Re: [PATCH v5.10] ceph: fix up non-directory creation in SGID directories

[Greg KH]

On Thu, Dec 23, 2021 at 10:57:33AM +0100, Christian Brauner wrote:
> From: Christian Brauner <christian.brauner@ubuntu.com>
> 
> Ceph 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 (cf. [1]).
> 
> Update ceph to strip the SGID bit just as inode_init_owner would.
> 
> This bug was detected by the mapped mount testsuite in [3]. The
> testsuite tests all core VFS functionality and semantics with and
> without mapped mounts. That is to say it functions as a generic VFS
> testsuite in addition to a mapped mount testsuite. While working on
> mapped mount support for ceph, SIGD inheritance was the only failing
> test for ceph after the port.
> 
> The same bug was detected by the mapped mount testsuite in XFS in
> January 2021 (cf. [2]).
> 
> [1]: commit 0fa3ecd87848 ("Fix up non-directory creation in SGID directories")
> [2]: commit 01ea173e103e ("xfs: fix up non-directory creation in SGID directories")
> [3]: https://git.kernel.org/fs/xfs/xfstests-dev.git
> 
> Cc: stable@vger.kernel.org (adapted to v5.10)
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> Reviewed-by: Jeff Layton <jlayton@kernel.org>
> Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
> ---
>  fs/ceph/file.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)

What is the git commit id in Linus's tree?

Re: [PATCH v5.10] ceph: fix up non-directory creation in SGID directories

[Christian Brauner]

On Thu, Dec 23, 2021 at 11:20:07AM +0100, Greg KH wrote:
> On Thu, Dec 23, 2021 at 10:57:33AM +0100, Christian Brauner wrote:
> > From: Christian Brauner <christian.brauner@ubuntu.com>
> > 
> > Ceph 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 (cf. [1]).
> > 
> > Update ceph to strip the SGID bit just as inode_init_owner would.
> > 
> > This bug was detected by the mapped mount testsuite in [3]. The
> > testsuite tests all core VFS functionality and semantics with and
> > without mapped mounts. That is to say it functions as a generic VFS
> > testsuite in addition to a mapped mount testsuite. While working on
> > mapped mount support for ceph, SIGD inheritance was the only failing
> > test for ceph after the port.
> > 
> > The same bug was detected by the mapped mount testsuite in XFS in
> > January 2021 (cf. [2]).
> > 
> > [1]: commit 0fa3ecd87848 ("Fix up non-directory creation in SGID directories")
> > [2]: commit 01ea173e103e ("xfs: fix up non-directory creation in SGID directories")
> > [3]: https://git.kernel.org/fs/xfs/xfstests-dev.git
> > 
> > Cc: stable@vger.kernel.org (adapted to v5.10)
> > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > Reviewed-by: Jeff Layton <jlayton@kernel.org>
> > Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
> > ---
> >  fs/ceph/file.c | 18 +++++++++++++++---
> >  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> What is the git commit id in Linus's tree?

commit fd84bfdddd169c219c3a637889a8b87f70a072c2
Author: Christian Brauner <christian.brauner@ubuntu.com>
Date:   Mon Nov 29 12:16:39 2021 +0100

    ceph: fix up non-directory creation in SGID directories

Re: [PATCH v5.10] ceph: fix up non-directory creation in SGID directories

[Greg KH]

On Thu, Dec 23, 2021 at 11:31:29AM +0100, Christian Brauner wrote:
> On Thu, Dec 23, 2021 at 11:20:07AM +0100, Greg KH wrote:
> > On Thu, Dec 23, 2021 at 10:57:33AM +0100, Christian Brauner wrote:
> > > From: Christian Brauner <christian.brauner@ubuntu.com>
> > > 
> > > Ceph 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 (cf. [1]).
> > > 
> > > Update ceph to strip the SGID bit just as inode_init_owner would.
> > > 
> > > This bug was detected by the mapped mount testsuite in [3]. The
> > > testsuite tests all core VFS functionality and semantics with and
> > > without mapped mounts. That is to say it functions as a generic VFS
> > > testsuite in addition to a mapped mount testsuite. While working on
> > > mapped mount support for ceph, SIGD inheritance was the only failing
> > > test for ceph after the port.
> > > 
> > > The same bug was detected by the mapped mount testsuite in XFS in
> > > January 2021 (cf. [2]).
> > > 
> > > [1]: commit 0fa3ecd87848 ("Fix up non-directory creation in SGID directories")
> > > [2]: commit 01ea173e103e ("xfs: fix up non-directory creation in SGID directories")
> > > [3]: https://git.kernel.org/fs/xfs/xfstests-dev.git
> > > 
> > > Cc: stable@vger.kernel.org (adapted to v5.10)
> > > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > > Reviewed-by: Jeff Layton <jlayton@kernel.org>
> > > Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
> > > ---
> > >  fs/ceph/file.c | 18 +++++++++++++++---
> > >  1 file changed, 15 insertions(+), 3 deletions(-)
> > 
> > What is the git commit id in Linus's tree?
> 
> commit fd84bfdddd169c219c3a637889a8b87f70a072c2
> Author: Christian Brauner <christian.brauner@ubuntu.com>
> Date:   Mon Nov 29 12:16:39 2021 +0100
> 
>     ceph: fix up non-directory creation in SGID directories

Great, now queued up, thanks.

greg k-h