On Mon, Sep 7, 2020 at 5:39 PM Lakshmi Ramasubramanian
<nramas@xxxxxxxxxxxxxxxxxxx> wrote:
>
> Critical data structures of security modules are currently not measured.
> Therefore an attestation service, for instance, would not be able to
> attest whether the security modules are always operating with the policies
> and configuration that the system administrator had setup. The policies
> and configuration for the security modules could be tampered with by
> rogue user mode agents or modified through some inadvertent actions on
> the system. Measuring such critical data would enable an attestation
> service to reliably assess the security configuration of the system.
>
> SELinux configuration and policy are some of the critical data for this
> security module that needs to be measured. This measurement can be used
> by an attestation service, for instance, to verify if the configuration
> and policies have been setup correctly and that they haven't been tampered
> with at runtime.
>
> Measure SELinux configuration, policy capabilities settings, and the hash
> of the loaded policy by calling the IMA hook ima_measure_critical_data().
> Since the size of the loaded policy can be quite large, hash of the policy
> is measured instead of the entire policy to avoid bloating the IMA log.
>
> Enable early boot measurement for SELinux in IMA since SELinux
> initializes its state and policy before custom IMA policy is loaded.
>
> Sample measurement of SELinux state and hash of the policy:
>
> 10 e32e...5ac3 ima-buf sha256:86e8...4594 selinux-state-1595389364:287899386 696e697469616c697a65643d313b656e61626c65643d313b656e666f7263696e673d303b636865636b72657170726f743d313b6e6574776f726b5f706565725f636f6e74726f6c733d313b6f70656e5f7065726d733d313b657874656e6465645f736f636b65745f636c6173733d313b616c776179735f636865636b5f6e6574776f726b3d303b6367726f75705f7365636c6162656c3d313b6e6e705f6e6f737569645f7472616e736974696f6e3d313b67656e66735f7365636c6162656c5f73796d6c696e6b733d303
> 10 9e81...0857 ima-buf sha256:4941...68fc selinux-policy-hash-1597335667:462051628 8d1d...1834
>
> To verify the measurement check the following:
>
> Execute the following command to extract the measured data
> from the IMA log for SELinux configuration (selinux-state).
>
> grep -m 1 "selinux-state" /sys/kernel/security/integrity/ima/ascii_runtime_measurements | cut -d' ' -f 6 | xxd -r -p
>
> The output should be the list of key-value pairs. For example,
> initialized=1;enabled=1;enforcing=0;checkreqprot=1;network_peer_controls=1;open_perms=1;extended_socket_class=1;always_check_network=0;cgroup_seclabel=1;nnp_nosuid_transition=1;genfs_seclabel_symlinks=0;
>
> To verify the measured data with the current SELinux state:
>
> => enabled should be set to 1 if /sys/fs/selinux folder exists,
> 0 otherwise
>
> For other entries, compare the integer value in the files
> => /sys/fs/selinux/enforce
> => /sys/fs/selinux/checkreqprot
> And, each of the policy capabilities files under
> => /sys/fs/selinux/policy_capabilities
>
> For selinux-policy-hash, the hash of SELinux policy is included
> in the IMA log entry.
>
> To verify the measured data with the current SELinux policy run
> the following commands and verify the output hash values match.
>
> sha256sum /sys/fs/selinux/policy | cut -d' ' -f 1
>
> grep -m 1 "selinux-policy-hash" /sys/kernel/security/integrity/ima/ascii_runtime_measurements | cut -d' ' -f 6
>
> This patch is based on commit 66ccd2560aff ("selinux: simplify away security_policydb_len()")
> in "next" branch in https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git
>
> This patch is dependent on the following patch series and must be
> applied in the given order:
> https://patchwork.kernel.org/patch/11709527/
> https://patchwork.kernel.org/patch/11730193/
> https://patchwork.kernel.org/patch/11730757/
>
> Signed-off-by: Lakshmi Ramasubramanian <nramas@xxxxxxxxxxxxxxxxxxx>
> Suggested-by: Stephen Smalley <stephen.smalley.work@xxxxxxxxx>
> ---
>
> diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> index 953314d145bb..9bf0f65d720b 100644
> --- a/security/integrity/ima/Kconfig
> +++ b/security/integrity/ima/Kconfig
> @@ -324,8 +324,7 @@ config IMA_MEASURE_ASYMMETRIC_KEYS
>
> config IMA_QUEUE_EARLY_BOOT_DATA
> bool
> - depends on IMA_MEASURE_ASYMMETRIC_KEYS
> - depends on SYSTEM_TRUSTED_KEYRING
> + depends on (IMA_MEASURE_ASYMMETRIC_KEYS && SYSTEM_TRUSTED_KEYRING) || SECURITY_SELINUX
> default y
I don't see why this is necessary or desirable. We should avoid
leaking dependencies on a single security module into other
subsystems.
It might not yet fully support other security modules but we shouldn't
preclude adding that in the future.
Up to the IMA maintainer but I would recommend dropping this part.
> diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
> index cbdd3c7aff8b..c971ec09d855 100644
> --- a/security/selinux/include/security.h
> +++ b/security/selinux/include/security.h
> @@ -209,6 +209,11 @@ static inline bool selinux_policycap_genfs_seclabel_symlinks(void)
> return state->policycap[POLICYDB_CAPABILITY_GENFS_SECLABEL_SYMLINKS];
> }
>
> +static inline bool selinux_checkreqprot(const struct selinux_state *state)
> +{
> + return READ_ONCE(state->checkreqprot);
> +}
> +
Since you are introducing this helper, you should also convert
existing reads of selinux_state.checkreqprot and
fsi->state->checkreqprot to use it, and writes to use WRITE_ONCE()
just like for enforcing and disabled. The introduction of the helper
and conversion to use it could be done as a separate patch before this
one.
> diff --git a/security/selinux/measure.c b/security/selinux/measure.c
> new file mode 100644
> index 000000000000..caf9107937d9
> --- /dev/null
> +++ b/security/selinux/measure.c
<snip>
> +static int read_selinux_state(char **state_str, int *state_str_len,
> + struct selinux_state *state)
> +{
> + char *buf, *str_fmt = "%s=%d;";
> + int i, buf_len, curr;
<snip>
> + for (i = 0; i < __POLICYDB_CAPABILITY_MAX; i++) {
> + buf_len += snprintf(NULL, 0, str_fmt,
> + selinux_policycap_names[i],
> + state->policycap[i]);
> + }
This will need to be converted to use
security_policycap_supported(state, i) rather than state->policycap[i]
since the latter is going to be removed by Ondrej's patches I think.
> + for (i = 0; i < __POLICYDB_CAPABILITY_MAX; i++) {
> + curr += snprintf((buf + curr), (buf_len - curr), str_fmt,
> + selinux_policycap_names[i],
> + state->policycap[i]);
Ditto.
> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> index 45e9efa9bf5b..bb460954de03 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -176,6 +176,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);
> + selinux_measure_state(state, false);
I think we should move this to after the avc_ss_reset call so that we
don't introduce a potentially long delay between setting the enforcing
mode and flushing the AVC at least. Potentially it could be moved to
the very end after selnl_notify_setenforce() too so that it doesn't
delay notifying userspace, but that's less crucial.
> if (new_value)
> avc_ss_reset(state->avc, 0);
> selnl_notify_setenforce(new_value);
> @@ -761,6 +762,8 @@ static ssize_t sel_write_checkreqprot(struct file *file, const char __user *buf,
>
> fsi->state->checkreqprot = new_value ? 1 : 0;
This should switch to using WRITE_ONCE() or a helper that uses it.
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index 8dc111fbe23a..04a9c3d8c19b 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -3874,6 +3875,30 @@ int security_netlbl_sid_to_secattr(struct selinux_state *state,
> }
> #endif /* CONFIG_NETLABEL */
>
> +/**
> + * security_read_selinux_policy - read the policy.
> + * @policy: SELinux policy
> + * @data: binary policy data
> + * @len: length of data in bytes
> + *
> + */
> +static int security_read_selinux_policy(struct selinux_policy *policy,
> + void **data, size_t *len)
> +{
Since this only uses *data, why not just pass that here?
> + int rc;
> + struct policy_file fp;
> +
> + fp.data = *data;
> + fp.len = *len;
> +
> + rc = policydb_write(&policy->policydb, &fp);
> + if (rc)
> + return rc;
> +
> + *len = (unsigned long)fp.data - (unsigned long)*data;
> + return 0;
> +}
> +
> /**
> * security_read_policy - read the policy.
> * @data: binary policy data
