Hi Nicolas, I believe these are described on page 19 of the old "A Security Policy Configuration for the Security-Enhanced Linux" [1]. There is still support for these in the kernel [2], so it seems unwise to me to drop them even if they are not used in policies. Good catch though! Karl 1. https://media.defense.gov/2021/Jul/29/2002815735/-1/-1/0/SELINUX-SECURITY-POLICY-CONFIGURATION-REPORT.PDF 2. https://github.com/torvalds/linux/blob/master/security/selinux/ss/policydb.h#L228 On Thu, Jun 30, 2022 at 5:05 PM Nicolas Iooss <nicolas.iooss@xxxxxxx> wrote: > > Hello, > > While studying some malloc calls in libsepol and checkpolicy, I > stumbled upon function define_fs_context(), which allocates a > fixed-size buffer in > https://github.com/SELinuxProject/selinux/blob/956bda08f6183078f13b70f6aa27d0529a3ec20a/checkpolicy/policy_define.c#L4631-L4637 > > newc->u.name = (char *)malloc(6); > if (!newc->u.name) { > yyerror("out of memory"); > free(newc); > return -1; > } > sprintf(newc->u.name, "%02x:%02x", major, minor); > > As major and minor are unsigned int (so 32-bit integers) without any > value checking, there seems to be a possible heap buffer overflow > issue. This function is called when parsing a fscon statement in a > "base" policy. So I copied tmp/base.conf from a build of the Reference > Policy, added "fscon 1000 1000 system_u:object_r:unlabeled_t > system_u:object_r:unlabeled_t" right after "sid security > system_u:object_r:security_t" (the order of the statements matters), > and ran: > > $ checkpolicy -o test.pol base.conf > *** buffer overflow detected ***: terminated > Aborted (core dumped) > > For whatever it's worth, the stack trace of this abort tells that the > buffer overflow occurs in a call to __sprintf_chk(): my gcc compiler > seems to be "smart enough" to find out that the size of newc->u.name > was 6, and it replaced sprintf() with __sprintf_chk() to ensure that > the buffer was not written past its bounds. > > Now, I can submit a patch to fix this issue, for example by replacing > malloc()+sprintf() with asprintf() and by checking that major and > minor are below 256. But before doing so, I was wondering: what is > this fscon syntax? I have never encountered it, did not find any > policy using it, and I am wondering whether we could instead drop its > support and remove function define_fs_context() from checkpolicy. > > Thanks, > Nicolas >
