LinuxLists.cc - [PATCH 0/5] LSM: Measure security module state

[permalink] [raw]

Subject: [PATCH 5/5] LSM: Define workqueue for measuring security module state

The data maintained by the security modules could be tampered with by
malware. The LSM needs to periodically query the state of
the security modules and measure the data when the state is changed.

Define a workqueue for handling this periodic query and measurement.

Signed-off-by: Lakshmi Ramasubramanian <[email protected]>
---
security/security.c | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)

diff --git a/security/security.c b/security/security.c
index e7175db5a093..3dad6766cb9d 100644
--- a/security/security.c
+++ b/security/security.c
@@ -89,6 +89,11 @@ static __initdata struct lsm_info *exclusive;
static struct lsm_info *security_state_lsms;
static int security_state_lsms_count;

+static long security_state_timeout = 300000; /* 5 Minutes */
+static void security_state_handler(struct work_struct *work);
+static DECLARE_DELAYED_WORK(security_state_delayed_work,
+ security_state_handler);
+
static __initdata bool debug;
#define init_debug(...) \
do { \
@@ -294,6 +299,26 @@ static void __init initialize_security_state_lsms(void)
security_state_lsms_count = count;
}

+static void initialize_security_state_monitor(void)
+{
+ if (security_state_lsms_count == 0)
+ return;
+
+ schedule_delayed_work(&security_state_delayed_work,
+ msecs_to_jiffies(security_state_timeout));
+}
+
+static void security_state_handler(struct work_struct *work)
+{
+ int inx;
+
+ for (inx = 0; inx < security_state_lsms_count; inx++)
+ measure_security_state(&(security_state_lsms[inx]));
+
+ schedule_delayed_work(&security_state_delayed_work,
+ msecs_to_jiffies(security_state_timeout));
+}
+
/* Populate ordered LSMs list from comma-separated LSM name list. */
static void __init ordered_lsm_parse(const char *order, const char *origin)
{
@@ -417,6 +442,7 @@ static void __init ordered_lsm_init(void)
}

initialize_security_state_lsms();
+ initialize_security_state_monitor();

kfree(ordered_lsms);
}
--
2.27.0

2020-06-13 02:45:05

[permalink] [raw]

Subject: [PATCH 2/5] IMA: Define an IMA hook to measure LSM data

LSM requires an IMA hook to be defined by the IMA subsystem to measure
the data gathered from the security modules.

Define a new IMA hook, namely ima_lsm_state(), that the LSM will call
to measure the data gathered from the security modules.

Sample IMA log entry for LSM measurement:

10 47eed9... ima-buf sha256:402f6b... lsm-state:selinux 656e61626c65643d313b656e666f7263696e673d30

Signed-off-by: Lakshmi Ramasubramanian <[email protected]>
---
include/linux/ima.h | 4 ++++
security/integrity/ima/ima_main.c | 30 ++++++++++++++++++++++++++++++
2 files changed, 34 insertions(+)

diff --git a/include/linux/ima.h b/include/linux/ima.h
index 9164e1534ec9..56681a648b3d 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -26,6 +26,7 @@ extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
extern void ima_post_path_mknod(struct dentry *dentry);
extern int ima_file_hash(struct file *file, char *buf, size_t buf_size);
extern void ima_kexec_cmdline(const void *buf, int size);
+extern void ima_lsm_state(const char *lsm_name, const void *buf, int size);

#ifdef CONFIG_IMA_KEXEC
extern void ima_add_kexec_buffer(struct kimage *image);
@@ -104,6 +105,9 @@ static inline int ima_file_hash(struct file *file, char *buf, size_t buf_size)
}

static inline void ima_kexec_cmdline(const void *buf, int size) {}
+
+static inline void ima_lsm_state(const char *lsm_name,
+ const void *buf, int size) {}
#endif /* CONFIG_IMA */

#ifndef CONFIG_IMA_KEXEC
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index c1583d98c5e5..34be962054fb 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -827,6 +827,36 @@ void ima_kexec_cmdline(const void *buf, int size)
KEXEC_CMDLINE, 0, NULL);
}

