Re: KASAN: slab-out-of-bounds Read in strcmp

[Stephen Smalley]

On Mon, 2017-12-04 at 17:39 +0100, Dmitry Vyukov wrote:
> On Mon, Dec 4, 2017 at 2:59 PM, Paul Moore <pmoore@xxxxxxxxxx> wrote:
> > > > > On 2017/12/02 3:52, syzbot wrote:
> > > > > > ===========================================================
> > > > > > =======
> > > > > > BUG: KASAN: slab-out-of-bounds in strcmp+0x96/0xb0
> > > > > > lib/string.c:328
> > > > > > Read of size 1 at addr ffff8801cd99d2c1 by task
> > > > > > syzkaller242593/3087
> > > > > > 
> > > > > > CPU: 0 PID: 3087 Comm: syzkaller242593 Not tainted 4.15.0-
> > > > > > rc1-next-
> > > > > > 20171201+ #57
> > > > > > Hardware name: Google Google Compute Engine/Google Compute
> > > > > > Engine,
> > > > > > BIOS Google 01/01/2011
> > > > > > Call Trace:
> > > > > > Â__dump_stack lib/dump_stack.c:17 [inline]
> > > > > > Âdump_stack+0x194/0x257 lib/dump_stack.c:53
> > > > > > Âprint_address_description+0x73/0x250 mm/kasan/report.c:252
> > > > > > Âkasan_report_error mm/kasan/report.c:351 [inline]
> > > > > > Âkasan_report+0x25b/0x340 mm/kasan/report.c:409
> > > > > > Â__asan_report_load1_noabort+0x14/0x20
> > > > > > mm/kasan/report.c:427
> > > > > > Âstrcmp+0x96/0xb0 lib/string.c:328
> > > > > 
> > > > > This seems to be out of bound read for "scontext" at
> > > > > 
> > > > > ÂÂÂÂÂÂfor (i = 1; i < SECINITSID_NUM; i++) {
> > > > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (!strcmp(initial_sid_to_string[i],
> > > > > scontext)) {
> > > > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ*sid = i;
> > > > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn 0;
> > > > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ}
> > > > > ÂÂÂÂÂÂ}
> > > > > 
> > > > > because "initial_sid_to_string[i]" is "const char *".
> > > > > 
> > > > > > Âsecurity_context_to_sid_core+0x437/0x620
> > > > > > security/selinux/ss/services.c:1420
> > > > > > Âsecurity_context_to_sid+0x32/0x40
> > > > > > security/selinux/ss/services.c:1479
> > > > > > Âselinux_setprocattr+0x51c/0xb50
> > > > > > security/selinux/hooks.c:5986
> > > > > > Âsecurity_setprocattr+0x85/0xc0 security/security.c:1264
> > > > > 
> > > > > If "value" does not terminate with '\0' or '\n', "value" and
> > > > > "size" are as-is passed to "scontext" and "scontext_len"
> > > > > above
> > > > > 
> > > > > ÂÂÂÂÂÂ/* Obtain a SID for the context, if one was specified.
> > > > > */
> > > > > ÂÂÂÂÂÂif (size && str[0] && str[0] != '\n') {
> > > > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (str[size-1] == '\n') {
> > > > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂstr[size-1] = 0;
> > > > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂsize--;
> > > > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ}
> > > > > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂerror = security_context_to_sid(value, size,
> > > > > &sid,
> > > > > GFP_KERNEL);
> > > > > 
> > > > > which will allow strcmp() to trigger out of bound read when
> > > > > "size" is
> > > > > larger than strlen(initial_sid_to_string[i]).
> > > > > 
> > > > > Thus, I guess the simplest fix is to use strncmp() instead of
> > > > > strcmp().
> > > > 
> > > > Already fixed by
> > > > https://www.spinics.net/lists/selinux/msg23589.html
> > > 
> > > 
> > > Paul, please also follow this part:
> > > 
> > > > syzbot will keep track of this bug report.
> > > > Once a fix for this bug is committed, please reply to this
> > > > email with:
> > > > #syz fix: exact-commit-title
> > > > Note: all commands must start from beginning of the line in the
> > > > email body.
> > > 
> > > This will greatly help to keep overall process running. Thanks.
> > 
> > When is the right time to do this?ÂÂThe text say to reply when a
> > patch
> > has been committed, but where?ÂÂMy selinux/next branch?ÂÂLinus'
> > master?ÂÂYour docs and the end of the email needs to be more clear
> > on
> > this.
> > 
> > For the record, I did see that part of the syzbot mail but I was
> > waiting until I merged that patch; v2 was posted late in the week
> > and
> > I was giving it a few days in case someone saw something
> > objectionable.
> > 
> > Also, while we are on the topic of syzbot, what SELinux policy (if
> > any) do you load on the system that is undergoing testing?ÂÂBased
> > on
> > some of the recent reports it would appear that you are running a
> > SELinux enabled kernel but might not be loading a SELinux policy,
> > is
> > that correct?
> 
> 
> This is good question. The problem is that we are testing almost all
> kernel subsystems, but are not experts in most of them. So frequently
> we doing some non-sense. And that's where we need you help.
> That's what we have for grep SECURITY .config:
> 
> CONFIG_EXT4_FS_SECURITY=y
> # CONFIG_9P_FS_SECURITY is not set
> # CONFIG_SECURITY_DMESG_RESTRICT is not set
> CONFIG_SECURITY=y
> CONFIG_SECURITY_WRITABLE_HOOKS=y
> # CONFIG_SECURITYFS is not set
> CONFIG_SECURITY_NETWORK=y
> CONFIG_SECURITY_NETWORK_XFRM=y
> CONFIG_SECURITY_PATH=y
> CONFIG_SECURITY_SELINUX=y
> CONFIG_SECURITY_SELINUX_BOOTPARAM=y
> CONFIG_SECURITY_SELINUX_BOOTPARAM_VALUE=1
> CONFIG_SECURITY_SELINUX_DISABLE=y
> CONFIG_SECURITY_SELINUX_DEVELOP=y
> CONFIG_SECURITY_SELINUX_AVC_STATS=y
> CONFIG_SECURITY_SELINUX_CHECKREQPROT_VALUE=0
> # CONFIG_SECURITY_SMACK is not set
> # CONFIG_SECURITY_TOMOYO is not set
> # CONFIG_SECURITY_APPARMOR is not set
> # CONFIG_SECURITY_LOADPIN is not set
> # CONFIG_SECURITY_YAMA is not set
> CONFIG_DEFAULT_SECURITY_SELINUX=y
> # CONFIG_DEFAULT_SECURITY_DAC is not set
> CONFIG_DEFAULT_SECURITY="selinux"
> 
> 
> I don't think we do anything else besides that.
> To be fair I don't know what is SELinux policy nor how to load it.
> Can
> you suggest a policy that would be good for testing of kernel in
> general and SELinux in particular? Obviously, we don't want to
> prohibit access to any major parts of kernel APIs entirely (because
> then we won't test them). syzkaller also creates a local temp dir per
> test process, and the process mostly accesses files there (except for
> opening /dev/* and /proc/self/*). Is it possible to selectively
> prohibit something there, so that we test both positive and negative
> cases?

SELinux policy is loaded by userspace; the support is integrated into
the various init programs, but you need to have a policy installed.

Best way would just be to run the test on Fedora (or CentOS) with
selinux-policy-targeted installed.  Then you would be testing with a
real policy loaded.  You could run syzkaller from the unconfined_t
domain and it should be largely unimpeded, and it could transition to a
different domain if you want to test denials.  For reference, the
selinux testsuite itself is at
Âhttps://github.com/SELinuxProject/selinux-testsuite/
It includes a defconfig fragment that can be merged, although portions
of that are only required for particular test programs (see the
comments in it).

If that's not viable, then another approach would be to generate a
minimal allow-all policy from the kernel source tree, see
Documentation/admin-guide/LSM/SELinux.rst and scripts/selinux/*.
The downside of that approach is that SELinux developers and users
don't use that policy themselves for anything, and it won't exercise
any permission denial code paths.