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: <CAHC9VhSVmBfsgGVoLd9b1HoZxECHu_Uj8mSi+UUE_CzgB=Y-tg@mail.gmail.com>
Date:   Fri, 8 Mar 2019 08:20:30 -0500
From:   Paul Moore <paul@...l-moore.com>
To:     maowenan <maowenan@...wei.com>
Cc:     davem@...emloft.net, netdev@...r.kernel.org,
        Stephen Smalley <sds@...ho.nsa.gov>,
        Eric Paris <eparis@...isplace.org>, selinux@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH net] net: selinux: fix memory leak in selinux_netlbl_socket_post_create()

On Fri, Mar 8, 2019 at 1:31 AM maowenan <maowenan@...wei.com> wrote:
> On 2019/3/8 4:36, Paul Moore wrote:
> > On Wed, Mar 6, 2019 at 9:44 PM Mao Wenan <maowenan@...wei.com> wrote:
> >>
> >> If netlbl_sock_setattr() is failed, it directly returns rc and forgets
> >> to free secattr.
> >>
> >> BUG: memory leak
> >> unreferenced object 0xffff8883c3ea4200 (size 2664):
> >>   comm "syz-executor.2", pid 8813, jiffies 4297264419 (age 156.090s)
> >>   hex dump (first 32 bytes):
> >>     7f 00 00 01 7f 00 00 01 eb 7f ed 71 4e 24 00 00  ...........qN$..
> >>     02 00 07 40 00 00 00 00 00 00 00 00 00 00 00 00  ...@............
> >>   backtrace:
> >>     [<000000004c0228da>] sk_alloc+0x3d/0xc00 net/core/sock.c:1523
> >>     [<00000000535a3da2>] inet_create+0x339/0xe10 net/ipv4/af_inet.c:321
> >>     [<000000009aec3cfe>] __sock_create+0x3fa/0x790 net/socket.c:1275
> >>     [<000000004274b384>] sock_create net/socket.c:1315 [inline]
> >>     [<000000004274b384>] __sys_socket+0xe7/0x1d0 net/socket.c:1345
> >>     [<00000000b3fdc826>] __do_sys_socket net/socket.c:1354 [inline]
> >>     [<00000000b3fdc826>] __se_sys_socket net/socket.c:1352 [inline]
> >>     [<00000000b3fdc826>] __x64_sys_socket+0x74/0xb0 net/socket.c:1352
> >>     [<000000004ae3186e>] do_syscall_64+0xc8/0x580 arch/x86/entry/common.c:290
> >>     [<00000000bc0d2230>] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> >>     [<00000000f737e62f>] 0xffffffffffffffff
> >>
> >> BUG: memory leak
> >> unreferenced object 0xffff8883de23d570 (size 32):
> >>   comm "syz-executor.2", pid 8813, jiffies 4297264419 (age 156.090s)
> >>   hex dump (first 32 bytes):
> >>     02 00 00 00 00 00 00 00 60 a9 40 ce 83 88 ff ff  ........`.@.....
> >>     01 00 00 00 03 00 00 00 10 00 00 00 00 00 00 00  ................
> >>   backtrace:
> >>     [<0000000035ba8b75>] security_sk_alloc+0x5e/0xb0 security/security.c:1473
> >>     [<00000000302cc426>] sk_prot_alloc+0x8e/0x290 net/core/sock.c:1472
> >>     [<000000004c0228da>] sk_alloc+0x3d/0xc00 net/core/sock.c:1523
> >>     [<00000000535a3da2>] inet_create+0x339/0xe10 net/ipv4/af_inet.c:321
> >>     [<000000009aec3cfe>] __sock_create+0x3fa/0x790 net/socket.c:1275
> >>     [<000000004274b384>] sock_create net/socket.c:1315 [inline]
> >>     [<000000004274b384>] __sys_socket+0xe7/0x1d0 net/socket.c:1345
> >>     [<00000000b3fdc826>] __do_sys_socket net/socket.c:1354 [inline]
> >>     [<00000000b3fdc826>] __se_sys_socket net/socket.c:1352 [inline]
> >>     [<00000000b3fdc826>] __x64_sys_socket+0x74/0xb0 net/socket.c:1352
> >>     [<000000004ae3186e>] do_syscall_64+0xc8/0x580 arch/x86/entry/common.c:290
> >>     [<00000000bc0d2230>] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> >>     [<00000000f737e62f>] 0xffffffffffffffff
> >>
> >> BUG: memory leak
> >> unreferenced object 0xffff8883ce40a960 (size 64):
> >>   comm "syz-executor.2", pid 8813, jiffies 4297264420 (age 156.089s)
> >>   hex dump (first 32 bytes):
> >>     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
> >>     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
> >>   backtrace:
> >>     [<0000000029add401>] selinux_netlbl_socket_post_create+0x68/0x130 security/selinux/netlabel.c:416
> >>     [<000000005368a19c>] selinux_socket_post_create+0x31a/0x7f0 security/selinux/hooks.c:4597
> >>     [<00000000bd4730e2>] security_socket_post_create+0x70/0xc0 security/security.c:1385
> >>     [<00000000671052a4>] __sock_create+0x5a6/0x790 net/socket.c:1291
> >>     [<000000004274b384>] sock_create net/socket.c:1315 [inline]
> >>     [<000000004274b384>] __sys_socket+0xe7/0x1d0 net/socket.c:1345
> >>     [<00000000b3fdc826>] __do_sys_socket net/socket.c:1354 [inline]
> >>     [<00000000b3fdc826>] __se_sys_socket net/socket.c:1352 [inline]
> >>     [<00000000b3fdc826>] __x64_sys_socket+0x74/0xb0 net/socket.c:1352
> >>     [<000000004ae3186e>] do_syscall_64+0xc8/0x580 arch/x86/entry/common.c:290
> >>     [<00000000bc0d2230>] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> >>     [<00000000f737e62f>] 0xffffffffffffffff
> >>
> >> Fixes: 389fb800ac8b("netlabel: Label incoming TCP connections correctly in SELinux")
> >> Reported-by: Hulk Robot <hulkci@...wei.com>
> >> Signed-off-by: Mao Wenan <maowenan@...wei.com>
> >> ---
> >>  security/selinux/netlabel.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/security/selinux/netlabel.c b/security/selinux/netlabel.c
> >> index 186e727b737b..f3da05338580 100644
> >> --- a/security/selinux/netlabel.c
> >> +++ b/security/selinux/netlabel.c
> >> @@ -426,6 +426,9 @@ int selinux_netlbl_socket_post_create(struct sock *sk, u16 family)
> >>                 rc = 0;
> >>                 break;
> >>         }
> >> +       if (rc != 0) {
> >> +               netlbl_secattr_free(secattr);
> >> +       }
> >
> > This is likely going to cause a problem as the
> > sock->sk_security->nlbl_secattr still has a reference to the secattr
> > pointer you are releasing here.  Assuming things are working correctly
> > elsewhere, I believe freeing secattr here will result in a double free
> > when the network stacks cleans up after the failed socket creation
> > (via the sock_release() call in the error handling code).
> Do you mean here?
>         err = security_socket_post_create(sock, family, type, protocol, kern);
>         if (err)
>                 goto out_sock_release;
>         ...
>
>         out_sock_release:
>                 sock_release(sock);
>                 return err;
> yes, the call trace may like below:
>         sock_release->__sock_release->inet_release->tcp_close->sock_put->sk_free->__sk_free->sk_destruct->
>         __sk_destruct->sk_prot_free->security_sk_free->selinux_sk_free_security->selinux_netlbl_sk_security_free->
>         netlbl_secattr_free
>
> so, it is no need to free in selinux_netlbl_socket_post_create().
> Please correct me if this is incorrect.

I haven't followed the entirely call stack lately, but that looks right.

> > It looks like you may have found this via a test tool (syzbot?), do
> > you have a reproducer you can share?
>
> The issue has been found by syzkaller, but unfortunately it is no reproducer, it is
> only some backtrace.

If you are able to reproduce this please let us know, it may be a sign
that something is wrong with the LSM hooks.

Thanks.

-- 
paul moore
www.paul-moore.com

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