* [PATCH ghak46 V1] audit: normalize MAC_STATUS record
@ 2018-04-09 23:34 ` Richard Guy Briggs
0 siblings, 0 replies; 18+ messages in thread
From: Richard Guy Briggs @ 2018-04-09 23:34 UTC (permalink / raw)
To: Linux-Audit Mailing List, LKML, SElinux list, Linux Security Module list
Cc: Eric Paris, Paul Moore, Steve Grubb, 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 <rgb@redhat.com>
---
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);
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);
}
length = count;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH ghak46 V1] audit: normalize MAC_STATUS record
@ 2018-04-09 23:34 ` Richard Guy Briggs
0 siblings, 0 replies; 18+ messages in thread
From: Richard Guy Briggs @ 2018-04-09 23:34 UTC (permalink / raw)
To: linux-security-module
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 <rgb@redhat.com>
---
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);
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);
}
length = count;
--
1.8.3.1
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH ghak46 V1] audit: normalize MAC_STATUS record
2018-04-09 23:34 ` Richard Guy Briggs
@ 2018-04-11 21:08 ` Paul Moore
-1 siblings, 0 replies; 18+ messages in thread
From: Paul Moore @ 2018-04-11 21:08 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: Linux-Audit Mailing List, LKML, SElinux list,
Linux Security Module list, Eric Paris, Steve Grubb
On Mon, Apr 9, 2018 at 7:34 PM, Richard Guy Briggs <rgb@redhat.com> 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 <rgb@redhat.com>
> ---
> 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.
> 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).
> }
>
> length = count;
> --
> 1.8.3.1
>
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH ghak46 V1] audit: normalize MAC_STATUS record
@ 2018-04-11 21:08 ` Paul Moore
0 siblings, 0 replies; 18+ messages in thread
From: Paul Moore @ 2018-04-11 21:08 UTC (permalink / raw)
To: linux-security-module
On Mon, Apr 9, 2018 at 7:34 PM, Richard Guy Briggs <rgb@redhat.com> 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 <rgb@redhat.com>
> ---
> 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.
> 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).
> }
>
> length = count;
> --
> 1.8.3.1
>
--
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH ghak46 V1] audit: normalize MAC_STATUS record
2018-04-09 23:34 ` Richard Guy Briggs
@ 2018-04-16 7:26 ` Ondrej Mosnacek
-1 siblings, 0 replies; 18+ messages in thread
From: Ondrej Mosnacek @ 2018-04-16 7:26 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: Linux-Audit Mailing List, LKML, SElinux list, Linux Security Module list
2018-04-10 1:34 GMT+02:00 Richard Guy Briggs <rgb@redhat.com>:
> 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 <rgb@redhat.com>
> ---
> 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(&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
> 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
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
--
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH ghak46 V1] audit: normalize MAC_STATUS record
@ 2018-04-16 7:26 ` Ondrej Mosnacek
0 siblings, 0 replies; 18+ messages in thread
From: Ondrej Mosnacek @ 2018-04-16 7:26 UTC (permalink / raw)
To: linux-security-module
2018-04-10 1:34 GMT+02:00 Richard Guy Briggs <rgb@redhat.com>:
> 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 <rgb@redhat.com>
> ---
> 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(&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
> 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
> Linux-audit at redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
--
Ondrej Mosnacek <omosnace@redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH ghak46 V1] audit: normalize MAC_STATUS record
2018-04-16 7:26 ` Ondrej Mosnacek
@ 2018-04-16 14:11 ` Richard Guy Briggs
-1 siblings, 0 replies; 18+ messages in thread
From: Richard Guy Briggs @ 2018-04-16 14:11 UTC (permalink / raw)
To: Ondrej Mosnacek
Cc: Linux-Audit Mailing List, LKML, SElinux list, Linux Security Module list
On 2018-04-16 09:26, Ondrej Mosnacek wrote:
> 2018-04-10 1:34 GMT+02:00 Richard Guy Briggs <rgb@redhat.com>:
> > 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 <rgb@redhat.com>
> > ---
> > 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(&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
>
> > 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 <rgb@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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH ghak46 V1] audit: normalize MAC_STATUS record
@ 2018-04-16 14:11 ` Richard Guy Briggs
0 siblings, 0 replies; 18+ messages in thread
From: Richard Guy Briggs @ 2018-04-16 14:11 UTC (permalink / raw)
To: linux-security-module
On 2018-04-16 09:26, Ondrej Mosnacek wrote:
> 2018-04-10 1:34 GMT+02:00 Richard Guy Briggs <rgb@redhat.com>:
> > 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 <rgb@redhat.com>
> > ---
> > 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(&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
>
> > from_kuid(&init_user_ns, audit_get_loginuid(current)),
> > - audit_get_sessionid(current));
> > + audit_get_sessionid(current), 0, 1);
> > }
> >
> > length = count;
>
> Ondrej Mosnacek <omosnace@redhat dot com>
- RGB
--
Richard Guy Briggs <rgb@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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH ghak46 V1] audit: normalize MAC_STATUS record
2018-04-16 14:11 ` Richard Guy Briggs
@ 2018-04-16 14:25 ` Ondrej Mosnacek
-1 siblings, 0 replies; 18+ messages in thread
From: Ondrej Mosnacek @ 2018-04-16 14:25 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: Linux-Audit Mailing List, LKML, SElinux list, Linux Security Module list
2018-04-16 16:11 GMT+02:00 Richard Guy Briggs <rgb@redhat.com>:
> On 2018-04-16 09:26, Ondrej Mosnacek wrote:
>> 2018-04-10 1:34 GMT+02:00 Richard Guy Briggs <rgb@redhat.com>:
>> > 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 <rgb@redhat.com>
>> > ---
>> > 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(&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
>>
>> > 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 <rgb@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 <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH ghak46 V1] audit: normalize MAC_STATUS record
@ 2018-04-16 14:25 ` Ondrej Mosnacek
0 siblings, 0 replies; 18+ messages in thread
From: Ondrej Mosnacek @ 2018-04-16 14:25 UTC (permalink / raw)
To: linux-security-module
2018-04-16 16:11 GMT+02:00 Richard Guy Briggs <rgb@redhat.com>:
> On 2018-04-16 09:26, Ondrej Mosnacek wrote:
>> 2018-04-10 1:34 GMT+02:00 Richard Guy Briggs <rgb@redhat.com>:
>> > 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 <rgb@redhat.com>
>> > ---
>> > 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(&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
>>
>> > from_kuid(&init_user_ns, audit_get_loginuid(current)),
>> > - audit_get_sessionid(current));
>> > + audit_get_sessionid(current), 0, 1);
>> > }
>> >
>> > length = count;
>>
>> Ondrej Mosnacek <omosnace@redhat dot com>
>
> - RGB
>
> --
> Richard Guy Briggs <rgb@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 <omosnace@redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH ghak46 V1] audit: normalize MAC_STATUS record
2018-04-16 14:11 ` Richard Guy Briggs
@ 2018-04-16 18:07 ` Steve Grubb
-1 siblings, 0 replies; 18+ messages in thread
From: Steve Grubb @ 2018-04-16 18:07 UTC (permalink / raw)
To: linux-audit
Cc: Richard Guy Briggs, Ondrej Mosnacek, Linux Security Module list,
LKML, SElinux list
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 <rgb@redhat.com>:
> > > 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 <rgb@redhat.com>
> > > ---
> > >
> > > 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(&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
> >
> > > 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 <rgb@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
>
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH ghak46 V1] audit: normalize MAC_STATUS record
@ 2018-04-16 18:07 ` Steve Grubb
0 siblings, 0 replies; 18+ messages in thread
From: Steve Grubb @ 2018-04-16 18:07 UTC (permalink / raw)
To: linux-security-module
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 <rgb@redhat.com>:
> > > 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 <rgb@redhat.com>
> > > ---
> > >
> > > 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(&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
> >
> > > from_kuid(&init_user_ns,
> > > audit_get_loginuid(current)),
> > >
> > > - audit_get_sessionid(current));
> > > + audit_get_sessionid(current), 0, 1);
> > >
> > > }
> > >
> > > length = count;
> >
> > Ondrej Mosnacek <omosnace@redhat dot com>
>
> - RGB
>
> --
> Richard Guy Briggs <rgb@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
>
> --
> Linux-audit mailing list
> Linux-audit at redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH ghak46 V1] audit: normalize MAC_STATUS record
2018-04-11 21:08 ` Paul Moore
@ 2018-04-17 21:59 ` Paul Moore
-1 siblings, 0 replies; 18+ messages in thread
From: Paul Moore @ 2018-04-17 21:59 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: Linux-Audit Mailing List, LKML, SElinux list,
Linux Security Module list, Eric Paris, Steve Grubb
On Wed, Apr 11, 2018 at 5:08 PM, Paul Moore <paul@paul-moore.com> wrote:
> On Mon, Apr 9, 2018 at 7:34 PM, Richard Guy Briggs <rgb@redhat.com> 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 <rgb@redhat.com>
>> ---
>> 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.
>
>> 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).
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH ghak46 V1] audit: normalize MAC_STATUS record
@ 2018-04-17 21:59 ` Paul Moore
0 siblings, 0 replies; 18+ messages in thread
From: Paul Moore @ 2018-04-17 21:59 UTC (permalink / raw)
To: linux-security-module
On Wed, Apr 11, 2018 at 5:08 PM, Paul Moore <paul@paul-moore.com> wrote:
> On Mon, Apr 9, 2018 at 7:34 PM, Richard Guy Briggs <rgb@redhat.com> 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 <rgb@redhat.com>
>> ---
>> 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.
>
>> 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).
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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH ghak46 V1] audit: normalize MAC_STATUS record
2018-04-17 21:59 ` Paul Moore
@ 2018-04-17 22:09 ` Richard Guy Briggs
-1 siblings, 0 replies; 18+ messages in thread
From: Richard Guy Briggs @ 2018-04-17 22:09 UTC (permalink / raw)
To: Paul Moore
Cc: Linux-Audit Mailing List, LKML, SElinux list,
Linux Security Module list, Eric Paris, Steve Grubb
On 2018-04-17 17:59, Paul Moore wrote:
> On Wed, Apr 11, 2018 at 5:08 PM, Paul Moore <paul@paul-moore.com> wrote:
> > On Mon, Apr 9, 2018 at 7:34 PM, Richard Guy Briggs <rgb@redhat.com> 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 <rgb@redhat.com>
> >> ---
> >> 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.
> >
> >> 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).
>
> 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 <rgb@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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH ghak46 V1] audit: normalize MAC_STATUS record
@ 2018-04-17 22:09 ` Richard Guy Briggs
0 siblings, 0 replies; 18+ messages in thread
From: Richard Guy Briggs @ 2018-04-17 22:09 UTC (permalink / raw)
To: linux-security-module
On 2018-04-17 17:59, Paul Moore wrote:
> On Wed, Apr 11, 2018 at 5:08 PM, Paul Moore <paul@paul-moore.com> wrote:
> > On Mon, Apr 9, 2018 at 7:34 PM, Richard Guy Briggs <rgb@redhat.com> 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 <rgb@redhat.com>
> >> ---
> >> 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.
> >
> >> 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).
>
> 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 <rgb@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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH ghak46 V1] audit: normalize MAC_STATUS record
2018-04-17 22:09 ` Richard Guy Briggs
@ 2018-04-18 1:51 ` Paul Moore
-1 siblings, 0 replies; 18+ messages in thread
From: Paul Moore @ 2018-04-18 1:51 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: Linux-Audit Mailing List, LKML, SElinux list,
Linux Security Module list, Eric Paris, Steve Grubb
On Tue, Apr 17, 2018 at 6:09 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2018-04-17 17:59, Paul Moore wrote:
>> On Wed, Apr 11, 2018 at 5:08 PM, Paul Moore <paul@paul-moore.com> wrote:
>> > On Mon, Apr 9, 2018 at 7:34 PM, Richard Guy Briggs <rgb@redhat.com> 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 <rgb@redhat.com>
>> >> ---
>> >> 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.
>> >
>> >> 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).
>>
>> 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH ghak46 V1] audit: normalize MAC_STATUS record
@ 2018-04-18 1:51 ` Paul Moore
0 siblings, 0 replies; 18+ messages in thread
From: Paul Moore @ 2018-04-18 1:51 UTC (permalink / raw)
To: linux-security-module
On Tue, Apr 17, 2018 at 6:09 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2018-04-17 17:59, Paul Moore wrote:
>> On Wed, Apr 11, 2018 at 5:08 PM, Paul Moore <paul@paul-moore.com> wrote:
>> > On Mon, Apr 9, 2018 at 7:34 PM, Richard Guy Briggs <rgb@redhat.com> 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 <rgb@redhat.com>
>> >> ---
>> >> 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.
>> >
>> >> 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).
>>
>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2018-04-18 1:51 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-09 23:34 [PATCH ghak46 V1] audit: normalize MAC_STATUS record Richard Guy Briggs
2018-04-09 23:34 ` Richard Guy Briggs
2018-04-11 21:08 ` Paul Moore
2018-04-11 21:08 ` Paul Moore
2018-04-17 21:59 ` Paul Moore
2018-04-17 21:59 ` Paul Moore
2018-04-17 22:09 ` Richard Guy Briggs
2018-04-17 22:09 ` Richard Guy Briggs
2018-04-18 1:51 ` Paul Moore
2018-04-18 1:51 ` Paul Moore
2018-04-16 7:26 ` Ondrej Mosnacek
2018-04-16 7:26 ` Ondrej Mosnacek
2018-04-16 14:11 ` Richard Guy Briggs
2018-04-16 14:11 ` Richard Guy Briggs
2018-04-16 14:25 ` Ondrej Mosnacek
2018-04-16 14:25 ` Ondrej Mosnacek
2018-04-16 18:07 ` Steve Grubb
2018-04-16 18:07 ` Steve Grubb
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.