Re: [PATCH v15 21/23] Audit: Include object data for all security modules

Casey Schaufler
2020-03-09T17:45:22Z

On 3/6/2020 6:31 PM, Paul Moore wrote:
> On Fri, Feb 21, 2020 at 7:06 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> When there is more than one context displaying security
>> module extend what goes into the audit record by supplimenting
>> the "obj=" with an "obj_<lsm>=" for each such security
>> module.
>>
>> Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>> Cc:linux-audit@redhat.com
>> ---
>> kernel/audit.h | 4 +-
>> kernel/auditsc.c | 106 ++++++++++++++++++++++++-----------------------
>> 2 files changed, 56 insertions(+), 54 deletions(-)
> ...
>
>> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> index 68ae5fa843c1..7dab48661e31 100644
>> --- a/kernel/auditsc.c
>> +++ b/kernel/auditsc.c
>> @@ -956,13 +953,57 @@ static inline void audit_free_context(struct audit_context *context)
>> kfree(context);
>> }
>>
>> +static int audit_log_object_context(struct audit_buffer *ab,
>> + struct lsmblob *blob)
>> +{
>> + struct lsmcontext context;
>> + const char *lsm;
>> + int i;
>> +
>> + /*
>> + * None of the installed modules have object labels.
>> + */
>> + if (security_lsm_slot_name(0) == NULL)
>> + return 0;
>> +
>> + if (blob->secid[0] != 0) {
>> + if (security_secid_to_secctx(blob, &context, 0)) {
>> + audit_log_format(ab, " obj=?");
>> + return 1;
>> + }
>> + audit_log_format(ab, " obj=%s", context.context);
>> + security_release_secctx(&context);
>> + }
>> +
>> + /*
>> + * Don't do anything more unless there is more than one LSM
>> + * with a security context to report.
>> + */
>> + if (security_lsm_slot_name(1) == NULL)
>> + return 0;
>> +
>> + for (i = 0; i < LSMBLOB_ENTRIES; i++) {
>> + lsm = security_lsm_slot_name(i);
>> + if (lsm == NULL)
>> + break;
>> + if (blob->secid[i] == 0)
>> + continue;
>> + if (security_secid_to_secctx(blob, &context, i)) {
>> + audit_log_format(ab, " obj_%s=?", lsm);
>> + continue;
>> + }
>> + audit_log_format(ab, " obj_%s=%s", lsm, context.context);
>> + security_release_secctx(&context);
>> + }
>> + return 0;
>> +}
>> +
>> static int audit_log_pid_context(struct audit_context *context, pid_t pid,
>> kuid_t auid, kuid_t uid,
>> unsigned int sessionid,
>> struct lsmblob *blob, char *comm)
>> {
>> struct audit_buffer *ab;
>> - struct lsmcontext lsmctx;
>> int rc = 0;
>>
>> ab = audit_log_start(context, GFP_KERNEL, AUDIT_OBJ_PID);
>> @@ -972,15 +1013,7 @@ static int audit_log_pid_context(struct audit_context *context, pid_t pid,
>> audit_log_format(ab, "opid=%d oauid=%d ouid=%d oses=%d", pid,
>> from_kuid(&init_user_ns, auid),
>> from_kuid(&init_user_ns, uid), sessionid);
>> - if (lsmblob_is_set(blob)) {
>> - if (security_secid_to_secctx(blob, &lsmctx, LSMBLOB_FIRST)) {
>> - audit_log_format(ab, " obj=(none)");
>> - rc = 1;
>> - } else {
>> - audit_log_format(ab, " obj=%s", lsmctx.context);
>> - security_release_secctx(&lsmctx);
>> - }
>> - }
>> + rc = audit_log_object_context(ab, blob);
>> audit_log_format(ab, " ocomm=");
>> audit_log_untrustedstring(ab, comm);
>> audit_log_end(ab);
> I realized you don't hang around linux-audit

I do, but having implemented audit systems in the past
I try not to interfere with someone else's approach for
fear of getting sucked in to working on it.

> (why would anyone want to do that?!)

Keeping an eye on trends or possible Smack impact.

> so let me tell you one of my most hated mantras: "new audit
> fields MUST go at the end of the audit record". The "MUST" is in all
> caps because either I'm being clever and reusing some IETF RFC
> concepts, or I'm tired of arguing this point and feel like
> capitalization is the best I can do for stress relief; maybe it is a
> combination of the two. Feel free to pick whichever reason you find
> most pleasing.

I'll go with stress relief. Glad to be helpful. ;)

> Either way, the "obj=" field should stay where it is, but the
> "obj_XXX=" fields need to find their way to the end of the record.

As Steve pointed out, there may be a bigger issue here. If the additional
fields aren't going to fit in MAX_AUDIT_MESSAGE_LENGTH bytes another
format may be required. I had hoped that perhaps obj_selinux= might count
as a refinement to obj= and hence not be considered a new field, but
it looks like that's not flying.