From patchwork Fri Dec 16 17:41:12 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Stephen Smalley X-Patchwork-Id: 9478133 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 18727607EE for ; Fri, 16 Dec 2016 17:39:50 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 0A81B288B7 for ; Fri, 16 Dec 2016 17:39:50 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id F0D75288C4; Fri, 16 Dec 2016 17:39:49 +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 2D88F288B7 for ; Fri, 16 Dec 2016 17:39:49 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753455AbcLPRjs (ORCPT ); Fri, 16 Dec 2016 12:39:48 -0500 Received: from emsm-gh1-uea11.nsa.gov ([8.44.101.9]:44411 "EHLO emsm-gh1-uea11.nsa.gov" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751359AbcLPRjr (ORCPT ); Fri, 16 Dec 2016 12:39:47 -0500 X-IronPort-AV: E=Sophos;i="5.33,358,1477958400"; d="scan'208";a="1653075" IronPort-PHdr: =?us-ascii?q?9a23=3AbhlCQxdY5jWUXQ4owAfng6GwlGMj4u6mDksu8pMi?= =?us-ascii?q?zoh2WeGdxc26ZhGN2/xhgRfzUJnB7Loc0qyN4vumBjVLuM7b+Fk5M7V0Hycfjs?= =?us-ascii?q?sXmwFySOWkMmbcaMDQUiohAc5ZX0Vk9XzoeWJcGcL5ekGA6ibqtW1aFRrwLxd6?= =?us-ascii?q?KfroEYDOkcu3y/qy+5rOaAlUmTaxe71/IRG4oAnLtMQanIRuJrstxhfXv3BFZ/?= =?us-ascii?q?lYyWR0KFyJgh3y/N2w/Jlt8yRRv/Iu6ctNWrjkcqo7ULJVEi0oP3g668P3uxbD?= =?us-ascii?q?SxCP5mYHXWUNjhVIGQnF4wrkUZr3ryD3q/By2CiePc3xULA0RTGv5LplRRP0lC?= =?us-ascii?q?sKMSMy/2/Nisx0kalVvhSvqRJiyILQeY2ZKuZycqbbcNgHR2ROQ9xRWjRBDI2i?= =?us-ascii?q?coUBAekPM+FaoInzvFYCsQeyCBOwCO711jNEmnn71rA63eQ7FgHG2RQtEc8SsH?= =?us-ascii?q?vKtNX1NLkdUeaox6fVyDXMdfdW2TPj54nIbxsspuqMUq9rccfK1UkuFx/KjlWX?= =?us-ascii?q?qYD/OTOVzf4Cv3KU7+pnS+KikmgqoBxyrDi33sogl4bEi40Pxl3E6Cl12pg5KN?= =?us-ascii?q?KmREJhfNKpFoZbuTuAOItsWMwiRnlluCM9yrIbp5G2ZDMKyJE7xx7HbPyHbpSI?= =?us-ascii?q?7grjVOmPJTd4g2poeK6liBao8Eig1/b8WtOo0FdKsiVFkt7MumoL1xPP8ciIVu?= =?us-ascii?q?Fx/kKg2TaLzwzT6+dELl4olafDNpIszbE9moATvEjeBCP6hkr7gLGMekk54uSo?= =?us-ascii?q?7v7oYrTipp+SLY90jQT+P7w1msOiGuQ1KRQOXmiH9uS8073v50v5QK5QgfEsna?= =?us-ascii?q?nZt47aKdwBpqGlGw9Vzpoj6xGnAjek19QYnX8HIEhHeBKAj4jmIVfOIOvmAve5?= =?us-ascii?q?mFmjjC1kx/bBPr3nA5XCMmLMkLP7cblh7E5czRI5zcpD6JJMFrEBPPXzV1ftu9?= =?us-ascii?q?PCFR82LQy1zv38CNph1oMRQ3+PAqGdMKzMq1+E//4gLPOWaIAJvzb9LuAv5+Ty?= =?us-ascii?q?gn8hhV8dYa6p0IMTaHC5GPRmPkqYbWP3gtgfDWgKoxA+TO32iFyCSDJTYnGyUL?= =?us-ascii?q?8h5jE/Fo2rFpnDRo+zj7ybxiu7HYNZZnpACl+SFXfkbYKEW+0DaCiKOM9ujiQE?= =?us-ascii?q?VaS9S48mzRyuthX1y795IerP4CEYsYjv1N1y5+3JjxEy9Cd0At+a02GXVW57gm?= =?us-ascii?q?cISCEs0K9jpkx9z0+J0bJkjPxACdxT+/RJXx8iNZHG0ux6D8v/WhrbcdeUTFaq?= =?us-ascii?q?W9CmATY2TtIr3dACeVpyG9KnjkOL4y3/GLIRlrqWFLQo46nc2D73PM87xHHYh4?= =?us-ascii?q?c7iFxzeddCLW2rgOZE8gHXA4PY2xGCm72CabUX3CmL8nyKi2WJohcLA0ZLTazZ?= =?us-ascii?q?UCVHNQPtptPj6xaHFeej?= X-IPAS-Result: =?us-ascii?q?A2GtAwCRJlRY/wHyM5BcGgEBAQECAQEBAQgBAQEBFQEBAQE?= =?us-ascii?q?CAQEBAQgBAQEBgwwBAQEBAR+BUBC3I4QYGoYIAoIYUwEBAQEBAQEBAgECXyiCM?= =?us-ascii?q?xqCHAYnUhAYOVcZiGuqIjwqAop0AQEBAQEFAQEBASSSGwuDCgWIYoYffYpykTU?= =?us-ascii?q?CijOGGJIoV3wFAhEOg30cGIFjIDSIfQEBAQ?= Received: from unknown (HELO tarius.tycho.ncsc.mil) ([144.51.242.1]) by emsm-gh1-uea11.nsa.gov with ESMTP; 16 Dec 2016 17:39:46 +0000 Received: from moss-pluto.infosec.tycho.ncsc.mil (moss-pluto [192.168.25.131]) by tarius.tycho.ncsc.mil (8.14.4/8.14.4) with ESMTP id uBGHcLrX009568; Fri, 16 Dec 2016 12:39:45 -0500 From: Stephen Smalley To: selinux@tycho.nsa.gov Cc: paul@paul-moore.com, james.l.morris@oracle.com, linux-security-module@vger.kernel.org, casey@schaufler-ca.com, john.johansen@canonical.com, Stephen Smalley Subject: [PATCH 2/2] proc, security: move restriction on writing /proc/pid/attr nodes to proc Date: Fri, 16 Dec 2016 12:41:12 -0500 Message-Id: <1481910072-11392-2-git-send-email-sds@tycho.nsa.gov> X-Mailer: git-send-email 2.7.4 In-Reply-To: <1481910072-11392-1-git-send-email-sds@tycho.nsa.gov> References: <1481910072-11392-1-git-send-email-sds@tycho.nsa.gov> Sender: owner-linux-security-module@vger.kernel.org Precedence: bulk List-ID: X-Virus-Scanned: ClamAV using ClamSMTP Processes can only alter their own security attributes via /proc/pid/attr nodes. This is presently enforced by each individual security module and is also imposed by the Linux credentials implementation, which only allows a task to alter its own credentials. Move the check enforcing this restriction from the individual security modules to proc_pid_attr_write() before calling the security hook, and drop the unnecessary task argument to the security hook since it can only ever be the current task. Signed-off-by: Stephen Smalley Acked-by: Casey Schaufler Acked-by: John Johansen Acked-by: Paul Moore --- fs/proc/base.c | 13 +++++++++---- include/linux/lsm_hooks.h | 3 +-- include/linux/security.h | 4 ++-- security/apparmor/lsm.c | 7 ++----- security/security.c | 4 ++-- security/selinux/hooks.c | 13 +------------ security/smack/smack_lsm.c | 11 +---------- 7 files changed, 18 insertions(+), 37 deletions(-) diff --git a/fs/proc/base.c b/fs/proc/base.c index 6eae4d0..7b228ea 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -2485,6 +2485,12 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf, length = -ESRCH; if (!task) goto out_no_task; + + /* A task may only write its own attributes. */ + length = -EACCES; + if (current != task) + goto out; + if (count > PAGE_SIZE) count = PAGE_SIZE; @@ -2500,14 +2506,13 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf, } /* Guard against adverse ptrace interaction */ - length = mutex_lock_interruptible(&task->signal->cred_guard_mutex); + length = mutex_lock_interruptible(¤t->signal->cred_guard_mutex); if (length < 0) goto out_free; - length = security_setprocattr(task, - (char*)file->f_path.dentry->d_name.name, + length = security_setprocattr(file->f_path.dentry->d_name.name, page, count); - mutex_unlock(&task->signal->cred_guard_mutex); + mutex_unlock(¤t->signal->cred_guard_mutex); out_free: kfree(page); out: diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index 558adfa..0dde959 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -1547,8 +1547,7 @@ union security_list_options { void (*d_instantiate)(struct dentry *dentry, struct inode *inode); int (*getprocattr)(struct task_struct *p, char *name, char **value); - int (*setprocattr)(struct task_struct *p, char *name, void *value, - size_t size); + int (*setprocattr)(const char *name, void *value, size_t size); int (*ismaclabel)(const char *name); int (*secid_to_secctx)(u32 secid, char **secdata, u32 *seclen); int (*secctx_to_secid)(const char *secdata, u32 seclen, u32 *secid); diff --git a/include/linux/security.h b/include/linux/security.h index c2125e9..f4ebac1 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -361,7 +361,7 @@ int security_sem_semop(struct sem_array *sma, struct sembuf *sops, unsigned nsops, int alter); void security_d_instantiate(struct dentry *dentry, struct inode *inode); int security_getprocattr(struct task_struct *p, char *name, char **value); -int security_setprocattr(struct task_struct *p, char *name, void *value, size_t size); +int security_setprocattr(const char *name, void *value, size_t size); int security_netlink_send(struct sock *sk, struct sk_buff *skb); int security_ismaclabel(const char *name); int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen); @@ -1106,7 +1106,7 @@ static inline int security_getprocattr(struct task_struct *p, char *name, char * return -EINVAL; } -static inline int security_setprocattr(struct task_struct *p, char *name, void *value, size_t size) +static inline int security_setprocattr(char *name, void *value, size_t size) { return -EINVAL; } diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c index 41b8cb1..8202e55 100644 --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c @@ -495,8 +495,8 @@ static int apparmor_getprocattr(struct task_struct *task, char *name, return error; } -static int apparmor_setprocattr(struct task_struct *task, char *name, - void *value, size_t size) +static int apparmor_setprocattr(const char *name, void *value, + size_t size) { struct common_audit_data sa; struct apparmor_audit_data aad = {0,}; @@ -506,9 +506,6 @@ static int apparmor_setprocattr(struct task_struct *task, char *name, if (size == 0) return -EINVAL; - /* task can only write its own attributes */ - if (current != task) - return -EACCES; /* AppArmor requires that the buffer must be null terminated atm */ if (args[size - 1] != '\0') { diff --git a/security/security.c b/security/security.c index f825304..32052f5 100644 --- a/security/security.c +++ b/security/security.c @@ -1170,9 +1170,9 @@ int security_getprocattr(struct task_struct *p, char *name, char **value) return call_int_hook(getprocattr, -EINVAL, p, name, value); } -int security_setprocattr(struct task_struct *p, char *name, void *value, size_t size) +int security_setprocattr(const char *name, void *value, size_t size) { - return call_int_hook(setprocattr, -EINVAL, p, name, value, size); + return call_int_hook(setprocattr, -EINVAL, name, value, size); } int security_netlink_send(struct sock *sk, struct sk_buff *skb) diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 9992626..762276b 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -5859,8 +5859,7 @@ static int selinux_getprocattr(struct task_struct *p, return error; } -static int selinux_setprocattr(struct task_struct *p, - char *name, void *value, size_t size) +static int selinux_setprocattr(const char *name, void *value, size_t size) { struct task_security_struct *tsec; struct cred *new; @@ -5868,16 +5867,6 @@ static int selinux_setprocattr(struct task_struct *p, int error; char *str = value; - if (current != p) { - /* - * A task may only alter its own credentials. - * SELinux has always enforced this restriction, - * and it is now mandated by the Linux credentials - * infrastructure; see Documentation/security/credentials.txt. - */ - return -EACCES; - } - /* * Basic control over ability to set these attributes at all. */ diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index 4d90257..9bde7f8 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -3620,7 +3620,6 @@ static int smack_getprocattr(struct task_struct *p, char *name, char **value) /** * smack_setprocattr - Smack process attribute setting - * @p: the object task * @name: the name of the attribute in /proc/.../attr * @value: the value to set * @size: the size of the value @@ -3630,8 +3629,7 @@ static int smack_getprocattr(struct task_struct *p, char *name, char **value) * * Returns the length of the smack label or an error code */ -static int smack_setprocattr(struct task_struct *p, char *name, - void *value, size_t size) +static int smack_setprocattr(const char *name, void *value, size_t size) { struct task_smack *tsp = current_security(); struct cred *new; @@ -3639,13 +3637,6 @@ static int smack_setprocattr(struct task_struct *p, char *name, struct smack_known_list_elem *sklep; int rc; - /* - * Changing another process' Smack value is too dangerous - * and supports no sane use case. - */ - if (p != current) - return -EPERM; - if (!smack_privileged(CAP_MAC_ADMIN) && list_empty(&tsp->smk_relabel)) return -EPERM;