Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755397AbcKNCMn (ORCPT ); Sun, 13 Nov 2016 21:12:43 -0500 Received: from shadbolt.e.decadent.org.uk ([88.96.1.126]:46289 "EHLO shadbolt.e.decadent.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754598AbcKNCGT (ORCPT ); Sun, 13 Nov 2016 21:06:19 -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, "Jeff Layton" , "Jan Kara" , "Christoph Hellwig" , "Andreas Gruenbacher" Date: Mon, 14 Nov 2016 00:14:07 +0000 Message-ID: X-Mailer: LinuxStableQueue (scripts by bwh) Subject: [PATCH 3.2 148/152] posix_acl: Clear SGID bit when setting file permissions 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: 10241 Lines: 370 3.2.84-rc1 review patch. If anyone has any objections, please let me know. ------------------ From: Jan Kara commit 073931017b49d9458aa351605b43a7e34598caef upstream. When file permissions are modified via chmod(2) and the user is not in the owning group or capable of CAP_FSETID, the setgid bit is cleared in inode_change_ok(). Setting a POSIX ACL via setxattr(2) sets the file permissions as well as the new ACL, but doesn't clear the setgid bit in a similar way; this allows to bypass the check in chmod(2). Fix that. References: CVE-2016-7097 Reviewed-by: Christoph Hellwig Reviewed-by: Jeff Layton Signed-off-by: Jan Kara Signed-off-by: Andreas Gruenbacher [bwh: Backported to 3.2: - Drop changes to ceph, f2fs, hfsplus, orangefs - Use capable() instead of capable_wrt_inode_uidgid() - Update ext3 and generic_acl.c as well - In gfs2, jfs, and xfs, take care to avoid leaking the allocated ACL if posix_acl_update_mode() determines it's not needed - Adjust context] Signed-off-by: Ben Hutchings --- --- a/fs/9p/acl.c +++ b/fs/9p/acl.c @@ -319,32 +319,26 @@ static int v9fs_xattr_set_acl(struct den case ACL_TYPE_ACCESS: name = POSIX_ACL_XATTR_ACCESS; if (acl) { - umode_t mode = inode->i_mode; - retval = posix_acl_equiv_mode(acl, &mode); - if (retval < 0) + struct iattr iattr; + + retval = posix_acl_update_mode(inode, &iattr.ia_mode, &acl); + if (retval) goto err_out; - else { - struct iattr iattr; - if (retval == 0) { - /* - * ACL can be represented - * by the mode bits. So don't - * update ACL. - */ - acl = NULL; - value = NULL; - size = 0; - } - /* Updte the mode bits */ - iattr.ia_mode = ((mode & S_IALLUGO) | - (inode->i_mode & ~S_IALLUGO)); - iattr.ia_valid = ATTR_MODE; - /* FIXME should we update ctime ? - * What is the following setxattr update the - * mode ? + if (!acl) { + /* + * ACL can be represented + * by the mode bits. So don't + * update ACL. */ - v9fs_vfs_setattr_dotl(dentry, &iattr); + value = NULL; + size = 0; } + iattr.ia_valid = ATTR_MODE; + /* FIXME should we update ctime ? + * What is the following setxattr update the + * mode ? + */ + v9fs_vfs_setattr_dotl(dentry, &iattr); } break; case ACL_TYPE_DEFAULT: --- a/fs/btrfs/acl.c +++ b/fs/btrfs/acl.c @@ -118,11 +118,9 @@ static int btrfs_set_acl(struct btrfs_tr case ACL_TYPE_ACCESS: name = POSIX_ACL_XATTR_ACCESS; if (acl) { - ret = posix_acl_equiv_mode(acl, &inode->i_mode); - if (ret < 0) + ret = posix_acl_update_mode(inode, &inode->i_mode, &acl); + if (ret) return ret; - if (ret == 0) - acl = NULL; } ret = 0; break; --- a/fs/ext2/acl.c +++ b/fs/ext2/acl.c @@ -194,15 +194,11 @@ ext2_set_acl(struct inode *inode, int ty case ACL_TYPE_ACCESS: name_index = EXT2_XATTR_INDEX_POSIX_ACL_ACCESS; if (acl) { - error = posix_acl_equiv_mode(acl, &inode->i_mode); - if (error < 0) + error = posix_acl_update_mode(inode, &inode->i_mode, &acl); + if (error) return error; - else { - inode->i_ctime = CURRENT_TIME_SEC; - mark_inode_dirty(inode); - if (error == 0) - acl = NULL; - } + inode->i_ctime = CURRENT_TIME_SEC; + mark_inode_dirty(inode); } break; --- a/fs/ext3/acl.c +++ b/fs/ext3/acl.c @@ -199,15 +199,11 @@ ext3_set_acl(handle_t *handle, struct in case ACL_TYPE_ACCESS: name_index = EXT3_XATTR_INDEX_POSIX_ACL_ACCESS; if (acl) { - error = posix_acl_equiv_mode(acl, &inode->i_mode); - if (error < 0) + error = posix_acl_update_mode(inode, &inode->i_mode, &acl); + if (error) return error; - else { - inode->i_ctime = CURRENT_TIME_SEC; - ext3_mark_inode_dirty(handle, inode); - if (error == 0) - acl = NULL; - } + inode->i_ctime = CURRENT_TIME_SEC; + ext3_mark_inode_dirty(handle, inode); } break; --- a/fs/ext4/acl.c +++ b/fs/ext4/acl.c @@ -198,15 +198,11 @@ ext4_set_acl(handle_t *handle, struct in case ACL_TYPE_ACCESS: name_index = EXT4_XATTR_INDEX_POSIX_ACL_ACCESS; if (acl) { - error = posix_acl_equiv_mode(acl, &inode->i_mode); - if (error < 0) + error = posix_acl_update_mode(inode, &inode->i_mode, &acl); + if (error) return error; - else { - inode->i_ctime = ext4_current_time(inode); - ext4_mark_inode_dirty(handle, inode); - if (error == 0) - acl = NULL; - } + inode->i_ctime = ext4_current_time(inode); + ext4_mark_inode_dirty(handle, inode); } break; --- a/fs/generic_acl.c +++ b/fs/generic_acl.c @@ -86,16 +86,17 @@ generic_acl_set(struct dentry *dentry, c if (error) goto failed; switch (type) { - case ACL_TYPE_ACCESS: - error = posix_acl_equiv_mode(acl, &inode->i_mode); - if (error < 0) + case ACL_TYPE_ACCESS: { + struct posix_acl *saved_acl = acl; + + error = posix_acl_update_mode(inode, &inode->i_mode, &acl); + if (acl == NULL) + posix_acl_release(saved_acl); + if (error) goto failed; inode->i_ctime = CURRENT_TIME; - if (error == 0) { - posix_acl_release(acl); - acl = NULL; - } break; + } case ACL_TYPE_DEFAULT: if (!S_ISDIR(inode->i_mode)) { error = -EINVAL; --- a/fs/gfs2/acl.c +++ b/fs/gfs2/acl.c @@ -277,16 +277,14 @@ static int gfs2_xattr_system_set(struct goto out_release; if (type == ACL_TYPE_ACCESS) { - umode_t mode = inode->i_mode; - error = posix_acl_equiv_mode(acl, &mode); + struct posix_acl *saved_acl = acl; + umode_t mode; - if (error <= 0) { - posix_acl_release(acl); - acl = NULL; - - if (error < 0) - return error; - } + error = posix_acl_update_mode(inode, &mode, &acl); + if (error || acl == NULL) + posix_acl_release(saved_acl); + if (error) + return error; error = gfs2_set_mode(inode, mode); if (error) --- a/fs/jffs2/acl.c +++ b/fs/jffs2/acl.c @@ -227,9 +227,10 @@ static int jffs2_set_acl(struct inode *i case ACL_TYPE_ACCESS: xprefix = JFFS2_XPREFIX_ACL_ACCESS; if (acl) { - umode_t mode = inode->i_mode; - rc = posix_acl_equiv_mode(acl, &mode); - if (rc < 0) + umode_t mode; + + rc = posix_acl_update_mode(inode, &mode, &acl); + if (rc) return rc; if (inode->i_mode != mode) { struct iattr attr; @@ -241,8 +242,6 @@ static int jffs2_set_acl(struct inode *i if (rc < 0) return rc; } - if (rc == 0) - acl = NULL; } break; case ACL_TYPE_DEFAULT: --- a/fs/jfs/xattr.c +++ b/fs/jfs/xattr.c @@ -693,9 +693,11 @@ static int can_set_system_xattr(struct i return rc; } if (acl) { - rc = posix_acl_equiv_mode(acl, &inode->i_mode); + struct posix_acl *dummy = acl; + + rc = posix_acl_update_mode(inode, &inode->i_mode, &dummy); posix_acl_release(acl); - if (rc < 0) { + if (rc) { printk(KERN_ERR "posix_acl_equiv_mode returned %d\n", rc); --- a/fs/ocfs2/acl.c +++ b/fs/ocfs2/acl.c @@ -247,14 +247,11 @@ static int ocfs2_set_acl(handle_t *handl case ACL_TYPE_ACCESS: name_index = OCFS2_XATTR_INDEX_POSIX_ACL_ACCESS; if (acl) { - umode_t mode = inode->i_mode; - ret = posix_acl_equiv_mode(acl, &mode); - if (ret < 0) + umode_t mode; + ret = posix_acl_update_mode(inode, &mode, &acl); + if (ret) return ret; else { - if (ret == 0) - acl = NULL; - ret = ocfs2_acl_set_mode(inode, di_bh, handle, mode); if (ret) --- a/fs/posix_acl.c +++ b/fs/posix_acl.c @@ -341,6 +341,36 @@ static int posix_acl_create_masq(struct return not_equiv; } +/** + * posix_acl_update_mode - update mode in set_acl + * + * Update the file mode when setting an ACL: compute the new file permission + * bits based on the ACL. In addition, if the ACL is equivalent to the new + * file mode, set *acl to NULL to indicate that no ACL should be set. + * + * As with chmod, clear the setgit bit if the caller is not in the owning group + * or capable of CAP_FSETID (see inode_change_ok). + * + * Called from set_acl inode operations. + */ +int posix_acl_update_mode(struct inode *inode, umode_t *mode_p, + struct posix_acl **acl) +{ + umode_t mode = inode->i_mode; + int error; + + error = posix_acl_equiv_mode(*acl, &mode); + if (error < 0) + return error; + if (error == 0) + *acl = NULL; + if (!in_group_p(inode->i_gid) && !capable(CAP_FSETID)) + mode &= ~S_ISGID; + *mode_p = mode; + return 0; +} +EXPORT_SYMBOL(posix_acl_update_mode); + /* * Modify the ACL for the chmod syscall. */ --- a/fs/reiserfs/xattr_acl.c +++ b/fs/reiserfs/xattr_acl.c @@ -272,13 +272,9 @@ reiserfs_set_acl(struct reiserfs_transac case ACL_TYPE_ACCESS: name = POSIX_ACL_XATTR_ACCESS; if (acl) { - error = posix_acl_equiv_mode(acl, &inode->i_mode); - if (error < 0) + error = posix_acl_update_mode(inode, &inode->i_mode, &acl); + if (error) return error; - else { - if (error == 0) - acl = NULL; - } } break; case ACL_TYPE_DEFAULT: --- a/fs/xfs/xfs_acl.c +++ b/fs/xfs/xfs_acl.c @@ -384,17 +384,14 @@ xfs_xattr_acl_set(struct dentry *dentry, goto out_release; if (type == ACL_TYPE_ACCESS) { - umode_t mode = inode->i_mode; - error = posix_acl_equiv_mode(acl, &mode); - - if (error <= 0) { - posix_acl_release(acl); - acl = NULL; - - if (error < 0) - return error; - } + struct posix_acl *saved_acl = acl; + umode_t mode; + error = posix_acl_update_mode(inode, &mode, &acl); + if (error || acl == NULL) + posix_acl_release(saved_acl); + if (error) + return error; error = xfs_set_mode(dentry, inode, mode); if (error) goto out_release; --- a/include/linux/posix_acl.h +++ b/include/linux/posix_acl.h @@ -83,6 +83,7 @@ extern struct posix_acl *posix_acl_from_ extern int posix_acl_equiv_mode(const struct posix_acl *, umode_t *); extern int posix_acl_create(struct posix_acl **, gfp_t, umode_t *); extern int posix_acl_chmod(struct posix_acl **, gfp_t, umode_t); +extern int posix_acl_update_mode(struct inode *, umode_t *, struct posix_acl **); extern struct posix_acl *get_posix_acl(struct inode *, int); extern int set_posix_acl(struct inode *, int, struct posix_acl *);