2022-04-19 02:24:33

by Casey Schaufler

[permalink] [raw]
Subject: [PATCH v35 26/29] Audit: Add record for multiple task security contexts

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 <[email protected]>
---
include/uapi/linux/audit.h | 1 +
kernel/audit.c | 93 +++++++++++++++++++++++++++++++++++---
2 files changed, 88 insertions(+), 6 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 4d44c05053b0..8ed2d717c217 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -2175,8 +2175,61 @@ void audit_log_key(struct audit_buffer *ab, char *key)
audit_log_format(ab, "(null)");
}

+/**
+ * audit_buffer_aux_new - Add an aux record buffer to the skb list
+ * @ab: audit_buffer
+ * @type: message type
+ *
+ * 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
+ * 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.
+ *
+ * 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)
+{
+ WARN_ON(ab->skb != skb_peek(&ab->skb_list));
+
+ ab->skb = nlmsg_new(AUDIT_BUFSIZ, ab->gfp_mask);
+ if (!ab->skb)
+ goto err;
+ if (!nlmsg_put(ab->skb, 0, 0, type, 0, 0))
+ goto err;
+ skb_queue_tail(&ab->skb_list, ab->skb);
+
+ audit_log_format(ab, "audit(%llu.%03lu:%u): ",
+ (unsigned long long)ab->stamp.ctime.tv_sec,
+ ab->stamp.ctime.tv_nsec/1000000,
+ ab->stamp.serial);
+
+ return 0;
+
+err:
+ kfree_skb(ab->skb);
+ ab->skb = skb_peek(&ab->skb_list);
+ return -ENOMEM;
+}
+
+/**
+ * audit_buffer_aux_end - Switch back to the "main" record from an aux record
+ * @ab: audit_buffer
+ *
+ * Restores the "main" audit record to ab->skb.
+ */
+static void audit_buffer_aux_end(struct audit_buffer *ab)
+{
+ ab->skb = skb_peek(&ab->skb_list);
+}
+
+
int audit_log_task_context(struct audit_buffer *ab)
{
+ int i;
int error;
struct lsmblob blob;
struct lsmcontext context;
@@ -2185,16 +2238,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) {
+ audit_log_format(ab, "%ssubj_%s=?",
+ i ? " " : "",
+ lsm_slot_to_name(i));
+ if (error != -EINVAL)
+ audit_panic("error in audit_log_task_context");
+ } 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:
--
2.35.1


2022-04-22 20:41:43

by Paul Moore

[permalink] [raw]
Subject: Re: [PATCH v35 26/29] Audit: Add record for multiple task security contexts

On Mon, Apr 18, 2022 at 11:12 AM Casey Schaufler <[email protected]> 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 <[email protected]>
> ---
> include/uapi/linux/audit.h | 1 +
> kernel/audit.c | 93 +++++++++++++++++++++++++++++++++++---
> 2 files changed, 88 insertions(+), 6 deletions(-)

The audit_buffer_aux_new() and audit_buffer_aux_end() belong in patch
25/29, but otherwise this looks okay.

Acked-by: Paul Moore <[email protected]>

--
paul-moore.com

2022-04-26 05:41:25

by John Johansen

[permalink] [raw]
Subject: Re: [PATCH v35 26/29] Audit: Add record for multiple task security contexts

On 4/18/22 07:59, Casey Schaufler 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 <[email protected]>

Besides moving the aux fns, and the whining below
Reviewed-by: John Johansen <[email protected]>

