[PATCH] Fix AUDIT_MAC_POLICY_LOAD event formatting

[PATCH] Fix AUDIT_MAC_POLICY_LOAD event formatting

[Steve Grubb]

The AUDIT_MAC_POLICY_LOAD event has dangling text that means the same thing
as the event type and is missing the uid and results field. The bigger issue
is that in some failure cases no event is emitted. This patch fixes the noted
problems.

Signed-off-by: Steve Grubb <sgrubb@redhat.com>

---

--- vanilla-4.9-rc5.orig/security/selinux/selinuxfs.c	2016-11-16 15:16:34.738723900 -0500
+++ linux-4.9.0-0.rc5.git0.1.fc24.x86_64/security/selinux/selinuxfs.c	2016-11-21 12:16:08.046787604 -0500
@@ -494,6 +494,7 @@ static ssize_t sel_write_load(struct fil
 {
 	ssize_t length;
 	void *data = NULL;
+	unsigned int result = 0;
 
 	mutex_lock(&sel_mutex);
 
@@ -525,24 +526,26 @@ static ssize_t sel_write_load(struct fil
 
 	length = sel_make_bools();
 	if (length)
-		goto out1;
+		goto out;
 
 	length = sel_make_classes();
 	if (length)
-		goto out1;
+		goto out;
 
 	length = sel_make_policycap();
 	if (length)
-		goto out1;
+		goto out;
 
 	length = count;
+	result = 1;
 
-out1:
+out:
 	audit_log(current->audit_context, GFP_KERNEL, AUDIT_MAC_POLICY_LOAD,
-		"policy loaded auid=%u ses=%u",
+		"uid=%u auid=%u ses=%u res=%u",
+		from_kuid(&init_user_ns, task_uid(current)),
 		from_kuid(&init_user_ns, audit_get_loginuid(current)),
-		audit_get_sessionid(current));
-out:
+		audit_get_sessionid(current), result);
+
 	mutex_unlock(&sel_mutex);
 	vfree(data);
 	return length;

Re: [PATCH] Fix AUDIT_MAC_POLICY_LOAD event formatting

[Paul Moore]

On Mon, Nov 21, 2016 at 12:30 PM, Steve Grubb <sgrubb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> The AUDIT_MAC_POLICY_LOAD event has dangling text that means the same thing
> as the event type and is missing the uid and results field. The bigger issue
> is that in some failure cases no event is emitted. This patch fixes the noted
> problems.
>
> Signed-off-by: Steve Grubb <sgrubb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

First off, for patches such as these, I think it is good to CC the
affected subsystem, SELinux in this case (fixed).

Beyond that, I'm a little concerned that you adding fields to record
in the middle.  In the past you've warned against inserting fields in
the middle of the record, or reordering fields in general ("you'll
break the world") due to some poor userspace practices, yet you do
these exact things when it suits you.

We need a consistent message when it comes to userspace record
processing so we know what we can do in the kernel without causing
massive failure.

> --- vanilla-4.9-rc5.orig/security/selinux/selinuxfs.c   2016-11-16 15:16:34.738723900 -0500
> +++ linux-4.9.0-0.rc5.git0.1.fc24.x86_64/security/selinux/selinuxfs.c   2016-11-21 12:16:08.046787604 -0500
> @@ -494,6 +494,7 @@ static ssize_t sel_write_load(struct fil
>  {
>         ssize_t length;
>         void *data = NULL;
> +       unsigned int result = 0;
>
>         mutex_lock(&sel_mutex);
>
> @@ -525,24 +526,26 @@ static ssize_t sel_write_load(struct fil
>
>         length = sel_make_bools();
>         if (length)
> -               goto out1;
> +               goto out;
>
>         length = sel_make_classes();
>         if (length)
> -               goto out1;
> +               goto out;
>
>         length = sel_make_policycap();
>         if (length)
> -               goto out1;
> +               goto out;
>
>         length = count;
> +       result = 1;
>
> -out1:
> +out:
>         audit_log(current->audit_context, GFP_KERNEL, AUDIT_MAC_POLICY_LOAD,
> -               "policy loaded auid=%u ses=%u",
> +               "uid=%u auid=%u ses=%u res=%u",
> +               from_kuid(&init_user_ns, task_uid(current)),
>                 from_kuid(&init_user_ns, audit_get_loginuid(current)),
> -               audit_get_sessionid(current));
> -out:
> +               audit_get_sessionid(current), result);
> +
>         mutex_unlock(&sel_mutex);
>         vfree(data);
>         return length;
>
> --
> Linux-audit mailing list
> Linux-audit-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org
> https://www.redhat.com/mailman/listinfo/linux-audit



-- 
paul moore
www.paul-moore.com
_______________________________________________
Selinux mailing list
Selinux-+05T5uksL2qpZYMLLGbcSA@public.gmane.org
To unsubscribe, send email to Selinux-leave-+05T5uksL2pAGbPMOrvdOA@public.gmane.org
To get help, send an email containing "help" to Selinux-request-+05T5uksL2pAGbPMOrvdOA@public.gmane.org

Re: [PATCH] Fix AUDIT_MAC_POLICY_LOAD event formatting

[Steve Grubb]

On Monday, November 21, 2016 4:50:03 PM EST Paul Moore wrote:
> On Mon, Nov 21, 2016 at 12:30 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> > The AUDIT_MAC_POLICY_LOAD event has dangling text that means the same
> > thing as the event type and is missing the uid and results field. The
> > bigger issue is that in some failure cases no event is emitted. This patch
> > fixes the noted problems.
> > 
> > Signed-off-by: Steve Grubb <sgrubb@redhat.com>
> 
> First off, for patches such as these, I think it is good to CC the
> affected subsystem, SELinux in this case (fixed).

OK.

> Beyond that, I'm a little concerned that you adding fields to record
> in the middle.  In the past you've warned against inserting fields in
> the middle of the record, or reordering fields in general ("you'll
> break the world") due to some poor userspace practices, yet you do
> these exact things when it suits you.

Its not when its suits me, its when it makes sense. This aligns 
AUDIT_MAC_POLICY_LOAD with more event patterns. What I think the solution is 
is to have a logging function that takes care of the sequence so that people 
do not have to hand code it like this is.

> We need a consistent message when it comes to userspace record
> processing so we know what we can do in the kernel without causing
> massive failure.

I'd like to finish aligning all simple events to a standard for the next kernel 
release.

-Steve

> > --- vanilla-4.9-rc5.orig/security/selinux/selinuxfs.c   2016-11-16
> > 15:16:34.738723900 -0500 +++
> > linux-4.9.0-0.rc5.git0.1.fc24.x86_64/security/selinux/selinuxfs.c  
> > 2016-11-21 12:16:08.046787604 -0500 @@ -494,6 +494,7 @@ static ssize_t
> > sel_write_load(struct fil
> > 
> >  {
> >  
> >         ssize_t length;
> >         void *data = NULL;
> > 
> > +       unsigned int result = 0;
> > 
> >         mutex_lock(&sel_mutex);
> > 
> > @@ -525,24 +526,26 @@ static ssize_t sel_write_load(struct fil
> > 
> >         length = sel_make_bools();
> >         if (length)
> > 
> > -               goto out1;
> > +               goto out;
> > 
> >         length = sel_make_classes();
> >         if (length)
> > 
> > -               goto out1;
> > +               goto out;
> > 
> >         length = sel_make_policycap();
> >         if (length)
> > 
> > -               goto out1;
> > +               goto out;
> > 
> >         length = count;
> > 
> > +       result = 1;
> > 
> > -out1:
> > 
> > +out:
> >         audit_log(current->audit_context, GFP_KERNEL,
> >         AUDIT_MAC_POLICY_LOAD,
> > 
> > -               "policy loaded auid=%u ses=%u",
> > +               "uid=%u auid=%u ses=%u res=%u",
> > +               from_kuid(&init_user_ns, task_uid(current)),
> > 
> >                 from_kuid(&init_user_ns, audit_get_loginuid(current)),
> > 
> > -               audit_get_sessionid(current));
> > -out:
> > +               audit_get_sessionid(current), result);
> > +
> > 
> >         mutex_unlock(&sel_mutex);
> >         vfree(data);
> >         return length;
> > 
> > --
> > Linux-audit mailing list
> > Linux-audit@redhat.com
> > https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH] Fix AUDIT_MAC_POLICY_LOAD event formatting

[Stephen Smalley]

On 11/21/2016 04:50 PM, Paul Moore wrote:
> On Mon, Nov 21, 2016 at 12:30 PM, Steve Grubb <sgrubb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> The AUDIT_MAC_POLICY_LOAD event has dangling text that means the same thing
>> as the event type and is missing the uid and results field. The bigger issue
>> is that in some failure cases no event is emitted. This patch fixes the noted
>> problems.

A potential problem with this patch is that it changes the semantic
meaning of this audit record, from meaning "a policy was loaded into the
kernel" to "there was an attempt to load a policy, check the res= field
to determine whether it succeeded".  So anything in userspace that used
the presence of this audit record to determine whether or not policy was
successfully loaded (e.g. audit2allow -l) will be confused.  While there
were failure cases that would still generate the audit record
previously, those were all selinuxfs node creation failures; the policy
would nonetheless have been loaded into the kernel and would be active
at that point, so saying res=0 is somewhat misleading.  This overlaps
with https://github.com/SELinuxProject/selinux-kernel/issues/1, which
highlights the fact that we can end up in an intermediate state where
policy is loaded but selinuxfs (particularly booleans, class/*, and
policy_capabilities/*) has not been regenerated.

>>
>> Signed-off-by: Steve Grubb <sgrubb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> 
> First off, for patches such as these, I think it is good to CC the
> affected subsystem, SELinux in this case (fixed).
> 
> Beyond that, I'm a little concerned that you adding fields to record
> in the middle.  In the past you've warned against inserting fields in
> the middle of the record, or reordering fields in general ("you'll
> break the world") due to some poor userspace practices, yet you do
> these exact things when it suits you.
> 
> We need a consistent message when it comes to userspace record
> processing so we know what we can do in the kernel without causing
> massive failure.
> 
>> --- vanilla-4.9-rc5.orig/security/selinux/selinuxfs.c   2016-11-16 15:16:34.738723900 -0500
>> +++ linux-4.9.0-0.rc5.git0.1.fc24.x86_64/security/selinux/selinuxfs.c   2016-11-21 12:16:08.046787604 -0500
>> @@ -494,6 +494,7 @@ static ssize_t sel_write_load(struct fil
>>  {
>>         ssize_t length;
>>         void *data = NULL;
>> +       unsigned int result = 0;
>>
>>         mutex_lock(&sel_mutex);
>>
>> @@ -525,24 +526,26 @@ static ssize_t sel_write_load(struct fil
>>
>>         length = sel_make_bools();
>>         if (length)
>> -               goto out1;
>> +               goto out;
>>
>>         length = sel_make_classes();
>>         if (length)
>> -               goto out1;
>> +               goto out;
>>
>>         length = sel_make_policycap();
>>         if (length)
>> -               goto out1;
>> +               goto out;
>>
>>         length = count;
>> +       result = 1;
>>
>> -out1:
>> +out:
>>         audit_log(current->audit_context, GFP_KERNEL, AUDIT_MAC_POLICY_LOAD,
>> -               "policy loaded auid=%u ses=%u",
>> +               "uid=%u auid=%u ses=%u res=%u",
>> +               from_kuid(&init_user_ns, task_uid(current)),
>>                 from_kuid(&init_user_ns, audit_get_loginuid(current)),
>> -               audit_get_sessionid(current));
>> -out:
>> +               audit_get_sessionid(current), result);
>> +
>>         mutex_unlock(&sel_mutex);
>>         vfree(data);
>>         return length;
>>
>> --
>> Linux-audit mailing list
>> Linux-audit-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org
>> https://www.redhat.com/mailman/listinfo/linux-audit
> 
> 
> 

_______________________________________________
Selinux mailing list
Selinux-+05T5uksL2qpZYMLLGbcSA@public.gmane.org
To unsubscribe, send email to Selinux-leave-+05T5uksL2pAGbPMOrvdOA@public.gmane.org
To get help, send an email containing "help" to Selinux-request-+05T5uksL2pAGbPMOrvdOA@public.gmane.org

Re: [PATCH] Fix AUDIT_MAC_POLICY_LOAD event formatting

[Steve Grubb]

On Tuesday, November 22, 2016 8:56:57 AM EST Stephen Smalley wrote:
> On 11/21/2016 04:50 PM, Paul Moore wrote:
> > On Mon, Nov 21, 2016 at 12:30 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> >> The AUDIT_MAC_POLICY_LOAD event has dangling text that means the same
> >> thing
> >> as the event type and is missing the uid and results field. The bigger
> >> issue is that in some failure cases no event is emitted. This patch
> >> fixes the noted problems.
> 
> A potential problem with this patch is that it changes the semantic
> meaning of this audit record, from meaning "a policy was loaded into the
> kernel" to "there was an attempt to load a policy, check the res= field
> to determine whether it succeeded".  So anything in userspace that used
> the presence of this audit record to determine whether or not policy was
> successfully loaded (e.g. audit2allow -l) will be confused.

I really can't have implicit success. I need to have a field to point to that 
says yes/no. It can be hard coded to res=1 (success), but it needs to be 
there.


> While there were failure cases that would still generate the audit record
> previously, those were all selinuxfs node creation failures; the policy
> would nonetheless have been loaded into the kernel and would be active
> at that point, so saying res=0 is somewhat misleading.

OK. We can move the point where res=1 is set. But I would think that its a 
requirement to have an audit record that states that policy failed to load. 
FMT_MSA.3 Static Attribute Initialization. Auditable events: All modifications 
of the initial value of security attributes. I would think this means changes 
such as booleans, modifying labels, loading a new policy, or failure to load a 
policy.


> This overlapswith https://github.com/SELinuxProject/selinux-kernel/issues/1,
> which highlights the fact that we can end up in an intermediate state where
> policy is loaded but selinuxfs (particularly booleans, class/*, and
> policy_capabilities/*) has not been regenerated.

I see. That should be an audited event. If you have a datacenter with a 
thousand machines, its best to get this in the audit trail so it can be 
alerted on at a central collector.

So, what should we do about the patch? I'm willing to modify it.

-Steve

> >> Signed-off-by: Steve Grubb <sgrubb@redhat.com>
> > 
> > First off, for patches such as these, I think it is good to CC the
> > affected subsystem, SELinux in this case (fixed).
> > 
> > Beyond that, I'm a little concerned that you adding fields to record
> > in the middle.  In the past you've warned against inserting fields in
> > the middle of the record, or reordering fields in general ("you'll
> > break the world") due to some poor userspace practices, yet you do
> > these exact things when it suits you.
> > 
> > We need a consistent message when it comes to userspace record
> > processing so we know what we can do in the kernel without causing
> > massive failure.
> > 
> >> --- vanilla-4.9-rc5.orig/security/selinux/selinuxfs.c   2016-11-16
> >> 15:16:34.738723900 -0500 +++
> >> linux-4.9.0-0.rc5.git0.1.fc24.x86_64/security/selinux/selinuxfs.c  
> >> 2016-11-21 12:16:08.046787604 -0500 @@ -494,6 +494,7 @@ static ssize_t
> >> sel_write_load(struct fil
> >> 
> >>  {
> >>  
> >>         ssize_t length;
> >>         void *data = NULL;
> >> 
> >> +       unsigned int result = 0;
> >> 
> >>         mutex_lock(&sel_mutex);
> >> 
> >> @@ -525,24 +526,26 @@ static ssize_t sel_write_load(struct fil
> >> 
> >>         length = sel_make_bools();
> >>         if (length)
> >> 
> >> -               goto out1;
> >> +               goto out;
> >> 
> >>         length = sel_make_classes();
> >>         if (length)
> >> 
> >> -               goto out1;
> >> +               goto out;
> >> 
> >>         length = sel_make_policycap();
> >>         if (length)
> >> 
> >> -               goto out1;
> >> +               goto out;
> >> 
> >>         length = count;
> >> 
> >> +       result = 1;
> >> 
> >> -out1:
> >> 
> >> +out:
> >>         audit_log(current->audit_context, GFP_KERNEL,
> >>         AUDIT_MAC_POLICY_LOAD,
> >> 
> >> -               "policy loaded auid=%u ses=%u",
> >> +               "uid=%u auid=%u ses=%u res=%u",
> >> +               from_kuid(&init_user_ns, task_uid(current)),
> >> 
> >>                 from_kuid(&init_user_ns, audit_get_loginuid(current)),
> >> 
> >> -               audit_get_sessionid(current));
> >> -out:
> >> +               audit_get_sessionid(current), result);
> >> +
> >> 
> >>         mutex_unlock(&sel_mutex);
> >>         vfree(data);
> >>         return length;
> >> 
> >> --
> >> Linux-audit mailing list
> >> Linux-audit@redhat.com
> >> https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH] Fix AUDIT_MAC_POLICY_LOAD event formatting

[Stephen Smalley]

On 11/22/2016 09:28 AM, Steve Grubb wrote:
> On Tuesday, November 22, 2016 8:56:57 AM EST Stephen Smalley wrote:
>> On 11/21/2016 04:50 PM, Paul Moore wrote:
>>> On Mon, Nov 21, 2016 at 12:30 PM, Steve Grubb <sgrubb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>>>> The AUDIT_MAC_POLICY_LOAD event has dangling text that means the same
>>>> thing
>>>> as the event type and is missing the uid and results field. The bigger
>>>> issue is that in some failure cases no event is emitted. This patch
>>>> fixes the noted problems.
>>
>> A potential problem with this patch is that it changes the semantic
>> meaning of this audit record, from meaning "a policy was loaded into the
>> kernel" to "there was an attempt to load a policy, check the res= field
>> to determine whether it succeeded".  So anything in userspace that used
>> the presence of this audit record to determine whether or not policy was
>> successfully loaded (e.g. audit2allow -l) will be confused.
> 
> I really can't have implicit success. I need to have a field to point to that 
> says yes/no. It can be hard coded to res=1 (success), but it needs to be 
> there.

Ok.  Why is it you use res=1|0 in these records but success=yes|no in
syscall records?

>> While there were failure cases that would still generate the audit record
>> previously, those were all selinuxfs node creation failures; the policy
>> would nonetheless have been loaded into the kernel and would be active
>> at that point, so saying res=0 is somewhat misleading.
> 
> OK. We can move the point where res=1 is set. But I would think that its a 
> requirement to have an audit record that states that policy failed to load. 
> FMT_MSA.3 Static Attribute Initialization. Auditable events: All modifications 
> of the initial value of security attributes. I would think this means changes 
> such as booleans, modifying labels, loading a new policy, or failure to load a 
> policy.

Failure to load a policy is not a modification to the initial value of
the security attribute, is it?

> 
> 
>> This overlapswith https://github.com/SELinuxProject/selinux-kernel/issues/1,
>> which highlights the fact that we can end up in an intermediate state where
>> policy is loaded but selinuxfs (particularly booleans, class/*, and
>> policy_capabilities/*) has not been regenerated.
> 
> I see. That should be an audited event. If you have a datacenter with a 
> thousand machines, its best to get this in the audit trail so it can be 
> alerted on at a central collector.
> 
> So, what should we do about the patch? I'm willing to modify it.

At present, we only generate AUDIT_MAC_STATUS, AUDIT_MAC_LOAD, and
AUDIT_MAC_CONFIG_CHANGE on success (or at least partial success).  If
you truly need to audit failures, then it seems like you either need to
a) do it through syscall audit filters, which already provide a success=
field or b) add new audit message types for this purpose (e.g.
AUDIT_MAC_STATUS_FAIL, AUDIT_MAC_LOAD_FAIL, ...).  To just add a res=
field to the existing ones and change them to always be generated is a
user-visible semantic change.

_______________________________________________
Selinux mailing list
Selinux-+05T5uksL2qpZYMLLGbcSA@public.gmane.org
To unsubscribe, send email to Selinux-leave-+05T5uksL2pAGbPMOrvdOA@public.gmane.org
To get help, send an email containing "help" to Selinux-request-+05T5uksL2pAGbPMOrvdOA@public.gmane.org

Re: [PATCH] Fix AUDIT_MAC_POLICY_LOAD event formatting

[Lenny Bruzenak]

[-- Attachment #1.1: Type: text/plain, Size: 710 bytes --]

On 11/22/2016 08:55 AM, Stephen Smalley wrote:

>> >OK. We can move the point where res=1 is set. But I would think that its a
>> >requirement to have an audit record that states that policy failed to load.
>> >FMT_MSA.3 Static Attribute Initialization. Auditable events: All modifications
>> >of the initial value of security attributes. I would think this means changes
>> >such as booleans, modifying labels, loading a new policy, or failure to load a
>> >policy.
> Failure to load a policy is not a modification to the initial value of
> the security attribute, is it?
>
It is definitely relevant, if it falls under another category.
Either a failed malicious intent or a failed supervisory function.

LCB

[-- Attachment #1.2: Type: text/html, Size: 1416 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] Fix AUDIT_MAC_POLICY_LOAD event formatting

[Steve Grubb]

On Tuesday, November 22, 2016 9:55:11 AM EST Stephen Smalley wrote:
> On 11/22/2016 09:28 AM, Steve Grubb wrote:
> > On Tuesday, November 22, 2016 8:56:57 AM EST Stephen Smalley wrote:
> >> On 11/21/2016 04:50 PM, Paul Moore wrote:
> >>> On Mon, Nov 21, 2016 at 12:30 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> >>>> The AUDIT_MAC_POLICY_LOAD event has dangling text that means the same
> >>>> thing as the event type and is missing the uid and results field. The
> >>>> bigger issue is that in some failure cases no event is emitted. This
> >>>> patch fixes the noted problems.
> >> 
> >> A potential problem with this patch is that it changes the semantic
> >> meaning of this audit record, from meaning "a policy was loaded into the
> >> kernel" to "there was an attempt to load a policy, check the res= field
> >> to determine whether it succeeded".  So anything in userspace that used
> >> the presence of this audit record to determine whether or not policy was
> >> successfully loaded (e.g. audit2allow -l) will be confused.
> > 
> > I really can't have implicit success. I need to have a field to point to
> > that says yes/no. It can be hard coded to res=1 (success), but it needs
> > to be there.
> 
> Ok.  Why is it you use res=1|0 in these records but success=yes|no in
> syscall records?

Success is only used in syscall records. It was created to disambiguate the 
exit field which can look like a failure on some arches but is actually OK. 
Originally the exit field was all that existed. We found that one or two arches 
actually have 2 return codes. The whole rest of the audit system uses res= to 
mean a crisp yes/no this did or didn't happen. This predates the creation of 
the success field. It could be argued success should have been res, but 
ausearch/report will use either to mean the same thing.

The new auparse field classifier work will also map both to mean the same thing. 
That is how I found AUDIT_MAC_POLICY_LOAD missing any kind of success/fail 
indication.


> >> While there were failure cases that would still generate the audit record
> >> previously, those were all selinuxfs node creation failures; the policy
> >> would nonetheless have been loaded into the kernel and would be active
> >> at that point, so saying res=0 is somewhat misleading.
> > 
> > OK. We can move the point where res=1 is set. But I would think that its a
> > requirement to have an audit record that states that policy failed to
> > load.
> > FMT_MSA.3 Static Attribute Initialization. Auditable events: All
> > modifications of the initial value of security attributes. I would think
> > this means changes such as booleans, modifying labels, loading a new
> > policy, or failure to load a policy.
> 
> Failure to load a policy is not a modification to the initial value of
> the security attribute, is it?

Sure. If the state is intended to enabled and enforcing and its not, that 
would be a surprising result that there was no indication in the audit trail. 
Also, if for some reason it booted fine and a new policy was loaded at some 
point in the future and it failed, then we have a modification to initial 
state.

> >> This overlapswith
> >> https://github.com/SELinuxProject/selinux-kernel/issues/1, which
> >> highlights the fact that we can end up in an intermediate state where
> >> policy is loaded but selinuxfs (particularly booleans, class/*, and
> >> policy_capabilities/*) has not been regenerated.
> > 
> > I see. That should be an audited event. If you have a datacenter with a
> > thousand machines, its best to get this in the audit trail so it can be
> > alerted on at a central collector.
> > 
> > So, what should we do about the patch? I'm willing to modify it.
> 
> At present, we only generate AUDIT_MAC_STATUS, AUDIT_MAC_LOAD, and
> AUDIT_MAC_CONFIG_CHANGE on success (or at least partial success).  If
> you truly need to audit failures, then it seems like you either need to
> a) do it through syscall audit filters, which already provide a success=
> field

I can't imagine what to audit on. There is an open syscall that has a path. 
But I suspect that does not fail because policy has not be written. There is a 
write syscall but triggering on that is pretty generic. This is not ideal.

> or b) add new audit message types for this purpose (e.g.
> AUDIT_MAC_STATUS_FAIL, AUDIT_MAC_LOAD_FAIL, ...).  To just add a res=
> field to the existing ones and change them to always be generated is a
> user-visible semantic change.

OK. This is do-able. I'll hard code the 'res' field for each of them.

-Steve

Re: [PATCH] Fix AUDIT_MAC_POLICY_LOAD event formatting

[Stephen Smalley]

On 11/22/2016 02:39 PM, Steve Grubb wrote:
> On Tuesday, November 22, 2016 9:55:11 AM EST Stephen Smalley wrote:
>> On 11/22/2016 09:28 AM, Steve Grubb wrote:
>>> On Tuesday, November 22, 2016 8:56:57 AM EST Stephen Smalley wrote:
>>>> On 11/21/2016 04:50 PM, Paul Moore wrote:
>>>>> On Mon, Nov 21, 2016 at 12:30 PM, Steve Grubb <sgrubb@redhat.com> wrote:
>>>>>> The AUDIT_MAC_POLICY_LOAD event has dangling text that means the same
>>>>>> thing as the event type and is missing the uid and results field. The
>>>>>> bigger issue is that in some failure cases no event is emitted. This
>>>>>> patch fixes the noted problems.
>>>>
>>>> A potential problem with this patch is that it changes the semantic
>>>> meaning of this audit record, from meaning "a policy was loaded into the
>>>> kernel" to "there was an attempt to load a policy, check the res= field
>>>> to determine whether it succeeded".  So anything in userspace that used
>>>> the presence of this audit record to determine whether or not policy was
>>>> successfully loaded (e.g. audit2allow -l) will be confused.
>>>
>>> I really can't have implicit success. I need to have a field to point to
>>> that says yes/no. It can be hard coded to res=1 (success), but it needs
>>> to be there.
>>
>> Ok.  Why is it you use res=1|0 in these records but success=yes|no in
>> syscall records?
> 
> Success is only used in syscall records. It was created to disambiguate the 
> exit field which can look like a failure on some arches but is actually OK. 
> Originally the exit field was all that existed. We found that one or two arches 
> actually have 2 return codes. The whole rest of the audit system uses res= to 
> mean a crisp yes/no this did or didn't happen. This predates the creation of 
> the success field. It could be argued success should have been res, but 
> ausearch/report will use either to mean the same thing.
> 
> The new auparse field classifier work will also map both to mean the same thing. 
> That is how I found AUDIT_MAC_POLICY_LOAD missing any kind of success/fail 
> indication.
> 
> 
>>>> While there were failure cases that would still generate the audit record
>>>> previously, those were all selinuxfs node creation failures; the policy
>>>> would nonetheless have been loaded into the kernel and would be active
>>>> at that point, so saying res=0 is somewhat misleading.
>>>
>>> OK. We can move the point where res=1 is set. But I would think that its a
>>> requirement to have an audit record that states that policy failed to
>>> load.
>>> FMT_MSA.3 Static Attribute Initialization. Auditable events: All
>>> modifications of the initial value of security attributes. I would think
>>> this means changes such as booleans, modifying labels, loading a new
>>> policy, or failure to load a policy.
>>
>> Failure to load a policy is not a modification to the initial value of
>> the security attribute, is it?
> 
> Sure. If the state is intended to enabled and enforcing and its not, that 
> would be a surprising result that there was no indication in the audit trail. 
> Also, if for some reason it booted fine and a new policy was loaded at some 
> point in the future and it failed, then we have a modification to initial 
> state.
> 
>>>> This overlapswith
>>>> https://github.com/SELinuxProject/selinux-kernel/issues/1, which
>>>> highlights the fact that we can end up in an intermediate state where
>>>> policy is loaded but selinuxfs (particularly booleans, class/*, and
>>>> policy_capabilities/*) has not been regenerated.
>>>
>>> I see. That should be an audited event. If you have a datacenter with a
>>> thousand machines, its best to get this in the audit trail so it can be
>>> alerted on at a central collector.
>>>
>>> So, what should we do about the patch? I'm willing to modify it.
>>
>> At present, we only generate AUDIT_MAC_STATUS, AUDIT_MAC_LOAD, and
>> AUDIT_MAC_CONFIG_CHANGE on success (or at least partial success).  If
>> you truly need to audit failures, then it seems like you either need to
>> a) do it through syscall audit filters, which already provide a success=
>> field
> 
> I can't imagine what to audit on. There is an open syscall that has a path. 
> But I suspect that does not fail because policy has not be written. There is a 
> write syscall but triggering on that is pretty generic. This is not ideal.

Can't you write an audit syscall filter or watch on
/sys/fs/selinux/load?  Ditto for /sys/fs/selinux/enforce,
/sys/fs/selinux/commit_pending_bools, etc.

> 
>> or b) add new audit message types for this purpose (e.g.
>> AUDIT_MAC_STATUS_FAIL, AUDIT_MAC_LOAD_FAIL, ...).  To just add a res=
>> field to the existing ones and change them to always be generated is a
>> user-visible semantic change.
> 
> OK. This is do-able. I'll hard code the 'res' field for each of them.
> 
> -Steve
> 

Re: [PATCH] Fix AUDIT_MAC_POLICY_LOAD event formatting

[Steve Grubb]

On Tuesday, November 22, 2016 2:47:15 PM EST Stephen Smalley wrote:
> >> At present, we only generate AUDIT_MAC_STATUS, AUDIT_MAC_LOAD, and
> >> AUDIT_MAC_CONFIG_CHANGE on success (or at least partial success).  If
> >> you truly need to audit failures, then it seems like you either need to
> >> a) do it through syscall audit filters, which already provide a success=
> >> field
> > 
> > I can't imagine what to audit on. There is an open syscall that has a
> > path. But I suspect that does not fail because policy has not be written.
> > There is a write syscall but triggering on that is pretty generic. This is
> > not ideal.
>
> Can't you write an audit syscall filter or watch on
> /sys/fs/selinux/load?  Ditto for /sys/fs/selinux/enforce,
> /sys/fs/selinux/commit_pending_bools, etc.

Yes, you can. But this is for the open syscall. sel_write_load() is the 
function where the auditing is done but its mapped to the .write member of 
sel_load_ops. Auditing on write is not a good thing.

So, if AUDIT_MAC_POLICY_LOAD must only appear when there is success, then its 
best to create a second event for failure and hard code the 'res' fields for 
both.

-Steve