Re: [GIT PULL] idmapped mounts for v5.12
Christian Brauner 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, , _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
Re: [GIT PULL] idmapped mounts for v5.12
On Wed, Mar 03, 2021 at 01:36:31PM -0600, Eric W. Biederman wrote: > Christian Brauner writes: > > > Hi Linus, Hi Eric, > > > 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 increadibly poor timing, there were 6 versions of this patchset published over several months and you were Cced on all of them. All that came from you during that time were a couple of odd comments. If chown is broken please give us a specific reproducer for when it yields the wrong ownership so we can fix it and add this as a test-case to the testsuite so it never breaks again. > > This is not considering that the entire concept is giving people a > loaded footgun, that is very difficult to use safely. The concept has seen a lot of interest by a lot of users during development of this patchset and is already being integrated in container runtimes and other tools by people who understand its behavior and implication. And fwiw, by this argument we simply should have never done user namespaces or unprivileged filesystem mounts too. > > > 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 I don't agree. This causes more confusion then it solves imho and is the whole basis for your argument. This reads a bit confusing to me. > 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) I don't see why we would need yet another type for this. It is simply a remapped or shifted kuid. A third type would introduce more confusion most likely but I'm open to it if you have a clear idea what you want and why you want it. It slightly feels like a strawman distinction to push for the revert. > - 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, , > _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. As it should. The idmapping of the mount is authorative. This attack only makes sense if you assume that there is a third type. > > The Tomoyo lsm cares. Please explain or send a patch to fix Tomoyo. > > > 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); >
Re: [GIT PULL] idmapped mounts for v5.12
The pull request you sent on Sat, 13 Feb 2021 14:00:42 +0100: > g...@gitolite.kernel.org:pub/scm/linux/kernel/git/brauner/linux > tags/idmapped-mounts-merged-v5.12 has been merged into torvalds/linux.git: https://git.kernel.org/torvalds/c/7d6beb71da3cc033649d641e1e608713b8220290 Thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/prtracker.html