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

SourceForge Headquarters 1320 Columbia Street Suite 310 San Diego, CA 92101 +1 (858) 422-6466

В 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.

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.

> > 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?
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.

-- 
Best regards,
Mikhail Kurinnoi