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

[Stephen Smalley]

On Fri, Jun 12, 2020 at 10:42 PM Lakshmi Ramasubramanian
<nramas@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx>
> ---
> 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.