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

Message-ID: <20180531113215.sbqqjip2gxvhl2eg@unicorn.suse.cz>
Date:   Thu, 31 May 2018 13:32:16 +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 05:40:40PM +0800, peter pi wrote:
> 
> My test method is very simple:
> 1, In copy_to_user, add a function call like my_examine(from, n) to check
> every 8 bytes. There is an kernel function called  virt_addr_valid which
> can check if the value is a address value.
> 2, Print a kernel log when there is a leak detected in function my_examine
> 3, Run iptables-save or ip6tables-save in shell, it will hit the kernel
> code path of the problem

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.

Unless there are other kernel pointers leaked, the solution should be
simple: explicitly zero the copy of .u.kernel.match (.u.kernel.target)
before we copy the name. I haven't checked yet if compat_ code path
suffers from the same problem.

Michal Kubecek