Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200613024130.3356-1-nramas@linux.microsoft.com> <20200613024130.3356-5-nramas@linux.microsoft.com>
In-Reply-To: <20200613024130.3356-5-nramas@linux.microsoft.com>
From:   Stephen Smalley <stephen.smalley.work@gmail.com>
Date:   Mon, 15 Jun 2020 07:57:40 -0400
Message-ID: <CAEjxPJ49UaZc9pc-+VN8Cx8rcdrjD6NMoLOO_zqENezobmfwVA@mail.gmail.com>
Subject: Re: [PATCH 4/5] LSM: Define SELinux function to measure security state
To:     Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
Cc:     Mimi Zohar <zohar@linux.ibm.com>,
        Stephen Smalley <stephen.smalley@gmail.com>,
        Casey Schaufler <casey@schaufler-ca.com>,
        James Morris <jmorris@namei.org>,
        linux-integrity@vger.kernel.org,
        LSM List <linux-security-module@vger.kernel.org>,
        linux-kernel <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Jun 12, 2020 at 10:42 PM Lakshmi Ramasubramanian
<nramas@linux.microsoft.com> wrote:
>
> SELinux needs to implement the interface function, security_state(), for
> the LSM to gather SELinux data for measuring. Define the security_state()
> function in SELinux.
>
> The security modules should be able to notify the LSM when there is
> a change in the module's data. Define a function namely
> security_state_change() in the LSM that the security modules
> can call to provide the updated data for measurement.
>
> Call security_state_change() function from SELinux to report data
> when SELinux's state is updated.
>
> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
> ---
> diff --git a/security/security.c b/security/security.c
> index a6e2d1cd95af..e7175db5a093 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -238,6 +238,11 @@ static void __init initialize_lsm(struct lsm_info *lsm)
>         }
>  }
>
> +void security_state_change(char *lsm_name, void *state, int state_len)
> +{
> +       ima_lsm_state(lsm_name, state, state_len);
> +}
> +

What's the benefit of this trivial function instead of just calling
ima_lsm_state() directly?

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 7e954b555be6..bbc908a1fcd1 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -7225,6 +7225,47 @@ static __init int selinux_init(void)
>         return 0;
>  }
>
> +static int selinux_security_state(char **lsm_name, void **state,
> +                                 int *state_len)
> +{
> +       int rc = 0;
> +       char *new_state;
> +       static char *security_state_string = "enabled=%d;enforcing=%d";
> +
> +       *lsm_name = kstrdup("selinux", GFP_KERNEL);
> +       if (!*lsm_name)
> +               return -ENOMEM;
> +
> +       new_state = kzalloc(strlen(security_state_string) + 1, GFP_KERNEL);
> +       if (!new_state) {
> +               kfree(*lsm_name);
> +               *lsm_name = NULL;
> +               rc = -ENOMEM;
> +               goto out;
> +       }
> +
> +       *state_len = sprintf(new_state, security_state_string,
> +                            !selinux_disabled(&selinux_state),
> +                            enforcing_enabled(&selinux_state));

I think I mentioned this on a previous version of these patches, but I
would recommend including more than just the enabled and enforcing
states in your measurement.  Other low-hanging fruit would be the
other selinux_state booleans (checkreqprot, initialized,
policycap[0..__POLICYDB_CAPABILITY_MAX]).  Going a bit further one
could take a hash of the loaded policy by using security_read_policy()
and then computing a hash using whatever hash ima prefers over the
returned data,len pair.  You likely also need to think about how to
allow future extensibility of the state in a backward-compatible
manner, so that future additions do not immediately break systems
relying on older measurements.