[PATCH ghak46 V1] audit: normalize MAC

Post by Richard Guy Briggs
There were two formats of the audit MAC_STATUS record, one of which was more
standard than the other. One listed enforcing status changes and the
other listed enabled status changes with a non-standard label. In
addition, the record was missing information about which LSM was
responsible and the operation's completion status. While this record is
only issued on success, the parser expects the res= field to be present.
type=MAC_STATUS msg=audit(1523312831.378:24514): enforcing=0 old_enforcing=1 auid=0 ses=1
type=MAC_STATUS msg=audit(1523312831.378:24514): selinux=0 auid=0 ses=1
List both sets of status and old values and add the lsm= field and the
res= field.
type=MAC_STATUS msg=audit(1523293828.657:891): enforcing=0 old_enforcing=1 auid=0 ses=1 enabled=1 old-enabled=1 lsm=selinux res=1
This record already accompanied a SYSCALL record.
See: https://github.com/linux-audit/audit-kernel/issues/46
---
security/selinux/selinuxfs.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index 00eed84..00b21b2 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -145,10 +145,11 @@ static ssize_t sel_write_enforce(struct file *file, const char __user *buf,
if (length)
goto out;
audit_log(current->audit_context, GFP_KERNEL, AUDIT_MAC_STATUS,
- "enforcing=%d old_enforcing=%d auid=%u ses=%u",
+ "enforcing=%d old_enforcing=%d auid=%u ses=%u"
+ " enabled=%d old-enabled=%d lsm=selinux res=1",
new_value, selinux_enforcing,
from_kuid(&init_user_ns, audit_get_loginuid(current)),
- audit_get_sessionid(current));
+ audit_get_sessionid(current), selinux_enabled, selinux_enabled);

This looks fine.

Post by Richard Guy Briggs
selinux_enforcing = new_value;
if (selinux_enforcing)
avc_ss_reset(0);
@@ -272,9 +273,11 @@ static ssize_t sel_write_disable(struct file *file, const char __user *buf,
if (length)
goto out;
audit_log(current->audit_context, GFP_KERNEL, AUDIT_MAC_STATUS,
- "selinux=0 auid=%u ses=%u",
+ "enforcing=%d old_enforcing=%d auid=%u ses=%u"
+ " enabled=%d old-enabled=%d lsm=selinux res=1",
+ selinux_enforcing, selinux_enforcing,
from_kuid(&init_user_ns, audit_get_loginuid(current)),
- audit_get_sessionid(current));
+ audit_get_sessionid(current), 0, 1);

It needs to be said again that I'm opposed to changes like this:
inserting new fields, removing fields, or otherwise changing the
format in ways that aren't strictly the addition of new fields to the
end of a record is a Bad Thing. However, there are exceptions (there
are *always* exceptions), and this seems like a reasonable change that
shouldn't negatively affect anyone.

I'll merge this once the merge window comes to a close (we are going
to need to base selinux/next on v4.17-rc1).

Post by Richard Guy Briggs
}
length = count;
--
1.8.3.1

--
paul moore
www.paul-moore.com

Paul Moore

2018-04-17 21:59:05 UTC

This looks fine.

inserting new fields, removing fields, or otherwise changing the
format in ways that aren't strictly the addition of new fields to the
end of a record is a Bad Thing. However, there are exceptions (there
are *always* exceptions), and this seems like a reasonable change that
shouldn't negatively affect anyone.
I'll merge this once the merge window comes to a close (we are going
to need to base selinux/next on v4.17-rc1).

Merged into selinux/next, although I should mention that there were
some actual code changes because of the SELinux state consolidation
patches that went into v4.17. The changes were small but please take
a look and make sure everything still looks okay to you.

Post by Richard Guy Briggs
}
length = count;
--
1.8.3.1

--
paul moore
www.paul-moore.com

Richard Guy Briggs

2018-04-17 22:09:47 UTC

This looks fine.

Ok, that was a bit disruptive, but looks ok to me.

Post by Richard Guy Briggs
}
length = count;
--
1.8.3.1

--
paul moore
www.paul-moore.com

- RGB

--
Richard Guy Briggs <***@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

Paul Moore

2018-04-18 01:51:27 UTC

This looks fine.

Ok, that was a bit disruptive, but looks ok to me.

Yes, it was a pretty big change, but it sets the stage for a few
things we are trying to do with SELinux.

Regardless, thanks for giving the merge a quick look.

--
paul moore
www.paul-moore.com

Ondrej Mosnacek

2018-04-16 07:26:23 UTC

This is just a tiny nit but why does "old_enforcing" use an underscore
and "old-enabled" a dash? Shouldn't the style be consistent across
fields?

Just my two cents...

