Re: [PATCH v8 0/6] evm: Do HMAC of multiple per LSM xattrs for new inodes

From: Roberto Sassu
Date: Tue Mar 14 2023 - 04:24:57 EST


On Tue, 2023-03-14 at 09:17 +0100, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
>
> One of the major goals of LSM stacking is to run multiple LSMs side by side
> without interfering with each other. The ultimate decision will depend on
> individual LSM decision.
>
> Several changes need to be made to the LSM infrastructure to be able to
> support that. This patch set tackles one of them: gives to each LSM the
> ability to specify one or multiple xattrs to be set at inode creation
> time and, at the same time, gives to EVM the ability to access all those
> xattrs and calculate the HMAC on them.
>
> The first problem that this patch set addresses is to make the
> inode_init_security hook definition suitable to use with EVM which, unlike
> other LSMs, needs to have visibility of all xattrs and not only the one
> that the LSM infrastructure passes to the LSM to be set.
>
> The solution is to replace in the inode_init_security definition the
> name/value/len parameters with the beginning of the array containing all
> xattrs set by LSMs. Due to security_old_inode_init_security() API
> limitation of setting only one xattr, it has been dropped and the remaining
> users, ocfs2 and reiserfs, switch to security_inode_init_security().
> However, due to the complexity of the changes required to fully exploit the
> ability of security_inode_init_security() to set multiple xattrs, those
> users can still set only one xattr (the first set in the xattr array) where
> previously they called security_old_inode_init_security().
>
> Furthermore, while EVM is invoked unlike before, its xattr will not be set
> as it would not be the first set in the xattr array, or if it is the first,
> there would not be protected xattrs to calculate the HMAC on.
>
> The second problem this patch set addresses is the limitation of the
> call_int_hook() of stopping the loop when the return value from a hook
> implementation is not zero. Unfortunately, for the inode_init_security hook
> it is a legitimate case to return -EOPNOTSUPP, but this would not
> necessarily mean that there is an error to report to the LSM infrastructure
> but just that an LSM does not will to set an xattr. Other LSMs should be
> still consulted as well.
>
> The solution for this specific case is to replace the call_int_hook() with
> the loop itself, so that -EOPNOTSUPP can be ignored.
>
> Next, this patch set removes the limitation of creating only two xattrs,
> one by an active LSM and another by EVM. This patch set extends the
> reservation mechanism of the LSM infrastructure, to allow each LSM to
> request one or multiple xattrs. While this could potentially lead to
> reaching the filesystem limits of number/size of the xattrs, it seems not
> an issue that need to be solved by the LSM infrastructure but by the
> filesystems themselves. Currently, if the limit is reached, the only
> workaround would be to use fewer LSMs.
>
> The reservation mechanism concept makes it very easy for LSMs to position
> themselves correctly in the xattr array, as the LSM infrastructure at
> initialization time changes the number of xattrs requested by each LSM with
> an offset. LSMs can just take that offset as the starting index in the
> xattr array and fill the next slots depending on how many xattrs they
> requested.
>
> However, while this concept is intuitive, it needs extra care. While for
> security blobs (the main reason of the reservation mechanism) it is not
> relevant for an LSM if other LSMs filled their portion, it matters for
> xattrs, as both EVM and initxattrs() callbacks scan the entire array until
> a terminator (xattr with NULL name). If an LSM did not provide an xattr,
> which could happen if it is loaded but not initialized, consumers of the
> xattr array would stop prematurely.
>
> This patch set avoids this problem by compacting the xattr array each time
> after an LSM executed its implementation of the inode_init_security hook.
> It needs to be done after each LSM, and not after all, since there might be
> LSMs scanning that xattr array too. Compacting the array after all LSMs
> would be too late.
>
> Finally, this patch set modifies the evm_inode_init_security() definition
> to be compatible with the inode_init_security hook definition and adds
> support for scanning the whole xattr array and for calculating the HMAC
> on all xattrs provided by LSMs.
>
> 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, one providing multiple xattrs
> and another providing an xattr but in a disabled state). The patch is not
> included in this set but it is available here:
>
> https://github.com/robertosassu/linux/commit/311b83a98757915cc3ccb363f545578e7c38df54
>
> The test, added to ima-evm-utils, is available here:
>
> https://github.com/robertosassu/ima-evm-utils/blob/evm-multiple-lsms-v8-devel-v2/tests/evm_multiple_lsms.test
>
> The test takes a UML kernel built by Github Actions and launches it several
> times, each time with a different combination of LSMs and filesystems (ext4,
> reiserfs, ocfs2). After boot, it first checks that there is an xattr for each
> LSM providing it (for reiserfs and ocfs2 just the first LSM), and then (for
> ext4) calculates the HMAC in user space and compares it with the HMAC
> calculated by EVM in kernel space.
>
> A test report can be obtained here:
>
> https://github.com/robertosassu/ima-evm-utils/actions/runs/4403223954/jobs/7722101662
>
> The patch set has been tested with both the SElinux and Smack test suites.
> Below, there is the summary of the test results:
>
> SELinux Test Suite result (without patches):
> All tests successful.
> Files=76, Tests=1343, 255 wallclock secs ( 0.51 usr 0.19 sys + 6.30 cusr 58.15 csys = 65.15 CPU)
> Result: PASS
>
> SELinux Test Suite result (with patches):
> All tests successful.
> Files=76, Tests=1343, 258 wallclock secs ( 0.49 usr 0.21 sys + 6.46 cusr 59.27 csys = 66.43 CPU)
> Result: PASS

