Re: [PATCH] selinux: include a consumer of the new IMA critical data hook
From: Tyler Hicks
Date: Thu Jan 14 2021 - 14:59:35 EST
On 2021-01-14 14:29:09, Paul Moore wrote:
> On Thu, Jan 14, 2021 at 2:15 PM Lakshmi Ramasubramanian
> <nramas@xxxxxxxxxxxxxxxxxxx> wrote:
> >
> > SELinux stores the active policy in memory, so the changes to this data
> > at runtime would have an impact on the security guarantees provided
> > by SELinux. Measuring in-memory SELinux policy through IMA subsystem
> > provides a secure way for the attestation service to remotely validate
> > the policy contents at runtime.
> >
> > Measure the hash of the loaded policy by calling the IMA hook
> > ima_measure_critical_data(). Since the size of the loaded policy
> > can be large (several MB), measure the hash of the policy instead of
> > the entire policy to avoid bloating the IMA log entry.
> >
> > To enable SELinux data measurement, the following steps are required:
> >
> > 1, Add "ima_policy=critical_data" to the kernel command line arguments
> > to enable measuring SELinux data at boot time.
> > For example,
> > BOOT_IMAGE=/boot/vmlinuz-5.10.0-rc1+ root=UUID=fd643309-a5d2-4ed3-b10d-3c579a5fab2f ro nomodeset security=selinux ima_policy=critical_data
> >
> > 2, Add the following rule to /etc/ima/ima-policy
> > measure func=CRITICAL_DATA label=selinux
> >
> > Sample measurement of the hash of SELinux policy:
> >
> > To verify the measured data with the current SELinux policy run
> > the following commands and verify the output hash values match.
> >
> > sha256sum /sys/fs/selinux/policy | cut -d' ' -f 1
> >
> > grep "selinux-policy-hash" /sys/kernel/security/integrity/ima/ascii_runtime_measurements | tail -1 | cut -d' ' -f 6
> >
> > Note that the actual verification of SELinux policy would require loading
> > the expected policy into an identical kernel on a pristine/known-safe
> > system and run the sha256sum /sys/kernel/selinux/policy there to get
> > the expected hash.
> >
> > Signed-off-by: Lakshmi Ramasubramanian <nramas@xxxxxxxxxxxxxxxxxxx>
> > Suggested-by: Stephen Smalley <stephen.smalley.work@xxxxxxxxx>
> > Acked-by: Paul Moore <paul@xxxxxxxxxxxxxx>
> > Reviewed-by: Tyler Hicks <tyhicks@xxxxxxxxxxxxxxxxxxx>
> > ---
> > Documentation/ABI/testing/ima_policy | 3 +-
> > security/selinux/Makefile | 2 +
> > security/selinux/ima.c | 44 +++++++++++++++++++
> > security/selinux/include/ima.h | 24 +++++++++++
> > security/selinux/include/security.h | 3 +-
> > security/selinux/ss/services.c | 64 ++++++++++++++++++++++++----
> > 6 files changed, 129 insertions(+), 11 deletions(-)
> > create mode 100644 security/selinux/ima.c
> > create mode 100644 security/selinux/include/ima.h
>
> I think this has changed enough that keeping the "Acked-by" and
> "Reviewed-by" tags is probably not a good choice. I took a quick look
> and this still looks okay from a SELinux perspective, I'll leave Mimi
> to comment on it from a IMA perspective.
>
> Unless Tyler has reviewed this version prior to your posting, it might
> be a good idea to remove his "Reviewed-by" unless he has a chance to
> look this over again before it is merged.
Thanks for calling this out. I hadn't reviewed it prior to the posting
but I was keeping an eye on the thread.
This new revision still looks good to me and I like the idea of
controlling re-measurements via policy. So,
Reviewed-by: Tyler Hicks <tyhicks@xxxxxxxxxxxxxxxxxxx>
Tyler
>
> > diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
> > index 54fe1c15ed50..8365596cb42b 100644
> > --- a/Documentation/ABI/testing/ima_policy
> > +++ b/Documentation/ABI/testing/ima_policy
> > @@ -52,8 +52,9 @@ Description:
> > template:= name of a defined IMA template type
> > (eg, ima-ng). Only valid when action is "measure".
> > pcr:= decimal value
> > - label:= [data_label]
> > + label:= [selinux]|[data_label]
> > data_label:= a unique string used for grouping and limiting critical data.
> > + For example, "selinux" to measure critical data for SELinux.
> >
> > default policy:
> > # PROC_SUPER_MAGIC
> > diff --git a/security/selinux/Makefile b/security/selinux/Makefile
> > index 4d8e0e8adf0b..776162444882 100644
> > --- a/security/selinux/Makefile
> > +++ b/security/selinux/Makefile
> > @@ -16,6 +16,8 @@ selinux-$(CONFIG_NETLABEL) += netlabel.o
> >
> > selinux-$(CONFIG_SECURITY_INFINIBAND) += ibpkey.o
> >
> > +selinux-$(CONFIG_IMA) += ima.o
> > +
> > ccflags-y := -I$(srctree)/security/selinux -I$(srctree)/security/selinux/include
> >
> > $(addprefix $(obj)/,$(selinux-y)): $(obj)/flask.h
> > diff --git a/security/selinux/ima.c b/security/selinux/ima.c
> > new file mode 100644
> > index 000000000000..03715893ff97
> > --- /dev/null
> > +++ b/security/selinux/ima.c
> > @@ -0,0 +1,44 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (C) 2021 Microsoft Corporation
> > + *
> > + * Author: Lakshmi Ramasubramanian (nramas@xxxxxxxxxxxxxxxxxxx)
> > + *
> > + * Measure critical data structures maintainted by SELinux
> > + * using IMA subsystem.
> > + */
> > +#include <linux/vmalloc.h>
> > +#include <linux/ima.h>
> > +#include "security.h"
> > +#include "ima.h"
> > +
> > +/*
> > + * selinux_ima_measure_state - Measure hash of the SELinux policy
> > + *
> > + * @state: selinux state struct
> > + *
> > + * NOTE: This function must be called with policy_mutex held.
> > + */
> > +void selinux_ima_measure_state(struct selinux_state *state)
> > +{
> > + void *policy = NULL;
> > + size_t policy_len;
> > + int rc = 0;
> > +
> > + /*
> > + * Measure SELinux policy only after initialization is completed.
> > + */
> > + if (!selinux_initialized(state))
> > + return;
> > +
> > + rc = security_read_state_kernel(state, &policy, &policy_len);
> > + if (rc) {
> > + pr_err("SELinux: %s: failed to read policy %d.\n", __func__, rc);
> > + return;
> > + }
> > +
> > + ima_measure_critical_data("selinux", "selinux-policy-hash",
> > + policy, policy_len, true);
> > +
> > + vfree(policy);
> > +}
> > diff --git a/security/selinux/include/ima.h b/security/selinux/include/ima.h
> > new file mode 100644
> > index 000000000000..d69c36611423
> > --- /dev/null
> > +++ b/security/selinux/include/ima.h
> > @@ -0,0 +1,24 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * Copyright (C) 2021 Microsoft Corporation
> > + *
> > + * Author: Lakshmi Ramasubramanian (nramas@xxxxxxxxxxxxxxxxxxx)
> > + *
> > + * Measure critical data structures maintainted by SELinux
> > + * using IMA subsystem.
> > + */
> > +
> > +#ifndef _SELINUX_IMA_H_
> > +#define _SELINUX_IMA_H_
> > +
> > +#include "security.h"
> > +
> > +#ifdef CONFIG_IMA
> > +extern void selinux_ima_measure_state(struct selinux_state *selinux_state);
> > +#else
> > +static inline void selinux_ima_measure_state(struct selinux_state *selinux_state)
> > +{
> > +}
> > +#endif
> > +
> > +#endif /* _SELINUX_IMA_H_ */
> > diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
> > index 3cc8bab31ea8..29cae32d3fc5 100644
> > --- a/security/selinux/include/security.h
> > +++ b/security/selinux/include/security.h
> > @@ -229,7 +229,8 @@ void selinux_policy_cancel(struct selinux_state *state,
> > struct selinux_policy *policy);
> > int security_read_policy(struct selinux_state *state,
> > void **data, size_t *len);
> > -
> > +int security_read_state_kernel(struct selinux_state *state,
> > + void **data, size_t *len);
> > int security_policycap_supported(struct selinux_state *state,
> > unsigned int req_cap);
> >
> > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> > index 9704c8a32303..cc8dbc4ed8db 100644
> > --- a/security/selinux/ss/services.c
> > +++ b/security/selinux/ss/services.c
> > @@ -65,6 +65,7 @@
> > #include "ebitmap.h"
> > #include "audit.h"
> > #include "policycap_names.h"
> > +#include "ima.h"
> >
> > /* Forward declaration. */
> > static int context_struct_to_string(struct policydb *policydb,
> > @@ -2180,6 +2181,7 @@ static void selinux_notify_policy_change(struct selinux_state *state,
> > selinux_status_update_policyload(state, seqno);
> > selinux_netlbl_cache_invalidate();
> > selinux_xfrm_notify_policyload();
> > + selinux_ima_measure_state(state);
> > }
> >
> > void selinux_policy_commit(struct selinux_state *state,
> > @@ -3875,8 +3877,33 @@ int security_netlbl_sid_to_secattr(struct selinux_state *state,
> > }
> > #endif /* CONFIG_NETLABEL */
> >
> > +/**
> > + * __security_read_policy - read the policy.
> > + * @policy: SELinux policy
> > + * @data: binary policy data
> > + * @len: length of data in bytes
> > + *
> > + */
> > +static int __security_read_policy(struct selinux_policy *policy,
> > + void *data, size_t *len)
> > +{
> > + int rc;
> > + struct policy_file fp;
> > +
> > + fp.data = data;
> > + fp.len = *len;
> > +
> > + rc = policydb_write(&policy->policydb, &fp);
> > + if (rc)
> > + return rc;
> > +
> > + *len = (unsigned long)fp.data - (unsigned long)data;
> > + return 0;
> > +}
> > +
> > /**
> > * security_read_policy - read the policy.
> > + * @state: selinux_state
> > * @data: binary policy data
> > * @len: length of data in bytes
> > *
> > @@ -3885,8 +3912,6 @@ int security_read_policy(struct selinux_state *state,
> > void **data, size_t *len)
> > {
> > struct selinux_policy *policy;
> > - int rc;
> > - struct policy_file fp;
> >
> > policy = rcu_dereference_protected(
> > state->policy, lockdep_is_held(&state->policy_mutex));
> > @@ -3898,14 +3923,35 @@ int security_read_policy(struct selinux_state *state,
> > if (!*data)
> > return -ENOMEM;
> >
> > - fp.data = *data;
> > - fp.len = *len;
> > + return __security_read_policy(policy, *data, len);
> > +}
> >
> > - rc = policydb_write(&policy->policydb, &fp);
> > - if (rc)
> > - return rc;
> > +/**
> > + * security_read_state_kernel - read the policy.
> > + * @state: selinux_state
> > + * @data: binary policy data
> > + * @len: length of data in bytes
> > + *
> > + * Allocates kernel memory for reading SELinux policy.
> > + * This function is for internal use only and should not
> > + * be used for returning data to user space.
> > + *
> > + * This function must be called with policy_mutex held.
> > + */
> > +int security_read_state_kernel(struct selinux_state *state,
> > + void **data, size_t *len)
> > +{
> > + struct selinux_policy *policy;
> >
> > - *len = (unsigned long)fp.data - (unsigned long)*data;
> > - return 0;
> > + policy = rcu_dereference_protected(
> > + state->policy, lockdep_is_held(&state->policy_mutex));
> > + if (!policy)
> > + return -EINVAL;
> > +
> > + *len = policy->policydb.len;
> > + *data = vmalloc(*len);
> > + if (!*data)
> > + return -ENOMEM;
> >
> > + return __security_read_policy(policy, *data, len);
> > }
> > --
> > 2.30.0
> >
>
>
> --
> paul moore
> www.paul-moore.com
>