From patchwork Wed Sep 6 15:51:15 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Juerg Haefliger X-Patchwork-Id: 810687 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=none (mailfrom) smtp.mailfrom=lists.ubuntu.com (client-ip=91.189.94.19; helo=huckleberry.canonical.com; envelope-from=kernel-team-bounces@lists.ubuntu.com; receiver=) Received: from huckleberry.canonical.com (huckleberry.canonical.com [91.189.94.19]) by ozlabs.org (Postfix) with ESMTP id 3xnSk54gJsz9sNV; Thu, 7 Sep 2017 01:51:25 +1000 (AEST) Received: from localhost ([127.0.0.1] helo=huckleberry.canonical.com) by huckleberry.canonical.com with esmtp (Exim 4.86_2) (envelope-from ) id 1dpcbq-0003Fx-QR; Wed, 06 Sep 2017 15:51:22 +0000 Received: from youngberry.canonical.com ([91.189.89.112]) by huckleberry.canonical.com with esmtps (TLS1.0:DHE_RSA_AES_128_CBC_SHA1:128) (Exim 4.86_2) (envelope-from ) id 1dpcbn-0003EW-Q9 for kernel-team@lists.ubuntu.com; Wed, 06 Sep 2017 15:51:19 +0000 Received: from mail-wm0-f71.google.com ([74.125.82.71]) by youngberry.canonical.com with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.76) (envelope-from ) id 1dpcbn-00015j-Br for kernel-team@lists.ubuntu.com; Wed, 06 Sep 2017 15:51:19 +0000 Received: by mail-wm0-f71.google.com with SMTP id 187so6461857wmn.2 for ; Wed, 06 Sep 2017 08:51:19 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:subject:date:message-id:in-reply-to :references; bh=B2R+PKto2f6zvTf/UDIKUcJgK2bEteMFPmVhhkBbYnY=; b=Mw4p7gpm5XAAZjZeQX/vEQjgmSJyUIv1awE9CjdHL6qqobkaBaUsaKDyHj8fgyb+Gb hcCYmecglhqvHIVYZGrYWzucTwy3fo2A3Z6AB2abIihbmfxeNG0mFIwLAkNbE7XQFxMZ mjK35RntHWP9/zsHkhEnbDpjn+H1agrY9y/d5FotMXYi5r0TmYLS0Yt4rANvGsIe806h 0TCtnGBGg+TzFpRzwlWWG1sffTiDK+v+peOZSUNWmbGEJQczD7+p9haWDP7dORz6zO74 TJl6IJK/xDwAOyQcGdi3kNSHyOSKSXjnB3d87jcp08d5eMq7/HkTzSj/nMILxmJ8Lwl+ WNQg== X-Gm-Message-State: AHPjjUjAh0pV1B+F+rgnm7bqK4c7DlC3OsanF3RAVUrhZeZCeAAGvz8v QcB28djVJMdADEglPH3tS7/z27IO1ae3XcW4HsH07uOZSbiw531appbKPmmyTUAUGfW4oDSE1ts AzZVgWGJMLX5pSIN7DubQu474SEptyks8 X-Received: by 10.80.206.68 with SMTP id k4mr171710edj.48.1504713078634; Wed, 06 Sep 2017 08:51:18 -0700 (PDT) X-Google-Smtp-Source: ADKCNb4vb2kXbzRqOoO8lu8RTcO6U863kEscv3I4NfBlJnvyYNRoY66CsLVpNtI+ZAVl/a24ucfirQ== X-Received: by 10.80.206.68 with SMTP id k4mr171691edj.48.1504713078190; Wed, 06 Sep 2017 08:51:18 -0700 (PDT) Received: from gollum.fritz.box (adsl-84-227-115-101.adslplus.ch. [84.227.115.101]) by smtp.gmail.com with ESMTPSA id x28sm1606799edd.83.2017.09.06.08.51.17 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 06 Sep 2017 08:51:17 -0700 (PDT) From: Juerg Haefliger To: kernel-team@lists.ubuntu.com Subject: [trusty CVE-2016-7097 v2 1/1] posix_acl: Clear SGID bit when setting file permissions Date: Wed, 6 Sep 2017 17:51:15 +0200 Message-Id: <20170906155115.24400-2-juerg.haefliger@canonical.com> X-Mailer: git-send-email 2.14.1 In-Reply-To: <20170906155115.24400-1-juerg.haefliger@canonical.com> References: <20170906085453.22382-1-juerg.haefliger@canonical.com> <20170906155115.24400-1-juerg.haefliger@canonical.com> X-BeenThere: kernel-team@lists.ubuntu.com X-Mailman-Version: 2.1.20 Precedence: list List-Id: Kernel team discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: kernel-team-bounces@lists.ubuntu.com Sender: "kernel-team" 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.16: - Drop changes to orangefs - Adjust context - Update ext3 as well] Signed-off-by: Ben Hutchings CVE-2016-7097 (backported from f2ba3e2310b3967720b83126db8684c69ce41894 3.16.y) [juergh: Backported to 3.13: - Drop changes to ceph - Update 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] Signed-off-by: Juerg Haefliger Acked-by: Kleber Sacilotto de Souza Acked-by: Thadeu Lima de Souza Cascardo --- fs/9p/acl.c | 40 +++++++++++++++++----------------------- fs/btrfs/acl.c | 6 ++---- fs/ext2/acl.c | 12 ++++-------- fs/ext3/acl.c | 12 ++++-------- fs/ext4/acl.c | 12 ++++-------- fs/f2fs/acl.c | 6 ++---- fs/generic_acl.c | 15 ++++++++------- fs/gfs2/acl.c | 16 +++++++--------- fs/hfsplus/posix_acl.c | 4 ++-- fs/jffs2/acl.c | 9 ++++----- fs/jfs/xattr.c | 6 ++++-- fs/ocfs2/acl.c | 9 +++------ fs/posix_acl.c | 31 +++++++++++++++++++++++++++++++ fs/reiserfs/xattr_acl.c | 8 ++------ fs/xfs/xfs_acl.c | 17 +++++++---------- include/linux/posix_acl.h | 1 + 16 files changed, 102 insertions(+), 102 deletions(-) diff --git a/fs/9p/acl.c b/fs/9p/acl.c index 7af425f53bee..9686c1f17653 100644 --- a/fs/9p/acl.c +++ b/fs/9p/acl.c @@ -320,32 +320,26 @@ static int v9fs_xattr_set_acl(struct dentry *dentry, const char *name, 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: diff --git a/fs/btrfs/acl.c b/fs/btrfs/acl.c index 0890c83643e9..d6d53e5e7945 100644 --- a/fs/btrfs/acl.c +++ b/fs/btrfs/acl.c @@ -118,11 +118,9 @@ static int btrfs_set_acl(struct btrfs_trans_handle *trans, 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; diff --git a/fs/ext2/acl.c b/fs/ext2/acl.c index 110b6b371a4e..48c3c2d7d261 100644 --- a/fs/ext2/acl.c +++ b/fs/ext2/acl.c @@ -206,15 +206,11 @@ ext2_set_acl(struct inode *inode, int type, struct posix_acl *acl) 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; diff --git a/fs/ext3/acl.c b/fs/ext3/acl.c index dbb5ad59a7fc..bb2f60a62d82 100644 --- a/fs/ext3/acl.c +++ b/fs/ext3/acl.c @@ -205,15 +205,11 @@ ext3_set_acl(handle_t *handle, struct inode *inode, int type, 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; diff --git a/fs/ext4/acl.c b/fs/ext4/acl.c index 39a54a0e9fe4..c844f1bfb451 100644 --- a/fs/ext4/acl.c +++ b/fs/ext4/acl.c @@ -211,15 +211,11 @@ ext4_set_acl(handle_t *handle, struct inode *inode, int type, 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; diff --git a/fs/f2fs/acl.c b/fs/f2fs/acl.c index d0fc287efeff..0eb2d66827ad 100644 --- a/fs/f2fs/acl.c +++ b/fs/f2fs/acl.c @@ -224,12 +224,10 @@ static int f2fs_set_acl(struct inode *inode, int type, case ACL_TYPE_ACCESS: name_index = F2FS_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; set_acl_inode(fi, inode->i_mode); - if (error == 0) - acl = NULL; } break; diff --git a/fs/generic_acl.c b/fs/generic_acl.c index b3f3676796d3..67319f168b42 100644 --- a/fs/generic_acl.c +++ b/fs/generic_acl.c @@ -86,16 +86,17 @@ generic_acl_set(struct dentry *dentry, const char *name, const void *value, 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; diff --git a/fs/gfs2/acl.c b/fs/gfs2/acl.c index f69ac0af5496..015809a066b5 100644 --- a/fs/gfs2/acl.c +++ b/fs/gfs2/acl.c @@ -267,16 +267,14 @@ static int gfs2_xattr_system_set(struct dentry *dentry, const char *name, 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) diff --git a/fs/hfsplus/posix_acl.c b/fs/hfsplus/posix_acl.c index b609cc14c72e..9f7cc491ffb1 100644 --- a/fs/hfsplus/posix_acl.c +++ b/fs/hfsplus/posix_acl.c @@ -72,8 +72,8 @@ static int hfsplus_set_posix_acl(struct inode *inode, case ACL_TYPE_ACCESS: xattr_name = POSIX_ACL_XATTR_ACCESS; if (acl) { - err = posix_acl_equiv_mode(acl, &inode->i_mode); - if (err < 0) + err = posix_acl_update_mode(inode, &inode->i_mode, &acl); + if (err) return err; } err = 0; diff --git a/fs/jffs2/acl.c b/fs/jffs2/acl.c index 223283c30111..9335b8d3cf52 100644 --- a/fs/jffs2/acl.c +++ b/fs/jffs2/acl.c @@ -243,9 +243,10 @@ static int jffs2_set_acl(struct inode *inode, int type, struct posix_acl *acl) 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; @@ -257,8 +258,6 @@ static int jffs2_set_acl(struct inode *inode, int type, struct posix_acl *acl) if (rc < 0) return rc; } - if (rc == 0) - acl = NULL; } break; case ACL_TYPE_DEFAULT: diff --git a/fs/jfs/xattr.c b/fs/jfs/xattr.c index d3472f4cd530..6910662a8bf5 100644 --- a/fs/jfs/xattr.c +++ b/fs/jfs/xattr.c @@ -693,9 +693,11 @@ static int can_set_system_xattr(struct inode *inode, const char *name, 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); diff --git a/fs/ocfs2/acl.c b/fs/ocfs2/acl.c index b4f788e0ca31..b16bb5c70bc8 100644 --- a/fs/ocfs2/acl.c +++ b/fs/ocfs2/acl.c @@ -270,14 +270,11 @@ static int ocfs2_set_acl(handle_t *handle, 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) diff --git a/fs/posix_acl.c b/fs/posix_acl.c index 3542f1f814e2..7a82cf1601d5 100644 --- a/fs/posix_acl.c +++ b/fs/posix_acl.c @@ -407,6 +407,37 @@ posix_acl_create(struct posix_acl **acl, gfp_t gfp, umode_t *mode_p) } EXPORT_SYMBOL(posix_acl_create); +/** + * 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_wrt_inode_uidgid(inode, CAP_FSETID)) + mode &= ~S_ISGID; + *mode_p = mode; + return 0; +} +EXPORT_SYMBOL(posix_acl_update_mode); + int posix_acl_chmod(struct posix_acl **acl, gfp_t gfp, umode_t mode) { diff --git a/fs/reiserfs/xattr_acl.c b/fs/reiserfs/xattr_acl.c index 06c04f73da65..a86ad7ec7957 100644 --- a/fs/reiserfs/xattr_acl.c +++ b/fs/reiserfs/xattr_acl.c @@ -288,13 +288,9 @@ reiserfs_set_acl(struct reiserfs_transaction_handle *th, struct inode *inode, 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: diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c index 370eb3e121d1..89ac0522b38d 100644 --- a/fs/xfs/xfs_acl.c +++ b/fs/xfs/xfs_acl.c @@ -402,17 +402,14 @@ xfs_xattr_acl_set(struct dentry *dentry, const char *name, 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(inode, mode); if (error) goto out_release; diff --git a/include/linux/posix_acl.h b/include/linux/posix_acl.h index 7931efe71175..2ae0bba45f12 100644 --- a/include/linux/posix_acl.h +++ b/include/linux/posix_acl.h @@ -90,6 +90,7 @@ extern struct posix_acl *posix_acl_from_mode(umode_t, gfp_t); 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 *);