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