Post by Richard Guy Briggs
new_value, selinux_enforcing,
from_kuid(&init_user_ns, audit_get_loginuid(current)),
- audit_get_sessionid(current));
+ audit_get_sessionid(current), selinux_enabled, selinux_enabled);
selinux_enforcing = new_value;
if (selinux_enforcing)
avc_ss_reset(0);
@@ -272,9 +273,11 @@ static ssize_t sel_write_disable(struct file *file, const char __user *buf,
if (length)
goto out;
audit_log(current->audit_context, GFP_KERNEL, AUDIT_MAC_STATUS,
- "selinux=0 auid=%u ses=%u",
+ "enforcing=%d old_enforcing=%d auid=%u ses=%u"
+ " enabled=%d old-enabled=%d lsm=selinux res=1",
+ selinux_enforcing, selinux_enforcing,

^ also here

Post by Richard Guy Briggs
from_kuid(&init_user_ns, audit_get_loginuid(current)),
- audit_get_sessionid(current));
+ audit_get_sessionid(current), 0, 1);
}
length = count;
--
1.8.3.1
--
Linux-audit mailing list
https://www.redhat.com/mailman/listinfo/linux-audit

--
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.

Richard Guy Briggs

2018-04-16 14:11:01 UTC

This is just a tiny nit but why does "old_enforcing" use an underscore
and "old-enabled" a dash? Shouldn't the style be consistent across
fields?

Yes, but my understanding is a preference for underscore, and not to
change existing field names.

Steve?

Post by Ondrej Mosnacek
Just my two cents...

These details are worth noticing, thank you.

^ also here

Post by Richard Guy Briggs
from_kuid(&init_user_ns, audit_get_loginuid(current)),
- audit_get_sessionid(current));
+ audit_get_sessionid(current), 0, 1);
}
length = count;

Ondrej Mosnacek <omosnace at redhat dot com>

- RGB

--
Richard Guy Briggs <***@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

Ondrej Mosnacek

2018-04-16 14:25:19 UTC

This is just a tiny nit but why does "old_enforcing" use an underscore
and "old-enabled" a dash? Shouldn't the style be consistent across
fields?

Yes, but my understanding is a preference for underscore, and not to
change existing field names.

Ah, I just noticed that the field is already used elsewhere in the
code, so it makes sense to keep it the same. I thought at first that
it is just a typo.

Post by Richard Guy Briggs
Steve?

Post by Ondrej Mosnacek
Just my two cents...

These details are worth noticing, thank you.

^ also here

Post by Richard Guy Briggs
from_kuid(&init_user_ns, audit_get_loginuid(current)),
- audit_get_sessionid(current));
+ audit_get_sessionid(current), 0, 1);
}
length = count;

Ondrej Mosnacek <omosnace at redhat dot com>

- RGB
--
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

--
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.

Steve Grubb

2018-04-16 18:07:21 UTC

Post by Richard Guy Briggs
There were two formats of the audit MAC_STATUS record, one of which was
more standard than the other. One listed enforcing status changes and
the other listed enabled status changes with a non-standard label. In
addition, the record was missing information about which LSM was
responsible and the operation's completion status. While this record is
only issued on success, the parser expects the res= field to be present.
type=MAC_STATUS msg=audit(1523312831.378:24514): enforcing=0
type=MAC_STATUS msg=audit(1523312831.378:24514): selinux=0 auid=0 ses=1
List both sets of status and old values and add the lsm= field and the
res= field.
type=MAC_STATUS msg=audit(1523293828.657:891): enforcing=0
old_enforcing=1 auid=0 ses=1 enabled=1 old-enabled=1 lsm=selinux res=1
This record already accompanied a SYSCALL record.
See: https://github.com/linux-audit/audit-kernel/issues/46
---
security/selinux/selinuxfs.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/security/selinux/selinuxfs.c
b/security/selinux/selinuxfs.c
index 00eed84..00b21b2 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -145,10 +145,11 @@ static ssize_t sel_write_enforce(struct file
*file, const char __user *buf,> >
if (length)
goto out;
audit_log(current->audit_context, GFP_KERNEL,
AUDIT_MAC_STATUS,
- "enforcing=%d old_enforcing=%d auid=%u ses=%u",
+ "enforcing=%d old_enforcing=%d auid=%u ses=%u"
+ " enabled=%d old-enabled=%d lsm=selinux res=1",

This is just a tiny nit but why does "old_enforcing" use an underscore
and "old-enabled" a dash? Shouldn't the style be consistent across
fields?

Well, we have this thing called the field dictionary:

https://github.com/linux-audit/audit-documentation/blob/master/specs/fields/
field-dictionary.csv

If a field exists, we should reuse it and follow the exact formatting for the
value side. In this case, old_enforcing is in the dictionary. So, it should
be used.

Post by Richard Guy Briggs
Yes, but my understanding is a preference for underscore, and not to
change existing field names.
Steve?

When you are gluing 2 words together, I prefer a dash. But, in this case we
alreday have precedent that the field name exists, so we should reuse it.

-Steve

Post by Ondrej Mosnacek
Just my two cents...

These details are worth noticing, thank you.