Re: [PATCH ghak46 V1] audit: normalize MAC_STATUS record

[Paul Moore]

On Tue, Apr 17, 2018 at 6:09 PM, Richard Guy Briggs  wrote:
> On 2018-04-17 17:59, Paul Moore wrote:
>> On Wed, Apr 11, 2018 at 5:08 PM, Paul Moore  wrote:
>> > On Mon, Apr 9, 2018 at 7:34 PM, Richard Guy Briggs  wrote:
>> >> 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.
>> >>
>> >> old enforcing/permissive:
>> >> type=MAC_STATUS msg=audit(1523312831.378:24514): enforcing=0 
>> >> old_enforcing=1 auid=0 ses=1
>> >> old enable/disable:
>> >> 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.
>> >>
>> >> Here is the new format:
>> >> 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
>> >> Signed-off-by: Richard Guy Briggs 
>> >> ---
>> >>  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(_user_ns, 
>> >> audit_get_loginuid(current)),
>> >> -   audit_get_sessionid(current));
>> >> +   audit_get_sessionid(current), selinux_enabled, 
>> >> selinux_enabled);
>> >
>> > This looks fine.
>> >
>> >> 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(_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).
>>
>> 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.
>
> 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

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH ghak46 V1] audit: normalize MAC_STATUS record

[Richard Guy Briggs]

On 2018-04-17 17:59, Paul Moore wrote:
> On Wed, Apr 11, 2018 at 5:08 PM, Paul Moore  wrote:
> > On Mon, Apr 9, 2018 at 7:34 PM, Richard Guy Briggs  wrote:
> >> 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.
> >>
> >> old enforcing/permissive:
> >> type=MAC_STATUS msg=audit(1523312831.378:24514): enforcing=0 
> >> old_enforcing=1 auid=0 ses=1
> >> old enable/disable:
> >> 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.
> >>
> >> Here is the new format:
> >> 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
> >> Signed-off-by: Richard Guy Briggs 
> >> ---
> >>  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(_user_ns, 
> >> audit_get_loginuid(current)),
> >> -   audit_get_sessionid(current));
> >> +   audit_get_sessionid(current), selinux_enabled, 
> >> selinux_enabled);
> >
> > This looks fine.
> >
> >> 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(_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).
> 
> 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.

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

> >> }
> >>
> >> length = count;
> >> --
> >> 1.8.3.1
> 
> -- 
> paul moore
> www.paul-moore.com

- RGB

--
Richard Guy Briggs 
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

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH ghak46 V1] audit: normalize MAC_STATUS record

[Paul Moore]

On Wed, Apr 11, 2018 at 5:08 PM, Paul Moore  wrote:
> On Mon, Apr 9, 2018 at 7:34 PM, Richard Guy Briggs  wrote:
>> 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.
>>
>> old enforcing/permissive:
>> type=MAC_STATUS msg=audit(1523312831.378:24514): enforcing=0 old_enforcing=1 
>> auid=0 ses=1
>> old enable/disable:
>> 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.
>>
>> Here is the new format:
>> 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
>> Signed-off-by: Richard Guy Briggs 
>> ---
>>  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(_user_ns, 
>> audit_get_loginuid(current)),
>> -   audit_get_sessionid(current));
>> +   audit_get_sessionid(current), selinux_enabled, 
>> selinux_enabled);
>
> This looks fine.
>
>> 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(_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).

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.

>> }
>>
>> length = count;
>> --
>> 1.8.3.1

-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH ghak46 V1] audit: normalize MAC_STATUS record

[Steve Grubb]

On Monday, April 16, 2018 10:11:01 AM EDT Richard Guy Briggs wrote:
> On 2018-04-16 09:26, Ondrej Mosnacek wrote:
> > 2018-04-10 1:34 GMT+02:00 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.
> > > 
> > > old enforcing/permissive:
> > > type=MAC_STATUS msg=audit(1523312831.378:24514): enforcing=0
> > > old_enforcing=1 auid=0 ses=1 old enable/disable:
> > > 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.
> > > 
> > > Here is the new format:
> > > 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
> > > Signed-off-by: Richard Guy Briggs 
> > > ---
> > > 
> > >  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.

> 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

