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

For ADD_RULE and DEL_RULE case when audit_enabled is in the AUDIT_LOCKEDstate, it just outputs "audit_enabled=2 res=0" to indicate locked andfailure, but doesn't appear to actually give the normal "op=<mumble>" toindicate a rule change was attempted and refused due to immutability ofthe rule set. Will this be a problem for the parser, and should anattempted 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 maybeauditd PID changes, every other config change should not be logged ifaudit is disabled. Furthermore, if CONFFIG_CHANGE records are to beaccompanied by syscall records, they should obey audit_dummy_context()to avoid unaccompanied records. Does this reasoning make sense?

Post by Richard Guy BriggsHi Steve, Paul,Looking at some AUDIT_CONFIG_CHANGE record formats, a couple of thingsFor ADD_RULE and DEL_RULE case when audit_enabled is in the AUDIT_LOCKEDstate, it just outputs "audit_enabled=2 res=0" to indicate locked andfailure, but doesn't appear to actually give the normal "op=<mumble>" toindicate a rule change was attempted and refused due to immutability ofthe rule set. Will this be a problem for the parser, and should anattempted 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 maybeauditd PID changes, every other config change should not be logged ifaudit is disabled. Furthermore, if CONFIG_CHANGE records are to beaccompanied 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 notdepend on syscall existing rules, and I can't really think of rules thatmake sense to catch them. Stick with the audit_enabled check.

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

Since this falls squarely on the audit devs, I would really hope we'redoing the right thing here. If not, we've only got ourselves, oractually probably our predecessors, to blame ... and I think most ofthem 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 Ithink we are probably okay from Steve's point of view. If not, we'velikely been broken for a long enough time that it isn't really so muchbroken as it is "the way it's done".

Post by Richard Guy BriggsThe 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 least2014, maybe longer.

Post by Richard Guy BriggsAnother is that other than a change to the enabled status and maybeauditd PID changes, every other config change should not be logged ifaudit is disabled.

At this point I'm beginning to think we just need to add a quickaudit_enabled check near the top of audit_log_start() and returns NULLif audit is disabled. It's clearly not as efficient as adding anexplicit check to the caller, but it should catch all of thesemiscellaneous little events that are not checking the enabled/disabledstatus.

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

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

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

Since this falls squarely on the audit devs, I would really hope we'redoing the right thing here. If not, we've only got ourselves, oractually probably our predecessors, to blame ... and I think most ofthem 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 Ithink we are probably okay from Steve's point of view. If not, we'velikely been broken for a long enough time that it isn't really so muchbroken as it is "the way it's done".

Post by Richard Guy BriggsThe 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 least2014, maybe longer.

Post by Richard Guy BriggsAnother is that other than a change to the enabled status and maybeauditd PID changes, every other config change should not be logged ifaudit is disabled.

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

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

Since this falls squarely on the audit devs, I would really hope we'redoing the right thing here. If not, we've only got ourselves, oractually probably our predecessors, to blame ... and I think most ofthem 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 Ithink we are probably okay from Steve's point of view. If not, we'velikely been broken for a long enough time that it isn't really so muchbroken as it is "the way it's done".

Post by Richard Guy BriggsThe 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 least2014, maybe longer.

Post by Richard Guy BriggsAnother is that other than a change to the enabled status and maybeauditd PID changes, every other config change should not be logged ifaudit is disabled.

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

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 ifaudit is disabled. However, I do understand your point that there arelikely 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 theidea of adding an audit_enabled check into audit_log_start(), perhapswe also introduce an audit_log_always()/audit_log_start_always() forthose callers that must emit records, regardless of the admin'saudit_enabled setting? At the very least this would help usinstrument the kernel code so we would have a clear understanding onwho requires this behavior, and who is blindly ignoring theaudit_enabled state.

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

Since this falls squarely on the audit devs, I would really hope we'redoing the right thing here. If not, we've only got ourselves, oractually probably our predecessors, to blame ... and I think most ofthem 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 Ithink we are probably okay from Steve's point of view. If not, we'velikely been broken for a long enough time that it isn't really so muchbroken as it is "the way it's done".

Post by Richard Guy BriggsThe 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 least2014, maybe longer.

Post by Richard Guy BriggsAnother is that other than a change to the enabled status and maybeauditd PID changes, every other config change should not be logged ifaudit is disabled.

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

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 ifaudit is disabled. However, I do understand your point that there arelikely 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()) fromseccomp_log() that has both that is quite intentional. Interestingto note that all the ones that check are non-security-tree calls whileall the ones that log regardless are from the security tree.

Post by Paul MoorePerhaps a solution is to make this behavior explicit. Building on theidea of adding an audit_enabled check into audit_log_start(), perhapswe also introduce an audit_log_always()/audit_log_start_always() forthose callers that must emit records, regardless of the admin'saudit_enabled setting? At the very least this would help usinstrument the kernel code so we would have a clear understanding onwho requires this behavior, and who is blindly ignoring theaudit_enabled state.

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

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

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

Post by Richard Guy BriggsThe 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 thatis not a rule add/delete. For example, if we enable audit, we need the oldand new values. This can be expressed either as old- new- or old- and theitem without a new prefix.

Post by Richard Guy BriggsAnother is that other than a change to the enabled status and maybeauditd PID changes, every other config change should not be logged ifaudit is disabled.

True.

Post by Richard Guy BriggsFurthermore, if CONFIG_CHANGE records are to be accompanied by syscallrecords, they should obey audit_dummy_context() to avoid unaccompaniedrecords. Does this reasoning make sense?

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