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

[Stephen Smalley]

On Wed, Jul 15, 2020 at 11:48 AM Lakshmi Ramasubramanian
<nramas@xxxxxxxxxxxxxxxxxxx> wrote:
>
> SELinux configuration and policy are some of the critical data for this
> security module that needs to be measured. To enable this measurement
> SELinux needs to implement the interface function, security_state(), that
> the LSM can call.
>
> Define the security_state() function in SELinux to measure SELinux
> configuration and policy. Call this function to measure SELinux data
> when there is a change in the security module's state.
>
> Sample measurement of SELinux state and hash of the policy:
>
> 10 e32e...5ac3 ima-buf sha256:86e8...4594 selinux-state 656e61626c65643d313b656e666f7263696e673d303b636865636b72657170726f743d313b6e6574706565723d313b6f70656e7065726d3d313b657874736f636b636c6173733d313b616c776179736e6574776f726b3d303b6367726f75707365636c6162656c3d313b6e6e706e6f737569647472616e736974696f6e3d313b67656e66737365636c6162656c73796d6c696e6b3d303b
> 10 f4a7...9408 ima-buf sha256:4941...68fc selinux-policy-hash 8d1d...1834
>
> The data for selinux-state in the above measurement is:
> enabled=1;enforcing=0;checkreqprot=1;netpeer=1;openperm=1;extsockclass=1;alwaysnetwork=0;cgroupseclabel=1;nnpnosuidtransition=1;genfsseclabelsymlink=0;
>
> The data for selinux-policy-hash in the above measurement is
> the SHA256 hash of the SELinux policy.
>
> Signed-off-by: Lakshmi Ramasubramanian <nramas@xxxxxxxxxxxxxxxxxxx>
> Suggested-by: Stephen Smalley <stephen.smalley.work@xxxxxxxxx>
> ---

> diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
> index b0e02cfe3ce1..691c7e35f82a 100644
> --- a/security/selinux/include/security.h
> +++ b/security/selinux/include/security.h
> @@ -222,16 +222,35 @@ static inline bool selinux_policycap_genfs_seclabel_symlinks(void)
>         return state->policycap[POLICYDB_CAPABILITY_GENFS_SECLABEL_SYMLINKS];
>  }
>
> +static inline bool selinux_checkreqprot(void)
> +{
> +       struct selinux_state *state = &selinux_state;
> +
> +       return state->checkreqprot;
> +}

Probably should use READ_ONCE().

> diff --git a/security/selinux/measure.c b/security/selinux/measure.c
> new file mode 100644
> index 000000000000..b909e8e61542
> --- /dev/null
> +++ b/security/selinux/measure.c
> @@ -0,0 +1,122 @@
> +int selinux_security_state(void)

Let's call this selinux_measure_state() or similar.  Needs a verb. And
pass it a struct selinux_state * pointer argument to be measured, even
though initially it will always be passed &selinux_state.  The
encapsulation of selinux state within selinux_state was to support
multiple selinux namespaces in the future, each with their own state.

> +{
> +       int rc = 0;
> +       int count;
> +       size_t buflen;
> +       int policy_hash_len;
> +       char *state = NULL;
> +       void *policy = NULL;
> +       void *policy_hash = NULL;
> +       static char *security_state_string =
> +                       "enabled=%d;enforcing=%d;checkreqprot=%d;"        \
> +                       "netpeer=%d;openperm=%d;extsockclass=%d;"         \
> +                       "alwaysnetwork=%d;cgroupseclabel=%d;"             \
> +                       "nnpnosuidtransition=%d;genfsseclabelsymlink=%d;";

Rather than hardcoding policy capability names here, I would recommend
using the selinux_policycap_names[] array for the names and the
selinux_state.policycap[] array for the values.  Also recommend
passing in a struct selinux_state * here to allow for future case
where there are multiple selinux states, one per selinux namespace.

> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index ef0afd878bfc..0c289d13ef6a 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -3724,10 +3724,11 @@ int security_netlbl_sid_to_secattr(struct selinux_state *state,
>   * security_read_policy - read the policy.
>   * @data: binary policy data
>   * @len: length of data in bytes
> - *
> + * @alloc_kernel_memory: flag to indicate memory allocation
>   */
> -int security_read_policy(struct selinux_state *state,
> -                        void **data, size_t *len)
> +int security_read_selinux_policy(struct selinux_state *state,
> +                                void **data, size_t *len,
> +                                bool alloc_kernel_memory)

Instead of passing in a boolean to control how the memory is
allocated, split this into a helper function that takes an
already-allocated buffer and two
different front-end wrappers, one for kernel-internal use and one for
userspace use.

> @@ -3738,7 +3739,10 @@ int security_read_policy(struct selinux_state *state,
>
>         *len = security_policydb_len(state);
>
> -       *data = vmalloc_user(*len);
> +       if (alloc_kernel_memory)
> +               *data = kzalloc(*len, GFP_KERNEL);

You need vmalloc() since policy image size may exceed kmalloc max (or
at least that used to be the case).