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

[Vivek Goyal <vgoyal@redhat.com>]

On Thu, Feb 14, 2013 at 11:17:19AM -0500, Vivek Goyal wrote:
> On Thu, Feb 14, 2013 at 10:35:59AM -0500, Mimi Zohar wrote:
> > On Thu, 2013-02-14 at 10:23 -0500, Vivek Goyal wrote:
> > > On Thu, Feb 14, 2013 at 07:57:16AM -0500, Mimi Zohar wrote:
> > > 
> > > [..]
> > > > > Ok, I will cleanup the code to do above. Just wanted to clear up one
> > > > > point. 
> > > > > 
> > > > > Above option will not have any effect on evm behavior? This only impacts
> > > > > IMA appraisal behavior. For example, if security.ima is not present it
> > > > > is fine and file access is allowed. But if EVM is enabled and initialized
> > > > > and EVM does not find security.evm label (INTEGRITY_NOLABEL) or returns
> > > > > INTEGRITY_NOXATTRS, file access should still be denied?
> > > > 
> > > > Can't happen.  evm_verifyxattr() is called from
> > > > ima_appraise_measurement(), only if 'security.ima' exists.
> > > 
> > > Actually what I meant is following.
> > > 
> > > Currently in process_measurement(), I will allow access if
> > > ima_appraise_measurement() returns INTEGRITY_NOLABEL.
> > 
> > I think you're making this more complicated than it needs to be.  Allow
> > the execution unless the file failed signature verification.  The
> > additional capability is given only if the signature verification
> > succeeds.
> 
> I am just trying to bring it inline with module signature verification.
> There also module loading fails if signatures are present but kernel
> can't verify it.
> 
> Following behavior is strange.
> 
>                 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;
>                 } else if (rc) {
>                         cause = "invalid-signature";
>                         status = INTEGRITY_FAIL;
>                 } else {
>                         status = INTEGRITY_PASS;
>                 }
> 
> signature verification can fail for so many reasons.
> 
> - EINVAL
> - keyring is not present
> - key is not present -ENOKEY
> - ENOTSUPP
> - ENOMEM
> ..
> ..
> 
> And in all these cases we return INTEGRITY_FAIL. But only in case of
> -EOPNOTSUPP we return INTEGRITY_UNKNOWN. So why this discrepancy.
> 
> So to me it makes sense to return INTEGRITY_FAIL if rc == -EOPNOTSUPP.
> This will bring it inline with other error codes.  And then in
> process_measurement() I can allow access in every case except
> INTEGRITY_FAIL.

Actually even this is not sufficient. Because if security.ima is present
and it contains digital signature but security.evm is not present (assume
EVM is enabled), then appraise_measurement() will return NOLABEL or
NOXATTRS and access to file will be allowed. 

But what I am trying to implement is that if digital signatures are
present, then they have to be valid. Any failure to assess the validity
of digital signature should result in failure of exec().

So to me it is important to come to a common understanding that
appraise_type=optional affects only IMA behavior and not EVM behavior. And
then there is a need to separate out IMA and EVM return codes so that
one can enforce appraise_type=optional properly. Otherwise we are leaving
lots of grey areas behind.

Thanks
Vivek

> 
> Thanks
> Vivek
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html