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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20180531115557.sxfbgtgzy5gh5ldl@unicorn.suse.cz>
Date:   Thu, 31 May 2018 13:55:57 +0200
From:   Michal Kubecek <mkubecek@...e.cz>
To:     peter pi <tiangangpi@...il.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        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: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.

Michal Kubecek