From: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
To: Paul Moore <paul@paul-moore.com>,
Tushar Sugandhi <tusharsu@linux.microsoft.com>
Cc: zohar@linux.ibm.com,
Stephen Smalley <stephen.smalley.work@gmail.com>,
casey@schaufler-ca.com, agk@redhat.com, snitzer@redhat.com,
gmazyland@gmail.com, tyhicks@linux.microsoft.com,
sashal@kernel.org, James Morris <jmorris@namei.org>,
linux-integrity@vger.kernel.org, selinux@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org, dm-devel@redhat.com
Subject: Re: [PATCH v9 8/8] selinux: include a consumer of the new IMA critical data hook
Date: Mon, 4 Jan 2021 15:30:40 -0800 [thread overview]
Message-ID: <2dce2244-adbd-df2a-e890-271bbcc8f9f2@linux.microsoft.com> (raw)
In-Reply-To: <CAHC9VhSao7DGtskbDMax8hN+PhQr8homFXUGjm+c7NtEUCtKhg@mail.gmail.com>
On 12/23/20 1:10 PM, Paul Moore wrote:
Hi Paul,
> ...
>
>> diff --git a/security/selinux/Makefile b/security/selinux/Makefile
>> index 4d8e0e8adf0b..83d512116341 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) += measure.o
>
> Naming things is hard, I get that, but I would prefer if we just
> called this file "ima.c" or something similar. The name "measure.c"
> implies a level of abstraction or general use which simply doesn't
> exist here. Let's help make it a bit more obvious what should belong
> in this file.
Agreed - I will rename the file to ima.c
>
>> diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
>> index 3cc8bab31ea8..18ee65c98446 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_policy_kernel(struct selinux_state *state,
>> + void **data, size_t *len);
>> int security_policycap_supported(struct selinux_state *state,
>> unsigned int req_cap);
>>
>> @@ -446,4 +447,12 @@ extern void ebitmap_cache_init(void);
>> extern void hashtab_cache_init(void);
>> extern int security_sidtab_hash_stats(struct selinux_state *state, char *page);
>>
>> +#ifdef CONFIG_IMA
>> +extern void selinux_measure_state(struct selinux_state *selinux_state);
>> +#else
>> +static inline void selinux_measure_state(struct selinux_state *selinux_state)
>> +{
>> +}
>> +#endif
>
> If you are going to put the SELinux/IMA function(s) into a separate
> source file, please put the function declarations into a separate
> header file too. For example, look at
> security/selinux/include/{netif,netnode,netport,etc.}.h.
I will create a new header file "security/selinux/include/ima.h" and
move the function declarations for IMA functions to that header.
>
>> diff --git a/security/selinux/measure.c b/security/selinux/measure.c
>> new file mode 100644
>> index 000000000000..b7e24358e11d
>> --- /dev/null
>> +++ b/security/selinux/measure.c
>> @@ -0,0 +1,79 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Measure SELinux state using IMA subsystem.
>> + */
>> +#include <linux/vmalloc.h>
>> +#include <linux/ktime.h>
>> +#include <linux/ima.h>
>> +#include "security.h"
>> +
>> +/*
>> + * This function creates a unique name by appending the timestamp to
>> + * the given string. This string is passed as "event_name" to the IMA
>> + * hook to measure the given SELinux data.
>> + *
>> + * The data provided by SELinux to the IMA subsystem for measuring may have
>> + * already been measured (for instance the same state existed earlier).
>> + * But for SELinux the current data represents a state change and hence
>> + * needs to be measured again. To enable this, pass a unique "event_name"
>> + * to the IMA hook so that IMA subsystem will always measure the given data.
>> + *
>> + * For example,
>> + * At time T0 SELinux data to be measured is "foo". IMA measures it.
>> + * At time T1 the data is changed to "bar". IMA measures it.
>> + * At time T2 the data is changed to "foo" again. IMA will not measure it
>> + * (since it was already measured) unless the event_name, for instance,
>> + * is different in this call.
>> + */
>> +static char *selinux_event_name(const char *name_prefix)
>> +{
>> + struct timespec64 cur_time;
>> +
>> + ktime_get_real_ts64(&cur_time);
>> + return kasprintf(GFP_KERNEL, "%s-%lld:%09ld", name_prefix,
>> + cur_time.tv_sec, cur_time.tv_nsec);
>> +}
>
> Why is this a separate function? It's three lines long and only
> called from selinux_measure_state(). Do you ever see the SELinux/IMA
> code in this file expanding to the point where this function is nice
> from a reuse standpoint?
Earlier I had two measurements - one for SELinux configuration/state and
another for SELinux policy. selinux_event_name() was used to generate
event name for each of them.
In this patch set I have included only one measurement - for SELinux
policy. I plan to add "SELinux configuration/state" measurement in a
separate patch - I can reuse selinux_event_name() in that patch.
Also, I think the comment in the function header for
selinux_event_name() is useful.
I prefer to have a separate function, if that's fine by you.
>
> Also, I assume you are not concerned about someone circumventing the
> IMA measurements by manipulating the time? In most systems I would
> expect the time to be a protected entity, but with many systems
> getting their time from remote systems I thought it was worth
> mentioning.
I am using time function to generate a unique name for the IMA
measurement event, such as, "selinux-policy-hash-1609790281:860232824".
This is to ensure that state changes in SELinux data are always measured.
If you think time manipulation can be an issue, please let me know a
better way to generate unique event names.
>
>> +/*
>> + * selinux_measure_state - Measure hash of the SELinux policy
>> + *
>> + * @state: selinux state struct
>> + *
>> + * NOTE: This function must be called with policy_mutex held.
>> + */
>> +void selinux_measure_state(struct selinux_state *state)
>
> Similar to the name of this source file, let's make it clear this is
> for IMA. How about calling this selinux_ima_measure_state() or
> similar?
Sure - I will change the function name to selinux_ima_measure_state().
>
>> +{
>> + void *policy = NULL;
>> + char *policy_event_name = NULL;
>> + size_t policy_len;
>> + int rc = 0;
>> + bool initialized = selinux_initialized(state);
>
> Why bother with the initialized variable? Unless I'm missing
> something it is only used once in the code below.
You are right - I will remove "initialized" variable and directly get
the state using selinux_initialized().
>
>> + /*
>> + * Measure SELinux policy only after initialization is completed.
>> + */
>> + if (!initialized)
>> + goto out;
>> +
>> + policy_event_name = selinux_event_name("selinux-policy-hash");
>> + if (!policy_event_name) {
>> + pr_err("SELinux: %s: event name for policy not allocated.\n",
>> + __func__);
>> + rc = -ENOMEM;
>
> This function doesn't return an error code, why bother with setting
> the rc variable here?
Yes - it is not necessary. I will remove the line.
>
>> + goto out;
>> + }
>> +
>> + rc = security_read_policy_kernel(state, &policy, &policy_len);
>> + if (rc) {
>> + pr_err("SELinux: %s: failed to read policy %d.\n", __func__, rc);
>> + goto out;
>> + }
>> +
>> + ima_measure_critical_data("selinux", policy_event_name,
>> + policy, policy_len, true);
>> +
>> + vfree(policy);
>> +
>> +out:
>> + kfree(policy_event_name);
>> +}
>> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
>> index 9704c8a32303..dfa2e00894ae 100644
>> --- a/security/selinux/ss/services.c
>> +++ b/security/selinux/ss/services.c
>> @@ -2180,6 +2180,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_measure_state(state);
>> }
>>
>> void selinux_policy_commit(struct selinux_state *state,
>> @@ -3875,8 +3876,33 @@ int security_netlbl_sid_to_secattr(struct selinux_state *state,
>> }
>> #endif /* CONFIG_NETLABEL */
>>
>> +/**
>> + * security_read_selinux_policy - read the policy.
>> + * @policy: SELinux policy
>> + * @data: binary policy data
>> + * @len: length of data in bytes
>> + *
>> + */
>> +static int security_read_selinux_policy(struct selinux_policy *policy,
>> + void *data, size_t *len)
>
> Let's just call this "security_read_policy()".
There is another function in this file with the name security_read_policy().
How about changing the above function name to "read_selinux_policy()"
since this is a local/static function.
>
>> +{
>> + 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 +3911,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 +3922,43 @@ int security_read_policy(struct selinux_state *state,
>> if (!*data)> --
>> 2.17.1
>>
>
>> return -ENOMEM;
>>
>> - fp.data = *data;
>> - fp.len = *len;
>> + return security_read_selinux_policy(policy, *data, len);
>> +}
>>
>> - rc = policydb_write(&policy->policydb, &fp);
>> - if (rc)
>> - return rc;
>> +/**
>> + * security_read_policy_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_policy_kernel(struct selinux_state *state,
>> + void **data, size_t *len)
>
> Let's call this "security_read_state_kernel()".
Sure - I will rename the function.
>
>> +{
>> + struct selinux_policy *policy;
>> + int rc = 0;
>
> See below, the rc variable is not needed.
>
>> - *len = (unsigned long)fp.data - (unsigned long)*data;
>> - return 0;
>> + policy = rcu_dereference_protected(
>> + state->policy, lockdep_is_held(&state->policy_mutex));
>> + if (!policy) {
>> + rc = -EINVAL;
>> + goto out;
>
> Jumping to the out label is a little silly since it is just a return;
> do a "return -EINVAL;" here instead.
>
>> + }
>> +
>> + *len = policy->policydb.len;
>> + *data = vmalloc(*len);
>> + if (!*data) {
>> + rc = -ENOMEM;
>> + goto out;
>
> Same as above, "return -ENOMEM;" please.
>
>> + }
>>
>> + rc = security_read_selinux_policy(policy, *data, len);
>
> You should be able to do "return security_read_selinux_policy(...);" here.
I will remove the local variable "rc" and make the three changes you've
stated above.
Thanks for reviewing the changes.
-lakshmi
>
>> +
>> +out:
>> + return rc;
>> }
>
next prev parent reply other threads:[~2021-01-04 23:31 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-12-12 18:02 [PATCH v9 0/8] IMA: support for measuring kernel integrity critical data Tushar Sugandhi
2020-12-12 18:02 ` [PATCH v9 1/8] IMA: generalize keyring specific measurement constructs Tushar Sugandhi
2020-12-24 13:06 ` Mimi Zohar
2021-01-05 18:48 ` Tushar Sugandhi
2020-12-12 18:02 ` [PATCH v9 2/8] IMA: add support to measure buffer data hash Tushar Sugandhi
2020-12-24 0:03 ` Mimi Zohar
2021-01-05 18:53 ` Tushar Sugandhi
2021-01-06 5:00 ` Tushar Sugandhi
2020-12-12 18:02 ` [PATCH v9 3/8] IMA: define a hook to measure kernel integrity critical data Tushar Sugandhi
2020-12-24 13:04 ` Mimi Zohar
2021-01-05 20:01 ` Tushar Sugandhi
2021-01-05 20:16 ` Mimi Zohar
2021-01-05 20:19 ` Tushar Sugandhi
2020-12-12 18:02 ` [PATCH v9 4/8] IMA: add policy rule to measure " Tushar Sugandhi
2020-12-12 19:20 ` Tyler Hicks
2020-12-13 1:21 ` Tushar Sugandhi
2020-12-24 13:48 ` Mimi Zohar
2021-01-05 20:12 ` Tushar Sugandhi
2020-12-12 18:02 ` [PATCH v9 5/8] IMA: limit critical data measurement based on a label Tushar Sugandhi
2020-12-12 19:20 ` Tyler Hicks
2020-12-13 1:21 ` Tushar Sugandhi
2020-12-24 14:29 ` Mimi Zohar
2021-01-05 20:28 ` Tushar Sugandhi
2020-12-12 18:02 ` [PATCH v9 6/8] IMA: extend critical data hook to limit the " Tushar Sugandhi
2020-12-12 18:02 ` [PATCH v9 7/8] IMA: define a builtin critical data measurement policy Tushar Sugandhi
2020-12-24 14:41 ` Mimi Zohar
2021-01-05 20:30 ` Tushar Sugandhi
2020-12-12 18:02 ` [PATCH v9 8/8] selinux: include a consumer of the new IMA critical data hook Tushar Sugandhi
2020-12-23 21:10 ` Paul Moore
2021-01-04 23:30 ` Lakshmi Ramasubramanian [this message]
2021-01-05 2:13 ` Paul Moore
2021-01-05 5:24 ` Lakshmi Ramasubramanian
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2dce2244-adbd-df2a-e890-271bbcc8f9f2@linux.microsoft.com \
--to=nramas@linux.microsoft.com \
--cc=agk@redhat.com \
--cc=casey@schaufler-ca.com \
--cc=dm-devel@redhat.com \
--cc=gmazyland@gmail.com \
--cc=jmorris@namei.org \
--cc=linux-integrity@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=paul@paul-moore.com \
--cc=sashal@kernel.org \
--cc=selinux@vger.kernel.org \
--cc=snitzer@redhat.com \
--cc=stephen.smalley.work@gmail.com \
--cc=tusharsu@linux.microsoft.com \
--cc=tyhicks@linux.microsoft.com \
--cc=zohar@linux.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).