Re: [PATCH 4/5] LSM: Define SELinux function to measure security state

[Lakshmi Ramasubramanian]

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