> > Just my two cents...
> 
> These details are worth noticing, thank you.
> 
> > > new_value, selinux_enforcing,
> > > from_kuid(_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
> > 
> > > from_kuid(_user_ns,
> > > audit_get_loginuid(current)),
> > > 
> > > -   audit_get_sessionid(current));
> > > +   audit_get_sessionid(current), 0, 1);
> > > 
> > > }
> > > 
> > > length = count;
> > 
> > Ondrej Mosnacek 
> 
> - RGB
> 
> --
> Richard Guy Briggs 
> 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
> 
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit




--
Linux-audit mailing 

Re: [PATCH ghak46 V1] audit: normalize MAC_STATUS record

[Ondrej Mosnacek]

2018-04-16 16:11 GMT+02:00 Richard Guy Briggs :
> On 2018-04-16 09:26, Ondrej Mosnacek wrote:
>> 2018-04-10 1:34 GMT+02:00 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.
>> >
>> > old enforcing/permissive:
>> > type=MAC_STATUS msg=audit(1523312831.378:24514): enforcing=0 
>> > old_enforcing=1 auid=0 ses=1
>> > old enable/disable:
>> > 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.
>> >
>> > Here is the new format:
>> > 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
>> > Signed-off-by: Richard Guy Briggs 
>> > ---
>> >  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?
>
> 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.

>
> Steve?
>
>> Just my two cents...
>
> These details are worth noticing, thank you.
>
>> > new_value, selinux_enforcing,
>> > from_kuid(_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
>>
>> > from_kuid(_user_ns, 
>> > audit_get_loginuid(current)),
>> > -   audit_get_sessionid(current));
>> > +   audit_get_sessionid(current), 0, 1);
>> > }
>> >
>> > length = count;
>>
>> Ondrej Mosnacek 
>
> - RGB
>
> --
> Richard Guy Briggs 
> 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 
Associate Software Engineer, Security Technologies
Red Hat, Inc.

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH ghak46 V1] audit: normalize MAC_STATUS record

[Richard Guy Briggs]

On 2018-04-16 09:26, Ondrej Mosnacek wrote:
> 2018-04-10 1:34 GMT+02:00 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.
> >
> > old enforcing/permissive:
> > type=MAC_STATUS msg=audit(1523312831.378:24514): enforcing=0 
> > old_enforcing=1 auid=0 ses=1
> > old enable/disable:
> > 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.
> >
> > Here is the new format:
> > 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
> > Signed-off-by: Richard Guy Briggs 
> > ---
> >  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?

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

Steve?

> Just my two cents...

These details are worth noticing, thank you.

> > new_value, selinux_enforcing,
> > from_kuid(_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
> 
> > from_kuid(_user_ns, 
> > audit_get_loginuid(current)),
> > -   audit_get_sessionid(current));
> > +   audit_get_sessionid(current), 0, 1);
> > }
> >
> > length = count;
> 
> Ondrej Mosnacek 

- RGB

--
Richard Guy Briggs 
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

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH ghak46 V1] audit: normalize MAC_STATUS record

[Ondrej Mosnacek]

2018-04-10 1:34 GMT+02:00 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.
>
> old enforcing/permissive:
> type=MAC_STATUS msg=audit(1523312831.378:24514): enforcing=0 old_enforcing=1 
> auid=0 ses=1
> old enable/disable:
> 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.
>
> Here is the new format:
> 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
> Signed-off-by: Richard Guy Briggs 
> ---
>  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?

Just my two cents...

> new_value, selinux_enforcing,
> from_kuid(_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

> from_kuid(_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
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit

-- 
Ondrej Mosnacek 
Associate Software Engineer, Security Technologies
Red Hat, Inc.

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH ghak46 V1] audit: normalize MAC_STATUS record

[Paul Moore]

On Mon, Apr 9, 2018 at 7:34 PM, Richard Guy Briggs  wrote:
> 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.
>
> old enforcing/permissive:
> type=MAC_STATUS msg=audit(1523312831.378:24514): enforcing=0 old_enforcing=1 
> auid=0 ses=1
> old enable/disable:
> 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.
>
> Here is the new format:
> 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
> Signed-off-by: Richard Guy Briggs 
> ---
>  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(_user_ns, audit_get_loginuid(current)),
> -   audit_get_sessionid(current));
> +   audit_get_sessionid(current), selinux_enabled, 
> selinux_enabled);

This looks fine.

> 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(_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).

> }
>
> length = count;
> --
> 1.8.3.1
>

-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit