| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 15 Mar 2022 17:17:06 -0700
From: Casey Schaufler <casey@...aufler-ca.com>
To: Paul Moore <paul@...l-moore.com>
Cc: casey.schaufler@...el.com, jmorris@...ei.org,
linux-security-module@...r.kernel.org, selinux@...r.kernel.org,
linux-audit@...hat.com, keescook@...omium.org,
john.johansen@...onical.com, penguin-kernel@...ove.sakura.ne.jp,
stephen.smalley.work@...il.com, linux-kernel@...r.kernel.org,
Casey Schaufler <casey@...aufler-ca.com>
Subject: Re: [PATCH v33 26/29] Audit: Add record for multiple task security
contexts
On 3/15/2022 4:47 PM, Paul Moore wrote:
> On Thu, Mar 10, 2022 at 6:59 PM Casey Schaufler <casey@...aufler-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@...aufler-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
Powered by blists - more mailing lists