* [RFC PATCH 1/1] libselinux: Revise userspace AVC avc_log() for auditable events.
@ 2020-09-15 13:05 Chris PeBenito
2020-09-15 13:34 ` Stephen Smalley
0 siblings, 1 reply; 6+ messages in thread
From: Chris PeBenito @ 2020-09-15 13:05 UTC (permalink / raw)
To: selinux; +Cc: sgrubb
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.
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.
Signed-off-by: Chris PeBenito <chpebeni@linux.microsoft.com>
---
libselinux/src/avc_internal.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
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);
return rc;
}
@@ -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);
return rc;
}
@@ -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;
}
@@ -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;
}
--
2.26.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC PATCH 1/1] libselinux: Revise userspace AVC avc_log() for auditable events.
2020-09-15 13:05 [RFC PATCH 1/1] libselinux: Revise userspace AVC avc_log() for auditable events Chris PeBenito
@ 2020-09-15 13:34 ` Stephen Smalley
2020-09-15 14:33 ` Chris PeBenito
0 siblings, 1 reply; 6+ messages in thread
From: Stephen Smalley @ 2020-09-15 13:34 UTC (permalink / raw)
To: Chris PeBenito; +Cc: SElinux list, Steve Grubb
On Tue, Sep 15, 2020 at 9:11 AM Chris PeBenito
<chpebeni@linux.microsoft.com> 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH 1/1] libselinux: Revise userspace AVC avc_log() for auditable events.
2020-09-15 13:34 ` Stephen Smalley
@ 2020-09-15 14:33 ` Chris PeBenito
2020-09-15 14:43 ` Stephen Smalley
0 siblings, 1 reply; 6+ messages in thread
From: Chris PeBenito @ 2020-09-15 14:33 UTC (permalink / raw)
To: Stephen Smalley; +Cc: SElinux list, Steve Grubb
On 9/15/20 9:34 AM, Stephen Smalley wrote:
> On Tue, Sep 15, 2020 at 9:11 AM Chris PeBenito
> <chpebeni@linux.microsoft.com> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH 1/1] libselinux: Revise userspace AVC avc_log() for auditable events.
2020-09-15 14:33 ` Chris PeBenito
@ 2020-09-15 14:43 ` Stephen Smalley
2020-09-15 15:04 ` Chris PeBenito
0 siblings, 1 reply; 6+ messages in thread
From: Stephen Smalley @ 2020-09-15 14:43 UTC (permalink / raw)
To: Chris PeBenito; +Cc: SElinux list, Steve Grubb
On Tue, Sep 15, 2020 at 10:33 AM Chris PeBenito
<chpebeni@linux.microsoft.com> wrote:
>
> On 9/15/20 9:34 AM, Stephen Smalley wrote:
> > On Tue, Sep 15, 2020 at 9:11 AM Chris PeBenito
> > <chpebeni@linux.microsoft.com> wrote:
> >> 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?
Yes. On second thought, I don't think any of the SELINUX_ERROR
messages are intended for audit and since that is already a different
type value, the callbacks can already redirect those to stderr or
syslog as appropriate instead of audit.
> > 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.
The AVC reset itself can't fail, only the callbacks registered by the
application. Original design/concept was e.g. if the object manager
could not flush all migrated permissions for some reason. Don't think
there are any such callbacks that can fail today.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH 1/1] libselinux: Revise userspace AVC avc_log() for auditable events.
2020-09-15 14:43 ` Stephen Smalley
@ 2020-09-15 15:04 ` Chris PeBenito
2020-09-15 15:20 ` Stephen Smalley
0 siblings, 1 reply; 6+ messages in thread
From: Chris PeBenito @ 2020-09-15 15:04 UTC (permalink / raw)
To: Stephen Smalley; +Cc: SElinux list, Steve Grubb
On 9/15/20 10:43 AM, Stephen Smalley wrote:
> On Tue, Sep 15, 2020 at 10:33 AM Chris PeBenito
> <chpebeni@linux.microsoft.com> wrote:
>>
>> On 9/15/20 9:34 AM, Stephen Smalley wrote:
>>> On Tue, Sep 15, 2020 at 9:11 AM Chris PeBenito
>>> <chpebeni@linux.microsoft.com> wrote:
>>>> 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?
>
> Yes. On second thought, I don't think any of the SELINUX_ERROR
> messages are intended for audit and since that is already a different
> type value, the callbacks can already redirect those to stderr or
> syslog as appropriate instead of audit.
Are the typebounds and validatetrans in UAVC passed to the kernel for
evaluation? The kernel audits failures on those as SELINUX_ERR. If the UAVC
handles them itself, it seems that those failures should be audited as
USER_SELINUX_ERR.
--
Chris PeBenito
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH 1/1] libselinux: Revise userspace AVC avc_log() for auditable events.
2020-09-15 15:04 ` Chris PeBenito
@ 2020-09-15 15:20 ` Stephen Smalley
0 siblings, 0 replies; 6+ messages in thread
From: Stephen Smalley @ 2020-09-15 15:20 UTC (permalink / raw)
To: Chris PeBenito; +Cc: SElinux list, Steve Grubb
On Tue, Sep 15, 2020 at 11:04 AM Chris PeBenito
<chpebeni@linux.microsoft.com> wrote:
>
> On 9/15/20 10:43 AM, Stephen Smalley wrote:
> > On Tue, Sep 15, 2020 at 10:33 AM Chris PeBenito
> > <chpebeni@linux.microsoft.com> wrote:
> >>
> >> On 9/15/20 9:34 AM, Stephen Smalley wrote:
> >>> On Tue, Sep 15, 2020 at 9:11 AM Chris PeBenito
> >>> <chpebeni@linux.microsoft.com> wrote:
> >>>> 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?
> >
> > Yes. On second thought, I don't think any of the SELINUX_ERROR
> > messages are intended for audit and since that is already a different
> > type value, the callbacks can already redirect those to stderr or
> > syslog as appropriate instead of audit.
>
> Are the typebounds and validatetrans in UAVC passed to the kernel for
> evaluation? The kernel audits failures on those as SELINUX_ERR. If the UAVC
> handles them itself, it seems that those failures should be audited as
> USER_SELINUX_ERR.
The userspace AVC defers all of the security_compute_av() logic to the kernel.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-09-15 23:44 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-15 13:05 [RFC PATCH 1/1] libselinux: Revise userspace AVC avc_log() for auditable events Chris PeBenito
2020-09-15 13:34 ` Stephen Smalley
2020-09-15 14:33 ` Chris PeBenito
2020-09-15 14:43 ` Stephen Smalley
2020-09-15 15:04 ` Chris PeBenito
2020-09-15 15:20 ` Stephen Smalley
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.