From patchwork Fri Dec 22 14:32:27 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dongsu Park X-Patchwork-Id: 10130547 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 391D760318 for ; Fri, 22 Dec 2017 14:31:34 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 2FF5629FA9 for ; Fri, 22 Dec 2017 14:31:34 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 19DA629FE9; Fri, 22 Dec 2017 14:31:34 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.0 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 8B23329FA9 for ; Fri, 22 Dec 2017 14:31:32 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756201AbdLVOb3 (ORCPT ); Fri, 22 Dec 2017 09:31:29 -0500 Received: from mail-wm0-f68.google.com ([74.125.82.68]:35516 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756076AbdLVObY (ORCPT ); Fri, 22 Dec 2017 09:31:24 -0500 Received: by mail-wm0-f68.google.com with SMTP id f9so22110387wmh.0 for ; Fri, 22 Dec 2017 06:31:23 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kinvolk.io; s=google; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=nBYTsyRz9sHsQsV21hMOPWT2Wh6kdzxr9cnSDaVapoA=; b=bSIwcRxyzdHoFQpRSuu2zQAvghmaXMh1sJrtLL/lIDH9MT86Rz76ikSoAcFWYkgGDK vFrhsihXB7W7Vt4Bx0osmt+iJZ7qdiYACxsEfvFfpgeFZDHZtp7vEAbQ9NNNc4rBuPdL Q+LmQnD+IeAAbQU4NwSz9hIGKl0/+VtB6I1M4= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=nBYTsyRz9sHsQsV21hMOPWT2Wh6kdzxr9cnSDaVapoA=; b=tsN2KRcSbDs+2KiIG1y+9gYN2pKrh9iLr6iZLRqcgM4KOlr9wKnCBVj38MX4sRH9v2 oH1uqMm3DJKGQcVEQ9UevkyUazKv0BaYenqpzwfSKnUMSeOM7040GHBgjQLTH3vpQvLg aQEONpcqMP28E3QsSm6Oi3kyyoSf47YX96m848spr9BOwtD0oUVlShr/YRMbB9hLBlet hxGA9VZaeqzruaBmjV8dBHZYf28JZR2vYH+AEg92DxSytzZX6wLXWQChHhmV/tY0zR0u dGU4reil72jwZdl04CGukAJeDtxIGo2DArL+eYVmcF04K90C3dIdm4K08ZtDNs+KtJKx uXyw== X-Gm-Message-State: AKGB3mJwIUdjlQ0RlpSkXS3lFTq0hMU46G5EYZX9GqyX+snTWbeFMS2x O6tQ4fXfvgX+Qq9ZAAKVnVNlHQ== X-Google-Smtp-Source: ACJfBosmTTHBPktPyUOR0lKBW2IDD5lAPbpk6QfW50xwY9kKhfhP499Tny2th1swR3dO0tzQYM0FfQ== X-Received: by 10.80.135.226 with SMTP id 31mr3829383edz.210.1513953082730; Fri, 22 Dec 2017 06:31:22 -0800 (PST) Received: from dberlin.localdomain ([178.19.216.175]) by smtp.gmail.com with ESMTPSA id j39sm19698065ede.38.2017.12.22.06.31.21 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Fri, 22 Dec 2017 06:31:22 -0800 (PST) From: Dongsu Park To: linux-kernel@vger.kernel.org Cc: containers@lists.linux-foundation.org, Alban Crequy , "Eric W . Biederman" , Miklos Szeredi , Seth Forshee , Sargun Dhillon , Dongsu Park , linux-fsdevel@vger.kernel.org, Alexander Viro , "Luis R. Rodriguez" , Kees Cook Subject: [PATCH 03/11] fs: Allow superblock owner to change ownership of inodes Date: Fri, 22 Dec 2017 15:32:27 +0100 Message-Id: X-Mailer: git-send-email 2.13.6 In-Reply-To: References: Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP From: Eric W. Biederman Allow users with CAP_SYS_CHOWN over the superblock of a filesystem to chown files. 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. For the proc filesystem this relaxation of permissions is not safe, as some files are owned by users (particularly GLOBAL_ROOT_UID) outside of the control of the mounter of the proc and that would be unsafe to grant chown access to. So update setattr on proc to disallow changing files whose uids or gids are outside of proc's s_user_ns. 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. Patch v4 is available: https://patchwork.kernel.org/patch/8944611/ Cc: linux-fsdevel@vger.kernel.org Cc: linux-kernel@vger.kernel.org Cc: Alexander Viro Cc: "Luis R. Rodriguez" Cc: Kees Cook Inspired-by: Seth Forshee Signed-off-by: Eric W. Biederman [saf: Resolve conflicts caused by s/inode_change_ok/setattr_prepare/] Signed-off-by: Dongsu Park Reviewed-by: Serge Hallyn --- fs/attr.c | 34 ++++++++++++++++++++++++++-------- fs/proc/base.c | 7 +++++++ fs/proc/generic.c | 7 +++++++ fs/proc/proc_sysctl.c | 7 +++++++ 4 files changed, 47 insertions(+), 8 deletions(-) diff --git a/fs/attr.c b/fs/attr.c index 12ffdb6f..bf8e94f3 100644 --- a/fs/attr.c +++ b/fs/attr.c @@ -18,6 +18,30 @@ #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 (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 (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 +76,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. */ diff --git a/fs/proc/base.c b/fs/proc/base.c index 31934cb9..9d50ec92 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -665,10 +665,17 @@ int proc_setattr(struct dentry *dentry, struct iattr *attr) { int error; struct inode *inode = d_inode(dentry); + struct user_namespace *s_user_ns; if (attr->ia_valid & ATTR_MODE) return -EPERM; + /* Don't let anyone mess with weird proc files */ + s_user_ns = inode->i_sb->s_user_ns; + if (!kuid_has_mapping(s_user_ns, inode->i_uid) || + !kgid_has_mapping(s_user_ns, inode->i_gid)) + return -EPERM; + error = setattr_prepare(dentry, attr); if (error) return error; diff --git a/fs/proc/generic.c b/fs/proc/generic.c index 793a6757..527d46c8 100644 --- a/fs/proc/generic.c +++ b/fs/proc/generic.c @@ -106,8 +106,15 @@ static int proc_notify_change(struct dentry *dentry, struct iattr *iattr) { struct inode *inode = d_inode(dentry); struct proc_dir_entry *de = PDE(inode); + struct user_namespace *s_user_ns; int error; + /* Don't let anyone mess with weird proc files */ + s_user_ns = inode->i_sb->s_user_ns; + if (!kuid_has_mapping(s_user_ns, inode->i_uid) || + !kgid_has_mapping(s_user_ns, inode->i_gid)) + return -EPERM; + error = setattr_prepare(dentry, iattr); if (error) return error; diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c index c5cbbdff..0f9562d1 100644 --- a/fs/proc/proc_sysctl.c +++ b/fs/proc/proc_sysctl.c @@ -802,11 +802,18 @@ static int proc_sys_permission(struct inode *inode, int mask) static int proc_sys_setattr(struct dentry *dentry, struct iattr *attr) { struct inode *inode = d_inode(dentry); + struct user_namespace *s_user_ns; int error; if (attr->ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID)) return -EPERM; + /* Don't let anyone mess with weird proc files */ + s_user_ns = inode->i_sb->s_user_ns; + if (!kuid_has_mapping(s_user_ns, inode->i_uid) || + !kgid_has_mapping(s_user_ns, inode->i_gid)) + return -EPERM; + error = setattr_prepare(dentry, attr); if (error) return error;