Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754906AbcKNCGu (ORCPT ); Sun, 13 Nov 2016 21:06:50 -0500 Received: from shadbolt.e.decadent.org.uk ([88.96.1.126]:46288 "EHLO shadbolt.e.decadent.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754754AbcKNCG1 (ORCPT ); Sun, 13 Nov 2016 21:06:27 -0500 Content-Type: text/plain; charset="UTF-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit MIME-Version: 1.0 From: Ben Hutchings To: linux-kernel@vger.kernel.org, stable@vger.kernel.org CC: akpm@linux-foundation.org, "Christoph Hellwig" , "Jan Kara" Date: Mon, 14 Nov 2016 00:14:07 +0000 Message-ID: X-Mailer: LinuxStableQueue (scripts by bwh) Subject: [PATCH 3.2 146/152] fs: Avoid premature clearing of capabilities In-Reply-To: X-SA-Exim-Connect-IP: 2a02:8011:400e:2:6f00:88c8:c921:d332 X-SA-Exim-Mail-From: ben@decadent.org.uk X-SA-Exim-Scanned: No (on shadbolt.decadent.org.uk); SAEximRunCond expanded to false Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2096 Lines: 72 3.2.84-rc1 review patch. If anyone has any objections, please let me know. ------------------ From: Jan Kara commit 030b533c4fd4d2ec3402363323de4bb2983c9cee upstream. Currently, notify_change() clears capabilities or IMA attributes by calling security_inode_killpriv() before calling into ->setattr. Thus it happens before any other permission checks in inode_change_ok() and user is thus allowed to trigger clearing of capabilities or IMA attributes for any file he can look up e.g. by calling chown for that file. This is unexpected and can lead to user DoSing a system. Fix the problem by calling security_inode_killpriv() at the end of inode_change_ok() instead of from notify_change(). At that moment we are sure user has permissions to do the requested change. References: CVE-2015-1350 Reviewed-by: Christoph Hellwig Signed-off-by: Jan Kara Signed-off-by: Ben Hutchings --- fs/attr.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) --- a/fs/attr.c +++ b/fs/attr.c @@ -46,7 +46,7 @@ int setattr_prepare(struct dentry *dentr /* If force is set do it anyway. */ if (ia_valid & ATTR_FORCE) - return 0; + goto kill_priv; /* Make sure a caller can chown. */ if ((ia_valid & ATTR_UID) && @@ -77,6 +77,16 @@ int setattr_prepare(struct dentry *dentr return -EPERM; } +kill_priv: + /* User has permission for the change */ + if (ia_valid & ATTR_KILL_PRIV) { + int error; + + error = security_inode_killpriv(dentry); + if (error) + return error; + } + return 0; } EXPORT_SYMBOL(setattr_prepare); @@ -199,13 +209,11 @@ int notify_change(struct dentry * dentry if (!(ia_valid & ATTR_MTIME_SET)) attr->ia_mtime = now; if (ia_valid & ATTR_KILL_PRIV) { - attr->ia_valid &= ~ATTR_KILL_PRIV; - ia_valid &= ~ATTR_KILL_PRIV; error = security_inode_need_killpriv(dentry); - if (error > 0) - error = security_inode_killpriv(dentry); - if (error) + if (error < 0) return error; + if (error == 0) + ia_valid = attr->ia_valid &= ~ATTR_KILL_PRIV; } /*