CONFIG_CHANGE record formats

[Richard Guy Briggs]

Hi Steve, Paul,

Looking at some AUDIT_CONFIG_CHANGE record formats, a couple of things
stand out as potential problems:

For ADD_RULE and DEL_RULE case when audit_enabled is in the AUDIT_LOCKED
state, it just outputs "audit_enabled=2 res=0" to indicate locked and
failure, but doesn't appear to actually give the normal "op=<mumble>" to
indicate a rule change was attempted and refused due to immutability of
the rule set. Will this be a problem for the parser, and should an
attempted rule change be logged as such?

The other is AUDIT_TTY_SET that has non-standard old-* and new-* fields,
but since there are two, I think it is unavoidable and can't be fixed.

Another is that other than a change to the enabled status and maybe
auditd PID changes, every other config change should not be logged if
audit is disabled. Furthermore, if CONFFIG_CHANGE records are to be
accompanied by syscall records, they should obey audit_dummy_context()
to avoid unaccompanied records. Does this reasoning make sense?


- RGB

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

[Richard Guy Briggs]

Post by Richard Guy Briggs
Hi Steve, Paul,
Looking at some AUDIT_CONFIG_CHANGE record formats, a couple of things
For ADD_RULE and DEL_RULE case when audit_enabled is in the AUDIT_LOCKED
state, it just outputs "audit_enabled=2 res=0" to indicate locked and
failure, but doesn't appear to actually give the normal "op=<mumble>" to
indicate a rule change was attempted and refused due to immutability of
the rule set. Will this be a problem for the parser, and should an
attempted rule change be logged as such?
The other is AUDIT_TTY_SET that has non-standard old-* and new-* fields,
but since there are two, I think it is unavoidable and can't be fixed.
Another is that other than a change to the enabled status and maybe
auditd PID changes, every other config change should not be logged if
audit is disabled. Furthermore, if CONFIG_CHANGE records are to be
accompanied by syscall records, they should obey audit_dummy_context()
to avoid unaccompanied records. Does this reasoning make sense?

I'll backtrack on audit_dummy_context since config changes should not
depend on syscall existing rules, and I can't really think of rules that
make sense to catch them. Stick with the audit_enabled check.

Post by Richard Guy Briggs
- RGB

- RGB

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

[Paul Moore]

Post by Richard Guy Briggs
Hi Steve, Paul,
Looking at some AUDIT_CONFIG_CHANGE record formats, a couple of things
For ADD_RULE and DEL_RULE case when audit_enabled is in the AUDIT_LOCKED
state, it just outputs "audit_enabled=2 res=0" to indicate locked and
failure, but doesn't appear to actually give the normal "op=<mumble>" to
indicate a rule change was attempted and refused due to immutability of
the rule set. Will this be a problem for the parser, and should an
attempted rule change be logged as such?

Since this falls squarely on the audit devs, I would really hope we're
doing the right thing here. If not, we've only got ourselves, or
actually probably our predecessors, to blame ... and I think most of
them have moved on from audit. One wonders why ...

Looking quickly at the AUDIT_CONFIG_CHANGE records in kernel/audit.c,
it looks like there is no well defined, single record format so I
think we are probably okay from Steve's point of view. If not, we've
likely been broken for a long enough time that it isn't really so much
broken as it is "the way it's done".

Post by Richard Guy Briggs
The other is AUDIT_TTY_SET that has non-standard old-* and new-* fields,
but since there are two, I think it is unavoidable and can't be fixed.

Similar to the above. That code has been that way since at least
2014, maybe longer.

Post by Richard Guy Briggs
Another is that other than a change to the enabled status and maybe
auditd PID changes, every other config change should not be logged if
audit is disabled.

At this point I'm beginning to think we just need to add a quick
audit_enabled check near the top of audit_log_start() and returns NULL
if audit is disabled. It's clearly not as efficient as adding an
explicit check to the caller, but it should catch all of these
miscellaneous little events that are not checking the enabled/disabled
status.

If we want to add checks to the callers properly we could always add a
WARN, but only for development purposes.

However, as you point out we may need to bypass that check for things
like status and audit PID changes, but we could always have the public
audit_log_start() function and a private __audit_log_start()
accessible only within kernel/audit.c. The private function would
actually be what audit_log_start() looks like now, and the new public
function would be an audit_enabled() check followed by a call to
__audit_log_start().

[Richard Guy Briggs]

Post by Paul Moore