> ---
> include/uapi/linux/audit.h | 1 +
> kernel/audit.c | 93 +++++++++++++++++++++++++++++++++++---
> 2 files changed, 88 insertions(+), 6 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 4d44c05053b0..8ed2d717c217 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -2175,8 +2175,61 @@ void audit_log_key(struct audit_buffer *ab, char *key)
> audit_log_format(ab, "(null)");
> }
>
> +/**
> + * audit_buffer_aux_new - Add an aux record buffer to the skb list
> + * @ab: audit_buffer
> + * @type: message type
> + *
> + * 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
> + * 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.
> + *
> + * 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)
> +{
> + WARN_ON(ab->skb != skb_peek(&ab->skb_list));
> +
> + ab->skb = nlmsg_new(AUDIT_BUFSIZ, ab->gfp_mask);
> + if (!ab->skb)
> + goto err;
> + if (!nlmsg_put(ab->skb, 0, 0, type, 0, 0))
> + goto err;
> + skb_queue_tail(&ab->skb_list, ab->skb);
> +
> + audit_log_format(ab, "audit(%llu.%03lu:%u): ",
> + (unsigned long long)ab->stamp.ctime.tv_sec,
> + ab->stamp.ctime.tv_nsec/1000000,
> + ab->stamp.serial);
> +
> + return 0;
> +
> +err:
> + kfree_skb(ab->skb);
> + ab->skb = skb_peek(&ab->skb_list);
> + return -ENOMEM;
> +}
> +
> +/**
> + * audit_buffer_aux_end - Switch back to the "main" record from an aux record
> + * @ab: audit_buffer
> + *
> + * Restores the "main" audit record to ab->skb.
> + */
> +static void audit_buffer_aux_end(struct audit_buffer *ab)
> +{
> + ab->skb = skb_peek(&ab->skb_list);
> +}
> +
> +
> int audit_log_task_context(struct audit_buffer *ab)
> {
> + int i;
> int error;
> struct lsmblob blob;
> struct lsmcontext context;
> @@ -2185,16 +2238,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=?");

just me whining, you sure we can't just drop subj= here

> + 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) {
> + audit_log_format(ab, "%ssubj_%s=?",
> + i ? " " : "",
> + lsm_slot_to_name(i));
> + if (error != -EINVAL)
> + audit_panic("error in audit_log_task_context");
> + } 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:

2022-04-27 09:34:48

by John Johansen

[permalink] [raw]
Subject: Re: [PATCH v35 26/29] Audit: Add record for multiple task security contexts

On 4/26/22 11:15, Paul Moore wrote:
> On Mon, Apr 25, 2022 at 9:08 PM John Johansen
> <[email protected]> wrote:
>> On 4/18/22 07:59, Casey Schaufler 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 <[email protected]>
>>
>> Besides moving the aux fns, and the whining below
>> Reviewed-by: John Johansen <[email protected]>
>
> ...
>
>>> diff --git a/kernel/audit.c b/kernel/audit.c
>>> index 4d44c05053b0..8ed2d717c217 100644
>>> --- a/kernel/audit.c
>>> +++ b/kernel/audit.c
>>> @@ -2185,16 +2238,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=?");
>>
>> just me whining, you sure we can't just drop subj= here
>
> Have I recently given you my "the audit code is crap" speech? ;)
>
hehehe, I get it, something about glass houses and stones. the whole newline
mess in path 28/29 that I would dearly love to drop.

> I more or less answered this with my comments on the earlier patch,
> but we need to keep this around for compatibility. It will get better
> in the future.
>

2022-04-27 09:35:28

by Paul Moore

[permalink] [raw]
Subject: Re: [PATCH v35 26/29] Audit: Add record for multiple task security contexts

On Mon, Apr 25, 2022 at 9:08 PM John Johansen
<[email protected]> wrote:
> On 4/18/22 07:59, Casey Schaufler 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 <[email protected]>
>
> Besides moving the aux fns, and the whining below
> Reviewed-by: John Johansen <[email protected]>

...

> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 4d44c05053b0..8ed2d717c217 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -2185,16 +2238,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=?");
>
> just me whining, you sure we can't just drop subj= here

Have I recently given you my "the audit code is crap" speech? ;)

I more or less answered this with my comments on the earlier patch,
but we need to keep this around for compatibility. It will get better
in the future.

--
paul-moore.com