Side note:

https://github.com/SELinuxProject/selinux-testsuite/blob/master/tests/mmap/test#L51

This test fails if CONFIG_LSM_MMAP_MIN_ADDR=0

Roberto

> Smack Test Suite result (without patches):
> 95 Passed, 0 Failed, 100% Success rate
>
> Smack Test Suite result (with patches):
> 95 Passed, 0 Failed, 100% Success rate
>
> Changelog
>
> v7:
> - Add a patch dependency comment in patch 1 (suggested by Mimi)
> - Restore check of -EOPNOTSUPP status in ocfs2_mknod() and ocfs2_symlink()
> (reported by Mimi)
> - Add explanation in evm_inode_init_security() why walking through the
> xattrs array is safe (suggested by Mimi)
> - Document the lbs_xattr field of struct lsm_blob_sizes (suggested by
> Casey)
> - Move documentation changes of the inode_init_security hook to security.c,
> after LSM documentation reorganization by Paul
> - Use attributes in plural form in the description of the xattrs parameter
> of smack_inode_init_security()
> - Check xattr name instead of xattr value in evm_inode_init_security(),
> for consistency with evm_init_hmac(); equivalent, since
> security_check_compact_filled_xattrs() rejects xattrs with xattr name
> NULL and value not NULL, and viceversa
>
> v6:
> - Add a comment in Smack to introduce its xattrs (suggested by Casey)
> - Document the overloaded meaning of -EOPNOTSUPP in
> security_inode_init_security() (suggested by Mimi)
>
> v5:
> - Modify the cover letter to explain that the goal of this patch set is
> supporting multiple per LSM xattrs in EVM, and not moving IMA and EVM to
> the LSM infrastructure (suggested by Mimi)
> - Remove references in the patches description about moving IMA and EVM
> to the LSM infrastructure (suggested by Mimi)
> - Explain that the additional EVM invocation due to the switch to
> security_inode_init_security() will not cause the EVM xattr to be added
> (suggested by Mimi)
>
> v4:
> - Remove patch to call reiserfs_security_free(), already queued
> - Switch ocfs2 and reiserfs to security_inode_init_security() (suggested by
> Mimi)
> - Remove security_old_inode_init_security() (suggested by Paul)
> - Rename security_check_compact_xattrs() to
> security_check_compact_filled_xattrs() and add function description
> (suggested by Mimi)
> - Rename checked_xattrs parameter of security_check_compact_filled_xattrs()
> to num_filled_xattrs (suggested by Mimi)
> - Rename cur_xattrs variable in security_inode_init_security() to
> num_filled_xattrs (suggested by Mimi)
>
> v3:
> - Don't free the xattr name in reiserfs_security_free()
> - Don't include fs_data parameter in inode_init_security hook
> - Don't change evm_inode_init_security(), as it will be removed if EVM is
> stacked
> - Fix inode_init_security hook documentation
> - Drop lsm_find_xattr_slot(), use simple xattr reservation mechanism and
> introduce security_check_compact_xattrs() to compact the xattr array
> - Don't allocate xattr array if LSMs didn't reserve any xattr
> - Return zero if initxattrs() is not provided to
> security_inode_init_security(), -EOPNOTSUPP if value is not provided to
> security_old_inode_init_security()
> - Request LSMs to fill xattrs if only value (not the triple) is provided to
> security_old_inode_init_security(), to avoid unnecessary memory
> allocation
>
> v2:
> - rewrite selinux_old_inode_init_security() to use
> security_inode_init_security()
> - add lbs_xattr field to lsm_blob_sizes structure, to give the ability to
> LSMs to reserve slots in the xattr array (suggested by Casey)
> - add new parameter base_slot to inode_init_security hook definition
>
> v1:
> - add calls to reiserfs_security_free() and initialize sec->value to NULL
> (suggested by Tetsuo and Mimi)
> - change definition of inode_init_security hook, replace the name, value
> and len triple with the xattr array (suggested by Casey)
> - introduce lsm_find_xattr_slot() helper for LSMs to find an unused slot in
> the passed xattr array
>
> Roberto Sassu (6):
> reiserfs: Switch to security_inode_init_security()
> ocfs2: Switch to security_inode_init_security()
> security: Remove security_old_inode_init_security()
> security: Allow all LSMs to provide xattrs for inode_init_security
> hook
> evm: Align evm_inode_init_security() definition with LSM
> infrastructure
> evm: Support multiple LSMs providing an xattr
>
> fs/ocfs2/namei.c | 2 +
> fs/ocfs2/xattr.c | 30 ++++++-
> fs/reiserfs/xattr_security.c | 23 +++--
> include/linux/evm.h | 12 +--
> include/linux/lsm_hook_defs.h | 3 +-
> include/linux/lsm_hooks.h | 1 +
> include/linux/security.h | 12 ---
> security/integrity/evm/evm.h | 2 +
> security/integrity/evm/evm_crypto.c | 9 +-
> security/integrity/evm/evm_main.c | 33 +++++--
> security/security.c | 131 +++++++++++++++++++++-------
> security/selinux/hooks.c | 19 ++--
> security/smack/smack_lsm.c | 33 ++++---
> 13 files changed, 224 insertions(+), 86 deletions(-)
>