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 12:59:18 UTC 2017
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 :-).
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