[PATCH] selinux: keep SELinux in sync with new capability definitions

[PATCH] selinux: keep SELinux in sync with new capability definitions

[Stephen Smalley]

When a new capability is defined, SELinux needs to be updated.
Trigger a build error if a new capability is defined without
corresponding update to security/selinux/include/classmap.h's
COMMON_CAP2_PERMS.  This is similar to BUILD_BUG_ON() guards
in the SELinux nlmsgtab code to ensure that SELinux tracks
new netlink message types as needed.

Note that there is already a similar build guard in
security/selinux/hooks.c to detect when more than 64
capabilities are defined, since that will require adding
a third capability class to SELinux.

A nicer way to do this would be to extend scripts/selinux/genheaders
or a similar tool to auto-generate the necessary definitions and code
for SELinux capability checking from include/uapi/linux/capability.h.
AppArmor does something similar in its Makefile, although it only
needs to generate a single table of names.  That is left as future work.

Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
---
 security/selinux/include/classmap.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
index 1f1f4b2..e2d4ad3a 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -24,6 +24,10 @@
 #define COMMON_CAP2_PERMS  "mac_override", "mac_admin", "syslog", \
 		"wake_alarm", "block_suspend", "audit_read"
 
+#if CAP_LAST_CAP > CAP_AUDIT_READ
+#error New capability defined, please update COMMON_CAP2_PERMS.
+#endif
+
 /*
  * Note: The name for any socket class should be suffixed by "socket",
  *	 and doesn't contain more than one substr of "socket".
-- 
2.7.4

Re: [PATCH] selinux: keep SELinux in sync with new capability definitions

[Paul Moore]

On Fri, Nov 18, 2016 at 10:23 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> When a new capability is defined, SELinux needs to be updated.
> Trigger a build error if a new capability is defined without
> corresponding update to security/selinux/include/classmap.h's
> COMMON_CAP2_PERMS.  This is similar to BUILD_BUG_ON() guards
> in the SELinux nlmsgtab code to ensure that SELinux tracks
> new netlink message types as needed.
>
> Note that there is already a similar build guard in
> security/selinux/hooks.c to detect when more than 64
> capabilities are defined, since that will require adding
> a third capability class to SELinux.
>
> A nicer way to do this would be to extend scripts/selinux/genheaders
> or a similar tool to auto-generate the necessary definitions and code
> for SELinux capability checking from include/uapi/linux/capability.h.
> AppArmor does something similar in its Makefile, although it only
> needs to generate a single table of names.  That is left as future work.
>
> Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
> ---
>  security/selinux/include/classmap.h | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
> index 1f1f4b2..e2d4ad3a 100644
> --- a/security/selinux/include/classmap.h
> +++ b/security/selinux/include/classmap.h
> @@ -24,6 +24,10 @@
>  #define COMMON_CAP2_PERMS  "mac_override", "mac_admin", "syslog", \
>                 "wake_alarm", "block_suspend", "audit_read"
>
> +#if CAP_LAST_CAP > CAP_AUDIT_READ
> +#error New capability defined, please update COMMON_CAP2_PERMS.
> +#endif

I think the obvious question here is why not use BUILD_BUG_ON() here?
I understand that it can be disabled, but it seems like the "good
neighbor" option compared to the #error pragma.

-- 
paul moore
www.paul-moore.com

Re: [PATCH] selinux: keep SELinux in sync with new capability definitions

[Stephen Smalley]

On 11/20/2016 05:29 PM, Paul Moore wrote:
> On Fri, Nov 18, 2016 at 10:23 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
>> When a new capability is defined, SELinux needs to be updated.
>> Trigger a build error if a new capability is defined without
>> corresponding update to security/selinux/include/classmap.h's
>> COMMON_CAP2_PERMS.  This is similar to BUILD_BUG_ON() guards
>> in the SELinux nlmsgtab code to ensure that SELinux tracks
>> new netlink message types as needed.
>>
>> Note that there is already a similar build guard in
>> security/selinux/hooks.c to detect when more than 64
>> capabilities are defined, since that will require adding
>> a third capability class to SELinux.
>>
>> A nicer way to do this would be to extend scripts/selinux/genheaders
>> or a similar tool to auto-generate the necessary definitions and code
>> for SELinux capability checking from include/uapi/linux/capability.h.
>> AppArmor does something similar in its Makefile, although it only
>> needs to generate a single table of names.  That is left as future work.
>>
>> Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
>> ---
>>  security/selinux/include/classmap.h | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
>> index 1f1f4b2..e2d4ad3a 100644
>> --- a/security/selinux/include/classmap.h
>> +++ b/security/selinux/include/classmap.h
>> @@ -24,6 +24,10 @@
>>  #define COMMON_CAP2_PERMS  "mac_override", "mac_admin", "syslog", \
>>                 "wake_alarm", "block_suspend", "audit_read"
>>
>> +#if CAP_LAST_CAP > CAP_AUDIT_READ
>> +#error New capability defined, please update COMMON_CAP2_PERMS.
>> +#endif
> 
> I think the obvious question here is why not use BUILD_BUG_ON() here?
> I understand that it can be disabled, but it seems like the "good
> neighbor" option compared to the #error pragma.

I wanted the error to be triggered in the file that needs to be updated,
and preferably close to the line that needs to be updated.
BUILD_BUG_ON() and friends can only be used within a function, and there
is no function in view in classmap.h.  We could put a BUILD_BUG_ON() in
one of the functions that use secclass_map[], e.g. avc_dump_av(), but it
will be less directly correlated to what needs to be updated - it won't
be the correct file.

Re: [PATCH] selinux: keep SELinux in sync with new capability definitions

[Paul Moore]

On Mon, Nov 21, 2016 at 9:49 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On 11/20/2016 05:29 PM, Paul Moore wrote:
>> On Fri, Nov 18, 2016 at 10:23 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
>>> When a new capability is defined, SELinux needs to be updated.
>>> Trigger a build error if a new capability is defined without
>>> corresponding update to security/selinux/include/classmap.h's
>>> COMMON_CAP2_PERMS.  This is similar to BUILD_BUG_ON() guards
>>> in the SELinux nlmsgtab code to ensure that SELinux tracks
>>> new netlink message types as needed.
>>>
>>> Note that there is already a similar build guard in
>>> security/selinux/hooks.c to detect when more than 64
>>> capabilities are defined, since that will require adding
>>> a third capability class to SELinux.
>>>
>>> A nicer way to do this would be to extend scripts/selinux/genheaders
>>> or a similar tool to auto-generate the necessary definitions and code
>>> for SELinux capability checking from include/uapi/linux/capability.h.
>>> AppArmor does something similar in its Makefile, although it only
>>> needs to generate a single table of names.  That is left as future work.
>>>
>>> Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
>>> ---
>>>  security/selinux/include/classmap.h | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
>>> index 1f1f4b2..e2d4ad3a 100644
>>> --- a/security/selinux/include/classmap.h
>>> +++ b/security/selinux/include/classmap.h
>>> @@ -24,6 +24,10 @@
>>>  #define COMMON_CAP2_PERMS  "mac_override", "mac_admin", "syslog", \
>>>                 "wake_alarm", "block_suspend", "audit_read"
>>>
>>> +#if CAP_LAST_CAP > CAP_AUDIT_READ
>>> +#error New capability defined, please update COMMON_CAP2_PERMS.
>>> +#endif
>>
>> I think the obvious question here is why not use BUILD_BUG_ON() here?
>> I understand that it can be disabled, but it seems like the "good
>> neighbor" option compared to the #error pragma.
>
> I wanted the error to be triggered in the file that needs to be updated,
> and preferably close to the line that needs to be updated.
> BUILD_BUG_ON() and friends can only be used within a function, and there
> is no function in view in classmap.h.  We could put a BUILD_BUG_ON() in
> one of the functions that use secclass_map[], e.g. avc_dump_av(), but it
> will be less directly correlated to what needs to be updated - it won't
> be the correct file.

Okay, I see the value in keeping the error close to the root cause.  Merged.

-- 
paul moore
www.paul-moore.com

Re: [PATCH] selinux: keep SELinux in sync with new capability definitions

[Nicolas Iooss]

On 18/11/16 16:23, Stephen Smalley wrote:
> When a new capability is defined, SELinux needs to be updated.
> Trigger a build error if a new capability is defined without
> corresponding update to security/selinux/include/classmap.h's
> COMMON_CAP2_PERMS.  This is similar to BUILD_BUG_ON() guards
> in the SELinux nlmsgtab code to ensure that SELinux tracks
> new netlink message types as needed.
> 
> Note that there is already a similar build guard in
> security/selinux/hooks.c to detect when more than 64
> capabilities are defined, since that will require adding
> a third capability class to SELinux.
> 
> A nicer way to do this would be to extend scripts/selinux/genheaders
> or a similar tool to auto-generate the necessary definitions and code
> for SELinux capability checking from include/uapi/linux/capability.h.
> AppArmor does something similar in its Makefile, although it only
> needs to generate a single table of names.  That is left as future work.
> 
> Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
> ---
>  security/selinux/include/classmap.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
> index 1f1f4b2..e2d4ad3a 100644
> --- a/security/selinux/include/classmap.h
> +++ b/security/selinux/include/classmap.h
> @@ -24,6 +24,10 @@
>  #define COMMON_CAP2_PERMS  "mac_override", "mac_admin", "syslog", \
>  		"wake_alarm", "block_suspend", "audit_read"
>  
> +#if CAP_LAST_CAP > CAP_AUDIT_READ
> +#error New capability defined, please update COMMON_CAP2_PERMS.
> +#endif
> +
>  /*
>   * Note: The name for any socket class should be suffixed by "socket",
>   *	 and doesn't contain more than one substr of "socket".
> 

Hello,
This patch made the compiler I am using to build the kernel (clang)
report two new warnings when building
scripts/selinux/genheaders/genheaders.c and scripts/selinux/mdp/mdp.c:

'CAP_LAST_CAP' is not defined, evaluates to 0 [-Wundef]
'CAP_AUDIT_READ' is not defined, evaluates to 0 [-Wundef]

Even though this is not detected by gcc, it seems like a bug to compare
undefined values. There is no issue where classmap.h is included from
security/selinux/avc.c because include/uapi/linux/capability.h got
included too.

I see two ways of fixing these warnings: either by defining the
capability values in genheaders and mdp by adding #include
<linux/capability.h>, or by adding "defined(__KERNEL__) &&" before the
test so that it is only processed from kernel code (avc.c). How would
you like this to be fixed?

Thanks,
Nicolas

Re: [PATCH] selinux: keep SELinux in sync with new capability definitions

[Stephen Smalley]

On Sun, 2016-12-18 at 21:06 +0100, Nicolas Iooss wrote:
> On 18/11/16 16:23, Stephen Smalley wrote:
> > 
> > When a new capability is defined, SELinux needs to be updated.
> > Trigger a build error if a new capability is defined without
> > corresponding update to security/selinux/include/classmap.h's
> > COMMON_CAP2_PERMS.  This is similar to BUILD_BUG_ON() guards
> > in the SELinux nlmsgtab code to ensure that SELinux tracks
> > new netlink message types as needed.
> > 
> > Note that there is already a similar build guard in
> > security/selinux/hooks.c to detect when more than 64
> > capabilities are defined, since that will require adding
> > a third capability class to SELinux.
> > 
> > A nicer way to do this would be to extend
> > scripts/selinux/genheaders
> > or a similar tool to auto-generate the necessary definitions and
> > code
> > for SELinux capability checking from
> > include/uapi/linux/capability.h.
> > AppArmor does something similar in its Makefile, although it only
> > needs to generate a single table of names.  That is left as future
> > work.
> > 
> > Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
> > ---
> >  security/selinux/include/classmap.h | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/security/selinux/include/classmap.h
> > b/security/selinux/include/classmap.h
> > index 1f1f4b2..e2d4ad3a 100644
> > --- a/security/selinux/include/classmap.h
> > +++ b/security/selinux/include/classmap.h
> > @@ -24,6 +24,10 @@
> >  #define COMMON_CAP2_PERMS  "mac_override", "mac_admin", "syslog",
> > \
> >  		"wake_alarm", "block_suspend", "audit_read"
> >  
> > +#if CAP_LAST_CAP > CAP_AUDIT_READ
> > +#error New capability defined, please update COMMON_CAP2_PERMS.
> > +#endif
> > +
> >  /*
> >   * Note: The name for any socket class should be suffixed by
> > "socket",
> >   *	 and doesn't contain more than one substr of "socket".
> > 
> 
> Hello,
> This patch made the compiler I am using to build the kernel (clang)
> report two new warnings when building
> scripts/selinux/genheaders/genheaders.c and
> scripts/selinux/mdp/mdp.c:
> 
> 'CAP_LAST_CAP' is not defined, evaluates to 0 [-Wundef]
> 'CAP_AUDIT_READ' is not defined, evaluates to 0 [-Wundef]
> 
> Even though this is not detected by gcc, it seems like a bug to
> compare
> undefined values. There is no issue where classmap.h is included from
> security/selinux/avc.c because include/uapi/linux/capability.h got
> included too.
> 
> I see two ways of fixing these warnings: either by defining the
> capability values in genheaders and mdp by adding #include
> <linux/capability.h>, or by adding "defined(__KERNEL__) &&" before
> the
> test so that it is only processed from kernel code (avc.c). How would
> you like this to be fixed?

I suppose we ought to #include <uapi/linux/capability.h> in classmap.h.

Re: [PATCH] selinux: keep SELinux in sync with new capability definitions

[Paul Moore]

On Mon, Dec 19, 2016 at 9:24 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On Sun, 2016-12-18 at 21:06 +0100, Nicolas Iooss wrote:
>> On 18/11/16 16:23, Stephen Smalley wrote:
>> >
>> > When a new capability is defined, SELinux needs to be updated.
>> > Trigger a build error if a new capability is defined without
>> > corresponding update to security/selinux/include/classmap.h's
>> > COMMON_CAP2_PERMS.  This is similar to BUILD_BUG_ON() guards
>> > in the SELinux nlmsgtab code to ensure that SELinux tracks
>> > new netlink message types as needed.
>> >
>> > Note that there is already a similar build guard in
>> > security/selinux/hooks.c to detect when more than 64
>> > capabilities are defined, since that will require adding
>> > a third capability class to SELinux.
>> >
>> > A nicer way to do this would be to extend
>> > scripts/selinux/genheaders
>> > or a similar tool to auto-generate the necessary definitions and
>> > code
>> > for SELinux capability checking from
>> > include/uapi/linux/capability.h.
>> > AppArmor does something similar in its Makefile, although it only
>> > needs to generate a single table of names.  That is left as future
>> > work.
>> >
>> > Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
>> > ---
>> >  security/selinux/include/classmap.h | 4 ++++
>> >  1 file changed, 4 insertions(+)
>> >
>> > diff --git a/security/selinux/include/classmap.h
>> > b/security/selinux/include/classmap.h
>> > index 1f1f4b2..e2d4ad3a 100644
>> > --- a/security/selinux/include/classmap.h
>> > +++ b/security/selinux/include/classmap.h
>> > @@ -24,6 +24,10 @@
>> >  #define COMMON_CAP2_PERMS  "mac_override", "mac_admin", "syslog",
>> > \
>> >             "wake_alarm", "block_suspend", "audit_read"
>> >
>> > +#if CAP_LAST_CAP > CAP_AUDIT_READ
>> > +#error New capability defined, please update COMMON_CAP2_PERMS.
>> > +#endif
>> > +
>> >  /*
>> >   * Note: The name for any socket class should be suffixed by
>> > "socket",
>> >   *  and doesn't contain more than one substr of "socket".
>> >
>>
>> Hello,
>> This patch made the compiler I am using to build the kernel (clang)
>> report two new warnings when building
>> scripts/selinux/genheaders/genheaders.c and
>> scripts/selinux/mdp/mdp.c:
>>
>> 'CAP_LAST_CAP' is not defined, evaluates to 0 [-Wundef]
>> 'CAP_AUDIT_READ' is not defined, evaluates to 0 [-Wundef]
>>
>> Even though this is not detected by gcc, it seems like a bug to
>> compare
>> undefined values. There is no issue where classmap.h is included from
>> security/selinux/avc.c because include/uapi/linux/capability.h got
>> included too.
>>
>> I see two ways of fixing these warnings: either by defining the
>> capability values in genheaders and mdp by adding #include
>> <linux/capability.h>, or by adding "defined(__KERNEL__) &&" before
>> the
>> test so that it is only processed from kernel code (avc.c). How would
>> you like this to be fixed?
>
> I suppose we ought to #include <uapi/linux/capability.h> in classmap.h.

Yep.  Unless one of you wants to beat me to it, I'll put a quick patch
together tomorrow.

-- 
paul moore
www.paul-moore.com

Re: [PATCH] selinux: keep SELinux in sync with new capability definitions

[Paul Moore]

On Mon, Dec 19, 2016 at 8:35 PM, Paul Moore <paul@paul-moore.com> wrote:
> On Mon, Dec 19, 2016 at 9:24 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
>> On Sun, 2016-12-18 at 21:06 +0100, Nicolas Iooss wrote:
>>> Hello,
>>> This patch made the compiler I am using to build the kernel (clang)
>>> report two new warnings when building
>>> scripts/selinux/genheaders/genheaders.c and
>>> scripts/selinux/mdp/mdp.c:
>>>
>>> 'CAP_LAST_CAP' is not defined, evaluates to 0 [-Wundef]
>>> 'CAP_AUDIT_READ' is not defined, evaluates to 0 [-Wundef]
>>>
>>> Even though this is not detected by gcc, it seems like a bug to
>>> compare
>>> undefined values. There is no issue where classmap.h is included from
>>> security/selinux/avc.c because include/uapi/linux/capability.h got
>>> included too.
>>>
>>> I see two ways of fixing these warnings: either by defining the
>>> capability values in genheaders and mdp by adding #include
>>> <linux/capability.h>, or by adding "defined(__KERNEL__) &&" before
>>> the
>>> test so that it is only processed from kernel code (avc.c). How would
>>> you like this to be fixed?
>>
>> I suppose we ought to #include <uapi/linux/capability.h> in classmap.h.
>
> Yep.  Unless one of you wants to beat me to it, I'll put a quick patch
> together tomorrow.

See the patch I just posted to the list.  It turns out it wasn't quite
that easy due to conflicts between the kernel and system among the
various nested includes, but I think the posted patch should solve
everything, if not please let me know.  If I don't hear anything, I'll
push this up to James later this week (tomorrow?) for inclusion into
v4.10.

-- 
paul moore
www.paul-moore.com

Re: [PATCH] selinux: keep SELinux in sync with new capability definitions

[Nicolas Iooss]

On 20/12/16 18:49, Paul Moore wrote:
> On Mon, Dec 19, 2016 at 8:35 PM, Paul Moore <paul@paul-moore.com> wrote:
>> On Mon, Dec 19, 2016 at 9:24 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
>>> On Sun, 2016-12-18 at 21:06 +0100, Nicolas Iooss wrote:
>>>> Hello,
>>>> This patch made the compiler I am using to build the kernel (clang)
>>>> report two new warnings when building
>>>> scripts/selinux/genheaders/genheaders.c and
>>>> scripts/selinux/mdp/mdp.c:
>>>>
>>>> 'CAP_LAST_CAP' is not defined, evaluates to 0 [-Wundef]
>>>> 'CAP_AUDIT_READ' is not defined, evaluates to 0 [-Wundef]
>>>>
>>>> Even though this is not detected by gcc, it seems like a bug to
>>>> compare
>>>> undefined values. There is no issue where classmap.h is included from
>>>> security/selinux/avc.c because include/uapi/linux/capability.h got
>>>> included too.
>>>>
>>>> I see two ways of fixing these warnings: either by defining the
>>>> capability values in genheaders and mdp by adding #include
>>>> <linux/capability.h>, or by adding "defined(__KERNEL__) &&" before
>>>> the
>>>> test so that it is only processed from kernel code (avc.c). How would
>>>> you like this to be fixed?
>>>
>>> I suppose we ought to #include <uapi/linux/capability.h> in classmap.h.
>>
>> Yep.  Unless one of you wants to beat me to it, I'll put a quick patch
>> together tomorrow.
> 
> See the patch I just posted to the list.  It turns out it wasn't quite
> that easy due to conflicts between the kernel and system among the
> various nested includes, but I think the posted patch should solve
> everything, if not please let me know.  If I don't hear anything, I'll
> push this up to James later this week (tomorrow?) for inclusion into
> v4.10.

Hello,
I confirm the patch you posted fixed the warnings I had. Nevertheless
when I take a look at which file got included by
scripts/selinux/mdp/mdp.c, it appears that classmap.h includes the
system header /usr/include/linux/capability.h instead of
include/uapi/linux/capability.h (unlike genheaders, which included the
last file). Is this something you wanted?

Thanks!
Nicolas

Re: [PATCH] selinux: keep SELinux in sync with new capability definitions

[Paul Moore]

On Wed, Dec 21, 2016 at 10:07 AM, Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
> On 20/12/16 18:49, Paul Moore wrote:
>> See the patch I just posted to the list.  It turns out it wasn't quite
>> that easy due to conflicts between the kernel and system among the
>> various nested includes, but I think the posted patch should solve
>> everything, if not please let me know.  If I don't hear anything, I'll
>> push this up to James later this week (tomorrow?) for inclusion into
>> v4.10.
>
> Hello,
> I confirm the patch you posted fixed the warnings I had. Nevertheless
> when I take a look at which file got included by
> scripts/selinux/mdp/mdp.c, it appears that classmap.h includes the
> system header /usr/include/linux/capability.h instead of
> include/uapi/linux/capability.h (unlike genheaders, which included the
> last file). Is this something you wanted?

Probably not, thanks for pointing this out; give me a few minutes and
I'll post a v2 patch.

-- 
paul moore
www.paul-moore.com