[PATCH 0/5] evm: Prepare for moving to the LSM infrastructure
roberto.sassu at huawei.com
Tue Apr 20 16:09:14 UTC 2021
> From: Casey Schaufler [mailto:casey at schaufler-ca.com]
> Sent: Friday, April 16, 2021 11:25 PM
> On 4/16/2021 9:37 AM, Roberto Sassu wrote:
> >> From: Casey Schaufler [mailto:casey at schaufler-ca.com]
> >> Sent: Thursday, April 15, 2021 10:44 PM
> >> On 4/15/2021 3:04 AM, Roberto Sassu wrote:
> >>> This patch set depends on:
> >>> https://lore.kernel.org/linux-integrity/20210409114313.4073-1-
> >> roberto.sassu at huawei.com/
> >>> https://lore.kernel.org/linux-integrity/20210407105252.30721-1-
> >> roberto.sassu at huawei.com/
> >>> One of the challenges that must be tackled to move IMA and EVM to the
> >>> infrastructure is to ensure that EVM is capable to correctly handle
> >>> multiple stacked LSMs providing an xattr at file creation. At the moment,
> >>> there are few issues that would prevent a correct integration. This patch
> >>> set aims at solving them.
> >>> From the LSM infrastructure side, the LSM stacking feature added the
> >>> possibility of registering multiple implementations of the security hooks,
> >>> that are called sequentially whenever someone calls the corresponding
> >>> security hook. However, security_inode_init_security() and
> >>> security_old_inode_init_security() are currently limited to support one
> >>> xattr provided by LSM and one by EVM.
> >> That is correct. At present the only two modules that provide extended
> >> attributes are SELinux and Smack. The LSM infrastructure requires more
> >> change, including change to security_inode_init_security(), before those
> >> modules can be used together.
> > One of the goals of this patch set is to solve the specific problem
> > of security_inode_init_security(), when arbitrary LSMs are added
> > to the LSM infrastructure. Given that some problems have
> > been already identified, and will arise when a new LSM
> > providing an implementation for the inode_init_security hook
> > will be added to the LSM infrastructure, it seems a good idea
> > fixing them. We could discuss about the solution, if there is
> > a better approach.
> >>> In addition, using the call_int_hook() macro causes some issues. According
> >>> to the documentation in include/linux/lsm_hooks.h, it is a legitimate
> >>> that an LSM returns -EOPNOTSUPP when it does not want to provide an
> >>> However, the loop defined in the macro would stop calling subsequent
> >>> if that happens. In the case of security_old_inode_init_security(), using
> >>> the macro would also cause a memory leak due to replacing the *value
> >>> pointer, if multiple LSMs provide an xattr.
> >> As there is no case where there will be multiple providers of hooks for
> >> inode_init_security this isn't an issue.
> > I could skip the patches that are not required to support
> > multiple LSMs registering to the inode_init_security hook
> > and just do the EVM changes (see below for the motivation).
> >>> From EVM side, the first operation to be done is to change the definition
> >>> of evm_inode_init_security() to be compatible with the security hook
> >>> definition. Unfortunately, the current definition does not provide enough
> >>> information for EVM, as it must have visibility of all xattrs provided by
> >>> LSMs to correctly calculate the HMAC. This patch set changes the security
> >>> hook definition by adding the full array of xattr as a parameter.
> >> Why do you want to call evm_inode_init_security() as a regular LSM hook?
> >> Except for the names evm_inode_init_security() and
> >> selinux_inode_init_security()
> >> have nothing in common. They do very different things and require different
> >> data, as comes out in the patches.
> > I thought that it would be more clean if all hooks are registered
> > to the LSM infrastructure. Otherwise, it could happen that some
> > hooks are still executed even if the LSM is not active, from the
> > perspective of the LSM infrastructure.
> > evm_inode_init_security() is still a provider of xattrs, like the
> > other LSMs, just it requires an extra parameter to calculate
> > the HMAC.
> >> There are evm functions that could be implemented as LSM hooks. I don't
> >> this is one of them. There's no point in going overboard.
> > IMA and EVM both use a cache to store the integrity verification,
> > which is currently not managed by the LSM infrastructure but
> > by an ad-hoc mechanism implemented with an rbtree.
> > One of the benefits of defining both IMA and EVM as an LSM
> > is that we can switch from this ad-hoc mechanism to the one
> > implemented for the LSM infrastructure, with a search in
> > constant time. Given that evm_inode_init_security() would
> > update the integrity status (xattrs are good at inode creation
> > time), I would add it as well to the LSM infrastructure.
> > One additional motivation for defining EVM as an LSM is that
> > it would solve one of the EVM limitations that affects its
> > usability: partial copy of xattrs (e.g. by cp and tar) would not
> > work when an HMAC key is loaded because, since EVM in
> > the post set/removexattr hook does not know the status
> > of the last integrity verification, it has to deny the permission
> > to perform the xattr operation, to avoid that the HMAC is
> > calculated on corrupted xattrs. Having the status in the
> > per-inode blob would solve this issue more efficiently than
> > adding a cache for each verified inode in the rbtree.
> > Would you see this as an useful modification?
> Yes, I think that would be worthwhile.
> My biggest objection is to adding a parameter to the hook calls.
> The security_inode_init_security() - security_old_inode_init_security()
> organization looks wrong to me as written.
> There are really three cases here:
> Neither EVM nor initxattrs - taken care of by the "old" variant.
> EVM, but no initxattrs - which doesn't gather the EVM data.
> EVM and initxattrs - which gathers and uses the EVM data.
> The code we have now is cleanest for the least common "old" variant,
> which is used in only two places, reiserfs and ocfs2. I would suggest
> a slightly different approach to getting you what you're after.
> Let's change the hook definition for inode_init_security to take a
> struct xattr * and the fs_data rather than the name/value/length triple.
I added struct xattr **, which is updated by LSMs with the slot they found,
so that security_inode_init_security() checks the validity of the xattr (if an
LSM returned 0, both the name and value must be set). If this is not
necessary, I switch to struct xattr *.
> It will require a temporary struct xattr in security_old_init_inode_security,
> but that's a corner case anyway. The security module specific code would be
> easy to adapt. In the current environment, where there can only be one
> module providing a hook, SELinux or Smack will fill in the xattr and return.
> In the future the modules will have to find an empty slot for their data.
I was thinking to add the code a new LSM should include both in SELinux
and Smack, given that probably people use them as a reference.
> If your evm_init_inode_security() is registered last you will get
> the desired behavior.
> The MAX_LSM_EVM_XATTR value can be easily computed at compile time:
> #define MAX_LSM_EVM_XATTR ( 1 + \
> ( IS_ENABLED(CONFIG_SECURITY_SELINUX) ? 1 : 0 ) + \
> ( IS_ENABLED(CONFIG_SECURITY_SMACK) ? 1 : 0) )
> Yes, you'll waste stack if only one of the modules is active.
> On the other hand, if you only compile in one the value will be
> perfect and you'll avoid allocation and associated headaches.
Given that there is allocation for the value anyway, would it be
a big problem to allocate the xattr array too? The advantage
would be to make the code ready for development of new LSMs.
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli
> > Thanks
> > Roberto
> > HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
> > Managing Director: Li Peng, Li Jian, Shi Yanli
> >>> Secondly, EVM must know how many elements are in the xattr array. It
> >> seems
> >>> that it is not necessary to add another parameter, as all filesystems
> >>> define an initxattr function, expect that the last element of the array
> >> is
> >>> one with the name field set to NULL. EVM reuses the same assumption.
> >>> This patch set has been tested by introducing several instances of a
> >>> TestLSM (some providing an xattr, some not, one with a wrong
> >> implementation
> >>> to see how the LSM infrastructure handles it). The patch is not included
> >>> in this set but it is available here:
> >> 8e65d697f956db
> >>> The test, added to ima-evm-utils, is available here:
> >>> https://github.com/robertosassu/ima-evm-utils/blob/evm-multiple-lsms-
> >> devel-v1/tests/evm_multiple_lsms.test
> >>> The test takes a UML kernel built by Travis and launches it several times,
> >>> each time with a different combination of LSMs. After boot, it first checks
> >>> that there is an xattr for each LSM providing it, and then calculates
> >>> HMAC in user space and compares it with the HMAC calculated by EVM in
> >>> kernel space.
> >>> A test report can be obtained here:
> >>> https://www.travis-ci.com/github/robertosassu/ima-evm-
> >> utils/jobs/498699540
> >>> Lastly, running the test on reiserfs to check
> >>> security_old_inode_init_security(), some issues have been discovered:
> >>> free of xattr->name which is not correct after commit 9548906b2bb7
> >>> Constify ->name member of "struct xattr"'), and a misalignment with
> >>> security_inode_init_security() (the old version expects the full xattr name
> >>> with the security. prefix, the new version just the suffix). The last
> >>> has not been fixed yet.
> >>> Roberto Sassu (5):
> >>> xattr: Complete constify ->name member of "struct xattr"
> >>> security: Support multiple LSMs implementing the inode_init_security
> >>> hook
> >>> security: Pass xattrs allocated by LSMs to the inode_init_security
> >>> hook
> >>> evm: Align evm_inode_init_security() definition with LSM
> >>> infrastructure
> >>> evm: Support multiple LSMs providing an xattr
> >>> fs/reiserfs/xattr_security.c | 2 -
> >>> include/linux/evm.h | 21 ++++---
> >>> include/linux/lsm_hook_defs.h | 2 +-
> >>> include/linux/lsm_hooks.h | 5 +-
> >>> security/integrity/evm/evm.h | 2 +
> >>> security/integrity/evm/evm_crypto.c | 9 ++-
> >>> security/integrity/evm/evm_main.c | 35 +++++++----
> >>> security/security.c | 95 +++++++++++++++++++++++------
> >>> security/selinux/hooks.c | 3 +-
> >>> security/smack/smack_lsm.c | 4 +-
> >>> 10 files changed, 135 insertions(+), 43 deletions(-)
More information about the Linux-security-module-archive