ceph: fix up non-directory creation in SGID directories

PATCH In review Christian Brauner 3 participants 4 messages Dec 23, 2021 – Dec 27, 2021 1 message not in the archive

Subsystems (2)

Share & tools

A message that is not in the archive (sent off-list or not yet received): https://lore.kernel.org/stable/[email protected]

Christian Brauner

author

2021-12-23 09:57 UTC #1 lore raw

PATCH 1/1 ceph: fix up non-directory creation in SGID directories

From: Christian Brauner <[email protected]>

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 [email protected] (adapted to v5.10)

Signed-off-by Christian Brauner <[email protected]>

Reviewed-by Jeff Layton <[email protected]>

Signed-off-by Ilya Dryomov <[email protected]>

---
 fs/ceph/file.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

fs/ceph/file.c +15 -3

index 8e6855e7ed83..8ed881fd7440 100644
@@ -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);

Signature · 1 line

2.30.2

Greg KH

maintainer

2021-12-23 10:20 UTC #2 lore raw

Quoted 22 lines · Christian Brauner

From: Christian Brauner <[email protected]>
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

Quoted 7 lines

Cc [email protected] (adapted to v5.10)
Signed-off-by Christian Brauner <[email protected]>
Reviewed-by Jeff Layton <[email protected]>
Signed-off-by Ilya Dryomov <[email protected]>
---
 fs/ceph/file.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

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

Christian Brauner

2021-12-23 10:31 UTC #3 lore raw

Quoted 31 lines · Greg KH

Quoted 22 lines · Christian Brauner
From: Christian Brauner <[email protected]>
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
Quoted 7 lines
Cc [email protected] (adapted to v5.10)
Signed-off-by Christian Brauner <[email protected]>
Reviewed-by Jeff Layton <[email protected]>
Signed-off-by Ilya Dryomov <[email protected]>
---
 fs/ceph/file.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

Quoted 1 line

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

commit fd84bfdddd169c219c3a637889a8b87f70a072c2

Author: Christian Brauner <[email protected]>
Date:   Mon Nov 29 12:16:39 2021 +0100

ceph: fix up non-directory creation in SGID directories

Greg KH

maintainer

2021-12-27 12:46 UTC #4 lore raw

Quoted 32 lines · Christian Brauner

Quoted 31 lines · Greg KH
Quoted 29 lines · Christian Brauner
From: Christian Brauner <[email protected]>
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 [email protected] (adapted to v5.10)
Signed-off-by Christian Brauner <[email protected]>
Reviewed-by Jeff Layton <[email protected]>
Signed-off-by Ilya Dryomov <[email protected]>
---
 fs/ceph/file.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

Quoted 7 lines

Quoted 1 line
What is the git commit id in Linus's tree?
commit fd84bfdddd169c219c3a637889a8b87f70a072c2
Author: Christian Brauner <[email protected]>
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