Messages in this thread | | | Subject | Re: [PATCH 4/5] LSM: Define SELinux function to measure security state | From | Lakshmi Ramasubramanian <> | Date | Mon, 15 Jun 2020 09:45:48 -0700 |
| |
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
| |