Re: [PATCH] evm: Correct inode_init_security hooks behaviors

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

Hi, thank for your reply,

On Thu, Oct 20, 2022 at 11:02:07AM -0400, Paul Moore wrote:
> On Thu, Oct 20, 2022 at 9:55 AM Nicolas Bouchinet
> <nicolas.bouchinet@clip-os.org> 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.
> 
> You'll have to forgive me, my connection is poor at the moment and my
> time is limited, but why not simply add some additional checking at
> the top of evm_inode_init_security()? The LSM hook already memset()'s
> the passed lsm_attrs to zero so xattr::{name,value,value_len} should
> all be zero/NULL.  Can you help me understand why that is not
> possible?
> 
> Based on my current understanding, I believe this is something that
> should be addressed at the IMA/EVM level and not necessairly at the
> LSM layer.

The NULL pointer dereference occurs in the `evm_protected_xattr_common()`
function which was originaly called in `evm_inode_init_security()`. I
directly fixed this part at the `evm_inode_init_security()` level.

This patch also addresses other problems which partially occurs at the
`security_inode_init_security()` hook level.
More precisely, based on my understanding, the 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 behavior 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. Thus, only the last security attribute is taken into account by
evm and freed. Checking the NULL pointer at evm level does not solve this
memory leak.

Based on other replies, I inlined the `call_int_hook()` macro directly into the
`security_inode_init_security()` hook.
> 
> > 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`.
> >
> > Fixes the `evm_inode_init_security` function to use `evm_init_hmacs`.
> >
> > The `MAX_LSM_EVM_XATTR` value has been raised to 5 which gives room for
> > SMACK, SELinux, Apparmor, BPF and IMA/EVM security attributes.
> >
> > Changes the default return value of the `inode_init_security` hook
> > definition to `-EOPNOTSUPP`.
> >
> > Changes the hook documentation to match the behavior of the LSMs using
> > it (only xattr->value is initialised with kmalloc and thus is the only
> > one that should be kfreed by the caller).
> >
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr>
> > ---
> >  include/linux/lsm_hook_defs.h       |  2 +-
> >  include/linux/lsm_hooks.h           |  4 ++--
> >  security/integrity/evm/evm.h        |  2 ++
> >  security/integrity/evm/evm_crypto.c | 23 ++++++++++++++++++++++-
> >  security/integrity/evm/evm_main.c   | 11 ++++++-----
> >  security/security.c                 | 29 ++++++++++++++++++++++++++---
> >  6 files changed, 59 insertions(+), 12 deletions(-)
> 
> -- 
> paul-moore.com

Thank for your time,

Best regards,

Nicolas Bouchinet