linux-kernel - Re: CVE-2016-7097 causes acl leak

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20161213160747.GA16622@quack2.suse.cz>
Date:   Tue, 13 Dec 2016 17:07:47 +0100
From:   Jan Kara <jack@...e.cz>
To:     Mark Salyzyn <salyzyn@...roid.com>
Cc:     Cong Wang <xiyou.wangcong@...il.com>,
        LKML <linux-kernel@...r.kernel.org>,
        aneesh.kumar@...ux.vnet.ibm.com, Jan Kara <jack@...e.cz>
Subject: Re: CVE-2016-7097 causes acl leak

On Tue 13-12-16 07:55:23, Mark Salyzyn wrote:
> On 12/12/2016 10:26 PM, Cong Wang wrote:
> >On Mon, Dec 12, 2016 at 4:26 PM, Mark Salyzyn <salyzyn@...roid.com> wrote:
> >>The leaks were introduced in 9p, gfs2, jfs and xfs drivers only.
> >
> >Only the 9p case is obvious to me:
> >
> >diff --git a/fs/9p/acl.c b/fs/9p/acl.c
> >index b3c2cc7..082d227 100644
> >--- a/fs/9p/acl.c
> >+++ b/fs/9p/acl.c
> >@@ -277,6 +277,7 @@ static int v9fs_xattr_set_acl(const struct
> >xattr_handler *handler,
> >         case ACL_TYPE_ACCESS:
> >                 if (acl) {
> >                         struct iattr iattr;
> >+                       struct posix_acl *old_acl = acl;
> >
> >                         retval = posix_acl_update_mode(inode,
> >&iattr.ia_mode, &acl);
> >                         if (retval)
> >@@ -287,6 +288,7 @@ static int v9fs_xattr_set_acl(const struct
> >xattr_handler *handler,
> >                                  * by the mode bits. So don't
> >                                  * update ACL.
> >                                  */
> >+                               posix_acl_release(old_acl);
> >                                 value = NULL;
> >                                 size = 0;
> >                         }
> >
> >
> >The rest are anti-pattern (modifying parameters on stack via address)
> >but look correct
> I chose to modify posix_acl_update_mode as follows and set release_acl in
> the specific drivers, it clears the *acl reference to nul preventing
> posix_acl_release from functioning in these other driver paths:
> 
>  */
> int posix_acl_update_mode(struct inode *inode, umode_t *mode_p,
>               struct posix_acl **acl, ++++bool release_acl)
> {
>     umode_t mode = inode->i_mode;
>     int error;
> 
>     error = posix_acl_equiv_mode(*acl, &mode);
>     if (error < 0)
>         return error;
>     if (error == 0) +{
> +        if (release_acl)
> +            posix_acl_release(*acl);
>         *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);

I much prefer what Cong Wang did. These bool parameters which subtly change
the function behavior are too easy to screw up and it is not really
visible by just looking at the function call...

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR