| Message ID | 20220418145945.38797-27-casey@schaufler-ca.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [v35,01/29] integrity: disassociate ima_filter_rule from security_audit_rule | expand |
On Mon, Apr 18, 2022 at 11:12 AM 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 | 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 <paul@paul-moore.com>
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 <casey@schaufler-ca.com> Besides moving the aux fns, and the whining below Reviewed-by: John Johansen <john.johansen@canonical.com> > --- > 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:
On Mon, Apr 25, 2022 at 9:08 PM John Johansen <john.johansen@canonical.com> 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 <casey@schaufler-ca.com> > > Besides moving the aux fns, and the whining below > Reviewed-by: John Johansen <john.johansen@canonical.com> ... > > 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.
On 4/26/22 11:15, Paul Moore wrote: > On Mon, Apr 25, 2022 at 9:08 PM John Johansen > <john.johansen@canonical.com> 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 <casey@schaufler-ca.com> >> >> Besides moving the aux fns, and the whining below >> Reviewed-by: John Johansen <john.johansen@canonical.com> > > ... > >>> 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. >
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:
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 | 93 +++++++++++++++++++++++++++++++++++--- 2 files changed, 88 insertions(+), 6 deletions(-)