selinux.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Casey Schaufler <casey@schaufler-ca.com>
To: Tyler Hicks <tyhicks@linux.microsoft.com>,
	Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
Cc: zohar@linux.ibm.com, stephen.smalley.work@gmail.com,
	sashal@kernel.org, jmorris@namei.org,
	linux-integrity@vger.kernel.org, selinux@vger.kernel.org,
	linux-security-module@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	Casey Schaufler <casey@schaufler-ca.com>
Subject: Re: [PATCH v5 1/4] IMA: Add func to measure LSM state and policy
Date: Thu, 30 Jul 2020 09:19:50 -0700	[thread overview]
Message-ID: <b4428195-7a68-365d-a792-2855609c2221@schaufler-ca.com> (raw)
In-Reply-To: <20200730150228.GV4181@sequoia>

On 7/30/2020 8:02 AM, Tyler Hicks wrote:
> On 2020-07-29 20:47:21, Lakshmi Ramasubramanian wrote:
>> Critical data structures of security modules need to be measured to
>> enable an attestation service to verify if the configuration and
>> policies for the security modules have been setup correctly and
>> that they haven't been tampered with at runtime. A new IMA policy is
>> required for handling this measurement.
>>
>> Define two new IMA policy func namely LSM_STATE and LSM_POLICY to
>> measure the state and the policy provided by the security modules.

