On Mon, Dec 4, 2017 at 6:33 PM, Stephen Smalley <s...@tycho.nsa.gov> wrote: > On Mon, 2017-12-04 at 17:39 +0100, Dmitry Vyukov wrote: >> On Mon, Dec 4, 2017 at 2:59 PM, Paul Moore <pmo...@redhat.com> 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.
Thanks, I will see what we can do here. Still lots to learn for me.