linux-kernel - [PATCH v2] ovl: Use current fsuid and fsgid in ovl

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-Id: <20220825130552.29587-1-zhangtianci.1997@bytedance.com>
Date:   Thu, 25 Aug 2022 21:05:52 +0800
From:   Zhang Tianci <zhangtianci.1997@...edance.com>
To:     miklos@...redi.hu
Cc:     Zhang Tianci <zhangtianci.1997@...edance.com>,
        linux-unionfs@...r.kernel.org, linux-kernel@...r.kernel.org,
        amir73il@...il.com,
        Jiachen Zhang <zhangjiachen.jaycee@...edance.com>,
        Christian Brauner <brauner@...nel.org>
Subject: [PATCH v2] ovl: Use current fsuid and fsgid in ovl_link()

There is a wrong case of link() on overlay:
  $ mkdir /lower /fuse /merge
  $ mount -t fuse /fuse
  $ mkdir /fuse/upper /fuse/work
  $ mount -t overlay /merge -o lowerdir=/lower,upperdir=/fuse/upper,\
    workdir=work
  $ touch /merge/file
  $ chown bin.bin /merge/file // the file's caller becomes "bin"
  $ ln /merge/file /merge/lnkfile

Then we will get an error(EACCES) because fuse daemon checks the link()'s
caller is "bin", it denied this request.

In the changing history of ovl_link(), there are two key commits:

The first is commit bb0d2b8ad296 ("ovl: fix sgid on directory") which
overrides the cred's fsuid/fsgid using the new inode. The new inode's
owner is initialized by inode_init_owner(), and inode->fsuid is
assigned to the current user. So the override fsuid becomes the
current user. We know link() is actually modifying the directory, so
the caller must have the MAY_WRITE permission on the directory. The
current caller may should have this permission. This is acceptable
to use the caller's fsuid.

The second is commit 51f7e52dc943 ("ovl: share inode for hard link")
which removed the inode creation in ovl_link(). This commit move
inode_init_owner() into ovl_create_object(), so the ovl_link() just
give the old inode to ovl_create_or_link(). Then the override fsuid
becomes the old inode's fsuid, neither the caller nor the overlay's
creator! So this is incorrect.

Fix this bug by using current fsuid/fsgid to do underlying fs's link().

Link: https://lore.kernel.org/all/20220817102951.xnvesg3a7rbv576x@wittgenstein/T

Signed-off-by: Zhang Tianci <zhangtianci.1997@...edance.com>
Signed-off-by: Jiachen Zhang <zhangjiachen.jaycee@...edance.com>
Signed-off-by: Christian Brauner (Microsoft) <brauner@...nel.org>
---
 fs/overlayfs/dir.c       | 16 +++++++++++-----
 fs/overlayfs/overlayfs.h |  2 ++
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 6b03457f72bb..dd84e6fc5f6e 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -595,8 +595,8 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
 	err = -ENOMEM;
 	override_cred = prepare_creds();
 	if (override_cred) {
-		override_cred->fsuid = inode->i_uid;
-		override_cred->fsgid = inode->i_gid;
+		override_cred->fsuid = attr->fsuid;
+		override_cred->fsgid = attr->fsgid;
 		if (!attr->hardlink) {
 			err = security_dentry_create_files_as(dentry,
 					attr->mode, &dentry->d_name, old_cred,
@@ -646,6 +646,8 @@ static int ovl_create_object(struct dentry *dentry, int mode, dev_t rdev,
 	inode_init_owner(&init_user_ns, inode, dentry->d_parent->d_inode, mode);
 	attr.mode = inode->i_mode;
 
+	attr.fsuid = inode->i_uid;
+	attr.fsgid = inode->i_gid;
 	err = ovl_create_or_link(dentry, inode, &attr, false);
 	/* Did we end up using the preallocated inode? */
 	if (inode != d_inode(dentry))
@@ -702,6 +704,7 @@ static int ovl_link(struct dentry *old, struct inode *newdir,
 {
 	int err;
 	struct inode *inode;
+	struct ovl_cattr attr;
 
 	err = ovl_want_write(old);
 	if (err)
@@ -728,9 +731,12 @@ static int ovl_link(struct dentry *old, struct inode *newdir,
 	inode = d_inode(old);
 	ihold(inode);
 
-	err = ovl_create_or_link(new, inode,
-			&(struct ovl_cattr) {.hardlink = ovl_dentry_upper(old)},
-			ovl_type_origin(old));
+	attr = (struct ovl_cattr) {
+		.hardlink = ovl_dentry_upper(old),
+		.fsuid = current_fsuid(),
+		.fsgid = current_fsgid(),
+	};
+	err = ovl_create_or_link(new, inode, &attr, ovl_type_origin(old));
 	if (err)
 		iput(inode);
 
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 87759165d32b..85043123a103 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -655,6 +655,8 @@ struct ovl_cattr {
 	umode_t mode;
 	const char *link;
 	struct dentry *hardlink;
+	kuid_t fsuid;
+	kgid_t fsgid;
 };
 
 #define OVL_CATTR(m) (&(struct ovl_cattr) { .mode = (m) })
-- 
2.32.1 (Apple Git-133)