[PATCH] selinux: measure state and policy capabilities
Lakshmi Ramasubramanian
nramas at linux.microsoft.com
Sun Jan 24 17:04:40 UTC 2021
On 1/22/21 1:21 PM, Paul Moore wrote:
Hi Paul,
Thanks for reviewing the changes.
...
>>
>> Signed-off-by: Lakshmi Ramasubramanian <nramas at linux.microsoft.com>
>> Suggested-by: Stephen Smalley <stephen.smalley.work at gmail.com>
>> ---
>> This patch is based on
>> commit e58bb688f2e4 "Merge branch 'measure-critical-data' into next-integrity"
>> in "next-integrity-testing" branch
>>
>> security/selinux/hooks.c | 5 +++
>> security/selinux/ima.c | 68 ++++++++++++++++++++++++++++++++++++
>> security/selinux/selinuxfs.c | 10 ++++++
>> 3 files changed, 83 insertions(+)
>>
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index 644b17ec9e63..879a0d90615d 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -103,6 +103,7 @@
>> #include "netlabel.h"
>> #include "audit.h"
>> #include "avc_ss.h"
>> +#include "ima.h"
>>
>> struct selinux_state selinux_state;
>>
>> @@ -7407,6 +7408,10 @@ int selinux_disable(struct selinux_state *state)
>>
>> selinux_mark_disabled(state);
>>
>> + mutex_lock(&state->policy_mutex);
>> + selinux_ima_measure_state(state);
>> + mutex_unlock(&state->policy_mutex);
>
> I'm not sure if this affects your decision to include this action in
> the measurements, but this function is hopefully going away in the not
> too distant future as we do away with support for disabling SELinux at
> runtime.
>
> FWIW, I'm not sure it's overly useful anyway; you only get here if you
> never had any SELinux policy/state configured and you decide to
> disable SELinux instead of loading a policy. However, I've got no
> objection to this code.
If support for disabling SELinux at runtime will be removed, then I
don't see a reason to trigger a measurement here. I'll remove this
measurement.
>
>> diff --git a/security/selinux/ima.c b/security/selinux/ima.c
>> index 03715893ff97..e65d462d2d30 100644
>> --- a/security/selinux/ima.c
>> +++ b/security/selinux/ima.c
>> @@ -12,6 +12,60 @@
>> #include "security.h"
>> #include "ima.h"
>>
>> +/*
>> + * read_selinux_state - Read selinux configuration settings
>> + *
>> + * @state_str: Return the configuration settings.
>> + * @state_str_len: Size of the configuration settings string
>> + * @state: selinux_state
>> + *
>> + * Return 0 on success, error code on failure
>> + */
>
> Yes, naming is hard, but let's try to be a bit more consistent within
> a single file. The existing function is prefixed with "selinux_ima_"
> perhaps we can do something similar here?
> "selinux_ima_collect_state()" or something similar perhaps?
Sure - will rename the function to "selinux_ima_collect_state()"
>
> Perhaps instead of returning zero on success you could return the
> length of the generated string? It's not a big deal, but it saves an
> argument for whatever that is worth these days. I also might pass the
> state as the first argument and the generated string pointer as the
> second argument, but that is pretty nit-picky.
Sure - will make this change.
>
>> +static int read_selinux_state(char **state_str, int *state_str_len,
>> + struct selinux_state *state)
>> +{
>> + char *buf;
>> + int i, buf_len, curr;
>> + bool initialized = selinux_initialized(state);
>> + bool enabled = !selinux_disabled(state);
>> + bool enforcing = enforcing_enabled(state);
>> + bool checkreqprot = checkreqprot_get(state);
>> +
>> + buf_len = snprintf(NULL, 0, "%s=%d;%s=%d;%s=%d;%s=%d;",
>> + "initialized", initialized,
>> + "enabled", enabled,
>> + "enforcing", enforcing,
>> + "checkreqprot", checkreqprot);
>> +
>> + for (i = 0; i < __POLICYDB_CAPABILITY_MAX; i++) {
>> + buf_len += snprintf(NULL, 0, "%s=%d;",
>> + selinux_policycap_names[i],
>> + state->policycap[i]);
>> + }
>> + ++buf_len;
>
> With all of the variables you are measuring being booleans, it seems
> like using snprintf() is a bit overkill, no? What about a series of
> strlen() calls with additional constants for the booleans and extra
> bits? For example:
>
> buf_len = 1; // '\0';
> buf_len += strlen("foo") + 3; // "foo=0;"
> buf_len += strlen("bar") + 3; // "bar=0;"
>
> Not that it matters a lot here, but the above must be more efficient
> than calling snprintf().
You are right - using strlen/strcat would be more efficient here. But I
feel it is safer to use snprintf() rather than computing the length of
each measured entity and concatenating it to the destination buffer.
I'll try strlen/strcat approach.
>
>> + buf = kzalloc(buf_len, GFP_KERNEL);
>> + if (!buf)
>> + return -ENOMEM;
>> +
>> + curr = scnprintf(buf, buf_len, "%s=%d;%s=%d;%s=%d;%s=%d;",
>> + "initialized", initialized,
>> + "enabled", enabled,
>> + "enforcing", enforcing,
>> + "checkreqprot", checkreqprot);
>> +
>> + for (i = 0; i < __POLICYDB_CAPABILITY_MAX; i++) {
>> + curr += scnprintf((buf + curr), (buf_len - curr), "%s=%d;",
>> + selinux_policycap_names[i],
>> + state->policycap[i]);
>> + }
>
> Similarly, you could probably replace all of this with
> strcat()/strlcat() calls since you don't have to render an integer
> into a string.
Sure - I'll give this a try.
>
>> + *state_str = buf;
>> + *state_str_len = curr;
>> +
>> + return 0;
>> +}
>> +
>> /*
>> * selinux_ima_measure_state - Measure hash of the SELinux policy
>> *
>> @@ -21,10 +75,24 @@
>> */
>> void selinux_ima_measure_state(struct selinux_state *state)
>> {
>> + char *state_str = NULL;
>> + int state_str_len;
>> void *policy = NULL;
>> size_t policy_len;
>> int rc = 0;
>>
>> + rc = read_selinux_state(&state_str, &state_str_len, state);
>> + if (rc) {
>> + pr_err("SELinux: %s: failed to read state %d.\n",
>> + __func__, rc);
>> + return;
>> + }
>> +
>> + ima_measure_critical_data("selinux", "selinux-state",
>> + state_str, state_str_len, false);
>> +
>> + kfree(state_str);
>> +
>> /*
>> * Measure SELinux policy only after initialization is completed.
>> */
>> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
>> index 4bde570d56a2..8b561e1c2caa 100644
>> --- a/security/selinux/selinuxfs.c
>> +++ b/security/selinux/selinuxfs.c
>> @@ -41,6 +41,7 @@
>> #include "security.h"
>> #include "objsec.h"
>> #include "conditional.h"
>> +#include "ima.h"
>>
>> enum sel_inos {
>> SEL_ROOT_INO = 2,
>> @@ -182,6 +183,10 @@ static ssize_t sel_write_enforce(struct file *file, const char __user *buf,
>> selinux_status_update_setenforce(state, new_value);
>> if (!new_value)
>> call_blocking_lsm_notifier(LSM_POLICY_CHANGE, NULL);
>> +
>> + mutex_lock(&state->policy_mutex);
>> + selinux_ima_measure_state(state);
>> + mutex_unlock(&state->policy_mutex);
>> }
>> length = count;
>> out:
>> @@ -762,6 +767,11 @@ static ssize_t sel_write_checkreqprot(struct file *file, const char __user *buf,
>>
>> checkreqprot_set(fsi->state, (new_value ? 1 : 0));
>> length = count;
>> +
>> + mutex_lock(&fsi->state->policy_mutex);
>> + selinux_ima_measure_state(fsi->state);
>> + mutex_unlock(&fsi->state->policy_mutex);
>> +
>
> The lock-measure-unlock pattern appears enough that I wonder if we
> should move the lock/unlock into selinux_ima_measure_state() and
> create a new function, selinux_ima_measure_state_unlocked(), to cover
> the existing case in selinux_notify_policy_change(). It would have
> the advantage of not requiring a pointless lock/unlock in the case
> where CONFIG_IMA=n.
>
Agreed.
thanks,
-lakshmi
More information about the Linux-security-module-archive
mailing list