fs: account for group membership

Commit Message

Christian Brauner June 13, 2022, 11:15 a.m. UTC

When calling setattr_prepare() to determine the validity of the
attributes the ia_{g,u}id fields contain the value that will be written
to inode->i_{g,u}id. This is exactly the same for idmapped and
non-idmapped mounts and allows callers to pass in the values they want
to see written to inode->i_{g,u}id.

When group ownership is changed a caller whose fsuid owns the inode can
change the group of the inode to any group they are a member of. When
searching through the caller's groups we need to use the gid mapped
according to the idmapped mount otherwise we will fail to change
ownership for unprivileged users.

Consider a caller running with fsuid and fsgid 1000 using an idmapped
mount that maps id 65534 to 1000 and 65535 to 1001. Consequently, a file
owned by 65534:65535 in the filesystem will be owned by 1000:1001 in the
idmapped mount.

The caller now requests the gid of the file to be changed to 1000 going
through the idmapped mount. In the vfs we will immediately map the
requested gid to the value that will need to be written to inode->i_gid
and place it in attr->ia_gid. Since this idmapped mount maps 65534 to
1000 we place 65534 in attr->ia_gid.

When we check whether the caller is allowed to change group ownership we
first validate that their fsuid matches the inode's uid. The
inode->i_uid is 65534 which is mapped to uid 1000 in the idmapped mount.
Since the caller's fsuid is 1000 we pass the check.

We now check whether the caller is allowed to change inode->i_gid to the
requested gid by calling in_group_p(). This will compare the passed in
gid to the caller's fsgid and search the caller's additional groups.

Since we're dealing with an idmapped mount we need to pass in the gid
mapped according to the idmapped mount. This is akin to checking whether
a caller is privileged over the future group the inode is owned by. And
that needs to take the idmapped mount into account. Note, all helpers
are nops without idmapped mounts.

New regression test sent to xfstests.

Link: https://github.com/lxc/lxd/issues/10537
Fixes: 2f221d6f7b88 ("attr: handle idmapped mounts")
Cc: Seth Forshee <[e-mail]>
Cc: Christoph Hellwig <[e-mail]>
Cc: Aleksa Sarai <[e-mail]>
Cc: Al Viro <[e-mail]>
Cc: [e-mail] # 5.15+
CC: [e-mail]
Signed-off-by: Christian Brauner (Microsoft) <[e-mail]>
---
Hey,

Detected while working on additional tests and also reported by a user
and brain on my part. I plan to work on patches to disentangle the
codepaths here a bit and make them easier to understand going forward.

Passes xfstests including new tests and LTP test suite.

Thanks!
Christian
---
 fs/attr.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)


base-commit: f2906aa863381afb0015a9eb7fefad885d4e5a56

Comments

Seth Forshee (DigitalOcean) June 13, 2022, 2:04 p.m. UTC | #1

