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 21:38:41 -0500
Mimi Zohar <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.

Exactly my point. We could use some error code returned by
evm_inode_setxattr() in order to prevent the portable xattr from being
written, but return 0 from vfs_setxattr() instead of error code what we
are using in the evm_inode_setxattr(), since we should return "all
is ok" if EVM portable signature verification is ok and we stay
with 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.  
> 
> 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. 

Exactly. We don't need any new signature format for EVM. We don't need
immutable EVM xattr or something like that for sure.

We just need new type that we could use for failed portable signatures
that will not be converted to an HMAC. And sure, it must have invalid
payload, I even propose stay with zero-filled xattr (what I am using
now in patch), since we don't really need anything else for our
purposes.

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

-- 
Best regards,
Mikhail Kurinnoi