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

[Mimi Z. <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 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?

Currently, after each xattr is written, the security.evm is updated.  Do
we remove the existing security.evm before verifying the portable
security.evm?

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

Mimi

> But, we also have security issue here. Since EVM xattr could be removed
> manually from archive, and during unpacking new file will have HMAC, we
> must have archiver patched on unpacking too (on dest computer), that
> will count on protected xattrs and provide some fake EVM portable
> signature if we have protected xattrs but don't have EVM xattr itself,
> or we could have security fail here. Probably, this should be controlled
> by archiver flags during unpacking (demand EVM xattr or not).