Re: [PATCH] evm: Correct inode_init_security hooks behaviors

[Nicolas Bouchinet <nicolas.bouchinet@clip-os.org>]

Hi Mimi,

Thanks for the IMA/EVM project which I enjoy very much.

On Thu, Oct 20, 2022 at 03:51:38PM -0400, Mimi Zohar wrote:
> On Thu, 2022-10-20 at 15:55 +0200, Nicolas Bouchinet wrote:
> > From: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
> > 
> > Fixes a NULL pointer dereference occuring in the
> > `evm_protected_xattr_common` function of the EVM LSM. The bug is
> > triggered if a `inode_init_security` hook returns 0 without initializing
> > the given `struct xattr` fields (which is the case of BPF) and if no
> > other LSM overrides thoses fields after. This also leads to memory
> > leaks.
> > 
> > Adds a `call_int_hook_xattr` macro that fetches and feed the
> > `new_xattrs` array with every called hook xattr values.
> > 
> > Adds a `evm_init_hmacs` function which init the EVM hmac using every
> > entry of the array contrary to `evm_init_hmac`.
>   
> Only EVM portable digital signatures include all of the protected
> xattrs.   Refer to commit 8c7a703ec978 ("evm: Verify portable
> signatures against all protected xattrs").
> 
Sorry, maybe I was not clear enough, the proposed patch does not change the
set of the protected security xattrs initialized by the LSMs and processed by EVM.

As I explained to Paul, based on my understanding, the `security_inode_init_security()`
hook is supposed to initialize every hooked LSM security xattr and next,
if evm is enabled, protect them using a HMAC algorithm.
However, in the current implementation, the use of the `call_int_hook()` macro by
`security_inode_init_security()` overwrites the previously initialized xattr for
each iteration of the `hlist_for_each_entry()` loop.

I have noticed that more than one LSM may initialize a security xattr at a time,
eg. SELinux + BPF.

IMHO my supplementary `evm_init_hmacs()` function name is a bit confusing, I would
enjoy if you have a better proposition. Note that `evm_init_hmacs()` have the same
behavior as `evm_init_hmac()` if only one security xattr is given as a parameter.

> > 
> > Fixes the `evm_inode_init_security` function to use `evm_init_hmacs`.
> 
> Won't this break existing EVM hmac usage?
I might be wrong, but as far as I understand it, the only working condition for
EVM now is when only one security xattr is involved, otherwise there will have
a mismatch between the initialization and the verification.
Indeed, the verification takes into account every security xattr written in its
refering dentry.

> 
> -- 
> thanks,
> 
> Mimi
> 

Thanks for your time,
Best regards,

Nicolas Bouchinet