BUG: normalize AUDIT_MAC_STATUS records

[rgbriggs]

Currently there are two formats for AUDIT_MAC_STATUS records:

• security/selinux/selinuxfs.c:sel_write_enforce(): "enforcing=%d old_enforcing=%d auid=%u ses=%u"
• security/selinux/selinuxfs.c:sel_write_disable(): "selinux=0 auid=%u ses=%u"

I assume the latter should be brought in line with the former? Or both be augmented to include all fields?

Additionally, should auid= and ses= be dropped if this record is accompanied by a syscall record? If not, then should the context passed to audit_log() be NULL?

[pcmoore]

Also related to AUDIT_MAC_STATUS: #60.

[rgbriggs]

@stevegrubb Since the "selinux=" field isn't yet documented, how would you like this record normalized? List both enabled and enforcing status old/new values? List only one pair of old/new values? Use a new record type to indicate selinux enable/disable status? Leave it alone and close it without modification?

[pcmoore]

My opinion is that for most cases the current audit records are the "standard", even when it isn't the most convenient format for Steve's audit userspace.

NOTE: I said "most cases", not "all cases". I just wanted to send another reminder that we are not going to be making major changes to existing records except under extreme cases.

[rgbriggs]

On 2018-02-12 05:15, Paul Moore wrote:

My opinion is that for most cases the current audit records are the "standard", even when it isn't the most convenient format for Steve's audit userspace.

Problem is this one has two standards, but one is more standard than the other (first one, it appears).

NOTE: I said "most cases", not "all cases". I just wanted to send another reminder that we are not going to be making major changes to existing records except under extreme cases.

That's why I'm asking the question before I code anything. Is this one worth fixing?

[pcmoore]

That's why I'm asking the question before I code anything. Is this one worth fixing?

Unless someone is complaining, I'm not sure I would spend time on it at the moment.

[rgbriggs]

@stevegrubb Does this cause parser or normalizer problems to have two formats?

[stevegrubb]

Yes it causes problems. What causes the one with selinux= to be output?

[rgbriggs]

On 2018-03-20 21:02, Steve Grubb wrote:

Yes it causes problems. What causes the one with selinux= to be output?

A write to "/sys/fs/selinux/disable".

[stevegrubb]

OK. That sounds like they are the same thing. I think this should be consolidated around the first event kind since old and new values are needed.

[rgbriggs]

On 2018-03-21 16:21, Steve Grubb wrote:

OK. That sounds like they are the same thing. I think this should be consolidated around the first event kind since old and new values are needed.

It is close, but not identical. If I understand correctly, there are three states expressed by two booleans: disabled, permissive, enforcing.

        | enforce | permiss |

enable | 1 | 0 | disable | -1 | -1 |

I'd suggest "old-state=X state=Y" to be clearer and not have the old-VAR= and VAR= fields swinging in and out, but this violates the "don't change the name/order of fields rule. What if the MAC is some other non-stock kernel LSM?

How about encoding the "selinux=0" value as "enforcing=-1"? Or -2 or +2?

[stevegrubb]

How about putting both (enabled & enforcing) in the same status event? I think that is the easiest fix.

[rgbriggs]

On 2018-03-26 22:18, Steve Grubb wrote:

How about putting both (enabled & enforcing) in the same status event? I think that is the easiest fix.

So how about: "enforcing=%d old_enforcing=%d auid=%u ses=%u enabled=%d old_enabled=%d"

Only one of enforcing or enabled will change with that message type so the other old* field will be redundant, but necessary for consistency. In fact, both "old*" values could be the same as the new values since sel_write_disable() doesn't check if the new value is different before setting it or reporting the action. (I recall you prefer "-", but the existing field uses "_".)

I understand that adding "res=%u" is unnecessary or will cause a problem for some user parsers.

[stevegrubb]

Events must have subj, obj, action, time, and results. This is mandated by common criteria. So, we must have a res= field even if its 1 all the time. Please add it at the end like other events. And curiously, what user parsers might have a problem with this? I only know of 1. Only 1. It needs all 5 fields in every event. Anything having a problem with this cannot possibly meet CC requirements and should not be considered.

[rgbriggs]

So how about: "enforcing=%d old_enforcing=%d auid=%u ses=%u enabled=%d old_enabled=%d lsm=selinux res=1"

[stevegrubb]

Sure.

[pcmoore]

Similar to #47, we need to verify this doesn't break audit2allow and audit2why, although I suspect that will not be an issue.

Also, these can be connected to a SYSCALL record, yes (I'm 99% certain the answer here is "yes)? Assuming so, do we need the "res" field in the MAC_STATUS record?

[rgbriggs]

Test with setenforce 0 followed by installing policycoreutils-python-utils: ausearch -ts boot --raw -m mac_status | audit2why presents no issue.

They are already connected to SYSCALL records.

According to this posting https://www.redhat.com/archives/linux-audit/2016-November/msg00032.html related to https://github.com/linux-audit/audit-kernel/issues/27#issuecomment-262008792 both are needed.

[rgbriggs]

Posted patch upstream to linux-audit, lkml, lsm, selinux: https://www.redhat.com/archives/linux-audit/2018-April/msg00027.html https://lkml.org/lkml/2018/4/9/824

[rgbriggs]

Upstreamed in v4.18-rc1, released in v4.18