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

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

On Tue, 2013-02-12 at 13:52 -0500, Vivek Goyal wrote:
> On Tue, Feb 12, 2013 at 12:14:07PM -0500, Mimi Zohar wrote:
> 
> [..]
> > > > > --- 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.
> 
> Ok, I can do that. There is a small concern though. That is what to do
> when rc = INTEGRITY_UKNOWN and IMA_DIGSIG_OPTIONAL flag is set.
> 
> ima_appraise_measurement() returns INTEGRITY_UKNOWN when file system
> does not support xattrs or if security xattr is not enabled. In this
> case it is desirable to allow access if IMA_DIGSIG_OPTIONAL flag is
> set.

Right, 'INTEGRITY_UNKNOWN' means that we can't reason, for whatever
reason, about the integrity of the file.

> But INTEGRITY_UNKNOWN is also also returned when integrity_digsig_verify()
> fails and returns -EOPNOTSUPP. 

In this case, it is Kconfig based. 

> I feel that in this case it is not very appropriate to pass appraisal and
> let executable run. If digital signatures are present but we can't verify
> those (Say some algorithm is not supported in kernel). In that case I
> think it makes sense to fail the signature. 
> 
>                rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA,
>                                              xattr_value->digest, rc - 1,
>                                              iint->ima_xattr.digest,
>                                              IMA_DIGEST_SIZE);
>                 if (rc == -EOPNOTSUPP) {
>                         status = INTEGRITY_UNKNOWN;
> 
> 
> So how to handle this case.
> 
> I am wondering why do we reutrn INTEGRITY_UNKNOWN above and not
> INTEGRITY_FAIL.

We still can't reason about the integrity of the file.  For all we know,
it could be a validly signed file, just verification wasn't enabled.

> Will it make sense to fail signature in case of -EOPNOTSUPP.
>                rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA,
>                                              xattr_value->digest, rc - 1,
>                                              iint->ima_xattr.digest,
>                                              IMA_DIGEST_SIZE);
> 		if (rc)
> 			status = INTEGRITY_FAIL;
> 		else
> 			status = INTEGRITY_PASS;
> 
> 

Please don't.

Mimi