Messages in this thread | | | Date | Tue, 15 Mar 2022 17:17:06 -0700 | Subject | Re: [PATCH v33 26/29] Audit: Add record for multiple task security contexts | From | Casey Schaufler <> |
| |
On 3/15/2022 4:47 PM, Paul Moore wrote: > On Thu, Mar 10, 2022 at 6:59 PM Casey Schaufler <casey@schaufler-ca.com> wrote: >> Create a new audit record AUDIT_MAC_TASK_CONTEXTS. >> An example of the MAC_TASK_CONTEXTS (1420) record is: >> >> type=MAC_TASK_CONTEXTS[1420] >> msg=audit(1600880931.832:113) >> subj_apparmor=unconfined >> subj_smack=_ >> >> When an audit event includes a AUDIT_MAC_TASK_CONTEXTS record >> the "subj=" field in other records in the event will be "subj=?". >> An AUDIT_MAC_TASK_CONTEXTS record is supplied when the system has >> multiple security modules that may make access decisions based >> on a subject security context. >> >> Functions are created to manage the skb list in the audit_buffer. >> >> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> >> --- >> include/uapi/linux/audit.h | 1 + >> kernel/audit.c | 104 ++++++++++++++++++++++++++++++++----- >> 2 files changed, 93 insertions(+), 12 deletions(-) >> >> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h >> index 8eda133ca4c1..af0aaccfaf57 100644 >> --- a/include/uapi/linux/audit.h >> +++ b/include/uapi/linux/audit.h >> @@ -143,6 +143,7 @@ >> #define AUDIT_MAC_UNLBL_STCDEL 1417 /* NetLabel: del a static label */ >> #define AUDIT_MAC_CALIPSO_ADD 1418 /* NetLabel: add CALIPSO DOI entry */ >> #define AUDIT_MAC_CALIPSO_DEL 1419 /* NetLabel: del CALIPSO DOI entry */ >> +#define AUDIT_MAC_TASK_CONTEXTS 1420 /* Multiple LSM task contexts */ >> >> #define AUDIT_FIRST_KERN_ANOM_MSG 1700 >> #define AUDIT_LAST_KERN_ANOM_MSG 1799 >> diff --git a/kernel/audit.c b/kernel/audit.c >> index 4713e66a12af..ad825af203cf 100644 >> --- a/kernel/audit.c >> +++ b/kernel/audit.c >> @@ -2147,8 +2147,65 @@ void audit_log_key(struct audit_buffer *ab, char *key) >> audit_log_format(ab, "(null)"); >> } >> >> +/* >> + * A brief note on aux record management. >> + * >> + * Aux records are allocated and added to the skb list of >> + * the "main" record. The ab->skb is reset to point to the >> + * aux record on its creation. When the aux record in complete > ^^ > "is" >> + * ab->skb has to be reset to point to the "main" record. >> + * This allows the audit_log_ functions to be ignorant of >> + * which kind of record it is logging to. It also avoids adding >> + * special data for aux records. >> + */ > It might be good to move the above comment into the > audit_buffer_aux_new() comment header (below) so it does not get > misplaced. > >> +/** >> + * audit_buffer_aux_new - Add an aux record buffer to the skb list >> + * @ab: audit_buffer >> + * @type: message type >> + * >> + * On success ab->skb will point to the new aux record. >> + * Returns 0 on success, -ENOMEM should allocation fail. >> + */ >> +static int audit_buffer_aux_new(struct audit_buffer *ab, int type) > ... > >> @@ -2157,16 +2214,44 @@ int audit_log_task_context(struct audit_buffer *ab) >> if (!lsmblob_is_set(&blob)) >> return 0; >> >> - error = security_secid_to_secctx(&blob, &context, LSMBLOB_FIRST); >> + if (!lsm_multiple_contexts()) { >> + error = security_secid_to_secctx(&blob, &context, >> + LSMBLOB_FIRST); >> + if (error) { >> + if (error != -EINVAL) >> + goto error_path; >> + return 0; >> + } >> >> - if (error) { >> - if (error != -EINVAL) >> + audit_log_format(ab, " subj=%s", context.context); >> + security_release_secctx(&context); >> + } else { >> + /* Multiple LSMs provide contexts. Include an aux record. */ >> + audit_log_format(ab, " subj=?"); >> + error = audit_buffer_aux_new(ab, AUDIT_MAC_TASK_CONTEXTS); >> + if (error) >> goto error_path; >> - return 0; >> + for (i = 0; i < LSMBLOB_ENTRIES; i++) { >> + if (blob.secid[i] == 0) >> + continue; >> + error = security_secid_to_secctx(&blob, &context, i); >> + if (error) { >> + if (error != -EINVAL) >> + audit_panic("error in audit_log_task_context"); >> + audit_log_format(ab, "%ssubj_%s=?", >> + i ? " " : "", >> + lsm_slot_to_name(i)); > I wonder if it might be better to record the "subj_smack=?" field > before checking @error and potentially calling audit_panic(). In > practice it likely shouldn't matter, I feel better if we at least > record the subject information before we call the wildcard that is > audit_panic().
Not a problem. Easy enough to fix.
> >> + } else { >> + audit_log_format(ab, "%ssubj_%s=%s", >> + i ? " " : "", >> + lsm_slot_to_name(i), >> + context.context); >> + security_release_secctx(&context); >> + } >> + } >> + audit_buffer_aux_end(ab); >> } >> >> - audit_log_format(ab, " subj=%s", context.context); >> - security_release_secctx(&context); >> return 0; >> >> error_path: >> @@ -2382,13 +2467,8 @@ int audit_signal_info(int sig, struct task_struct *t) >> } >> >> /** >> - * __audit_log_end - end one audit record >> + * __audit_log_end - send one audit record > If we want to be very nit-picky here, "end" is more correct than > "send". First, audit_log_end() doesn't actually send the record, it > just queues the record for the kauditd_thread which then attempts to > send it. Second, there is no guarantee that the record will actually > be sent at this point, although it would be nice if that were true :)
My bad. I thought I had deleted the 's', so I fixed it.
>> * @skb: the buffer to send >> - * >> - * We can not do a netlink send inside an irq context because it blocks (last >> - * arg, flags, is not set to MSG_DONTWAIT), so the audit buffer is placed on a >> - * queue and a kthread is scheduled to remove them from the queue outside the >> - * irq context. May be called in any context. >> */ > This should probably be moved to patch 25/29 as it has more to do with > the __audit_log_end() introduction than this patch. > > > -- > paul-moore.com
| |