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? > 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. 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. > @@ -81,12 +81,12 @@ int avc_process_policyload(uint32_t seqno) > int rc = 0; > > avc_log(SELINUX_POLICYLOAD, > - "%s: received policyload notice (seqno=%u)\n", > + "%s: op=load_policy lsm=selinux_uavc seqno=%u res=1", > avc_prefix, seqno); > rc = avc_ss_reset(seqno); > if (rc < 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); Ditto. > @@ -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. > @@ -214,7 +214,7 @@ static int avc_netlink_process(void *buf) > > errno = -err->error; > avc_log(SELINUX_ERROR, > - "%s: netlink error: %d\n", avc_prefix, errno); > + "%s: op=netlink_msgtype lsm=selinux_uavc errno=%d res=0", avc_prefix, errno); > return -1; > } Ditto.
