Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional

[Mimi Zohar <zohar@linux.vnet.ibm.com>]

On Tue, 2013-02-12 at 09:26 -0500, Vivek Goyal wrote:
> On Mon, Feb 11, 2013 at 05:10:14PM -0500, Mimi Zohar wrote:
> > On Mon, 2013-02-11 at 15:11 -0500, Vivek Goyal wrote:
> > > appraise_type=imasig_optional will allow appraisal to pass even if no
> > > signatures are present on the file. If signatures are present, then it
> > > has to be valid digital signature, otherwise appraisal will fail.
> > > 
> > > This can allow to selectively sign executables in the system and based
> > > on appraisal results, signed executables with valid signatures can be
> > > given extra capability to perform priviliged operations in secureboot
> > > mode.
> > > 
> > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > 
> > Thanks, Vivek, the patch looks a lot better.  Here are a couple of
> > suggestions:  
> > - the patch description needs to start with the problem description, not
> > the solution.
> 
> Sure will do.
> 
> > - the patch name should reflect the problem.
> 
> Will change.
> 
> > 
> > A few comments are inline below.
> > 
> > thanks,
> > 
> > Mimi
> > 
> > > ---
> > >  Documentation/ABI/testing/ima_policy  |    2 +-
> > >  security/integrity/ima/ima_appraise.c |   24 +++++++++++++++++++-----
> > >  security/integrity/ima/ima_policy.c   |    2 ++
> > >  security/integrity/integrity.h        |    1 +
> > >  4 files changed, 23 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
> > > index de16de3..5ca0c23 100644
> > > --- a/Documentation/ABI/testing/ima_policy
> > > +++ b/Documentation/ABI/testing/ima_policy
> > > @@ -30,7 +30,7 @@ Description:
> > >  			uid:= decimal value
> > >  			fowner:=decimal value
> > >  		lsm:  	are LSM specific
> > > -		option:	appraise_type:= [imasig]
> > > +		option:	appraise_type:= [imasig] | [imasig_optional]
> > > 
> > >  		default policy:
> > >  			# PROC_SUPER_MAGIC
> > > diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> > > index 3710f44..222ade0 100644
> > > --- a/security/integrity/ima/ima_appraise.c
> > > +++ b/security/integrity/ima/ima_appraise.c
> > > @@ -124,19 +124,26 @@ int ima_appraise_measurement(int func, struct integrity_iint_cache *iint,
> > >  	enum integrity_status status = INTEGRITY_UNKNOWN;
> > >  	const char *op = "appraise_data";
> > >  	char *cause = "unknown";
> > > -	int rc;
> > > +	int rc, audit_info = 0;
> > > 
> > >  	if (!ima_appraise)
> > >  		return 0;
> > > -	if (!inode->i_op->getxattr)
> > > +	if (!inode->i_op->getxattr) {
> > > +		/* getxattr not supported. file couldn't have been signed */
> > > +		if (iint->flags & IMA_DIGSIG_OPTIONAL)
> > > +			return INTEGRITY_PASS;
> > >  		return INTEGRITY_UNKNOWN;
> > > +	}
> > > 
> > 
> > Please don't change the result of the appraisal like this.  A single
> > change can be made towards the bottom of process_measurement().
> 
> I don't want to pass integrity in all cases of INTEGRITY_UNKNOWN. So
> I can probably maintain a bool variable, say pass_appraisal, and set
> that here and at the end of function, parse that variable and change
> the status accordingly.

process_measurement() is the only caller of ima_appraise_measurement().
Leave the results of ima_appraise_measurement() alone.  There's already
code at the end of process_measurement() which decides what to return.
Just modify it based on the appraisal results.

> > 
> > >  	rc = vfs_getxattr_alloc(dentry, XATTR_NAME_IMA, (char **)&xattr_value,
> > >  				0, GFP_NOFS);
> > >  	if (rc <= 0) {
> > >  		/* File system does not support security xattr */
> > > -		if (rc == -EOPNOTSUPP)
> > > +		if (rc == -EOPNOTSUPP) {
> > > +			if (iint->flags & IMA_DIGSIG_OPTIONAL)
> > > +				return INTEGRITY_PASS;
> > >  			return INTEGRITY_UNKNOWN;
> > > +		}
> > 
> > ditto 
> 
> Will do.
> 
> > 
> > > 
> > >  		if (rc && rc != -ENODATA)
> > >  			goto out;
> > > @@ -158,7 +165,8 @@ int ima_appraise_measurement(int func, struct integrity_iint_cache *iint,
> > >  	}
> > >  	switch (xattr_value->type) {
> > >  	case IMA_XATTR_DIGEST:
> > > -		if (iint->flags & IMA_DIGSIG_REQUIRED) {
> > > +		if (iint->flags & IMA_DIGSIG_REQUIRED ||
> > > +		    iint->flags & IMA_DIGSIG_OPTIONAL) {
> > >  			cause = "IMA signature required";
> > >  			status = INTEGRITY_FAIL;
> > >  			break;
> > > @@ -201,8 +209,14 @@ out:
> > >  			if (!ima_fix_xattr(dentry, iint))
> > >  				status = INTEGRITY_PASS;
> > >  		}
> > > +		if (status == INTEGRITY_NOLABEL &&
> > > +		    iint->flags & IMA_DIGSIG_OPTIONAL) {
> > > +			status = INTEGRITY_PASS;
> > > +			/* Don't flood audit logs with skipped appraise */
> > > +			audit_info = 1;
> > > +		}
> > >  		integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
> > > -				    op, cause, rc, 0);
> > > +				    op, cause, rc, audit_info);
> > >  	} else {
> > >  		ima_cache_flags(iint, func);
> > >  	}
> > > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> > > index 4adcd0f..8b8cd5f 100644
> > > --- a/security/integrity/ima/ima_policy.c
> > > +++ b/security/integrity/ima/ima_policy.c
> > > @@ -598,6 +598,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
> > >  			ima_log_string(ab, "appraise_type", args[0].from);
> > >  			if ((strcmp(args[0].from, "imasig")) == 0)
> > >  				entry->flags |= IMA_DIGSIG_REQUIRED;
> > > +			else if ((strcmp(args[0].from, "imasig_optional")) == 0)
> > > +				entry->flags |= IMA_DIGSIG_OPTIONAL;
> > 
> > By setting IMA_DIGSIG_REQUIRED, here, as well, you'll be able to clean
> > up the code a bit more.
> 
> I don't understand this part. So imasig_optional sets both 
> IMA_DIGSIG_REQUIRED as well as IMA_DIGSIG_OPTIONAL? That seems to be
> quite contradictory for a reader. 

> We only add one extra line and that is when "hash" is detected in
> security.ima, we check for IMA_DIGSIG_OPTIONAL and return an error. So
> we are probably not saving on code.
> 
> IMHO, not setting IMA_DIGSIG_REQUIRED makes sense in this context.

'imasig_optional' does not only mean that the signature is optional, but
also implies that it has to be a digital signature, not a hash.  This
latter part is what IMA_DIGSIG_REQUIRED means.

Remember the rule 'action' determines whether or not the file needs to
be appraised.

thanks,

Mimi