+/**
+ * ima_lsm_state - measure LSM specific state
+ * @lsm_name: Name of the LSM
+ * @buf: pointer to buffer containing LSM specific state
+ * @size: Number of bytes in buf
+ *
+ * Buffers can only be measured, not appraised.
+ */
+void ima_lsm_state(const char *lsm_name, const void *buf, int size)
+{
+ const char *eventname = "lsm-state:";
+ char *lsmstatestring;
+ int lsmstatelen;
+
+ if (!lsm_name || !buf || !size)
+ return;
+
+ lsmstatelen = strlen(eventname) + strlen(lsm_name) + 1;
+ lsmstatestring = kzalloc(lsmstatelen, GFP_KERNEL);
+ if (!lsmstatestring)
+ return;
+
+ strcpy(lsmstatestring, eventname);
+ strcat(lsmstatestring, lsm_name);
+
+ process_buffer_measurement(buf, size, lsmstatestring,
+ LSM_STATE, 0, NULL);
+ kfree(lsmstatestring);
+}
+
static int __init init_ima(void)
{
int error;
--
2.27.0

2020-06-13 02:45:05

[permalink] [raw]

Subject: [PATCH 4/5] LSM: Define SELinux function to measure security state

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 <[email protected]>
---
include/linux/lsm_hooks.h | 3 ++
security/security.c | 5 ++++
security/selinux/hooks.c | 43 +++++++++++++++++++++++++++++
security/selinux/include/security.h | 2 ++
security/selinux/selinuxfs.c | 1 +
5 files changed, 54 insertions(+)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index da248c3fd4ac..a63de046139e 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1572,6 +1572,9 @@ struct lsm_info {
int *state_len); /*Optional */
};

+/* Called by LSMs to notify security state change */
+extern void security_state_change(char *lsm_name, void *state, int state_len);
+
extern struct lsm_info __start_lsm_info[], __end_lsm_info[];
extern struct lsm_info __start_early_lsm_info[], __end_early_lsm_info[];

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);
+}
+
static int measure_security_state(struct lsm_info *lsm)
{
char *lsm_name = NULL;
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));
+ *state = new_state;
+
+out:
+ return rc;
+}
+
+void notify_security_state_change(void)
+{
+ char *lsm_name = NULL;
+ void *state = NULL;
+ int state_len = 0;
+
+ if (!selinux_security_state(&lsm_name, &state, &state_len)) {
+ security_state_change(lsm_name, state, state_len);
+ kfree(state);
+ kfree(lsm_name);
+ }
+}
+
static void delayed_superblock_init(struct super_block *sb, void *unused)
{
selinux_set_mnt_opts(sb, NULL, 0, NULL);
@@ -7247,6 +7288,7 @@ DEFINE_LSM(selinux) = {
.enabled = &selinux_enabled_boot,
.blobs = &selinux_blob_sizes,
.init = selinux_init,
+ .security_state = selinux_security_state,
};

#if defined(CONFIG_NETFILTER)
@@ -7357,6 +7399,7 @@ int selinux_disable(struct selinux_state *state)
}

selinux_mark_disabled(state);
+ notify_security_state_change();

pr_info("SELinux: Disabled at runtime.\n");

diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
index b0e02cfe3ce1..83c6ada45c7c 100644
--- a/security/selinux/include/security.h
+++ b/security/selinux/include/security.h
@@ -232,6 +232,8 @@ size_t security_policydb_len(struct selinux_state *state);
int security_policycap_supported(struct selinux_state *state,
unsigned int req_cap);

+void notify_security_state_change(void);
+
#define SEL_VEC_MAX 32
struct av_decision {
u32 allowed;
diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index 4781314c2510..8a5ba32a7775 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -173,6 +173,7 @@ static ssize_t sel_write_enforce(struct file *file, const char __user *buf,
from_kuid(&init_user_ns, audit_get_loginuid(current)),
audit_get_sessionid(current));
enforcing_set(state, new_value);
+ notify_security_state_change();
if (new_value)
avc_ss_reset(state->avc, 0);
selnl_notify_setenforce(new_value);
--
2.27.0

2020-06-13 02:45:05

[permalink] [raw]

Subject: [PATCH 1/5] IMA: Add LSM_STATE func to measure LSM data

Data provided by security modules need to be measured. A new IMA policy
is required for handling this measurement.

Define a new IMA policy func namely LSM_STATE to measure data provided
by security modules. Update ima_match_rules() to check for LSM_STATE
and ima_parse_rule() to handle LSM_STATE.

Signed-off-by: Lakshmi Ramasubramanian <[email protected]>
---
Documentation/ABI/testing/ima_policy | 6 +++++-
security/integrity/ima/ima.h | 1 +
security/integrity/ima/ima_api.c | 2 +-
security/integrity/ima/ima_policy.c | 28 +++++++++++++++++++++++-----
4 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index cd572912c593..355bc3eade33 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -29,7 +29,7 @@ Description:
base: func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK]
[FIRMWARE_CHECK]
[KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
- [KEXEC_CMDLINE] [KEY_CHECK]
+ [KEXEC_CMDLINE] [KEY_CHECK] [LSM_STATE]
mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND]
[[^]MAY_EXEC]
fsmagic:= hex value
@@ -125,3 +125,7 @@ Description:
keys added to .builtin_trusted_keys or .ima keyring:

