Received: by 2002:ac0:a594:0:0:0:0:0 with SMTP id m20-v6csp2951891imm; Thu, 24 May 2018 19:46:01 -0700 (PDT) X-Google-Smtp-Source: AB8JxZpkuhe9zB3cVIUtxsNgYJ0kC7d5DrMC+59FAYL9nlbYnoodV8KaVqY1WO56X2YxZ40knkR9 X-Received: by 2002:a63:81c7:: with SMTP id t190-v6mr477232pgd.378.1527216361881; Thu, 24 May 2018 19:46:01 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1527216361; cv=none; d=google.com; s=arc-20160816; b=cZ7WOqyqkZWVyHgiwZ0jaR7WIbcsMbgLkSvkBSSio99ASClGfN+2NEgH4l7lPrIrIt K75FLIunnURcyw4vjy7SIHQXcsZu6t7Qy+E53oPjchRrP0W7tjIT+CmFSerTKDjR318t z56X6p0hs1uZUyZ5aud+QqNoPN+z8bgO+NzmayzqvvEePVzV4gWpQRU7LjBX/QWHoatz xoAT1Gm9WJw6kX/v4YW3YFE2lCBDbr622a9gD+5tPzTpcOT/iyOrxoEWQIVizS9SrPxA 03A3/qYrRauLdsHpo9Y6uB/ye8682zXCGZtERAF47I39EHYV2EMWCPoFpi0gsNFk2Cg2 UKgg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:in-reply-to:content-disposition :mime-version:references:message-id:subject:cc:to:from:date :arc-authentication-results; bh=aENKUx2wPPf1iAs62GqN0n06U67xRycUvyfQN9WORVU=; b=BPyf3hoEriyk10Tdy4v6uczqG7k7hVyl1vk/6Z/sNoQzMleeoIO5PsCB5Tb4ExM50/ BJ9zXjo+TnQARXI+oZAF22Fjne/Wc3MWZ1wlnAleIX/PhGnP+7AhCSyueW3fIjXz/l7l NwzcbQBadkicQZLjrjP3dC8jHExHgpRzpUKOB4qWM/ZMTbvDx5ShndqGt6kK3/7+GIsI HWCC6Q+q6WVE2f92U31rGn10kXFQ5G7XvHXHCLBdbe4iQLABj7FoyOF7f4xqsAMfn1Rj 51faDA9jJnqvBh+4bTeUKn7w874Rgjmu0IpRBb6CdRYupIQB6WIE2c0SoItlEiaXOSL5 K59Q== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id p2-v6si13303893pgn.187.2018.05.24.19.45.47; Thu, 24 May 2018 19:46:01 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1034764AbeEXWaT (ORCPT + 99 others); Thu, 24 May 2018 18:30:19 -0400 Received: from mx1.mailbox.org ([80.241.60.212]:39896 "EHLO mx1.mailbox.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S966192AbeEXWaO (ORCPT ); Thu, 24 May 2018 18:30:14 -0400 Received: from smtp2.mailbox.org (smtp2.mailbox.org [80.241.60.241]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.mailbox.org (Postfix) with ESMTPS id E491F48CFD; Fri, 25 May 2018 00:30:11 +0200 (CEST) X-Virus-Scanned: amavisd-new at heinlein-support.de Received: from smtp2.mailbox.org ([80.241.60.241]) by spamfilter01.heinlein-hosting.de (spamfilter01.heinlein-hosting.de [80.241.56.115]) (amavisd-new, port 10030) with ESMTP id QPTDAWxPgjWh; Fri, 25 May 2018 00:30:10 +0200 (CEST) Date: Fri, 25 May 2018 00:30:08 +0200 From: Christian Brauner To: "Eric W. Biederman" Cc: Linux Containers , linux-fsdevel@vger.kernel.org, Seth Forshee , "Serge E. Hallyn" , linux-kernel@vger.kernel.org Subject: Re: [REVIEW][PATCH v2 3/6] fs: Allow superblock owner to replace invalid owners of inodes Message-ID: <20180524223008.GA17493@mailbox.org> References: <87o9h6554f.fsf@xmission.com> <20180523232538.4880-3-ebiederm@xmission.com> <87bmd6549i.fsf_-_@xmission.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <87bmd6549i.fsf_-_@xmission.com> Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, May 23, 2018 at 06:41:29PM -0500, Eric W. Biederman wrote: > > Allow users with CAP_SYS_CHOWN over the superblock of a filesystem to > chown files when inode owner is invalid. Ordinarily the > capable_wrt_inode_uidgid check is sufficient to allow access to files > but when the underlying filesystem has uids or gids that don't map to > the current user namespace it is not enough, so the chown permission > checks need to be extended to allow this case. > > Calling chown on filesystem nodes whose uid or gid don't map is > necessary if those nodes are going to be modified as writing back > inodes which contain uids or gids that don't map is likely to cause > filesystem corruption of the uid or gid fields. > > Once chown has been called the existing capable_wrt_inode_uidgid > checks are sufficient to allow the owner of a superblock to do anything > the global root user can do with an appropriate set of capabilities. > > An ordinary filesystem mountable by a userns root will limit all uids > and gids in s_user_ns or the INVALID_UID and INVALID_GID to flag all > others. So having this added permission limited to just INVALID_UID > and INVALID_GID is sufficient to handle every case on an ordinary filesystem. > > Of the virtual filesystems at least proc is known to set s_user_ns to > something other than &init_user_ns, while at the same time presenting > some files owned by GLOBAL_ROOT_UID. Those files the mounter of proc > in a user namespace should not be able to chown to get access to. > Limiting the relaxation in permission to just the minimum of allowing > changing INVALID_UID and INVALID_GID prevents problems with cases like > that. > > The original version of this patch was written by: Seth Forshee. I > have rewritten and rethought this patch enough so it's really not the > same thing (certainly it needs a different description), but he > deserves credit for getting out there and getting the conversation > started, and finding the potential gotcha's and putting up with my > semi-paranoid feedback. Ok, took me a little longer to reason about this. Acked-by: Christian Brauner > > Inspired-by: Seth Forshee > Acked-by: Seth Forshee > Signed-off-by: Eric W. Biederman > --- > > Sigh. In simplifying this change so it would not require a change to > proc (or any other similar filesystem) I accidentally introduced some > badly placed semicolons. The kbuild test robot was very nice and found > those for me. Resend with those unnecessary semicolons removed. > > fs/attr.c | 36 ++++++++++++++++++++++++++++-------- > 1 file changed, 28 insertions(+), 8 deletions(-) > > diff --git a/fs/attr.c b/fs/attr.c > index 12ffdb6fb63c..d0b4d34878fb 100644 > --- a/fs/attr.c > +++ b/fs/attr.c > @@ -18,6 +18,32 @@ > #include > #include > > +static bool chown_ok(const struct inode *inode, kuid_t uid) > +{ > + if (uid_eq(current_fsuid(), inode->i_uid) && > + uid_eq(uid, inode->i_uid)) > + return true; > + if (capable_wrt_inode_uidgid(inode, CAP_CHOWN)) > + return true; > + if (uid_eq(inode->i_uid, INVALID_UID) && > + ns_capable(inode->i_sb->s_user_ns, CAP_CHOWN)) > + return true; > + return false; > +} > + > +static bool chgrp_ok(const struct inode *inode, kgid_t gid) > +{ > + if (uid_eq(current_fsuid(), inode->i_uid) && > + (in_group_p(gid) || gid_eq(gid, inode->i_gid))) > + return true; > + if (capable_wrt_inode_uidgid(inode, CAP_CHOWN)) > + return true; > + if (gid_eq(inode->i_gid, INVALID_GID) && > + ns_capable(inode->i_sb->s_user_ns, CAP_CHOWN)) > + return true; > + return false; > +} > + > /** > * setattr_prepare - check if attribute changes to a dentry are allowed > * @dentry: dentry to check > @@ -52,17 +78,11 @@ int setattr_prepare(struct dentry *dentry, struct iattr *attr) > goto kill_priv; > > /* Make sure a caller can chown. */ > - if ((ia_valid & ATTR_UID) && > - (!uid_eq(current_fsuid(), inode->i_uid) || > - !uid_eq(attr->ia_uid, inode->i_uid)) && > - !capable_wrt_inode_uidgid(inode, CAP_CHOWN)) > + if ((ia_valid & ATTR_UID) && !chown_ok(inode, attr->ia_uid)) > return -EPERM; > > /* Make sure caller can chgrp. */ > - if ((ia_valid & ATTR_GID) && > - (!uid_eq(current_fsuid(), inode->i_uid) || > - (!in_group_p(attr->ia_gid) && !gid_eq(attr->ia_gid, inode->i_gid))) && > - !capable_wrt_inode_uidgid(inode, CAP_CHOWN)) > + if ((ia_valid & ATTR_GID) && !chgrp_ok(inode, attr->ia_gid)) > return -EPERM; > > /* Make sure a caller can chmod. */