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@xxxxxxxxxxx> wrote:
> >
> > From: Nicolas Bouchinet <nicolas.bouchinet@xxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx
> > Signed-off-by: Nicolas Bouchinet <nicolas.bouchinet@xxxxxxxxxxx>
> > ---
> > 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
