Re: [PATCH] selinux: measure state and policy capabilities

[Lakshmi Ramasubramanian]

On 1/22/21 1:21 PM, Paul Moore wrote:

Hi Paul,

Thanks for reviewing the changes.

...

> > Signed-off-by: Lakshmi Ramasubramanian <nramas@xxxxxxxxxxxxxxxxxxx>
> > Suggested-by: Stephen Smalley <stephen.smalley.work@xxxxxxxxx>
> > ---
> > 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