On Mon, Jun 13, 2022 at 01:15:17PM +0200, Christian Brauner wrote:
> When calling setattr_prepare() to determine the validity of the
> attributes the ia_{g,u}id fields contain the value that will be written
> to inode->i_{g,u}id. This is exactly the same for idmapped and
> non-idmapped mounts and allows callers to pass in the values they want
> to see written to inode->i_{g,u}id.
> 
> When group ownership is changed a caller whose fsuid owns the inode can
> change the group of the inode to any group they are a member of. When
> searching through the caller's groups we need to use the gid mapped
> according to the idmapped mount otherwise we will fail to change
> ownership for unprivileged users.
> 
> Consider a caller running with fsuid and fsgid 1000 using an idmapped
> mount that maps id 65534 to 1000 and 65535 to 1001. Consequently, a file
> owned by 65534:65535 in the filesystem will be owned by 1000:1001 in the
> idmapped mount.
> 
> The caller now requests the gid of the file to be changed to 1000 going
> through the idmapped mount. In the vfs we will immediately map the
> requested gid to the value that will need to be written to inode->i_gid
> and place it in attr->ia_gid. Since this idmapped mount maps 65534 to
> 1000 we place 65534 in attr->ia_gid.
> 
> When we check whether the caller is allowed to change group ownership we
> first validate that their fsuid matches the inode's uid. The
> inode->i_uid is 65534 which is mapped to uid 1000 in the idmapped mount.
> Since the caller's fsuid is 1000 we pass the check.
> 
> We now check whether the caller is allowed to change inode->i_gid to the
> requested gid by calling in_group_p(). This will compare the passed in
> gid to the caller's fsgid and search the caller's additional groups.
> 
> Since we're dealing with an idmapped mount we need to pass in the gid
> mapped according to the idmapped mount. This is akin to checking whether
> a caller is privileged over the future group the inode is owned by. And
> that needs to take the idmapped mount into account. Note, all helpers
> are nops without idmapped mounts.
> 
> New regression test sent to xfstests.
> 
> Link: https://github.com/lxc/lxd/issues/10537
> Fixes: 2f221d6f7b88 ("attr: handle idmapped mounts")
> Cc: Seth Forshee <[e-mail]>
> Cc: Christoph Hellwig <[e-mail]>
> Cc: Aleksa Sarai <[e-mail]>
> Cc: Al Viro <[e-mail]>
> Cc: [e-mail] # 5.15+
> CC: [e-mail]
> Signed-off-by: Christian Brauner (Microsoft) <[e-mail]>

Looks correct to me.

Reviewed-by: Seth Forshee <[e-mail]>

Patch

diff --git a/fs/attr.c b/fs/attr.c
index 66899b6e9bd8..dbe996b0dedf 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -61,9 +61,15 @@  static bool chgrp_ok(struct user_namespace *mnt_userns,
 		     const struct inode *inode, kgid_t gid)
 {
 	kgid_t kgid = i_gid_into_mnt(mnt_userns, inode);
-	if (uid_eq(current_fsuid(), i_uid_into_mnt(mnt_userns, inode)) &&
-	    (in_group_p(gid) || gid_eq(gid, inode->i_gid)))
-		return true;
+	if (uid_eq(current_fsuid(), i_uid_into_mnt(mnt_userns, inode))) {
+		kgid_t mapped_gid;
+
+		if (gid_eq(gid, inode->i_gid))
+			return true;
+		mapped_gid = mapped_kgid_fs(mnt_userns, i_user_ns(inode), gid);
+		if (in_group_p(mapped_gid))
+			return true;
+	}
 	if (capable_wrt_inode_uidgid(mnt_userns, inode, CAP_CHOWN))
 		return true;
 	if (gid_eq(kgid, INVALID_GID) &&
@@ -123,12 +129,20 @@  int setattr_prepare(struct user_namespace *mnt_userns, struct dentry *dentry,
 
 	/* Make sure a caller can chmod. */
 	if (ia_valid & ATTR_MODE) {
+		kgid_t mapped_gid;
+
 		if (!inode_owner_or_capable(mnt_userns, inode))
 			return -EPERM;
+
+		if (ia_valid & ATTR_GID)
+			mapped_gid = mapped_kgid_fs(mnt_userns,
+						i_user_ns(inode), attr->ia_gid);
+		else
+			mapped_gid = i_gid_into_mnt(mnt_userns, inode);
+
 		/* Also check the setgid bit! */
-               if (!in_group_p((ia_valid & ATTR_GID) ? attr->ia_gid :
-                                i_gid_into_mnt(mnt_userns, inode)) &&
-                    !capable_wrt_inode_uidgid(mnt_userns, inode, CAP_FSETID))
+		if (!in_group_p(mapped_gid) &&
+		    !capable_wrt_inode_uidgid(mnt_userns, inode, CAP_FSETID))
 			attr->ia_mode &= ~S_ISGID;
 	}