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

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

Hi Paul,

Thanks for reviewing the changes.

...

quoted

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

quoted

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.

quoted

+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.

quoted

+       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.

quoted

+       *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