| Message ID | 20200613024130.3356-5-nramas@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | LSM: Measure security module state | expand |
On Fri, Jun 12, 2020 at 10:42 PM Lakshmi Ramasubramanian <nramas@linux.microsoft.com> wrote: > > SELinux needs to implement the interface function, security_state(), for > the LSM to gather SELinux data for measuring. Define the security_state() > function in SELinux. > > The security modules should be able to notify the LSM when there is > a change in the module's data. Define a function namely > security_state_change() in the LSM that the security modules > can call to provide the updated data for measurement. > > Call security_state_change() function from SELinux to report data > when SELinux's state is updated. > > Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> > --- > diff --git a/security/security.c b/security/security.c > index a6e2d1cd95af..e7175db5a093 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -238,6 +238,11 @@ static void __init initialize_lsm(struct lsm_info *lsm) > } > } > > +void security_state_change(char *lsm_name, void *state, int state_len) > +{ > + ima_lsm_state(lsm_name, state, state_len); > +} > + What's the benefit of this trivial function instead of just calling ima_lsm_state() directly? > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 7e954b555be6..bbc908a1fcd1 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -7225,6 +7225,47 @@ static __init int selinux_init(void) > return 0; > } > > +static int selinux_security_state(char **lsm_name, void **state, > + int *state_len) > +{ > + int rc = 0; > + char *new_state; > + static char *security_state_string = "enabled=%d;enforcing=%d"; > + > + *lsm_name = kstrdup("selinux", GFP_KERNEL); > + if (!*lsm_name) > + return -ENOMEM; > + > + new_state = kzalloc(strlen(security_state_string) + 1, GFP_KERNEL); > + if (!new_state) { > + kfree(*lsm_name); > + *lsm_name = NULL; > + rc = -ENOMEM; > + goto out; > + } > + > + *state_len = sprintf(new_state, security_state_string, > + !selinux_disabled(&selinux_state), > + enforcing_enabled(&selinux_state)); I think I mentioned this on a previous version of these patches, but I would recommend including more than just the enabled and enforcing states in your measurement. Other low-hanging fruit would be the other selinux_state booleans (checkreqprot, initialized, policycap[0..__POLICYDB_CAPABILITY_MAX]). Going a bit further one could take a hash of the loaded policy by using security_read_policy() and then computing a hash using whatever hash ima prefers over the returned data,len pair. You likely also need to think about how to allow future extensibility of the state in a backward-compatible manner, so that future additions do not immediately break systems relying on older measurements.
On Mon, Jun 15, 2020 at 7:57 AM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > I think I mentioned this on a previous version of these patches, but I > would recommend including more than just the enabled and enforcing > states in your measurement. Other low-hanging fruit would be the > other selinux_state booleans (checkreqprot, initialized, > policycap[0..__POLICYDB_CAPABILITY_MAX]). Going a bit further one > could take a hash of the loaded policy by using security_read_policy() On second thought, you probably a variant of security_read_policy() since it would be a kernel-internal allocation and thus shouldn't use vmalloc_user(). > and then computing a hash using whatever hash ima prefers over the > returned data,len pair. You likely also need to think about how to > allow future extensibility of the state in a backward-compatible > manner, so that future additions do not immediately break systems > relying on older measurements.
On 6/15/20 4:57 AM, Stephen Smalley wrote: Hi Stephen, Thanks for reviewing the patches. >> +void security_state_change(char *lsm_name, void *state, int state_len) >> +{ >> + ima_lsm_state(lsm_name, state, state_len); >> +} >> + > > What's the benefit of this trivial function instead of just calling > ima_lsm_state() directly? One of the feedback Casey Schaufler had given earlier was that calling an IMA function directly from SELinux (or, any of the Security Modules) would be a layering violation. LSM framework (security/security.c) already calls IMA functions now (for example, ima_bprm_check() is called from security_bprm_check()). I followed the same pattern for measuring LSM data as well. Please let me know if I misunderstood Casey's comment. >> +static int selinux_security_state(char **lsm_name, void **state, >> + int *state_len) >> +{ >> + int rc = 0; >> + char *new_state; >> + static char *security_state_string = "enabled=%d;enforcing=%d"; >> + >> + *lsm_name = kstrdup("selinux", GFP_KERNEL); >> + if (!*lsm_name) >> + return -ENOMEM; >> + >> + new_state = kzalloc(strlen(security_state_string) + 1, GFP_KERNEL); >> + if (!new_state) { >> + kfree(*lsm_name); >> + *lsm_name = NULL; >> + rc = -ENOMEM; >> + goto out; >> + } >> + >> + *state_len = sprintf(new_state, security_state_string, >> + !selinux_disabled(&selinux_state), >> + enforcing_enabled(&selinux_state)); > > I think I mentioned this on a previous version of these patches, but I > would recommend including more than just the enabled and enforcing > states in your measurement. Other low-hanging fruit would be the > other selinux_state booleans (checkreqprot, initialized, > policycap[0..__POLICYDB_CAPABILITY_MAX]). Going a bit further one > could take a hash of the loaded policy by using security_read_policy() > and then computing a hash using whatever hash ima prefers over the > returned data,len pair. You likely also need to think about how to > allow future extensibility of the state in a backward-compatible > manner, so that future additions do not immediately break systems > relying on older measurements. > Sure - I will address this one in the next update. thanks, -lakshmi
On 6/15/2020 9:45 AM, Lakshmi Ramasubramanian wrote: > On 6/15/20 4:57 AM, Stephen Smalley wrote: > > Hi Stephen, > > Thanks for reviewing the patches. > >>> +void security_state_change(char *lsm_name, void *state, int state_len) >>> +{ >>> + ima_lsm_state(lsm_name, state, state_len); >>> +} >>> + >> >> What's the benefit of this trivial function instead of just calling >> ima_lsm_state() directly? > > One of the feedback Casey Schaufler had given earlier was that calling an IMA function directly from SELinux (or, any of the Security Modules) would be a layering violation. Hiding the ima_lsm_state() call doesn't address the layering. The point is that SELinux code being called from IMA (or the other way around) breaks the subsystem isolation. Unfortunately, it isn't obvious to me how you would go about what you're doing without integrating the subsystems. > > LSM framework (security/security.c) already calls IMA functions now (for example, ima_bprm_check() is called from security_bprm_check()). I followed the same pattern for measuring LSM data as well. > > Please let me know if I misunderstood Casey's comment. >
(Cc'ing John) On Mon, 2020-06-15 at 10:33 -0700, Casey Schaufler wrote: > On 6/15/2020 9:45 AM, Lakshmi Ramasubramanian wrote: > > On 6/15/20 4:57 AM, Stephen Smalley wrote: > > > > Hi Stephen, > > > > Thanks for reviewing the patches. > > > >>> +void security_state_change(char *lsm_name, void *state, int state_len) > >>> +{ > >>> + ima_lsm_state(lsm_name, state, state_len); > >>> +} > >>> + > >> > >> What's the benefit of this trivial function instead of just calling > >> ima_lsm_state() directly? > > > > One of the feedback Casey Schaufler had given earlier was that calling an IMA function directly from SELinux (or, any of the Security Modules) would be a layering violation. > > Hiding the ima_lsm_state() call doesn't address the layering. > The point is that SELinux code being called from IMA (or the > other way around) breaks the subsystem isolation. Unfortunately, > it isn't obvious to me how you would go about what you're doing > without integrating the subsystems. Casey, I'm not sure why you think there is a layering issue here. There were multiple iterations of IMA before it was upstreamed. One iteration had separate integrity hooks(LIM). Only when the IMA calls and the security hooks are co-located, are they combined, as requested by Linus. There was some AppArmour discussion about calling IMA directly, but I haven't heard about it in a while or seen the patch. Mimi
On Mon, Jun 15, 2020 at 12:45 PM Lakshmi Ramasubramanian <nramas@linux.microsoft.com> wrote: > > On 6/15/20 4:57 AM, Stephen Smalley wrote: > > I think I mentioned this on a previous version of these patches, but I > > would recommend including more than just the enabled and enforcing > > states in your measurement. Other low-hanging fruit would be the > > other selinux_state booleans (checkreqprot, initialized, > > policycap[0..__POLICYDB_CAPABILITY_MAX]). Going a bit further one > > could take a hash of the loaded policy by using security_read_policy() > > and then computing a hash using whatever hash ima prefers over the > > returned data,len pair. You likely also need to think about how to > > allow future extensibility of the state in a backward-compatible > > manner, so that future additions do not immediately break systems > > relying on older measurements. > > > > Sure - I will address this one in the next update. Please add selinux list to the cc for future versions too.
On 6/15/2020 10:44 AM, Mimi Zohar wrote: > (Cc'ing John) > > On Mon, 2020-06-15 at 10:33 -0700, Casey Schaufler wrote: >> On 6/15/2020 9:45 AM, Lakshmi Ramasubramanian wrote: >>> On 6/15/20 4:57 AM, Stephen Smalley wrote: >>> >>> Hi Stephen, >>> >>> Thanks for reviewing the patches. >>> >>>>> +void security_state_change(char *lsm_name, void *state, int state_len) >>>>> +{ >>>>> + ima_lsm_state(lsm_name, state, state_len); >>>>> +} >>>>> + >>>> What's the benefit of this trivial function instead of just calling >>>> ima_lsm_state() directly? >>> One of the feedback Casey Schaufler had given earlier was that calling an IMA function directly from SELinux (or, any of the Security Modules) would be a layering violation. >> Hiding the ima_lsm_state() call doesn't address the layering. >> The point is that SELinux code being called from IMA (or the >> other way around) breaks the subsystem isolation. Unfortunately, >> it isn't obvious to me how you would go about what you're doing >> without integrating the subsystems. > Casey, I'm not sure why you think there is a layering issue here. I don't think there is, after further review. If the IMA code called selinux_dosomething() directly I'd be very concerned, but calling security_dosomething() which then calls selinux_dosomething() is fine. If YAMA called security_dosomething() I'd be very concerned, but that's not what's happening here. > There were multiple iterations of IMA before it was upstreamed. One > iteration had separate integrity hooks(LIM). Only when the IMA calls > and the security hooks are co-located, are they combined, as requested > by Linus. > > There was some AppArmour discussion about calling IMA directly, but I > haven't heard about it in a while or seen the patch. > > Mimi
On Mon, 2020-06-15 at 16:18 -0700, Casey Schaufler wrote: > On 6/15/2020 10:44 AM, Mimi Zohar wrote: > > (Cc'ing John) > > > > On Mon, 2020-06-15 at 10:33 -0700, Casey Schaufler wrote: > >> On 6/15/2020 9:45 AM, Lakshmi Ramasubramanian wrote: > >>> On 6/15/20 4:57 AM, Stephen Smalley wrote: > >>> > >>> Hi Stephen, > >>> > >>> Thanks for reviewing the patches. > >>> > >>>>> +void security_state_change(char *lsm_name, void *state, int state_len) > >>>>> +{ > >>>>> + ima_lsm_state(lsm_name, state, state_len); > >>>>> +} > >>>>> + > >>>> What's the benefit of this trivial function instead of just calling > >>>> ima_lsm_state() directly? > >>> One of the feedback Casey Schaufler had given earlier was that calling an IMA function directly from SELinux (or, any of the Security Modules) would be a layering violation. > >> Hiding the ima_lsm_state() call doesn't address the layering. > >> The point is that SELinux code being called from IMA (or the > >> other way around) breaks the subsystem isolation. Unfortunately, > >> it isn't obvious to me how you would go about what you're doing > >> without integrating the subsystems. > > Casey, I'm not sure why you think there is a layering issue here. > > I don't think there is, after further review. If the IMA code called > selinux_dosomething() directly I'd be very concerned, but calling > security_dosomething() which then calls selinux_dosomething() is fine. > If YAMA called security_dosomething() I'd be very concerned, but that's > not what's happening here. As long as the call to IMA is not an LSM hook, there shouldn't be a problem with an LSM calling IMA directly. A perfect example is measuring, appraising and/or auditing LSM policies. Mimi > > > There were multiple iterations of IMA before it was upstreamed. One > > iteration had separate integrity hooks(LIM). Only when the IMA calls > > and the security hooks are co-located, are they combined, as requested > > by Linus. > > > > There was some AppArmour discussion about calling IMA directly, but I > > haven't heard about it in a while or seen the patch.
On 6/15/20 10:44 AM, Mimi Zohar wrote: > (Cc'ing John) > > On Mon, 2020-06-15 at 10:33 -0700, Casey Schaufler wrote: >> On 6/15/2020 9:45 AM, Lakshmi Ramasubramanian wrote: >>> On 6/15/20 4:57 AM, Stephen Smalley wrote: >>> >>> Hi Stephen, >>> >>> Thanks for reviewing the patches. >>> >>>>> +void security_state_change(char *lsm_name, void *state, int state_len) >>>>> +{ >>>>> + ima_lsm_state(lsm_name, state, state_len); >>>>> +} >>>>> + >>>> >>>> What's the benefit of this trivial function instead of just calling >>>> ima_lsm_state() directly? >>> >>> One of the feedback Casey Schaufler had given earlier was that calling an IMA function directly from SELinux (or, any of the Security Modules) would be a layering violation. >> >> Hiding the ima_lsm_state() call doesn't address the layering. >> The point is that SELinux code being called from IMA (or the >> other way around) breaks the subsystem isolation. Unfortunately, >> it isn't obvious to me how you would go about what you're doing >> without integrating the subsystems. > > Casey, I'm not sure why you think there is a layering issue here. > There were multiple iterations of IMA before it was upstreamed. One > iteration had separate integrity hooks(LIM). Only when the IMA calls > and the security hooks are co-located, are they combined, as requested > by Linus. > I don't see the layering violation here either, Casey has already responded and I don't have anything to add > There was some AppArmour discussion about calling IMA directly, but I > haven't heard about it in a while or seen the patch. > its lower priority than other work. I think calling IMA directly has use cases but must be done very carefully, and well reviewed. I have would have more concerns with IMA calling directly into the various LSMs.
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index da248c3fd4ac..a63de046139e 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -1572,6 +1572,9 @@ struct lsm_info { int *state_len); /*Optional */ }; +/* Called by LSMs to notify security state change */ +extern void security_state_change(char *lsm_name, void *state, int state_len); + extern struct lsm_info __start_lsm_info[], __end_lsm_info[]; extern struct lsm_info __start_early_lsm_info[], __end_early_lsm_info[]; diff --git a/security/security.c b/security/security.c index a6e2d1cd95af..e7175db5a093 100644 --- a/security/security.c +++ b/security/security.c @@ -238,6 +238,11 @@ static void __init initialize_lsm(struct lsm_info *lsm) } } +void security_state_change(char *lsm_name, void *state, int state_len) +{ + ima_lsm_state(lsm_name, state, state_len); +} + static int measure_security_state(struct lsm_info *lsm) { char *lsm_name = NULL; diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 7e954b555be6..bbc908a1fcd1 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -7225,6 +7225,47 @@ static __init int selinux_init(void) return 0; } +static int selinux_security_state(char **lsm_name, void **state, + int *state_len) +{ + int rc = 0; + char *new_state; + static char *security_state_string = "enabled=%d;enforcing=%d"; + + *lsm_name = kstrdup("selinux", GFP_KERNEL); + if (!*lsm_name) + return -ENOMEM; + + new_state = kzalloc(strlen(security_state_string) + 1, GFP_KERNEL); + if (!new_state) { + kfree(*lsm_name); + *lsm_name = NULL; + rc = -ENOMEM; + goto out; + } + + *state_len = sprintf(new_state, security_state_string, + !selinux_disabled(&selinux_state), + enforcing_enabled(&selinux_state)); + *state = new_state; + +out: + return rc; +} + +void notify_security_state_change(void) +{ + char *lsm_name = NULL; + void *state = NULL; + int state_len = 0; + + if (!selinux_security_state(&lsm_name, &state, &state_len)) { + security_state_change(lsm_name, state, state_len); + kfree(state); + kfree(lsm_name); + } +} + static void delayed_superblock_init(struct super_block *sb, void *unused) { selinux_set_mnt_opts(sb, NULL, 0, NULL); @@ -7247,6 +7288,7 @@ DEFINE_LSM(selinux) = { .enabled = &selinux_enabled_boot, .blobs = &selinux_blob_sizes, .init = selinux_init, + .security_state = selinux_security_state, }; #if defined(CONFIG_NETFILTER) @@ -7357,6 +7399,7 @@ int selinux_disable(struct selinux_state *state) } selinux_mark_disabled(state); + notify_security_state_change(); pr_info("SELinux: Disabled at runtime.\n"); diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h index b0e02cfe3ce1..83c6ada45c7c 100644 --- a/security/selinux/include/security.h +++ b/security/selinux/include/security.h @@ -232,6 +232,8 @@ size_t security_policydb_len(struct selinux_state *state); int security_policycap_supported(struct selinux_state *state, unsigned int req_cap); +void notify_security_state_change(void); + #define SEL_VEC_MAX 32 struct av_decision { u32 allowed; diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c index 4781314c2510..8a5ba32a7775 100644 --- a/security/selinux/selinuxfs.c +++ b/security/selinux/selinuxfs.c @@ -173,6 +173,7 @@ static ssize_t sel_write_enforce(struct file *file, const char __user *buf, from_kuid(&init_user_ns, audit_get_loginuid(current)), audit_get_sessionid(current)); enforcing_set(state, new_value); + notify_security_state_change(); if (new_value) avc_ss_reset(state->avc, 0); selnl_notify_setenforce(new_value);
SELinux needs to implement the interface function, security_state(), for the LSM to gather SELinux data for measuring. Define the security_state() function in SELinux. The security modules should be able to notify the LSM when there is a change in the module's data. Define a function namely security_state_change() in the LSM that the security modules can call to provide the updated data for measurement. Call security_state_change() function from SELinux to report data when SELinux's state is updated. Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> --- include/linux/lsm_hooks.h | 3 ++ security/security.c | 5 ++++ security/selinux/hooks.c | 43 +++++++++++++++++++++++++++++ security/selinux/include/security.h | 2 ++ security/selinux/selinuxfs.c | 1 + 5 files changed, 54 insertions(+)