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

From: Stephen Smalley <stephen.smalley.work@gmail.com>
To: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
Cc: Mimi Zohar <zohar@linux.ibm.com>,
	Casey Schaufler <casey@schaufler-ca.com>,
	James Morris <jmorris@namei.org>,
	linux-integrity@vger.kernel.org,
	SElinux list <selinux@vger.kernel.org>,
	LSM List <linux-security-module@vger.kernel.org>,
	linux-kernel <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 4/5] LSM: Define SELinux function to measure security state
Date: Thu, 16 Jul 2020 14:54:06 -0400	[thread overview]
Message-ID: <CAEjxPJ43eXK0xgrE=gDxZVg2SDTz4bkd7N4otjk-cvxf3fKL-g@mail.gmail.com> (raw)
In-Reply-To: <20200716174351.20128-5-nramas@linux.microsoft.com>

On Thu, Jul 16, 2020 at 1:44 PM Lakshmi Ramasubramanian
<nramas@linux.microsoft.com> 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_measure_data(), 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;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;
>
> The data for selinux-policy-hash in the above measurement is
> the SHA256 hash of the SELinux policy.

Can you show an example of how to verify that the above measurement
matches a given state and policy, e.g. the sha256sum commands and
inputs to reproduce the same from an expected state and policy?

>
> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
> Suggested-by: Stephen Smalley <stephen.smalley.work@gmail.com>
> ---

> diff --git a/security/selinux/measure.c b/security/selinux/measure.c
> new file mode 100644
> index 000000000000..27cbb309e926
> --- /dev/null
> +++ b/security/selinux/measure.c
> @@ -0,0 +1,158 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Measure SELinux state using IMA subsystem.
> + */
> +#include <linux/ima.h>
> +#include "security.h"
> +
> +/* Pre-allocated buffer used for measuring state */
> +static char *selinux_state_string;
> +static size_t selinux_state_string_len;
> +static char *selinux_state_string_fmt =
> +       "%s=%d;%s=%d;%s=%d;%s=%d;%s=%d;%s=%d;%s=%d;%s=%d;%s=%d;%s=%d;";
> +
> +void __init selinux_init_measurement(void)
> +{
> +       selinux_state_string_len =
> +       snprintf(NULL, 0, selinux_state_string_fmt,
> +       "enabled", 0,
> +       "enforcing", 0,
> +       "checkreqprot", 0,
> +       selinux_policycap_names[POLICYDB_CAPABILITY_NETPEER], 0,
> +       selinux_policycap_names[POLICYDB_CAPABILITY_OPENPERM], 0,
> +       selinux_policycap_names[POLICYDB_CAPABILITY_EXTSOCKCLASS], 0,
> +       selinux_policycap_names[POLICYDB_CAPABILITY_ALWAYSNETWORK], 0,
> +       selinux_policycap_names[POLICYDB_CAPABILITY_CGROUPSECLABEL], 0,
> +       selinux_policycap_names[POLICYDB_CAPABILITY_NNP_NOSUID_TRANSITION], 0,
> +       selinux_policycap_names[POLICYDB_CAPABILITY_GENFS_SECLABEL_SYMLINKS],
> +       0);

I was thinking you'd dynamically construct the format string with a
for loop from 0 to POLICYDB_CAPABILITY_MAX
and likewise for the values so that we wouldn't have to patch this
code every time we add a new one.

> +
> +       if (selinux_state_string_len < 0)
> +               return;

How can this happen legitimately (i.e. as a result of something other
than a kernel bug)?

> +
> +       ++selinux_state_string_len;
> +
> +       selinux_state_string = kzalloc(selinux_state_string_len, GFP_KERNEL);
> +       if (!selinux_state_string)
> +               selinux_state_string_len = 0;
> +}

Not sure about this error handling approach (silent, proceeding as if
the length was zero and then later failing with ENOMEM on every
attempt?). I'd be more inclined to panic/BUG here but I know Linus
doesn't like that.

> +       if (ret)
> +               pr_err("%s: error %d\n", __func__, ret);

This doesn't seem terribly useful as an error message; I'd be inclined
to drop it.