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

[Mikhail K. <vie...@vi...>]

В Tue, 10 Jan 2017 06:33:29 -0500
Mimi Zohar <zo...@li...> пишет:

> On Tue, 2017-01-10 at 07:25 +0300, Mikhail Kurinnoi wrote:
> > В Mon, 09 Jan 2017 21:38:41 -0500
> > Mimi Zohar <zo...@li...> пишет:
> >      
> > > > > 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.  
> 
> Agreed this works.  The LSM hooks expect 0 on success and non zero on
> failure.  Here we're inverting that.   Please introduce this concept
> of a valid, non-zero return code from evm_inode_setxattr() in
> security_inode_setxattr() as a separate patch.

Ok.

> > > > 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.  
> 
> "immutable" in this context just means that the security.evm xattr can
> not be replaced with an HMAC.
> 
> > 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.  
> 
> Others have requested an immutable file signature type.  The only
> difference between this signature type and the original one, is that
> this type is never replaced with an HMAC.

I think, we could use feature we already have. For now
EVM_IMA_XATTR_DIGSIG in EVM have "versions", this parameter stored
in xattr_data[1] (xattr_data[0] - signature type).
We have version 2 for standard, version 3 used for portable (since we
already use it in evm-ima-util code), version 4 could be "immutable".

In this way, we could sign file with:
1) standard digsig (version 2, that will be HMACed on first access);
2) "portable" digsig (version 3, that could be stored depending of
verification status as HMAC or "immutable" signature with invalid
payload);
3) "immutable" digsig (version 4, same as version 2, but never replaced
with HMAC).


> We've defined the "portable" signature to not include certain fields
> (eg. i_version, i_ino).   Instead of removing them, would it make
> sense to zero out these fields.  Then converting a "portable"
> signature to an "immutable" one is a matter of changing just the file
> signature type. With these fields zeroed out, the normal file
> signature verification will fail.

I think, no reason store previous failed "portable" signature into xattr
at all. Sure, it will have verification fail, but, why we need store
100+ bytes into inode xattr with useless digsig if we need only 2 bytes?
Don't forget, this is an exception, we need invalid payload and even 1
byte will be enough in this case. We never should check this xattr
data, all what we check - xattr type, we will fail signature
verification right after xattr type check since we have xattr_len <= 2
(for example), we don't even need call evm_calc_hash() in this case
before integrity_digsig_verify() return fail. Also, no reason do any
offline modification with signature type at all, since we don't have
data stored in xattr.


> Assuming you agree with an "immutable" file signature type, the first
> patch in this series should introduce the concept.

Agreed, we need introduce "immutable" file signature type first, since
we need it for portable signature work.


-- 
Best regards,
Mikhail Kurinnoi