Re: [Linux-ima-user] [PATCH v3] evm: provide portable EVM signature version.

[Mimi Z. <zo...@li...>]

On Tue, 2017-01-10 at 04:29 +0300, Mikhail Kurinnoi wrote:
> В Mon, 09 Jan 2017 18:11:01 -0500
> Mimi Zohar <zo...@li...> пишет:
> 
> > On Tue, 2017-01-10 at 00:06 +0300, Mikhail Kurinnoi wrote:
> > > В Mon, 09 Jan 2017 08:40:45 -0500
> > > Mimi Zohar <zo...@li...> пишет:
> > >   
> > > > On Sat, 2017-01-07 at 01:23 +0300, Mikhail Kurinnoi wrote:  
> > > > > version 3 changes:
> > > > > 1) Prevent ima_fix_xattr() use __vfs_setxattr_noperm() if EVM
> > > > > signature verification failed.    
> > > > 
> > > > This can't be right.  The boot comnand line option
> > > > "ima_appraise=fix" allows IMA to go into a special mode to "fix"
> > > > the xattrs. 
> > > > > Since, __vfs_setxattr_noperm() don't count on
> > > > > evm_inode_setxattr(), during file unpacking from atchive EVM
> > > > > zero-filled sign could be updated by IMA without EVM signature
> > > > > status checking. So, even if you have EVM portable signature
> > > > > signed by another key, IMA will update EVM signature to HMAC
> > > > > any way, no matter what you have in security.evm xattr or you
> > > > > don't have it at all.    
> > > > 
> > > > EVM and IMA are two separate modules, which communicate with each
> > > > other via the evm_verifyxattr() API.  Changes to EVM, should not
> > > > affect IMA. If they do, then something is not right.  
> > > 
> > > Something is wrong, because if we have FS mounted with iversion
> > > flag, we call ima_file_free() -> ima_check_last_writer(), that
> > > don't care about evm_verifyxattr() API at all.  
> > 
> >  This is working as designed.  The file integrity verification
> > occurred on file open.  On __fput() the file hash is updated, which
> > causes security.evm to be updated. 
> 
> I see.
> 
> Probably, we could add into evm_ima_xattr_type enumeration
> EVM_XATTR_PROTECTED, that we could use as EVM xattr signature type
> if we have EVM portable signature verification fail, that will be used
> only for "security.evm" attr, as additional check with
> evm_protected_xattr() in evm_inode_post_setxattr() and
> evm_inode_post_removexattr(). In this way we have INTEGRITY_FAIL in
> evm_verify_hmac(), since evm_verify_hmac() have no idea about
> EVM_XATTR_PROTECTED signature type, and prevent EVM xattr update on
> __fput() if we use IMA xattr hash but not IMA_DIGSIG.
> 
> Is it possible?
> 
> 
> > > I have this issue without IMA/EVM fix mode. I mean, I faced with
> > > this issue on IMA/EVM normal mode during archive unpacking with EVM
> > > portable signature signed by wrong cert, that was replaced on EVM
> > > zero-filled signature with evm_reset_status(). And after that IMA
> > > update inode EVM xattr with proper HMAC by ima_file_free() call.  
> > 
> > >   
> > > > > 2) Added integrity_audit_msg() in order to get some error in
> > > > > audit syslog on EVM portable signature verification fail. 
> > > > > 
> > > > > version 2 changes:
> > > > > 1) evm_verify_hmac() prevent work with EVM portable signature.
> > > > > If file signed by EVM portable signature, evm_verify_hmac()
> > > > > return INTEGRITY_FAIL;
> > > > > 2) evm_inode_post_setxattr() now count on EVM portable
> > > > > signature.     
> > > > 
> > > > I originally suggested that the portable EVM xattr should be
> > > > converted before it is written (in evm_inode_post_setxattr()),
> > > > but by the time we get to evm_inode_post_setxattr(),  the
> > > > portable EVM xattr has already been written.  Can the
> > > > verification be done earlier in evm_inode_setxattr(), before the
> > > > portable EVM xattr is written?   
> > > 
> > > We could do everything in evm_inode_setxattr() and return some
> > > special error code into vfs_setxattr() that will:
> > > 1) don't allow call __vfs_setxattr_noperm();
> > > 2) prevent special error code return and return 0 instead in the
> > > same time.  
> > 
> > I'm wondering if there will be an issue with the order in which the
> > security xattrs are installed.   For example, what guarantees that
> > security.ima is installed before security.evm?
> 
> This is archiver's part of work. As I wrote before (and you also could
> see this in my patch for tar), archiver must copy EVM xattr the last
> one. We have nothing to do with this in kernel.
> 
> 
> > Currently, after each xattr is written, the security.evm is updated.
> > Do we remove the existing security.evm before verifying the portable
> > security.evm?
> 
> After each xattr is written, the security.evm is updated, so, we have
> proper HMAC all the time, this is exactly what we need. We don't need
> remove existing security.evm nor before, nor after if EVM portable
> signature verification is ok.
> 
> If we move all work with EVM portable signature into
> evm_inode_setxattr():
> 
> 1) If we have EVM portable signature verification is ok - no changes in
> EVM xattr are needed. We should only prevent EVM xattr changes by
> __vfs_setxattr_noperm() in vfs_setxattr(). EVM code will do everything
> what we need. We don't need update EVM xattr or something. Everything
> is fine by current EVM/IMA code. This mean, we should have HMAC on
> protected xattrs copy and/or file data copy, lets EVM/IMA care about
> HMAC.

