Re: [PATCH] evm: Correct inode_init_security hooks behaviors

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

Hi !

On Mon, Oct 24, 2022 at 12:35:52PM -0400, Mimi Zohar wrote:
> Hi Nicolas,
> 
> On Fri, 2022-10-21 at 15:47 +0200, Nicolas Bouchinet wrote:
> > 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.
> 
> Does BPF have a security xattr and, if so, does it need to be
> protected?   It would need to be defined and included in the list of
> evm_config_xattrnames[].  If it doesn't define a security bpf xattr,
> then bpf should not be on the security_inode_init_security() hook.  (I
> assume Roberto's patch is going in this direction.)
> 
> Before the EVM hmac is updated, the existing EVM hmac is verified.  I
> would be concerned if bpf defined a protected security xattr.   Could
> the same guarantees, that security.evm isn't updated without first
> being verified, be enforced with bpf?
> 

I am not that comfortable with BPF programs but based on what Alexei Starovoitov pointed out here
https://lore.kernel.org/bpf/20221021164626.3729012-1-roberto.sassu@huaweicloud.com
BPF should not be able to write into the xattrs pointers. And thus shouldn't be included
in `evm_config_xattrnames[]`.
> > 
> > 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.
> 
> I'm missing something here.  As evm_inode_init_security() is the only
> caller of evm_init_hmac(), why is a new function defined instead of
> updating the existing one?   If there is a valid reason, then one
> function should be a wrapper for the other.
> 

There is no valid reasons, I was just unsure about replacing existing functions, will update it.
> > > > 
> > > > 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.
> 
> Agreed, independently as to whether BPF defines a security xattr, if
> two LSMs initialize security xattrs, then this change is needed.  Are
> there any other examples?

I think that in its current state the kernel cannot load two LSM capable of xattr
initialization as they are all defined with the `LSM_FLAG_EXCLUSIVE` flag set.
But I may be unaware of other LSM in development stage.
> 
> (nit: I understand the line size has generally been relaxed, but for
> IMA/EVM I would prefer it to be remain as 80 chars.)
> 

No problem, will change it !
> Mimi
> 

I'll take time to run few tests with BPF and send a patch v3 with new changes.

Regards,

Nicolas Bouchinet