Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <8be54cd7-61e1-5534-5f27-e3c24c6ad8fa@schaufler-ca.com>
Date:   Tue, 15 Mar 2022 17:17:06 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.7.0
Subject: Re: [PATCH v33 26/29] Audit: Add record for multiple task security
 contexts
Content-Language: en-US
To:     Paul Moore <paul@paul-moore.com>
Cc:     casey.schaufler@intel.com, jmorris@namei.org,
        linux-security-module@vger.kernel.org, selinux@vger.kernel.org,
        linux-audit@redhat.com, keescook@chromium.org,
        john.johansen@canonical.com, penguin-kernel@i-love.sakura.ne.jp,
        stephen.smalley.work@gmail.com, linux-kernel@vger.kernel.org,
        Casey Schaufler <casey@schaufler-ca.com>
References: <20220310234632.16194-1-casey@schaufler-ca.com>
 <20220310234632.16194-27-casey@schaufler-ca.com>
 <CAHC9VhSYr3nC87jk+cPzvVLgYZO9p9vaM7n+jvGEF60RaNJw7A@mail.gmail.com>
From:   Casey Schaufler <casey@schaufler-ca.com>
In-Reply-To: <CAHC9VhSYr3nC87jk+cPzvVLgYZO9p9vaM7n+jvGEF60RaNJw7A@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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