Failing the evm_inode_setxattr() will prevent the portable xattr from
being written.

> 2) If we have EVM portable signature verification is failed - we should
> prevent EVM xattr changes by __vfs_setxattr_noperm() in
> vfs_setxattr(), we should replace EVM xattr by some type of EVM
> signature that:

> a) should not be changed/removed if we not in fix mode;
> b) return INTEGRITY_FAIL in evm_verify_hmac().
> This is all about my previous proposition about EVM_XATTR_PROTECTED.

Unlike IMA, where file signatures are considered immutable, EVM converts
signatures to HMACs.  We probably don't need a new signature format,
just a new type which isn't converted to an HMAC.

For failed portable signatures, we would write security.evm as this new
type, but with an invalid payload. 

> > > What I mean:
> > > 
> > > ./security/integrity/evm/evm_main.c
> > > 
> > > int evm_inode_setxattr(struct dentry *dentry, const char
> > > *xattr_name, const void *xattr_value, size_t xattr_value_len)
> > > {
> > > 	const struct evm_ima_xattr_data *xattr_data = xattr_value;
> > > 
> > > 	if (strcmp(xattr_name, XATTR_NAME_EVM) == 0) {
> > > 		if (!xattr_value_len)
> > > 			return -EINVAL;
> > > 		if (xattr_data->type != EVM_IMA_XATTR_DIGSIG)
> > > 			return -EPERM;
> > > +		if (evm_update_portable_digsig(dentry, xattr_name,
> > > +					       xattr_value,
> > > +					       xattr_value_len))
> > > +			return -ESPECIALERRORCODE;
> > > 	}
> > > 	return evm_protect_xattr(dentry, xattr_name, xattr_value,
> > > 				 xattr_value_len);
> > > }
> > > 
> > > ./fs/xattr.c
> > > 
> > > int
> > > vfs_setxattr(struct dentry *dentry, const char *name, const void
> > > *value, size_t size, int flags)
> > > {
> > > 
> > > ...
> > > 
> > > out:
> > > 	inode_unlock(inode);
> > > +	return (error == -ESPECIALERRORCODE) ? 0 : error;
> > > }
> > > 
> > > But, what error code we could use in this case?
> > > 
> > >   
> > > > Stefan Berger, a colleague, asked if the portable EVM xattr is
> > > > never written to disk or immediately converted to an HMAC, how
> > > > will it be included in an archive?  
> > > 
> > > If EVM is enabled, the only way is patched archiver that will store
> > > EVM portable signature during file packing instead of HMAC (I am
> > > testing such patch now for tar with libimaevm). The idea is -
> > > prevent HMAC to be stored in archive (store EVM portable digsig in
> > > this case), since we can copy only EVM digsig xattr from archive
> > > during unpacking any way.  
> > 
> > The archiver is normally general purpose.  Getting EVM or IMA specific
> > code upstreamed in the archive code will be difficult.
> > 
> > I think adding support for a portable EVM signature is important, but
> > there seems to be a lot of details that need to be resolved first.
> > We've touched on a few of them in this thread.
> 
> Yes, but we could start from kernel, right?

Yes

> As you can see, archivers (at least tar) care about SELinux security
> context and this is not a general purpose. I believe, after some
> time we will have EVM portable version support in some archivers.
> We have a lot of details, but almost all of them connected to
> archiver's work, but not kernel itself.

There's a long history of GNU not accepting any IMA patches.  Doubtful,
but perhaps they'll differentiate between EVM and IMA, and be willing to
accept EVM patches.

Mimi