linux-kernel - Re: [PATCH v5 1/4] IMA: Add func to measure LSM state and policy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 30 Jul 2020 10:17:40 -0500
From:   Tyler Hicks <tyhicks@...ux.microsoft.com>
To:     Lakshmi Ramasubramanian <nramas@...ux.microsoft.com>
Cc:     zohar@...ux.ibm.com, stephen.smalley.work@...il.com,
        casey@...aufler-ca.com, sashal@...nel.org, jmorris@...ei.org,
        linux-integrity@...r.kernel.org, selinux@...r.kernel.org,
        linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 1/4] IMA: Add func to measure LSM state and policy

On 2020-07-30 08:15:34, Lakshmi Ramasubramanian wrote:
> On 7/30/20 8:02 AM, Tyler Hicks wrote:
> 
> > > 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().
> > 
> 
> The LSM hook can be used by any security module (not just SELinux) to
> measure their data.
> 
> I have implemented measurement in SELinux to illustrate the usage. Maybe, I
> can add the check you have suggested for now and when more security modules
> start using this IMA policy additional checks can be added as appropriate.

Yes, that's what I envision.

The main idea is that there's negative feedback to userspace when IMA
can't possibly do anything with an LSM_STATE/LSM_POLICY rule.

Tyler

> 
> thanks,
>  -lakshmi