Re: [PATCH] Fix AUDIT_MAC_POLICY_LOAD event formatting

[Steve Grubb <sgrubb@redhat.com>]

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