linux-kernel - Re: [PATCH 2/2] ima: Support appraise_type=imasig

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]
Message-ID: <20130212185203.GA29958@redhat.com>
Date:	Tue, 12 Feb 2013 13:52:03 -0500
From:	Vivek Goyal <vgoyal@...hat.com>
To:	Mimi Zohar <zohar@...ux.vnet.ibm.com>
Cc:	linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] ima: Support appraise_type=imasig_optional

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.

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

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.

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;


[..]
> > > > --- 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.

Ok, I will change it and set IMA_DIGSIG_REQUIRED flag too.

Thanks
Vivek
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/