Post by Richard Guy Briggs
Hi Steve, Paul,
Looking at some AUDIT_CONFIG_CHANGE record formats, a couple of things
For ADD_RULE and DEL_RULE case when audit_enabled is in the AUDIT_LOCKED
state, it just outputs "audit_enabled=2 res=0" to indicate locked and
failure, but doesn't appear to actually give the normal "op=<mumble>" to
indicate a rule change was attempted and refused due to immutability of
the rule set. Will this be a problem for the parser, and should an
attempted rule change be logged as such?

Since this falls squarely on the audit devs, I would really hope we're
doing the right thing here. If not, we've only got ourselves, or
actually probably our predecessors, to blame ... and I think most of
them have moved on from audit. One wonders why ...
Looking quickly at the AUDIT_CONFIG_CHANGE records in kernel/audit.c,
it looks like there is no well defined, single record format so I
think we are probably okay from Steve's point of view. If not, we've
likely been broken for a long enough time that it isn't really so much
broken as it is "the way it's done".

Post by Richard Guy Briggs
The other is AUDIT_TTY_SET that has non-standard old-* and new-* fields,
but since there are two, I think it is unavoidable and can't be fixed.

Similar to the above. That code has been that way since at least
2014, maybe longer.

Post by Richard Guy Briggs
Another is that other than a change to the enabled status and maybe
auditd PID changes, every other config change should not be logged if
audit is disabled.

At this point I'm beginning to think we just need to add a quick
audit_enabled check near the top of audit_log_start() and returns NULL
if audit is disabled. It's clearly not as efficient as adding an
explicit check to the caller, but it should catch all of these
miscellaneous little events that are not checking the enabled/disabled
status.
If we want to add checks to the callers properly we could always add a
WARN, but only for development purposes.
However, as you point out we may need to bypass that check for things
like status and audit PID changes, but we could always have the public
audit_log_start() function and a private __audit_log_start()
accessible only within kernel/audit.c. The private function would
actually be what audit_log_start() looks like now, and the new public
function would be an audit_enabled() check followed by a call to
__audit_log_start().

