netdev - Re: [PATCH v2] netfilter: properly initialize xt_table

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] [day] [month] [year] [list]

Message-ID: <20180531162530.GA20226@kroah.com>
Date:   Thu, 31 May 2018 18:25:30 +0200
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Michal Kubecek <mkubecek@...e.cz>
Cc:     peter pi <tiangangpi@...il.com>, Florian Westphal <fw@...len.de>,
        Jan Engelhardt <jengelh@...i.de>,
        Eric Dumazet <eric.dumazet@...il.com>,
        Greg Hackmann <ghackmann@...gle.com>,
        Pablo Neira Ayuso <pablo@...filter.org>,
        Jozsef Kadlecsik <kadlec@...ckhole.kfki.hu>,
        netfilter-devel@...r.kernel.org, coreteam@...filter.org,
        netdev@...r.kernel.org
Subject: Re: [PATCH v2] netfilter: properly initialize xt_table_info structure

On Thu, May 31, 2018 at 01:55:57PM +0200, Michal Kubecek wrote:
> On Thu, May 31, 2018 at 01:32:16PM +0200, Michal Kubecek wrote:
> > I think I start to understand the problem. IPT_SO_GET_ENTRIES leads to
> > calling copy_entries_to_user() which copies the entries as they are to
> > user provided buffer. It also copies instances of struct xt_entry_match
> > and struct xt_entry_target which contain kernel pointers. We then
> > rewrite them with match/target name for userspace but the layout looks
> > (on x86_64) like this
> > 
> > /* offset    |  size */  type = struct xt_entry_match {
> > /*    0      |    32 */    union {
> > /*                32 */        struct {
> > /*    0      |     2 */            __u16 match_size;
> > /*    2      |    29 */            char name[29];
> > /*   31      |     1 */            __u8 revision;
> > 
> >                                    /* total size (bytes):   32 */
> >                                } user;
> > /*                16 */        struct {
> > /*    0      |     2 */            __u16 match_size;
> > /* XXX  6-byte hole  */
> > /*    8      |     8 */            struct xt_match *match;
> > 
> >                                    /* total size (bytes):   16 */
> >                                } kernel;
> > /*                 2 */        __u16 match_size;
> > 
> >                                /* total size (bytes):   32 */
> >                            } u;
> > /*   32      |     0 */    unsigned char data[];
> > 
> >                            /* total size (bytes):   32 */
> >                          }
> > 
> > 
> > so that if match name is no longer than five characters (which is often
> > the case), writing to .u.user.name leaves .u.kernel.match untouched. The
> > same problem exists in struct xt_entry_target.
> 
> And this should no longer happen since the series
> 
>  f32815d21d4d ("xtables: add xt_match, xt_target and data copy_to_user functions")
>  f77bc5b23fb1 ("iptables: use match, target and data copy_to_user helpers")
>  e47ddb2c4691 ("ip6tables: use match, target and data copy_to_user helpers")
>  244b531bee2b ("arptables: use match, target and data copy_to_user helpers")
>  b5040f6c33a5 ("ebtables: use match, target and data copy_to_user helpers")
>  4915f7bbc402 ("xtables: use match, target and data copy_to_user helpers in compat")
>  ec2318904965 ("xtables: extend matches and targets with .usersize")
> 
> changed the logic in 4.11-rc1.

Thank you so much for the detailed description.  And sorry for digging
up this old issue.  Peter, if you could verify that you do not see this
issue on a kernel newer than 4.11, that would be wonderful.

Michal, do you think it is worth backporting those commits to the 4.9.y
and 4.4.y stable kernels to remove this problem there?

thanks,

greg k-h