measure func=KEY_CHECK keyrings=.builtin_trusted_keys|.ima
+
+ Example of measure rule using LSM_STATE to measure LSM data:
+
+ measure func=LSM_STATE
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index df93ac258e01..58c62269028a 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -200,6 +200,7 @@ static inline unsigned int ima_hash_key(u8 *digest)
hook(POLICY_CHECK) \
hook(KEXEC_CMDLINE) \
hook(KEY_CHECK) \
+ hook(LSM_STATE) \
hook(MAX_CHECK)
#define __ima_hook_enumify(ENUM) ENUM,

diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index bf22de8b7ce0..0cebd2404dcf 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -176,7 +176,7 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
* subj=, obj=, type=, func=, mask=, fsmagic=
* subj,obj, and type: are LSM specific.
* func: FILE_CHECK | BPRM_CHECK | CREDS_CHECK | MMAP_CHECK | MODULE_CHECK
- * | KEXEC_CMDLINE | KEY_CHECK
+ * | KEXEC_CMDLINE | KEY_CHECK | LSM_STATE
* mask: contains the permission mask
* fsmagic: hex value
*
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index e493063a3c34..1a6ee09e6993 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -417,15 +417,31 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
const char *keyring)
{
int i;
+ int funcmatch = 0;

- if ((func == KEXEC_CMDLINE) || (func == KEY_CHECK)) {
+ switch (func) {
+ case KEXEC_CMDLINE:
+ case KEY_CHECK:
+ case LSM_STATE:
if ((rule->flags & IMA_FUNC) && (rule->func == func)) {
if (func == KEY_CHECK)
- return ima_match_keyring(rule, keyring, cred);
- return true;
- }
- return false;
+ funcmatch = ima_match_keyring(rule, keyring,
+ cred) ? 1 : -1;
+ else
+ funcmatch = 1;
+ } else
+ funcmatch = -1;
+
+ break;
+
+ default:
+ funcmatch = 0;
+ break;
}
+
+ if (funcmatch)
+ return (funcmatch == 1) ? true : false;
+
if ((rule->flags & IMA_FUNC) &&
(rule->func != func && func != POST_SETATTR))
return false;
@@ -1068,6 +1084,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
entry->func = KEXEC_CMDLINE;
else if (strcmp(args[0].from, "KEY_CHECK") == 0)
entry->func = KEY_CHECK;
+ else if (strcmp(args[0].from, "LSM_STATE") == 0)
+ entry->func = LSM_STATE;
else
result = -EINVAL;
if (!result)
--
2.27.0

2020-06-15 12:02:57

[permalink] [raw]

Subject: Re: [PATCH 4/5] LSM: Define SELinux function to measure security state

On Fri, Jun 12, 2020 at 10:42 PM Lakshmi Ramasubramanian
<[email protected]> 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 <[email protected]>
> ---
> 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.

2020-06-15 12:18:05

[permalink] [raw]

Subject: Re: [PATCH 4/5] LSM: Define SELinux function to measure security state

On Mon, Jun 15, 2020 at 7:57 AM Stephen Smalley
<[email protected]> wrote:
> 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()

On second thought, you probably a variant of security_read_policy()
since it would be a kernel-internal allocation and thus shouldn't use
vmalloc_user().

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

2020-06-15 13:36:25

[permalink] [raw]

Subject: Re: [PATCH 5/5] LSM: Define workqueue for measuring security module state

On Fri, Jun 12, 2020 at 10:42 PM Lakshmi Ramasubramanian
<[email protected]> wrote:
>
> The data maintained by the security modules could be tampered with by
> malware. The LSM needs to periodically query the state of
> the security modules and measure the data when the state is changed.
>
> Define a workqueue for handling this periodic query and measurement.

Won't this make it difficult/impossible to predict the IMA PCR value?
Unless I missed it, you are going to end up measuring every N minutes
even if there was no change and therefore constantly be extending the
PCR. That will break attestation or sealing against the IMA PCR.

2020-06-15 15:03:26

[permalink] [raw]

Subject: Re: [PATCH 5/5] LSM: Define workqueue for measuring security module state

On Mon, 2020-06-15 at 09:33 -0400, Stephen Smalley wrote:
> On Fri, Jun 12, 2020 at 10:42 PM Lakshmi Ramasubramanian
> <[email protected]> wrote:
> >
> > The data maintained by the security modules could be tampered with by
> > malware. The LSM needs to periodically query the state of
> > the security modules and measure the data when the state is changed.
> >
> > Define a workqueue for handling this periodic query and measurement.
>
> Won't this make it difficult/impossible to predict the IMA PCR value?
> Unless I missed it, you are going to end up measuring every N minutes
> even if there was no change and therefore constantly be extending the
> PCR. That will break attestation or sealing against the IMA PCR.

Even if it attempts to add the same measurement to the list multiple
times, unless something changed, there should only be one measurement
in the list.

Mimi

2020-06-15 15:50:21

[permalink] [raw]

Subject: Re: [PATCH 5/5] LSM: Define workqueue for measuring security module state

On Mon, Jun 15, 2020 at 10:59 AM Mimi Zohar <[email protected]> wrote:
>
> On Mon, 2020-06-15 at 09:33 -0400, Stephen Smalley wrote:
> > On Fri, Jun 12, 2020 at 10:42 PM Lakshmi Ramasubramanian
> > <[email protected]> wrote:
> > >
> > > The data maintained by the security modules could be tampered with by
> > > malware. The LSM needs to periodically query the state of
> > > the security modules and measure the data when the state is changed.
> > >
> > > Define a workqueue for handling this periodic query and measurement.
> >
> > Won't this make it difficult/impossible to predict the IMA PCR value?
> > Unless I missed it, you are going to end up measuring every N minutes
> > even if there was no change and therefore constantly be extending the
> > PCR. That will break attestation or sealing against the IMA PCR.
>
> Even if it attempts to add the same measurement to the list multiple
> times, unless something changed, there should only be one measurement
> in the list.

Is the PCR only extended once?

2020-06-15 16:13:03

[permalink] [raw]

Subject: Re: [PATCH 5/5] LSM: Define workqueue for measuring security module state

On Mon, 2020-06-15 at 11:47 -0400, Stephen Smalley wrote:
> On Mon, Jun 15, 2020 at 10:59 AM Mimi Zohar <[email protected]> wrote:
> >
> > On Mon, 2020-06-15 at 09:33 -0400, Stephen Smalley wrote:
> > > On Fri, Jun 12, 2020 at 10:42 PM Lakshmi Ramasubramanian
> > > <[email protected]> wrote:
> > > >
> > > > The data maintained by the security modules could be tampered with by
> > > > malware. The LSM needs to periodically query the state of
> > > > the security modules and measure the data when the state is changed.
> > > >
> > > > Define a workqueue for handling this periodic query and measurement.
> > >
> > > Won't this make it difficult/impossible to predict the IMA PCR value?
> > > Unless I missed it, you are going to end up measuring every N minutes
> > > even if there was no change and therefore constantly be extending the
> > > PCR. That will break attestation or sealing against the IMA PCR.
> >
> > Even if it attempts to add the same measurement to the list multiple
> > times, unless something changed, there should only be one measurement
> > in the list.
>
> Is the PCR only extended once?

Yes, otherwise you wouldn't be able to verify a quote.
ima_lookup_digest_entry() first verifies the hash isn't in the cache,
before adding it to the measurement list and then extending the TPM.

Mimi

2020-06-15 16:50:26

[permalink] [raw]

Subject: Re: [PATCH 4/5] LSM: Define SELinux function to measure security state

On 6/15/20 4:57 AM, Stephen Smalley wrote:

Hi Stephen,

Thanks for reviewing the patches.

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

One of the feedback Casey Schaufler had given earlier was that calling
an IMA function directly from SELinux (or, any of the Security Modules)
would be a layering violation.

LSM framework (security/security.c) already calls IMA functions now (for
example, ima_bprm_check() is called from security_bprm_check()). I
followed the same pattern for measuring LSM data as well.

Please let me know if I misunderstood Casey's comment.

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

Sure - I will address this one in the next update.

thanks,
-lakshmi

2020-06-15 17:36:22

by Casey Schaufler

[permalink] [raw]

Subject: Re: [PATCH 4/5] LSM: Define SELinux function to measure security state

On 6/15/2020 9:45 AM, Lakshmi Ramasubramanian wrote:
> On 6/15/20 4:57 AM, Stephen Smalley wrote:
>
> Hi Stephen,
>
> Thanks for reviewing the patches.
>
>>> +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?
>
> One of the feedback Casey Schaufler had given earlier was that calling an IMA function directly from SELinux (or, any of the Security Modules) would be a layering violation.

Hiding the ima_lsm_state() call doesn't address the layering.
The point is that SELinux code being called from IMA (or the
other way around) breaks the subsystem isolation. Unfortunately,
it isn't obvious to me how you would go about what you're doing
without integrating the subsystems.

>
> LSM framework (security/security.c) already calls IMA functions now (for example, ima_bprm_check() is called from security_bprm_check()). I followed the same pattern for measuring LSM data as well.
>
> Please let me know if I misunderstood Casey's comment.
>

2020-06-15 17:47:15

[permalink] [raw]

Subject: Re: [PATCH 4/5] LSM: Define SELinux function to measure security state

(Cc'ing John)

On Mon, 2020-06-15 at 10:33 -0700, Casey Schaufler wrote:
> On 6/15/2020 9:45 AM, Lakshmi Ramasubramanian wrote:
> > On 6/15/20 4:57 AM, Stephen Smalley wrote:
> >
> > Hi Stephen,
> >
> > Thanks for reviewing the patches.
> >
> >>> +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?
> >
> > One of the feedback Casey Schaufler had given earlier was that calling an IMA function directly from SELinux (or, any of the Security Modules) would be a layering violation.
>
> Hiding the ima_lsm_state() call doesn't address the layering.
> The point is that SELinux code being called from IMA (or the
> other way around) breaks the subsystem isolation. Unfortunately,
> it isn't obvious to me how you would go about what you're doing
> without integrating the subsystems.

Casey, I'm not sure why you think there is a layering issue here.
There were multiple iterations of IMA before it was upstreamed. One
iteration had separate integrity hooks(LIM). Only when the IMA calls
and the security hooks are co-located, are they combined, as requested
by Linus.

There was some AppArmour discussion about calling IMA directly, but I
haven't heard about it in a while or seen the patch.

Mimi

2020-06-15 20:37:28

[permalink] [raw]

Subject: Re: [PATCH 4/5] LSM: Define SELinux function to measure security state

On Mon, Jun 15, 2020 at 12:45 PM Lakshmi Ramasubramanian
<[email protected]> wrote:
>
> On 6/15/20 4:57 AM, Stephen Smalley wrote:
> > 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.
> >
>
> Sure - I will address this one in the next update.

Please add selinux list to the cc for future versions too.

2020-06-15 23:20:46

by Casey Schaufler

[permalink] [raw]

Subject: Re: [PATCH 4/5] LSM: Define SELinux function to measure security state

On 6/15/2020 10:44 AM, Mimi Zohar wrote:
> (Cc'ing John)
>
> On Mon, 2020-06-15 at 10:33 -0700, Casey Schaufler wrote:
>> On 6/15/2020 9:45 AM, Lakshmi Ramasubramanian wrote:
>>> On 6/15/20 4:57 AM, Stephen Smalley wrote:
>>>
>>> Hi Stephen,
>>>
>>> Thanks for reviewing the patches.
>>>
>>>>> +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?
>>> One of the feedback Casey Schaufler had given earlier was that calling an IMA function directly from SELinux (or, any of the Security Modules) would be a layering violation.
>> Hiding the ima_lsm_state() call doesn't address the layering.
>> The point is that SELinux code being called from IMA (or the
>> other way around) breaks the subsystem isolation. Unfortunately,
>> it isn't obvious to me how you would go about what you're doing
>> without integrating the subsystems.
> Casey, I'm not sure why you think there is a layering issue here.

I don't think there is, after further review. If the IMA code called
selinux_dosomething() directly I'd be very concerned, but calling
security_dosomething() which then calls selinux_dosomething() is fine.
If YAMA called security_dosomething() I'd be very concerned, but that's
not what's happening here.

> There were multiple iterations of IMA before it was upstreamed. One
> iteration had separate integrity hooks(LIM). Only when the IMA calls
> and the security hooks are co-located, are they combined, as requested
> by Linus.
>
> There was some AppArmour discussion about calling IMA directly, but I
> haven't heard about it in a while or seen the patch.
>
> Mimi

2020-06-16 00:47:32