From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-yb0-f176.google.com ([209.85.213.176]:34350 "EHLO mail-yb0-f176.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754132AbcITQyW (ORCPT ); Tue, 20 Sep 2016 12:54:22 -0400 Received: by mail-yb0-f176.google.com with SMTP id x93so10065047ybh.1 for ; Tue, 20 Sep 2016 09:54:22 -0700 (PDT) Message-ID: <1474390458.19989.37.camel@redhat.com> Subject: Re: [PATCH v2 RESEND] posix_acl: Clear SGID bit when setting file permissions From: Jeff Layton To: Jan Kara , Al Viro Cc: Andrew Morton , linux-fsdevel@vger.kernel.org, Andreas Gruenbacher Date: Tue, 20 Sep 2016 12:54:18 -0400 In-Reply-To: <1474299768-15150-1-git-send-email-jack@suse.cz> References: <1474299768-15150-1-git-send-email-jack@suse.cz> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Mon, 2016-09-19 at 17:42 +0200, Jan Kara wrote: > 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 > > Signed-off-by: Jan Kara > > Signed-off-by: Andreas Gruenbacher > --- >  fs/9p/acl.c               | 40 +++++++++++++++++----------------------- >  fs/btrfs/acl.c            |  6 ++---- >  fs/ceph/acl.c             |  6 ++---- >  fs/ext2/acl.c             | 12 ++++-------- >  fs/ext4/acl.c             | 12 ++++-------- >  fs/f2fs/acl.c             |  6 ++---- >  fs/gfs2/acl.c             | 12 +++--------- >  fs/hfsplus/posix_acl.c    |  4 ++-- >  fs/jffs2/acl.c            |  9 ++++----- >  fs/jfs/acl.c              |  6 ++---- >  fs/ocfs2/acl.c            | 10 ++++------ >  fs/orangefs/acl.c         | 15 +++++---------- >  fs/posix_acl.c            | 31 +++++++++++++++++++++++++++++++ >  fs/reiserfs/xattr_acl.c   |  8 ++------ >  fs/xfs/xfs_acl.c          | 13 ++++--------- >  include/linux/posix_acl.h |  1 + >  16 files changed, 89 insertions(+), 102 deletions(-) > > Another forgotten patch. It was posted on May 27 and Aug 19. Al, can you > please pick it up? Andrew, if Al does not respond, can you please take care > of pushing it to Linus? Thanks! > > diff --git a/fs/9p/acl.c b/fs/9p/acl.c > index 5b6a1743ea17..b3c2cc79c20d 100644 > --- a/fs/9p/acl.c > +++ b/fs/9p/acl.c > @@ -276,32 +276,26 @@ static int v9fs_xattr_set_acl(const struct xattr_handler *handler, > >   switch (handler->flags) { > >   case ACL_TYPE_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 53bb7af4e5f0..247b8dfaf6e5 100644 > --- a/fs/btrfs/acl.c > +++ b/fs/btrfs/acl.c > @@ -79,11 +79,9 @@ static int __btrfs_set_acl(struct btrfs_trans_handle *trans, > >   case ACL_TYPE_ACCESS: > >   name = XATTR_NAME_POSIX_ACL_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/ceph/acl.c b/fs/ceph/acl.c > index 4f67227f69a5..d0b6b342dff9 100644 > --- a/fs/ceph/acl.c > +++ b/fs/ceph/acl.c > @@ -95,11 +95,9 @@ int ceph_set_acl(struct inode *inode, struct posix_acl *acl, int type) > >   case ACL_TYPE_ACCESS: > >   name = XATTR_NAME_POSIX_ACL_ACCESS; > >   if (acl) { > > - ret = posix_acl_equiv_mode(acl, &new_mode); > > - if (ret < 0) > > + ret = posix_acl_update_mode(inode, &new_mode, &acl); > > + if (ret) > >   goto out; > > - if (ret == 0) > > - acl = NULL; > >   } > >   break; > >   case ACL_TYPE_DEFAULT: > diff --git a/fs/ext2/acl.c b/fs/ext2/acl.c > index 42f1d1814083..e725aa0890e0 100644 > --- a/fs/ext2/acl.c > +++ b/fs/ext2/acl.c > @@ -190,15 +190,11 @@ ext2_set_acl(struct inode *inode, struct posix_acl *acl, int type) > >   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/ext4/acl.c b/fs/ext4/acl.c > index c6601a476c02..dfa519979038 100644 > --- a/fs/ext4/acl.c > +++ b/fs/ext4/acl.c > @@ -193,15 +193,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 4dcc9e28dc5c..31344247ce89 100644 > --- a/fs/f2fs/acl.c > +++ b/fs/f2fs/acl.c > @@ -210,12 +210,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(inode, inode->i_mode); > > - if (error == 0) > > - acl = NULL; > >   } > >   break; >   > diff --git a/fs/gfs2/acl.c b/fs/gfs2/acl.c > index 363ba9e9d8d0..2524807ee070 100644 > --- a/fs/gfs2/acl.c > +++ b/fs/gfs2/acl.c > @@ -92,17 +92,11 @@ int __gfs2_set_acl(struct inode *inode, struct posix_acl *acl, int type) > >   if (type == ACL_TYPE_ACCESS) { > >   umode_t mode = inode->i_mode; >   > > - error = posix_acl_equiv_mode(acl, &mode); > > - if (error < 0) > > + error = posix_acl_update_mode(inode, &inode->i_mode, &acl); > > + if (error) > >   return error; > - > > - if (error == 0) > > - acl = NULL; > - > > - if (mode != inode->i_mode) { > > - inode->i_mode = mode; > > + if (mode != inode->i_mode) > >   mark_inode_dirty(inode); > > - } > >   } >   > >   if (acl) { > diff --git a/fs/hfsplus/posix_acl.c b/fs/hfsplus/posix_acl.c > index ab7ea2506b4d..9b92058a1240 100644 > --- a/fs/hfsplus/posix_acl.c > +++ b/fs/hfsplus/posix_acl.c > @@ -65,8 +65,8 @@ int hfsplus_set_posix_acl(struct inode *inode, struct posix_acl *acl, > >   case ACL_TYPE_ACCESS: > >   xattr_name = XATTR_NAME_POSIX_ACL_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 bc2693d56298..2a0f2a1044c1 100644 > --- a/fs/jffs2/acl.c > +++ b/fs/jffs2/acl.c > @@ -233,9 +233,10 @@ int jffs2_set_acl(struct inode *inode, struct posix_acl *acl, int type) > >   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; > @@ -247,8 +248,6 @@ int jffs2_set_acl(struct inode *inode, struct posix_acl *acl, int type) > >   if (rc < 0) > >   return rc; > >   } > > - if (rc == 0) > > - acl = NULL; > >   } > >   break; > >   case ACL_TYPE_DEFAULT: > diff --git a/fs/jfs/acl.c b/fs/jfs/acl.c > index 21fa92ba2c19..3a1e1554a4e3 100644 > --- a/fs/jfs/acl.c > +++ b/fs/jfs/acl.c > @@ -78,13 +78,11 @@ static int __jfs_set_acl(tid_t tid, struct inode *inode, int type, > >   case ACL_TYPE_ACCESS: > >   ea_name = XATTR_NAME_POSIX_ACL_ACCESS; > >   if (acl) { > > - rc = posix_acl_equiv_mode(acl, &inode->i_mode); > > - if (rc < 0) > > + rc = posix_acl_update_mode(inode, &inode->i_mode, &acl); > > + if (rc) > >   return rc; > >   inode->i_ctime = CURRENT_TIME; > >   mark_inode_dirty(inode); > > - if (rc == 0) > > - acl = NULL; > >   } > >   break; > >   case ACL_TYPE_DEFAULT: > diff --git a/fs/ocfs2/acl.c b/fs/ocfs2/acl.c > index 2162434728c0..164307b99405 100644 > --- a/fs/ocfs2/acl.c > +++ b/fs/ocfs2/acl.c > @@ -241,13 +241,11 @@ 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) > > - return ret; > > + umode_t mode; >   > > - if (ret == 0) > > - acl = NULL; > > + ret = posix_acl_update_mode(inode, &mode, &acl); > > + if (ret) > > + return ret; >   > >   ret = ocfs2_acl_set_mode(inode, di_bh, > >    handle, mode); > diff --git a/fs/orangefs/acl.c b/fs/orangefs/acl.c > index 28f2195cd798..7a3754488312 100644 > --- a/fs/orangefs/acl.c > +++ b/fs/orangefs/acl.c > @@ -73,14 +73,11 @@ int orangefs_set_acl(struct inode *inode, struct posix_acl *acl, int type) > >   case ACL_TYPE_ACCESS: > >   name = XATTR_NAME_POSIX_ACL_ACCESS; > >   if (acl) { > > - umode_t mode = inode->i_mode; > > - /* > > -  * can we represent this with the traditional file > > -  * mode permission bits? > > -  */ > > - error = posix_acl_equiv_mode(acl, &mode); > > - if (error < 0) { > > - gossip_err("%s: posix_acl_equiv_mode err: %d\n", > > + umode_t mode; > + > > + error = posix_acl_update_mode(inode, &mode, &acl); > > + if (error) { > > + gossip_err("%s: posix_acl_update_mode err: %d\n", > >      __func__, > >      error); > >   return error; > @@ -90,8 +87,6 @@ int orangefs_set_acl(struct inode *inode, struct posix_acl *acl, int type) > >   SetModeFlag(orangefs_inode); > >   inode->i_mode = mode; > >   mark_inode_dirty_sync(inode); > > - if (error == 0) > > - acl = NULL; > >   } > >   break; > >   case ACL_TYPE_DEFAULT: > diff --git a/fs/posix_acl.c b/fs/posix_acl.c > index 59d47ab0791a..bfc3ec388322 100644 > --- a/fs/posix_acl.c > +++ b/fs/posix_acl.c > @@ -626,6 +626,37 @@ no_mem: >  } >  EXPORT_SYMBOL_GPL(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); > + >  /* >   * Fix up the uids and gids in posix acl extended attributes in place. >   */ > diff --git a/fs/reiserfs/xattr_acl.c b/fs/reiserfs/xattr_acl.c > index dbed42f755e0..27376681c640 100644 > --- a/fs/reiserfs/xattr_acl.c > +++ b/fs/reiserfs/xattr_acl.c > @@ -242,13 +242,9 @@ __reiserfs_set_acl(struct reiserfs_transaction_handle *th, struct inode *inode, > >   case ACL_TYPE_ACCESS: > >   name = XATTR_NAME_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 { > > - 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 b6e527b8eccb..8a0dec89ca56 100644 > --- a/fs/xfs/xfs_acl.c > +++ b/fs/xfs/xfs_acl.c > @@ -257,16 +257,11 @@ xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type) > >   return error; >   > >   if (type == ACL_TYPE_ACCESS) { > > - umode_t mode = inode->i_mode; > > - error = posix_acl_equiv_mode(acl, &mode); > - > > - if (error <= 0) { > > - acl = NULL; > - > > - if (error < 0) > > - return error; > > - } > > + umode_t mode; >   > > + error = posix_acl_update_mode(inode, &mode, &acl); > > + if (error) > > + return error; > >   error = xfs_set_mode(inode, mode); > >   if (error) > >   return error; > diff --git a/include/linux/posix_acl.h b/include/linux/posix_acl.h > index d5d3d741f028..bf1046d0397b 100644 > --- a/include/linux/posix_acl.h > +++ b/include/linux/posix_acl.h > @@ -93,6 +93,7 @@ extern int set_posix_acl(struct inode *, int, struct posix_acl *); >  extern int posix_acl_chmod(struct inode *, umode_t); >  extern int posix_acl_create(struct inode *, umode_t *, struct posix_acl **, > >   struct posix_acl **); > +extern int posix_acl_update_mode(struct inode *, umode_t *, struct posix_acl **); >   >  extern int simple_set_acl(struct inode *, struct posix_acl *, int); >  extern int simple_acl_create(struct inode *, struct inode *); Looks correct: Reviewed-by: Jeff Layton