Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   ebiederm@xmission.com (Eric W. Biederman)
To:     Christian Brauner <christian.brauner@ubuntu.com>
Cc:     Linus Torvalds <torvalds@linux-foundation.org>,
        linux-kernel@vger.kernel.org
References: <20210213130042.828076-1-christian.brauner@ubuntu.com>
Date:   Wed, 03 Mar 2021 13:36:31 -0600
In-Reply-To: <20210213130042.828076-1-christian.brauner@ubuntu.com> (Christian
        Brauner's message of "Sat, 13 Feb 2021 14:00:42 +0100")
Message-ID: <m18s7481xc.fsf@fess.ebiederm.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [GIT PULL] idmapped mounts for v5.12
Precedence: bulk

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

> Hi Linus,

> This series comes with an extensive xfstests suite covering both ext4 and xfs
> https://git.kernel.org/brauner/xfstests-dev/h/idmapped_mounts
> It covers truncation, creation, opening, xattrs, vfscaps, setid execution,
> setgid inheritance and more both with idmapped and non-idmapped mounts.
> It already helped to discover an unrelated xfs setgid inheritance bug which has
> since been fixed in mainline. It will be sent for inclusion with the xfstests
> project should you decide to merge this.

And yet chown is broken (details below), and in a very predictable way.

This is not considering that the entire concept is giving people a
loaded footgun, that is very difficult to use safely.


When the user namespace was implemented the two kinds of uids were very
carefully separated from each other by type, so it would be take
deliberate action to mix them.  These changes introduces a third type
of uid and does not use the type system to keep them separate.  In just
a little bit of looking since I realized this problem I have found two
bugs in chown where the wrong values are compared.

We now have the following types of uids and gids:
- The userspace values.
- The kernel values that are used for comparisons.
  (The old fashioned kuid_t and kgid_t)
- The values used for interfacing with the filesystems
  underneath a mount.
  (The beneath mount kuid_t and kgid_t)
- The values stored in the filesystem.

The third type is new, and the code mixes old fashioned kuid_t and
kgid_t with the below mount kuid_t and kgid_t.

Starting with chown_common the code does:

int chown_common(const struct path *path, uid_t user, gid_t group)
{
	...
	uid = make_kuid(current_user_ns(), user);
	gid = make_kgid(current_user_ns(), group);

	mnt_userns = mnt_user_ns(path->mnt);
	uid = kuid_from_mnt(mnt_userns, uid);
	gid = kgid_from_mnt(mnt_userns, gid);

retry_deleg:
	newattrs.ia_valid =  ATTR_CTIME;
	if (user != (uid_t) -1) {
		if (!uid_valid(uid))
			return -EINVAL;
		newattrs.ia_valid |= ATTR_UID;
		newattrs.ia_uid = uid;
	}
	if (group != (gid_t) -1) {
		if (!gid_valid(gid))
			return -EINVAL;
		newattrs.ia_valid |= ATTR_GID;
		newattrs.ia_gid = gid;
	}
	if (!S_ISDIR(inode->i_mode))
		newattrs.ia_valid |=
			ATTR_KILL_SUID | ATTR_KILL_SGID | ATTR_KILL_PRIV;
	inode_lock(inode);
	error = security_path_chown(path, uid, gid);
	if (!error)
		error = notify_change(mnt_userns, path->dentry, &newattrs,
				      &delegated_inode);
	inode_unlock(inode);
	...
}

Here security_path_chown is expecting the old fashioned kuid_t and
kgid_t but looking at the top of the function we can see that
security_path_chown is getting the kuid_t and kgid_t from below the
mount.

The Tomoyo lsm cares.


Notice that ia_uid and ia_gid in struct newattrs are below mount values.


Now looking in notify_change:

int notify_change(struct user_namespace *mnt_userns, struct dentry *dentry,
		  struct iattr *attr, struct inode **delegated_inode)
{
	...
       	if (inode->i_op->setattr)
		error = inode->i_op->setattr(mnt_userns, dentry, attr);
	else
		error = simple_setattr(mnt_userns, dentry, attr);
        ...
}


int simple_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
		   struct iattr *iattr)
{
	...
	error = setattr_prepare(mnt_userns, dentry, iattr);
	if (error)
		return error;
	...
}


int setattr_prepare(struct user_namespace *mnt_userns, struct dentry *dentry,
		    struct iattr *attr)
{
	...
	/* Make sure a caller can chown. */
	if ((ia_valid & ATTR_UID) && !chown_ok(mnt_userns, inode, attr->ia_uid))
		return -EPERM;

	/* Make sure caller can chgrp. */
	if ((ia_valid & ATTR_GID) && !chgrp_ok(mnt_userns, inode, attr->ia_gid))
		return -EPERM;
	...
}

static bool chown_ok(struct user_namespace *mnt_userns,
		     const struct inode *inode,
		     kuid_t uid)
{
	kuid_t kuid = i_uid_into_mnt(mnt_userns, inode);
	if (uid_eq(current_fsuid(), kuid) && uid_eq(uid, kuid))
                                             ^^^^^^^^^^^^^^^^^^
		return true;
 	....               
}

The comparison of uid and kuid in chown_ok is nonsense.  As the kuid is
the old fashioned kuid.  While the uid is attr->ia_uid is the below
mount value.

I found these both within just a couple of minutes by creating
a type vfsuid_t and vfsgid_t and using it for the values stored in
struct inode and struct iattr.

There are probably more cases of inappropriate mixing.  I stopped as I
don't have the energy or the inclination to dig through and find more.

Unfortunately what I found was that the current design of using kuid_t
and kgid_t for both the old fashioned kuids and for the new below mount
values is so error prone that even the author of the change while
performing a lot of testing can not get it right.

Christian do you think you can add a type for the below mount values
and separate everything by type quickly?

Otherwise I think the better part of valor would be to revert this code
and come back it has made not error prone.

Eric