The following usage not available to the private function is already protected
by audit_enable:
- drivers/tty/tty_audit.c: ab = audit_log_start(context, GFP_KERNEL, AUDIT_TTY);
- include/net/xfrm.h: audit_buf = audit_log_start(current->audit_context, GFP_ATOMIC,
- net/bridge/netfilter/ebtables.c: audit_log(current->audit_context, GFP_KERNEL,
- net/core/dev.c audit_log(current->audit_context, GFP_ATOMIC,
- net/netfilter/x_tables.c audit_log(current->audit_context, GFP_KERNEL,
- net/netfilter/xt_AUDIT.c: ab = audit_log_start(context, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
- net/netlabel/netlabel_user.c: audit_buf = audit_log_start(current->audit_context, GFP_ATOMIC, type);

But these are not, so adding an audit_enable check to audit_log_start() is
going to change their behaviour:
- security/integrity/ima/ima_api.c: ab = audit_log_start(current->audit_context, GFP_KERNEL,
- security/integrity/ima/ima_policy.c: ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_INTEGRITY_RULE);
- security/integrity/integrity_audit.c: ab = audit_log_start(current->audit_context, GFP_KERNEL, audit_msgno);
- security/lsm_audit.c: ab = audit_log_start(current->audit_context, GFP_ATOMIC | __GFP_NOWARN,
- aa_audit_msg() which calls common_lsm_audit()
- slow_avc_audit()
- smack_log()
- security/selinux/hooks.c: ab = audit_log_start(current->audit_context, GFP_ATOMIC, AUDIT_SELINUX_ERR);
- security/selinux/hooks.c: ab = audit_log_start(current->audit_context, GFP_ATOMIC, AUDIT_SELINUX_ERR);
- security/selinux/selinuxfs.c: audit_log(current->audit_context, GFP_KERNEL, AUDIT_MAC_STAT
- security/selinux/selinuxfs.c: audit_log(current->audit_context, GFP_KERNEL, AUDIT_MAC_STAT
- security/selinux/selinuxfs.c: audit_log(current->audit_context, GFP_KERNEL, AUDIT_MAC_POLICY_LOAD,
- security/selinux/ss/services.c: ab = audit_log_start(current->audit_context,
- security/selinux/ss/services.c: audit_log(current->audit_context, GFP_ATOMIC, AUDIT_SELINUX_ERR,
- security/selinux/ss/services.c: audit_log(current->audit_context,
- security/selinux/ss/services.c: audit_log(current->audit_context, GFP_ATOMIC, AUDIT_SELINUX_ERR,
- security/selinux/ss/services.c: audit_log(current->audit_context, GFP_ATOMIC,
- security/selinux/ss/services.c: audit_log(current->audit_context,

There is already an exception for USER AVC messages. Which other messages
are important enough to ignore audit_enabled?

Post by Paul Moore
paul moore

- RGB

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

[Paul Moore]

Post by Richard Guy Briggs

Post by Paul Moore

Post by Richard Guy Briggs
Hi Steve, Paul,
Looking at some AUDIT_CONFIG_CHANGE record formats, a couple of things
For ADD_RULE and DEL_RULE case when audit_enabled is in the AUDIT_LOCKED
state, it just outputs "audit_enabled=2 res=0" to indicate locked and
failure, but doesn't appear to actually give the normal "op=<mumble>" to
indicate a rule change was attempted and refused due to immutability of
the rule set. Will this be a problem for the parser, and should an
attempted rule change be logged as such?

Since this falls squarely on the audit devs, I would really hope we're
doing the right thing here. If not, we've only got ourselves, or
actually probably our predecessors, to blame ... and I think most of
them have moved on from audit. One wonders why ...
Looking quickly at the AUDIT_CONFIG_CHANGE records in kernel/audit.c,
it looks like there is no well defined, single record format so I
think we are probably okay from Steve's point of view. If not, we've
likely been broken for a long enough time that it isn't really so much
broken as it is "the way it's done".

Post by Richard Guy Briggs
The other is AUDIT_TTY_SET that has non-standard old-* and new-* fields,
but since there are two, I think it is unavoidable and can't be fixed.

Similar to the above. That code has been that way since at least
2014, maybe longer.

Post by Richard Guy Briggs
Another is that other than a change to the enabled status and maybe
auditd PID changes, every other config change should not be logged if
audit is disabled.

At this point I'm beginning to think we just need to add a quick
audit_enabled check near the top of audit_log_start() and returns NULL
if audit is disabled. It's clearly not as efficient as adding an
explicit check to the caller, but it should catch all of these
miscellaneous little events that are not checking the enabled/disabled
status.
If we want to add checks to the callers properly we could always add a
WARN, but only for development purposes.
However, as you point out we may need to bypass that check for things
like status and audit PID changes, but we could always have the public
audit_log_start() function and a private __audit_log_start()
accessible only within kernel/audit.c. The private function would
actually be what audit_log_start() looks like now, and the new public
function would be an audit_enabled() check followed by a call to
__audit_log_start().

The following usage not available to the private function is already protected
- drivers/tty/tty_audit.c: ab = audit_log_start(context, GFP_KERNEL, AUDIT_TTY);
- include/net/xfrm.h: audit_buf = audit_log_start(current->audit_context, GFP_ATOMIC,
- net/bridge/netfilter/ebtables.c: audit_log(current->audit_context, GFP_KERNEL,
- net/core/dev.c audit_log(current->audit_context, GFP_ATOMIC,
- net/netfilter/x_tables.c audit_log(current->audit_context, GFP_KERNEL,
- net/netfilter/xt_AUDIT.c: ab = audit_log_start(context, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
- net/netlabel/netlabel_user.c: audit_buf = audit_log_start(current->audit_context, GFP_ATOMIC, type);
But these are not, so adding an audit_enable check to audit_log_start() is
- security/integrity/ima/ima_api.c: ab = audit_log_start(current->audit_context, GFP_KERNEL,
- security/integrity/ima/ima_policy.c: ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_INTEGRITY_RULE);
- security/integrity/integrity_audit.c: ab = audit_log_start(current->audit_context, GFP_KERNEL, audit_msgno);
- security/lsm_audit.c: ab = audit_log_start(current->audit_context, GFP_ATOMIC | __GFP_NOWARN,
- aa_audit_msg() which calls common_lsm_audit()
- slow_avc_audit()
- smack_log()
- security/selinux/hooks.c: ab = audit_log_start(current->audit_context, GFP_ATOMIC, AUDIT_SELINUX_ERR);
- security/selinux/hooks.c: ab = audit_log_start(current->audit_context, GFP_ATOMIC, AUDIT_SELINUX_ERR);
- security/selinux/selinuxfs.c: audit_log(current->audit_context, GFP_KERNEL, AUDIT_MAC_STAT
- security/selinux/selinuxfs.c: audit_log(current->audit_context, GFP_KERNEL, AUDIT_MAC_STAT
- security/selinux/selinuxfs.c: audit_log(current->audit_context, GFP_KERNEL, AUDIT_MAC_POLICY_LOAD,
- security/selinux/ss/services.c: ab = audit_log_start(current->audit_context,
- security/selinux/ss/services.c: audit_log(current->audit_context, GFP_ATOMIC, AUDIT_SELINUX_ERR,
- security/selinux/ss/services.c: audit_log(current->audit_context,
- security/selinux/ss/services.c: audit_log(current->audit_context, GFP_ATOMIC, AUDIT_SELINUX_ERR,
- security/selinux/ss/services.c: audit_log(current->audit_context, GFP_ATOMIC,
- security/selinux/ss/services.c: audit_log(current->audit_context,
There is already an exception for USER AVC messages. Which other messages
are important enough to ignore audit_enabled?

Based on comments we've gotten from users on this list over the years,
I tend to think that we really should squelch all audit records if
audit is disabled. However, I do understand your point that there are
likely some callers which rely on audit records always being printed,
at least to the ring buffer if not the audit log.

Perhaps a solution is to make this behavior explicit. Building on the
idea of adding an audit_enabled check into audit_log_start(), perhaps
we also introduce an audit_log_always()/audit_log_start_always() for
those callers that must emit records, regardless of the admin's
audit_enabled setting? At the very least this would help us
instrument the kernel code so we would have a clear understanding on
who requires this behavior, and who is blindly ignoring the
audit_enabled state.

[Richard Guy Briggs]

Post by Paul Moore

Post by Richard Guy Briggs

Post by Paul Moore

Post by Richard Guy Briggs
Hi Steve, Paul,
Looking at some AUDIT_CONFIG_CHANGE record formats, a couple of things
For ADD_RULE and DEL_RULE case when audit_enabled is in the AUDIT_LOCKED
state, it just outputs "audit_enabled=2 res=0" to indicate locked and
failure, but doesn't appear to actually give the normal "op=<mumble>" to
indicate a rule change was attempted and refused due to immutability of
the rule set. Will this be a problem for the parser, and should an
attempted rule change be logged as such?

Since this falls squarely on the audit devs, I would really hope we're
doing the right thing here. If not, we've only got ourselves, or
actually probably our predecessors, to blame ... and I think most of
them have moved on from audit. One wonders why ...
Looking quickly at the AUDIT_CONFIG_CHANGE records in kernel/audit.c,
it looks like there is no well defined, single record format so I
think we are probably okay from Steve's point of view. If not, we've
likely been broken for a long enough time that it isn't really so much
broken as it is "the way it's done".

Post by Richard Guy Briggs
The other is AUDIT_TTY_SET that has non-standard old-* and new-* fields,
but since there are two, I think it is unavoidable and can't be fixed.

Similar to the above. That code has been that way since at least
2014, maybe longer.

Post by Richard Guy Briggs
Another is that other than a change to the enabled status and maybe
auditd PID changes, every other config change should not be logged if
audit is disabled.

At this point I'm beginning to think we just need to add a quick
audit_enabled check near the top of audit_log_start() and returns NULL
if audit is disabled. It's clearly not as efficient as adding an
explicit check to the caller, but it should catch all of these
miscellaneous little events that are not checking the enabled/disabled
status.
If we want to add checks to the callers properly we could always add a
WARN, but only for development purposes.
However, as you point out we may need to bypass that check for things
like status and audit PID changes, but we could always have the public
audit_log_start() function and a private __audit_log_start()
accessible only within kernel/audit.c. The private function would
actually be what audit_log_start() looks like now, and the new public
function would be an audit_enabled() check followed by a call to
__audit_log_start().

The following usage not available to the private function is already protected
- drivers/tty/tty_audit.c: ab = audit_log_start(context, GFP_KERNEL, AUDIT_TTY);
- include/net/xfrm.h: audit_buf = audit_log_start(current->audit_context, GFP_ATOMIC,
- net/bridge/netfilter/ebtables.c: audit_log(current->audit_context, GFP_KERNEL,
- net/core/dev.c audit_log(current->audit_context, GFP_ATOMIC,
- net/netfilter/x_tables.c audit_log(current->audit_context, GFP_KERNEL,
- net/netfilter/xt_AUDIT.c: ab = audit_log_start(context, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
- net/netlabel/netlabel_user.c: audit_buf = audit_log_start(current->audit_context, GFP_ATOMIC, type);
But these are not, so adding an audit_enable check to audit_log_start() is
- security/integrity/ima/ima_api.c: ab = audit_log_start(current->audit_context, GFP_KERNEL,
- security/integrity/ima/ima_policy.c: ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_INTEGRITY_RULE);
- security/integrity/integrity_audit.c: ab = audit_log_start(current->audit_context, GFP_KERNEL, audit_msgno);
- security/lsm_audit.c: ab = audit_log_start(current->audit_context, GFP_ATOMIC | __GFP_NOWARN,
- aa_audit_msg() which calls common_lsm_audit()
- slow_avc_audit()
- smack_log()
- security/selinux/hooks.c: ab = audit_log_start(current->audit_context, GFP_ATOMIC, AUDIT_SELINUX_ERR);
- security/selinux/hooks.c: ab = audit_log_start(current->audit_context, GFP_ATOMIC, AUDIT_SELINUX_ERR);
- security/selinux/selinuxfs.c: audit_log(current->audit_context, GFP_KERNEL, AUDIT_MAC_STAT
- security/selinux/selinuxfs.c: audit_log(current->audit_context, GFP_KERNEL, AUDIT_MAC_STAT
- security/selinux/selinuxfs.c: audit_log(current->audit_context, GFP_KERNEL, AUDIT_MAC_POLICY_LOAD,
- security/selinux/ss/services.c: ab = audit_log_start(current->audit_context,
- security/selinux/ss/services.c: audit_log(current->audit_context, GFP_ATOMIC, AUDIT_SELINUX_ERR,
- security/selinux/ss/services.c: audit_log(current->audit_context,
- security/selinux/ss/services.c: audit_log(current->audit_context, GFP_ATOMIC, AUDIT_SELINUX_ERR,
- security/selinux/ss/services.c: audit_log(current->audit_context, GFP_ATOMIC,
- security/selinux/ss/services.c: audit_log(current->audit_context,
There is already an exception for USER AVC messages. Which other messages
are important enough to ignore audit_enabled?

Based on comments we've gotten from users on this list over the years,
I tend to think that we really should squelch all audit records if
audit is disabled. However, I do understand your point that there are
likely some callers which rely on audit records always being printed,
at least to the ring buffer if not the audit log.

There's a seccomp audit call (audit_seccomp() vs __audit_seccomp()) from
seccomp_log() that has both that is quite intentional. Interesting
to note that all the ones that check are non-security-tree calls while
all the ones that log regardless are from the security tree.

Post by Paul Moore
Perhaps a solution is to make this behavior explicit. Building on the
idea of adding an audit_enabled check into audit_log_start(), perhaps
we also introduce an audit_log_always()/audit_log_start_always() for
those callers that must emit records, regardless of the admin's
audit_enabled setting? At the very least this would help us
instrument the kernel code so we would have a clear understanding on
who requires this behavior, and who is blindly ignoring the
audit_enabled state.

I like this idea of making it more evident. I was even thinking
audit_log_start_forced()

Post by Paul Moore
paul moore

- RGB

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

[Steve Grubb]

Post by Richard Guy Briggs
Hi Steve, Paul,
Looking at some AUDIT_CONFIG_CHANGE record formats, a couple of things
For ADD_RULE and DEL_RULE case when audit_enabled is in the AUDIT_LOCKED
state, it just outputs "audit_enabled=2 res=0" to indicate locked and
failure, but doesn't appear to actually give the normal "op=<mumble>" to
indicate a rule change was attempted and refused due to immutability of
the rule set. Will this be a problem for the parser, and should an
attempted rule change be logged as such?

If its the only rule change event that does not have an op= field, then make
it have one.

Post by Richard Guy Briggs
The other is AUDIT_TTY_SET that has non-standard old-* and new-* fields,
but since there are two, I think it is unavoidable and can't be fixed.

We actually have to have old and new values for any configuration change that
is not a rule add/delete. For example, if we enable audit, we need the old
and new values. This can be expressed either as old- new- or old- and the
item without a new prefix.

Post by Richard Guy Briggs
Another is that other than a change to the enabled status and maybe
auditd PID changes, every other config change should not be logged if
audit is disabled.

True.

Post by Richard Guy Briggs
Furthermore, if CONFIG_CHANGE records are to be accompanied by syscall
records, they should obey audit_dummy_context() to avoid unaccompanied
records. Does this reasoning make sense?

CONFIG_CHANGE records are simple events and not compound events. They should
contain all the pertinent information in their one record.

-Steve