What is "fscon" statement in a base policy?

What is "fscon" statement in a base policy?

[Nicolas Iooss]

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

Re: What is "fscon" statement in a base policy?

[Karl MacMillan]

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@m4x.org> 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
>

Re: What is "fscon" statement in a base policy?

[Christian Göttsche]

On Fri, 1 Jul 2022 at 03:58, Karl MacMillan <karl@bigbadwolfsecurity.com> wrote:
>
> Hi Nicolas,
>
> I believe these are described on page 19 of the old "A Security Policy
> Configuration for the Security-Enhanced Linux" [1].

Quote from 7.2 File System Contexts:

    Currently, this configuration is unused.

> 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.

git log -S OCON_FS lists

    335c818c5a7a can: mcp251xfd: move chip FIFO init into separate file
    55e5b97f003e can: mcp25xxfd: add driver for Microchip MCP25xxFD SPI CAN
    875347fe5756 can: mcp25xxfd: add regmap infrastructure
    cee74f47a6ba SELinux: allow userspace to read policy back out of the kernel
    1da177e4c3f4 (tag: v2.6.12-rc2) Linux-2.6.12-rc2

and grepping the source shows

    $ grep -Rw OCON_FS security/selinux/
    security/selinux/ss/policydb.h:#define OCON_FS          1 /*
unlabeled file systems */
    security/selinux/ss/policydb.c: if (i == OCON_ISID || i == OCON_FS ||
    security/selinux/ss/policydb.c:                 case OCON_FS:
    security/selinux/ss/policydb.c:                 case OCON_FS:

OCON_FS is only used while parsing a policy and on cleanup, but there
is no actual usage, e.g. for OCON_FSUSE:

    security/selinux/ss/services.c: c = policydb->ocontexts[OCON_FSUSE];

So for me fscon is not used at all.

> 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@m4x.org> 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
> >

Re: What is "fscon" statement in a base policy?

[Karl MacMillan]

> On Jul 1, 2022, at 5:26 AM, Christian Göttsche <cgzones@googlemail.com> wrote:
> 
> On Fri, 1 Jul 2022 at 03:58, Karl MacMillan <karl@bigbadwolfsecurity.com> wrote:
>> 
>> Hi Nicolas,
>> 
>> I believe these are described on page 19 of the old "A Security Policy
>> Configuration for the Security-Enhanced Linux" [1].
> 
> Quote from 7.2 File System Contexts:
> 
>    Currently, this configuration is unused.
> 
>> 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.
> 
> git log -S OCON_FS lists
> 
>    335c818c5a7a can: mcp251xfd: move chip FIFO init into separate file
>    55e5b97f003e can: mcp25xxfd: add driver for Microchip MCP25xxFD SPI CAN
>    875347fe5756 can: mcp25xxfd: add regmap infrastructure
>    cee74f47a6ba SELinux: allow userspace to read policy back out of the kernel
>    1da177e4c3f4 (tag: v2.6.12-rc2) Linux-2.6.12-rc2
> 
> and grepping the source shows
> 
>    $ grep -Rw OCON_FS security/selinux/
>    security/selinux/ss/policydb.h:#define OCON_FS          1 /*
> unlabeled file systems */
>    security/selinux/ss/policydb.c: if (i == OCON_ISID || i == OCON_FS ||
>    security/selinux/ss/policydb.c:                 case OCON_FS:
>    security/selinux/ss/policydb.c:                 case OCON_FS:
> 
> OCON_FS is only used while parsing a policy and on cleanup, but there
> is no actual usage, e.g. for OCON_FSUSE:
> 
>    security/selinux/ss/services.c: c = policydb->ocontexts[OCON_FSUSE];
> 
> So for me fscon is not used at all.
> 

Your assessment is better than mine - thanks for digging deeper. What a shame that code is in-kernel then since it does nothing and there was likely never a policy that had these. 

Karl

>> 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@m4x.org> 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
>>>