Message ID | 20250307183701.16970-5-casey@schaufler-ca.com (mailing list archive) |
---|---|
State | Handled Elsewhere |
Delegated to: | Paul Moore |
Headers | show |
Series | [v2,1/6] Audit: Create audit_stamp structure | expand |
On Mar 7, 2025 Casey Schaufler <casey@schaufler-ca.com> wrote: > > Create a new audit record AUDIT_MAC_TASK_CONTEXTS. > An example of the MAC_TASK_CONTEXTS (1423) record is: > > type=MAC_TASK_CONTEXTS[1423] > 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. > > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> > --- > include/linux/lsm_hooks.h | 1 + > include/linux/security.h | 1 + > include/uapi/linux/audit.h | 1 + > kernel/audit.c | 45 ++++++++++++++++++++++++++++++++------ > security/apparmor/lsm.c | 1 + > security/bpf/hooks.c | 1 + > security/security.c | 3 +++ > security/selinux/hooks.c | 1 + > security/smack/smack_lsm.c | 1 + > 9 files changed, 48 insertions(+), 7 deletions(-) > > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > index 090d1d3e19fe..e4d303ab1f20 100644 > --- a/include/linux/lsm_hooks.h > +++ b/include/linux/lsm_hooks.h > @@ -81,6 +81,7 @@ struct lsm_static_calls_table { > struct lsm_id { > const char *name; > u64 id; > + bool subjctx; > }; The new field needs to be documented in the comment block for the struct, and while I'll reserve judgement until I see the description, I believe this field either needs to be renamed, moved elsewhere, or simply "disappeared" in favor of something else. > diff --git a/include/linux/security.h b/include/linux/security.h > index 540894695c4b..79a9bf4a7cdd 100644 > --- a/include/linux/security.h > +++ b/include/linux/security.h > @@ -168,6 +168,7 @@ struct lsm_prop { > > extern const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1]; > extern u32 lsm_active_cnt; > +extern u32 lsm_subjctx_cnt; I'm not loving this. More below, but I'd really like to hide some of this detail inside a LSM API/hook if possible. > extern const struct lsm_id *lsm_idlist[]; > > /* These functions are in security/commoncap.c */ > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h > index d9a069b4a775..5ebb5d80363d 100644 > --- a/include/uapi/linux/audit.h > +++ b/include/uapi/linux/audit.h > @@ -146,6 +146,7 @@ > #define AUDIT_IPE_ACCESS 1420 /* IPE denial or grant */ > #define AUDIT_IPE_CONFIG_CHANGE 1421 /* IPE config change */ > #define AUDIT_IPE_POLICY_LOAD 1422 /* IPE policy load */ > +#define AUDIT_MAC_TASK_CONTEXTS 1423 /* 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 293364bba961..59eaf69ee8ac 100644 > --- a/kernel/audit.c > +++ b/kernel/audit.c > @@ -54,6 +54,7 @@ > #include <net/netlink.h> > #include <linux/skbuff.h> > #include <linux/security.h> > +#include <linux/lsm_hooks.h> > #include <linux/freezer.h> > #include <linux/pid_namespace.h> > #include <net/netns/generic.h> > @@ -2241,21 +2242,51 @@ int audit_log_task_context(struct audit_buffer *ab) > { > struct lsm_prop prop; > struct lsm_context ctx; > + bool space = false; > int error; > + int i; > > security_current_getlsmprop_subj(&prop); > if (!lsmprop_is_set(&prop)) > return 0; > > - error = security_lsmprop_to_secctx(&prop, &ctx, LSM_ID_UNDEF); > - if (error < 0) { > - if (error != -EINVAL) > - goto error_path; > + if (lsm_subjctx_cnt < 2) { > + error = security_lsmprop_to_secctx(&prop, &ctx, LSM_ID_UNDEF); > + if (error < 0) { > + if (error != -EINVAL) > + goto error_path; > + return 0; > + } > + audit_log_format(ab, " subj=%s", ctx.context); > + security_release_secctx(&ctx); > return 0; > } > - > - audit_log_format(ab, " subj=%s", ctx.context); > - security_release_secctx(&ctx); > + /* 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; > + > + for (i = 0; i < lsm_active_cnt; i++) { > + if (!lsm_idlist[i]->subjctx) > + continue; > + error = security_lsmprop_to_secctx(&prop, &ctx, > + lsm_idlist[i]->id); > + if (error < 0) { > + if (error == -EOPNOTSUPP) > + continue; > + audit_log_format(ab, "%ssubj_%s=?", space ? " " : "", > + lsm_idlist[i]->name); > + if (error != -EINVAL) > + audit_panic("error in audit_log_task_context"); > + } else { > + audit_log_format(ab, "%ssubj_%s=%s", space ? " " : "", > + lsm_idlist[i]->name, ctx.context); > + security_release_secctx(&ctx); > + } > + space = true; > + } > + audit_buffer_aux_end(ab); > return 0; I'd really like to see us develop something a bit cleaner than the code above. It looks like there are two things that we're currently missing in the LSM API: a count of the number of LSMs supplying data in a lsm_prop struct, and a mechanism to iterate through a lsm_prop and act on each LSM's data. As far as the count is concerned, I think we can take a similar approach to what we do with MAX_LSM_COUNT to generate a value at build time, for example (pardon the lazy pseudo code): >>> include/linux/lsm_count.h #if IS_ENABLED(CONFIG_SECURITY_SMACK) #define SMACK_ENABLED 1, #define SMACK_LSMPROP 1, #else #define SMACK_ENABLED #define SMACK_LSMPROP #endif #define MAX_LSM_PROPS \ COUNT_LSMS( \ SMACK_LSMPROP \ ...) \ The iterator mechanism is a little more complex, but I think the interface and resulting will be a lot cleaner, at least to in my mind. >>> LSM hook int security_lsmprop_walk(void *data, void (*iter)(struct lsm_id *lsm, void *data, char *secctx, u32 secid)); >>> example audit code struct lsmprop_walk_data { struct audit_buffer *ab; unsigned int count; }; void audit_lsmprop_walk(struct lsm_id *lsm, void *data, char *secctx, u32 secid) { struct lsmprop_walk_data *walk = data; audit_log_format(data->ab, "%ssubj_%s=%s", (data->count++ ? " " : ""), lsm->name, (secctx ? secctx : "?")); if (secctx) security_release_secctx(secctx); } int audit_log_task_context(...) { ... if (MAX_LSM_PROPS <= 1) { security_lsmprop_to_secctx(...) audit_log_format(...); } else { struct lsmprop_walk_data data = { .ab = ab, count = 0 }; audit_buffer_aux_new(ab, AUDIT_MAC_TASK_CONTEXTS); security_lsmprop_walk(data, audit_lsmprop_walk); audit_buffer_aux_end( } ... } -- paul-moore.com
On March 12, 2025 7:51:36 PM Paul Moore <paul@paul-moore.com> wrote: > On Mar 7, 2025 Casey Schaufler <casey@schaufler-ca.com> wrote: ... > >> diff --git a/include/linux/security.h b/include/linux/security.h >> index 540894695c4b..79a9bf4a7cdd 100644 >> --- a/include/linux/security.h >> +++ b/include/linux/security.h >> @@ -168,6 +168,7 @@ struct lsm_prop { >> >> extern const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1]; >> extern u32 lsm_active_cnt; >> +extern u32 lsm_subjctx_cnt; > > I'm not loving this. More below, but I'd really like to hide some of > this detail inside a LSM API/hook if possible. Thinking more about this I think we can't go with a LSM_MAX_PROPS, or similar determined at build time since we have the ability to toggle LSMs at boot. Need to think on this some more, but the answer is going to have to be a variable and not a #define. The LSM init work I'm doing right now directly impacts this, and I'm in the final stages now. Let me see what looks reasonable and I'll get back to you. -- paul-moore.com >
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index 090d1d3e19fe..e4d303ab1f20 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -81,6 +81,7 @@ struct lsm_static_calls_table { struct lsm_id { const char *name; u64 id; + bool subjctx; }; /* diff --git a/include/linux/security.h b/include/linux/security.h index 540894695c4b..79a9bf4a7cdd 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -168,6 +168,7 @@ struct lsm_prop { extern const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1]; extern u32 lsm_active_cnt; +extern u32 lsm_subjctx_cnt; extern const struct lsm_id *lsm_idlist[]; /* These functions are in security/commoncap.c */ diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h index d9a069b4a775..5ebb5d80363d 100644 --- a/include/uapi/linux/audit.h +++ b/include/uapi/linux/audit.h @@ -146,6 +146,7 @@ #define AUDIT_IPE_ACCESS 1420 /* IPE denial or grant */ #define AUDIT_IPE_CONFIG_CHANGE 1421 /* IPE config change */ #define AUDIT_IPE_POLICY_LOAD 1422 /* IPE policy load */ +#define AUDIT_MAC_TASK_CONTEXTS 1423 /* 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 293364bba961..59eaf69ee8ac 100644 --- a/kernel/audit.c +++ b/kernel/audit.c @@ -54,6 +54,7 @@ #include <net/netlink.h> #include <linux/skbuff.h> #include <linux/security.h> +#include <linux/lsm_hooks.h> #include <linux/freezer.h> #include <linux/pid_namespace.h> #include <net/netns/generic.h> @@ -2241,21 +2242,51 @@ int audit_log_task_context(struct audit_buffer *ab) { struct lsm_prop prop; struct lsm_context ctx; + bool space = false; int error; + int i; security_current_getlsmprop_subj(&prop); if (!lsmprop_is_set(&prop)) return 0; - error = security_lsmprop_to_secctx(&prop, &ctx, LSM_ID_UNDEF); - if (error < 0) { - if (error != -EINVAL) - goto error_path; + if (lsm_subjctx_cnt < 2) { + error = security_lsmprop_to_secctx(&prop, &ctx, LSM_ID_UNDEF); + if (error < 0) { + if (error != -EINVAL) + goto error_path; + return 0; + } + audit_log_format(ab, " subj=%s", ctx.context); + security_release_secctx(&ctx); return 0; } - - audit_log_format(ab, " subj=%s", ctx.context); - security_release_secctx(&ctx); + /* 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; + + for (i = 0; i < lsm_active_cnt; i++) { + if (!lsm_idlist[i]->subjctx) + continue; + error = security_lsmprop_to_secctx(&prop, &ctx, + lsm_idlist[i]->id); + if (error < 0) { + if (error == -EOPNOTSUPP) + continue; + audit_log_format(ab, "%ssubj_%s=?", space ? " " : "", + lsm_idlist[i]->name); + if (error != -EINVAL) + audit_panic("error in audit_log_task_context"); + } else { + audit_log_format(ab, "%ssubj_%s=%s", space ? " " : "", + lsm_idlist[i]->name, ctx.context); + security_release_secctx(&ctx); + } + space = true; + } + audit_buffer_aux_end(ab); return 0; error_path: diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c index 9b6c2f157f83..17ec93a8d3fc 100644 --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c @@ -1427,6 +1427,7 @@ struct lsm_blob_sizes apparmor_blob_sizes __ro_after_init = { static const struct lsm_id apparmor_lsmid = { .name = "apparmor", .id = LSM_ID_APPARMOR, + .subjctx = true, }; static struct security_hook_list apparmor_hooks[] __ro_after_init = { diff --git a/security/bpf/hooks.c b/security/bpf/hooks.c index db759025abe1..aaaa1227ce13 100644 --- a/security/bpf/hooks.c +++ b/security/bpf/hooks.c @@ -18,6 +18,7 @@ static struct security_hook_list bpf_lsm_hooks[] __ro_after_init = { static const struct lsm_id bpf_lsmid = { .name = "bpf", .id = LSM_ID_BPF, + .subjctx = false, /* property exists, but will not be used */ }; static int __init bpf_lsm_init(void) diff --git a/security/security.c b/security/security.c index 55f9c7ad3f89..8450cc5f82d5 100644 --- a/security/security.c +++ b/security/security.c @@ -320,6 +320,7 @@ static void __init initialize_lsm(struct lsm_info *lsm) * Current index to use while initializing the lsm id list. */ u32 lsm_active_cnt __ro_after_init; +u32 lsm_subjctx_cnt __ro_after_init; const struct lsm_id *lsm_idlist[MAX_LSM_COUNT]; /* Populate ordered LSMs list from comma-separated LSM name list. */ @@ -626,6 +627,8 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count, if (lsm_active_cnt >= MAX_LSM_COUNT) panic("%s Too many LSMs registered.\n", __func__); lsm_idlist[lsm_active_cnt++] = lsmid; + if (lsmid->subjctx) + lsm_subjctx_cnt++; } for (i = 0; i < count; i++) { diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 7b867dfec88b..1e2e1545eb2e 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -7142,6 +7142,7 @@ static int selinux_uring_cmd(struct io_uring_cmd *ioucmd) static const struct lsm_id selinux_lsmid = { .name = "selinux", .id = LSM_ID_SELINUX, + .subjctx = true, }; /* diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index 239773cdcdcf..75bd62fe1513 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -5057,6 +5057,7 @@ struct lsm_blob_sizes smack_blob_sizes __ro_after_init = { static const struct lsm_id smack_lsmid = { .name = "smack", .id = LSM_ID_SMACK, + .subjctx = true, }; static struct security_hook_list smack_hooks[] __ro_after_init = {
Create a new audit record AUDIT_MAC_TASK_CONTEXTS. An example of the MAC_TASK_CONTEXTS (1423) record is: type=MAC_TASK_CONTEXTS[1423] 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. Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> --- include/linux/lsm_hooks.h | 1 + include/linux/security.h | 1 + include/uapi/linux/audit.h | 1 + kernel/audit.c | 45 ++++++++++++++++++++++++++++++++------ security/apparmor/lsm.c | 1 + security/bpf/hooks.c | 1 + security/security.c | 3 +++ security/selinux/hooks.c | 1 + security/smack/smack_lsm.c | 1 + 9 files changed, 48 insertions(+), 7 deletions(-)