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
