Message-ID: <2dce2244-adbd-df2a-e890-271bbcc8f9f2@linux.microsoft.com>
Date:   Mon, 4 Jan 2021 15:30:40 -0800
From:   Lakshmi Ramasubramanian <nramas@...ux.microsoft.com>
To:     Paul Moore <paul@...l-moore.com>,
        Tushar Sugandhi <tusharsu@...ux.microsoft.com>
Cc:     zohar@...ux.ibm.com,
        Stephen Smalley <stephen.smalley.work@...il.com>,
        casey@...aufler-ca.com, agk@...hat.com, snitzer@...hat.com,
        gmazyland@...il.com, tyhicks@...ux.microsoft.com,
        sashal@...nel.org, James Morris <jmorris@...ei.org>,
        linux-integrity@...r.kernel.org, selinux@...r.kernel.org,
        linux-security-module@...r.kernel.org,
        linux-kernel@...r.kernel.org, dm-devel@...hat.com
Subject: Re: [PATCH v9 8/8] selinux: include a consumer of the new IMA
 critical data hook

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;
>>   }
> 

Powered by blists - more mailing lists