If, as you suggest below, this is SELinux specific,
these should be SELINUX_STATE and SELINUX_POLICY.
It makes me very uncomfortable when I see LSM used
in cases where SELinux is required. The LSM is supposed
to be an agnostic interface, so if you need to throw

	if (IS_ENABLED(CONFIG_SECURITY_SELINUX) &&

into the IMA code you're clearly not thinking in terms
of the LSM layer. I have no problem with seeing SELinux
oriented and/or specific code in IMA if that's what you want.
Just don't call it LSM.

>> Update ima_match_rules() and ima_validate_rule() to check for
>> the new func and ima_parse_rule() to handle the new func.
>>
>> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
>> ---
>>  Documentation/ABI/testing/ima_policy |  9 ++++++++
>>  security/integrity/ima/ima.h         |  2 ++
>>  security/integrity/ima/ima_api.c     |  2 +-
>>  security/integrity/ima/ima_policy.c  | 31 ++++++++++++++++++++++++----
>>  4 files changed, 39 insertions(+), 5 deletions(-)
>>
>> diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
>> index cd572912c593..b7c7fb548c0c 100644
>> --- a/Documentation/ABI/testing/ima_policy
>> +++ b/Documentation/ABI/testing/ima_policy
>> @@ -30,6 +30,7 @@ Description:
>>  				[FIRMWARE_CHECK]
>>  				[KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
>>  				[KEXEC_CMDLINE] [KEY_CHECK]
>> +				[LSM_STATE] [LSM_POLICY]
>>  			mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND]
>>  			       [[^]MAY_EXEC]
>>  			fsmagic:= hex value
>> @@ -125,3 +126,11 @@ 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 state:
>> +
>> +			measure func=LSM_STATE template=ima-buf
>> +
>> +		Example of measure rule using LSM_POLICY to measure LSM policy:
>> +
>> +			measure func=LSM_POLICY template=ima-ng
>> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
>> index 38043074ce5e..1b5f4b2f17d0 100644
>> --- a/security/integrity/ima/ima.h
>> +++ b/security/integrity/ima/ima.h
>> @@ -200,6 +200,8 @@ static inline unsigned int ima_hash_key(u8 *digest)
>>  	hook(POLICY_CHECK, policy)			\
>>  	hook(KEXEC_CMDLINE, kexec_cmdline)		\
>>  	hook(KEY_CHECK, key)				\
>> +	hook(LSM_STATE, lsm_state)			\
>> +	hook(LSM_POLICY, lsm_policy)			\
>>  	hook(MAX_CHECK, none)
>>  
>>  #define __ima_hook_enumify(ENUM, str)	ENUM,
>> diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
>> index 4f39fb93f278..8c8b4e4a6493 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 | LSM_POLICY
>>   *	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 07f033634b27..a0f5c39d9084 100644
>> --- a/security/integrity/ima/ima_policy.c
>> +++ b/security/integrity/ima/ima_policy.c
>> @@ -442,13 +442,20 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
>>  {
>>  	int i;
>>  
>> -	if (func == KEY_CHECK) {
>> -		return (rule->flags & IMA_FUNC) && (rule->func == func) &&
>> -		       ima_match_keyring(rule, keyring, cred);
>> -	}
>>  	if ((rule->flags & IMA_FUNC) &&
>>  	    (rule->func != func && func != POST_SETATTR))
>>  		return false;
>> +
>> +	switch (func) {
>> +	case KEY_CHECK:
>> +		return ima_match_keyring(rule, keyring, cred);
>> +	case LSM_STATE:
>> +	case LSM_POLICY:
>> +		return true;
>> +	default:
>> +		break;
>> +	}
>> +
>>  	if ((rule->flags & IMA_MASK) &&
>>  	    (rule->mask != mask && func != POST_SETATTR))
>>  		return false;
>> @@ -1044,6 +1051,18 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
>>  		if (ima_rule_contains_lsm_cond(entry))
>>  			return false;
>>  
>> +		break;
>> +	case LSM_STATE:
>> +	case LSM_POLICY:
>> +		if (entry->action & ~(MEASURE | DONT_MEASURE))
>> +			return false;
>> +
>> +		if (entry->flags & ~(IMA_FUNC | IMA_PCR))
>> +			return false;
>> +
>> +		if (ima_rule_contains_lsm_cond(entry))
>> +			return false;
>> +
>>  		break;
>>  	default:
>>  		return false;
>> @@ -1176,6 +1195,10 @@ 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 if (strcmp(args[0].from, "LSM_POLICY") == 0)
>> +				entry->func = LSM_POLICY;
> This patch generally looks really good to me with the exception of one
> thing...
>
> We should only accept rules with these specified hook functions when an
> LSM that has measurement support is enabled. This messes up the ordering
> of your patch series but it could be as simple as doing this:
>
> 			else if (IS_ENABLED(CONFIG_SECURITY_SELINUX) &&
> 				 strcmp(args[0].from, "LSM_STATE") == 0)
> 				 entry->func = LSM_STATE;
>
> Or you could do something a little more complex, like what's done with
> CONFIG_IMA_LSM_RULES. You could create a CONFIG_IMA_MEASURE_LSM option
> that's default enabled but depends on CONFIG_SECURITY_SELINUX and then
> check for IS_ENABLED(CONFIG_IMA_MEASURE_LSM) in ima_parse_rule().
>
> I'd personally opt for just placing the
> IS_ENABLED(CONFIG_SECURITY_SELINUX) check directly into
> ima_parse_rule().
>
> Tyler
>
>>  			else
>>  				result = -EINVAL;
>>  			if (!result)
>> -- 
>> 2.27.0

  parent reply	other threads:[~2020-07-30 16:19 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-30  3:47 [PATCH v5 0/4] LSM: Measure security module data Lakshmi Ramasubramanian
2020-07-30  3:47 ` [PATCH v5 1/4] IMA: Add func to measure LSM state and policy Lakshmi Ramasubramanian
2020-07-30 15:02   ` Tyler Hicks
2020-07-30 15:15     ` Lakshmi Ramasubramanian
2020-07-30 15:17       ` Tyler Hicks
2020-07-30 16:19     ` Casey Schaufler [this message]
2020-07-30 16:33       ` Lakshmi Ramasubramanian
2020-07-30  3:47 ` [PATCH v5 2/4] IMA: Define IMA hooks " Lakshmi Ramasubramanian
2020-07-30 15:04   ` Tyler Hicks
2020-07-30  3:47 ` [PATCH v5 3/4] LSM: Define SELinux function to measure " Lakshmi Ramasubramanian
2020-08-03 15:11   ` Stephen Smalley
2020-08-03 16:14     ` Lakshmi Ramasubramanian
2020-08-03 20:00       ` Stephen Smalley
2020-08-03 20:29         ` Stephen Smalley
2020-08-03 20:37           ` Lakshmi Ramasubramanian
2020-08-03 21:07             ` Stephen Smalley
2020-08-03 22:08               ` Lakshmi Ramasubramanian
2020-08-04 15:20                 ` Stephen Smalley
2020-08-04 15:29                   ` Stephen Smalley
2020-08-04 15:57                     ` Lakshmi Ramasubramanian
2020-07-30  3:47 ` [PATCH v5 4/4] IMA: Handle early boot data measurement Lakshmi Ramasubramanian
2020-07-30 18:02   ` Lakshmi Ramasubramanian
2020-07-30 20:04     ` Tyler Hicks

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=b4428195-7a68-365d-a792-2855609c2221@schaufler-ca.com \
    --to=casey@schaufler-ca.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=nramas@linux.microsoft.com \
    --cc=sashal@kernel.org \
    --cc=selinux@vger.kernel.org \
    --cc=stephen.smalley.work@gmail.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).