ACK/cmnt: [trusty CVE-2016-7097 1/1] posix_acl: Clear SGID bit when setting file permissions
Kleber Souza
kleber.souza at canonical.com
Wed Sep 6 13:00:31 UTC 2017
On 09/06/17 14:59, Kleber Souza wrote:
> On 09/06/17 10:54, Juerg Haefliger wrote:
>> From: Jan Kara <jack at suse.cz>
>>
>> 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 <hch at lst.de>
>> Reviewed-by: Jeff Layton <jlayton at redhat.com>
>> Signed-off-by: Jan Kara <jack at suse.cz>
>> Signed-off-by: Andreas Gruenbacher <agruenba at redhat.com>
>> [bwh: Backported to 3.16:
>> - Drop changes to orangefs
>> - Adjust context
>> - Update ext3 as well]
>> Signed-off-by: Ben Hutchings <ben at decadent.org.uk>
>>
>> CVE-2016-7097
>>
>
> Should we add here the sha1 of the 3.16 backport commit since the
> original SOB comes from it?
>
> Probably:
> (backported from f2ba3e2310b3967720b83126db8684c69ce41894 3.16.y)
>
> I think we can add that while applying the patch.
>
> Otherwise it looks a sane backport and a nice combination of the patches
> from 3.2 and 3.16 :-).
Now with the actual ACK:
Acked-by: Kleber Sacilotto de Souza <kleber.souza at canonical.com>
>
>
> Kleber
>
>> [juergh: Backported to 3.13:
>> - Drop changes to ceph
>> - Use capable() instead of capable_wrt_inode_uidgid()
>> - 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 <juerg.haefliger at canonical.com>
>> ---
>> 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 | 30 ++++++++++++++++++++++++++++++
>> fs/reiserfs/xattr_acl.c | 8 ++------
>> fs/xfs/xfs_acl.c | 17 +++++++----------
>> include/linux/posix_acl.h | 1 +
>> 16 files changed, 101 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..8161e5c9dc31 100644
>> --- a/fs/posix_acl.c
>> +++ b/fs/posix_acl.c
>> @@ -407,6 +407,36 @@ 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(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 *);
>>
More information about the kernel-team
mailing list