From patchwork Mon Oct 2 14:38:20 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Eric W. Biederman" X-Patchwork-Id: 9980889 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 4177B602A0 for ; Mon, 2 Oct 2017 14:38:43 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id D38652880D for ; Mon, 2 Oct 2017 14:38:42 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id C821428863; Mon, 2 Oct 2017 14:38:42 +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=-6.9 required=2.0 tests=BAYES_00,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 D9EA02880D for ; Mon, 2 Oct 2017 14:38:41 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751193AbdJBOil (ORCPT ); Mon, 2 Oct 2017 10:38:41 -0400 Received: from out02.mta.xmission.com ([166.70.13.232]:38195 "EHLO out02.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751067AbdJBOij (ORCPT ); Mon, 2 Oct 2017 10:38:39 -0400 Received: from in02.mta.xmission.com ([166.70.13.52]) by out02.mta.xmission.com with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.87) (envelope-from ) id 1dz1rh-0000Q4-Dp; Mon, 02 Oct 2017 08:38:37 -0600 Received: from 67-3-200-44.omah.qwest.net ([67.3.200.44] helo=x220.xmission.com) by in02.mta.xmission.com with esmtpsa (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.87) (envelope-from ) id 1dz1rb-0007ih-7Y; Mon, 02 Oct 2017 08:38:37 -0600 From: ebiederm@xmission.com (Eric W. Biederman) To: Stephen Smalley Cc: linux-security-module@vger.kernel.org, Linux Containers , Serge Hallyn , Chris Wright , James Morris , Paul Moore , Eric Paris , Casey Schaufler References: <87tvzmqwoi.fsf@xmission.com> <1506688601.5571.1.camel@tycho.nsa.gov> Date: Mon, 02 Oct 2017 09:38:20 -0500 In-Reply-To: <1506688601.5571.1.camel@tycho.nsa.gov> (Stephen Smalley's message of "Fri, 29 Sep 2017 08:36:41 -0400") Message-ID: <873771ipib.fsf_-_@xmission.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) MIME-Version: 1.0 X-XM-SPF: eid=1dz1rb-0007ih-7Y; ; ; mid=<873771ipib.fsf_-_@xmission.com>; ; ; hst=in02.mta.xmission.com; ; ; ip=67.3.200.44; ; ; frm=ebiederm@xmission.com; ; ; spf=neutral X-XM-AID: U2FsdGVkX1/ey0KaVM0WgWamgzw37/rg2DwE68aTgNc= X-SA-Exim-Connect-IP: 67.3.200.44 X-SA-Exim-Mail-From: ebiederm@xmission.com Subject: [PATCH] selinux: Perform both commoncap and selinux xattr checks X-SA-Exim-Version: 4.2.1 (built Thu, 05 May 2016 13:38:54 -0600) X-SA-Exim-Scanned: Yes (on in02.mta.xmission.com) Sender: owner-linux-security-module@vger.kernel.org Precedence: bulk List-ID: X-Virus-Scanned: ClamAV using ClamSMTP When selinux is loaded the relax permission checks for writing security.capable are not honored. Which keeps file capabilities from being used in user namespaces. Stephen Smalley writes: > Originally SELinux called the cap functions directly since there was no > stacking support in the infrastructure and one had to manually stack a > secondary module internally. inode_setxattr and inode_removexattr > however were special cases because the cap functions would check > CAP_SYS_ADMIN for any non-capability attributes in the security.* > namespace, and we don't want to impose that requirement on setting > security.selinux. Thus, we inlined the capabilities logic into the > selinux hook functions and adapted it appropriately. Now that the permission checks in commoncap have evolved this inlining of their contents has become a problem. So restructure selinux_inode_removexattr, and selinux_inode_setxattr to call both the corresponding cap_inode_ function and dentry_has_perm when the attribute is not a selinux security xattr. This ensures the policies of both commoncap and selinux are enforced. This results in smack and selinux having the same basic structure for setxattr and removexattr. Performing their own special permission checks when it is their modules xattr being written to, and deferring to commoncap when that is not the case. Then finally performing their generic module policy on all xattr writes. This structure is fine when you only consider stacking with the commoncap lsm, but it becomes a problem if two lsms that don't want the commoncap security checks on their own attributes need to be stack. This means there will need to be updates in the future as lsm stacking is improved, but at least now the structure between smack and selinux is common making the code easier to refactor. This change also has the effect that selinux_linux_setotherxattr becomes unnecessary so it is removed. Fixes: 8db6c34f1dbc ("Introduce v3 namespaced file capabilities") Fixes: 7bbf0e052b76 ("[PATCH] selinux merge") Historical Tree: https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git Signed-off-by: "Eric W. Biederman" Reviewed-by: Serge Hallyn Acked-by: Stephen Smalley --- While this fixes some things this isn't a regression so it should be able to wait until the next merge window to be merged. Would you like to take this through the selinux tree? Or shall I take it through mine? security/selinux/hooks.c | 43 ++++++++++++++++++------------------------- 1 file changed, 18 insertions(+), 25 deletions(-) diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index f5d304736852..c78dbec627f6 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -3124,27 +3124,6 @@ static int selinux_inode_getattr(const struct path *path) return path_has_perm(current_cred(), path, FILE__GETATTR); } -static int selinux_inode_setotherxattr(struct dentry *dentry, const char *name) -{ - const struct cred *cred = current_cred(); - - if (!strncmp(name, XATTR_SECURITY_PREFIX, - sizeof XATTR_SECURITY_PREFIX - 1)) { - if (!strcmp(name, XATTR_NAME_CAPS)) { - if (!capable(CAP_SETFCAP)) - return -EPERM; - } else if (!capable(CAP_SYS_ADMIN)) { - /* A different attribute in the security namespace. - Restrict to administrator. */ - return -EPERM; - } - } - - /* Not an attribute we recognize, so just check the - ordinary setattr permission. */ - return dentry_has_perm(cred, dentry, FILE__SETATTR); -} - static bool has_cap_mac_admin(bool audit) { const struct cred *cred = current_cred(); @@ -3167,8 +3146,15 @@ static int selinux_inode_setxattr(struct dentry *dentry, const char *name, u32 newsid, sid = current_sid(); int rc = 0; - if (strcmp(name, XATTR_NAME_SELINUX)) - return selinux_inode_setotherxattr(dentry, name); + if (strcmp(name, XATTR_NAME_SELINUX)) { + rc = cap_inode_setxattr(dentry, name, value, size, flags); + if (rc) + return rc; + + /* Not an attribute we recognize, so just check the + ordinary setattr permission. */ + return dentry_has_perm(current_cred(), dentry, FILE__SETATTR); + } sbsec = inode->i_sb->s_security; if (!(sbsec->flags & SBLABEL_MNT)) @@ -3282,8 +3268,15 @@ static int selinux_inode_listxattr(struct dentry *dentry) static int selinux_inode_removexattr(struct dentry *dentry, const char *name) { - if (strcmp(name, XATTR_NAME_SELINUX)) - return selinux_inode_setotherxattr(dentry, name); + if (strcmp(name, XATTR_NAME_SELINUX)) { + int rc = cap_inode_removexattr(dentry, name); + if (rc) + return rc; + + /* Not an attribute we recognize, so just check the + ordinary setattr permission. */ + return dentry_has_perm(current_cred(), dentry, FILE__SETATTR); + } /* No one is allowed to remove a SELinux security label. You can change the label, but all data must be labeled. */