linux-kernel - Re: [BUG] security_release

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: <c007fac4-4e2a-6076-513d-6f52f7133a2d@schaufler-ca.com>
Date:   Mon, 18 Sep 2017 13:25:50 -0700
From:   Casey Schaufler <casey@...aufler-ca.com>
To:     Konstantin Khlebnikov <khlebnikov@...dex-team.ru>,
        linux-kernel <linux-kernel@...r.kernel.org>
Cc:     Serge Hallyn <serge@...lyn.com>,
        James Morris <james.l.morris@...cle.com>,
        LSM List <linux-security-module@...r.kernel.org>,
        Casey Schaufler <casey@...aufler-ca.com>
Subject: Re: [BUG] security_release_secctx seems broken

On 9/16/2017 11:18 AM, Konstantin Khlebnikov wrote:
> I've got this kmemleak splat
>
> unreferenced object 0xffff880f687ff6a8 (size 32):
>   comm "cp", pid 4279, jiffies 4295784487 (age 2866.296s)
>   hex dump (first 32 bytes):
>     01 00 00 02 02 00 08 00 00 00 00 00 00 00 00 00  ................
>     00 00 00 00 00 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b a5  .....kkkkkkkkkk.
>   backtrace:
>     [<ffffffff8192d77a>] kmemleak_alloc+0x4a/0xa0
>     [<ffffffff8125b721>] __kmalloc_track_caller+0x171/0x280
>     [<ffffffff812125c7>] krealloc+0xa7/0xc0
>     [<ffffffff812abb2d>] vfs_getxattr_alloc+0xbd/0x110
>     [<ffffffff813ba789>] cap_inode_getsecurity+0x79/0x200
>     [<ffffffff813be4d2>] security_inode_getsecurity+0x52/0x80
>     [<ffffffff812aad1a>] xattr_getsecurity+0x3a/0xa0
>     [<ffffffff812aadf4>] vfs_getxattr+0x74/0xa0
>     [<ffffffff812ab27e>] getxattr+0x9e/0x170
>     [<ffffffff812abd2a>] SyS_fgetxattr+0x5a/0x90
>     [<ffffffff8193a67b>] entry_SYSCALL_64_fastpath+0x1e/0xae
>     [<ffffffffffffffff>] 0xffffffffffffffff
>
> allocated in  xattr_getsecurity() security_inode_getsecurity() -> cap_inode_getsecurity()
> should be freed by security_release_secctx() but there is no release_secctx hook.

There is a bug here, but the fix suggested is not correct.
security_inode_getsecurity() provides the security attribute
with a specified name. In the past there would be exactly one
use for this call, which was to return the value of the
attribute that happened to match the security "context" of
the inode. Freeing the value with security_release_secctx()
works coincidentally. Because security_inode_getsecurity() is
called with the "true" value for the alloc parameter the correct
fix is:

	- fix smack_inode_getsecurity() to honor the alloc flag
	- change the security_release_secctx() to kfree here.

I can provide a patch in a day or so.

>
> I don't know details about modern security models stack but it
> seems security_release_secctx() have to know which layer owns
> secdata to free it property: selinux just calls kfree,
> capability_hooks should do the same, but other modules returns
> non-freeable pointers. Currently security_release_secctx() calls
> release_secctx for all models, this is obviously wrong.
>


.