On 9/15/20 9:34 AM, Stephen Smalley wrote:
On Tue, Sep 15, 2020 at 9:11 AM Chris PeBenito
<chpebeni@xxxxxxxxxxxxxxxxxxx> wrote:
I put up a PR for dbus-broker to revise its auditing:
https://github.com/bus1/dbus-broker/pull/240
Steve Grubb mentioned that there wasn't much useful info in terms of the audit
message itself, since it isn't key:value pairs.
Does that matter since it is all just stuffed as a string value for
the msg= field?
I'm looking to revise the avc_log() messages for SELINUX_ERROR,
SELINUX_SETENFORCE, and SELINUX_POLICYLOAD messages such that they
more closely reseble the kernel audits.
*This patch is _incomplete*; I implemented a few changes to get early feedback
on the direction I'm taking. What seems potentially contentious is the
'lsm=selinux_uavc' and op= choices.
Yes, I don't see the point of using a different lsm= value than just
"selinux"; the fact that it is a userspace event should get reflected
in some other way, right?
Fair point. It's reflected in USER_MAC_POLICY_LOAD instead of MAC_POLICY_LOAD,
as an example.
diff --git a/libselinux/src/avc_internal.c b/libselinux/src/avc_internal.c
index 572b2159..35ea59b6 100644
--- a/libselinux/src/avc_internal.c
+++ b/libselinux/src/avc_internal.c
@@ -59,14 +59,14 @@ int avc_process_setenforce(int enforcing)
int rc = 0;
avc_log(SELINUX_SETENFORCE,
- "%s: received setenforce notice (enforcing=%d)\n",
+ "%s: op=setenforce lsm=selinux_uavc enforcing=%d res=1",
avc_prefix, enforcing);
if (avc_setenforce)
goto out;
avc_enforcing = enforcing;
if (avc_enforcing && (rc = avc_ss_reset(0)) < 0) {
avc_log(SELINUX_ERROR,
- "%s: cache reset returned %d (errno %d)\n",
+ "%s: op=cache_reset lsm=selinux_uavc rc=%d errno=%d res=0",
avc_prefix, rc, errno);
If we do this at all, I would think the op= would still be setenforce
and this would just be an error for it.
At this point we already audited success for the setenforce operation. Wouldn't
it be confusing to have a op=setenforce res=1 and then immediately op=setenforce
res=0?
Looking at the kernel, we aren't even checking avc_ss_reset() for
failure and none of the kernel avc callbacks ever return an error.
I'm not deeply familiar with the AVC intricacies. If the cache fails to clear,
then there could be wrong cache entries, no? If so, I would argue that it's
worthy of an audit. If not, then I agree that it's not auditable.
@@ -157,7 +157,7 @@ static int avc_netlink_receive(void *buf, unsigned buflen, int blocking)
return -1;
}
else if (rc < 1) {
- avc_log(SELINUX_ERROR, "%s: netlink poll: error %d\n",
+ avc_log(SELINUX_ERROR, "%s: op=netlink_poll lsm=selinux_uavc errno=%d res=0",
avc_prefix, errno);
return rc;
}
Not sure what to do with these; arguably they aren't audit events at
all, just error logging.
Perhaps the question is: are any of SELINUX_ERROR actually auditable? If so, it
would seem the answer is to either add another log level for auditable errors.
If not, then the few UAVC users that audit SELINUX_ERROR need to be fixed.
Alternatively we could change these non-auditable SELINUX_ERRORs to
SELINUX_WARNING, since admins can't do much about these in most (all?) cases.
Maybe we should add a log callback function to libselinux that UAVC users could
optionally use. It would audit where appropriate and otherwise syslog/stderr
the message. That would provide an upgrade path for consistent auditing while
still allowing UAVC users the option to handle the log callback another way when
desired. A another option would be to introduce a new callback specifically for
auditing.
--
Chris PeBenito
