Extend the interoperability with IMA, to give wider flexibility for the implementation of integrity-focused LSMs based on eBPF.
Patch 1 fixes some style issues.
Patches 2-6 give the ability to eBPF-based LSMs to take advantage of the measurement capability of IMA without needing to setup a policy in IMA (those LSMs might implement the policy capability themselves).
Patches 7-9 allow eBPF-based LSMs to evaluate files read by the kernel.
Changelog
v2: - Add better description to patch 1 (suggested by Shuah) - Recalculate digest if it is not fresh (when IMA_COLLECTED flag not set) - Move declaration of bpf_ima_file_hash() at the end (suggested by Yonghong) - Add tests to check if the digest has been recalculated - Add deny test for bpf_kernel_read_file() - Add description to tests
v1: - Modify ima_file_hash() only and allow the usage of the function with the modified behavior by eBPF-based LSMs through the new function bpf_ima_file_hash() (suggested by Mimi) - Make bpf_lsm_kernel_read_file() sleepable so that bpf_ima_inode_hash() and bpf_ima_file_hash() can be called inside the implementation of eBPF-based LSMs for this hook
Roberto Sassu (9): ima: Fix documentation-related warnings in ima_main.c ima: Always return a file measurement in ima_file_hash() bpf-lsm: Introduce new helper bpf_ima_file_hash() selftests/bpf: Move sample generation code to ima_test_common() selftests/bpf: Add test for bpf_ima_file_hash() selftests/bpf: Check if the digest is refreshed after a file write bpf-lsm: Make bpf_lsm_kernel_read_file() as sleepable selftests/bpf: Add test for bpf_lsm_kernel_read_file() selftests/bpf: Check that bpf_kernel_read_file() denies reading IMA policy
include/uapi/linux/bpf.h | 11 ++ kernel/bpf/bpf_lsm.c | 21 +++ security/integrity/ima/ima_main.c | 57 ++++--- tools/include/uapi/linux/bpf.h | 11 ++ tools/testing/selftests/bpf/ima_setup.sh | 35 +++- .../selftests/bpf/prog_tests/test_ima.c | 149 +++++++++++++++++- tools/testing/selftests/bpf/progs/ima.c | 66 +++++++- 7 files changed, 321 insertions(+), 29 deletions(-)
Fix the following warnings in ima_main.c, displayed with W=n make argument:
security/integrity/ima/ima_main.c:432: warning: Function parameter or member 'vma' not described in 'ima_file_mprotect' security/integrity/ima/ima_main.c:636: warning: Function parameter or member 'inode' not described in 'ima_post_create_tmpfile' security/integrity/ima/ima_main.c:636: warning: Excess function parameter 'file' description in 'ima_post_create_tmpfile' security/integrity/ima/ima_main.c:843: warning: Function parameter or member 'load_id' not described in 'ima_post_load_data' security/integrity/ima/ima_main.c:843: warning: Excess function parameter 'id' description in 'ima_post_load_data'
Also, fix some style issues in the description of ima_post_create_tmpfile() and ima_post_path_mknod().
Reviewed-by: Shuah Khan skhan@linuxfoundation.org Signed-off-by: Roberto Sassu roberto.sassu@huawei.com --- security/integrity/ima/ima_main.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 8c6e4514d494..946ba8a12eab 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -418,6 +418,7 @@ int ima_file_mmap(struct file *file, unsigned long prot)
/** * ima_file_mprotect - based on policy, limit mprotect change + * @vma: vm_area_struct protection is set to * @prot: contains the protection that will be applied by the kernel. * * Files can be mmap'ed read/write and later changed to execute to circumvent @@ -610,8 +611,8 @@ EXPORT_SYMBOL_GPL(ima_inode_hash);
/** * ima_post_create_tmpfile - mark newly created tmpfile as new - * @mnt_userns: user namespace of the mount the inode was found from - * @file : newly created tmpfile + * @mnt_userns: user namespace of the mount the inode was found from + * @inode: inode of the newly created tmpfile * * No measuring, appraising or auditing of newly created tmpfiles is needed. * Skip calling process_measurement(), but indicate which newly, created @@ -643,7 +644,7 @@ void ima_post_create_tmpfile(struct user_namespace *mnt_userns,
/** * ima_post_path_mknod - mark as a new inode - * @mnt_userns: user namespace of the mount the inode was found from + * @mnt_userns: user namespace of the mount the inode was found from * @dentry: newly created dentry * * Mark files created via the mknodat syscall as new, so that the @@ -814,8 +815,8 @@ int ima_load_data(enum kernel_load_data_id id, bool contents) * ima_post_load_data - appraise decision based on policy * @buf: pointer to in memory file contents * @size: size of in memory file contents - * @id: kernel load data caller identifier - * @description: @id-specific description of contents + * @load_id: kernel load data caller identifier + * @description: @load_id-specific description of contents * * Measure/appraise/audit in memory buffer based on policy. Policy rules * are written in terms of a policy identifier.
On Wed, 2022-03-02 at 12:13 +0100, Roberto Sassu wrote:
Fix the following warnings in ima_main.c, displayed with W=n make argument:
security/integrity/ima/ima_main.c:432: warning: Function parameter or member 'vma' not described in 'ima_file_mprotect' security/integrity/ima/ima_main.c:636: warning: Function parameter or member 'inode' not described in 'ima_post_create_tmpfile' security/integrity/ima/ima_main.c:636: warning: Excess function parameter 'file' description in 'ima_post_create_tmpfile' security/integrity/ima/ima_main.c:843: warning: Function parameter or member 'load_id' not described in 'ima_post_load_data' security/integrity/ima/ima_main.c:843: warning: Excess function parameter 'id' description in 'ima_post_load_data'
Also, fix some style issues in the description of ima_post_create_tmpfile() and ima_post_path_mknod().
Reviewed-by: Shuah Khan skhan@linuxfoundation.org Signed-off-by: Roberto Sassu roberto.sassu@huawei.com
Reviewed-by: Mimi Zohar zohar@linux.ibm.com
__ima_inode_hash() checks if a digest has been already calculated by looking for the integrity_iint_cache structure associated to the passed inode.
Users of ima_file_hash() (e.g. eBPF) might be interested in obtaining the information without having to setup an IMA policy so that the digest is always available at the time they call this function.
In addition, they likely expect the digest to be fresh, e.g. recalculated by IMA after a file write. Although getting the digest from the bprm_committed_creds hook (as in the eBPF test) ensures that the digest is fresh, as the IMA hook is executed before that hook, this is not always the case (e.g. for the mmap_file hook).
Call ima_collect_measurement() in __ima_inode_hash(), if the file descriptor is available (passed by ima_file_hash()) and the digest is not available/not fresh, and store the file measurement in a temporary integrity_iint_cache structure.
This change does not cause memory usage increase, due to using the temporary integrity_iint_cache structure, and due to freeing the ima_digest_data structure inside integrity_iint_cache before exiting from __ima_inode_hash().
For compatibility reasons, the behavior of ima_inode_hash() remains unchanged.
Signed-off-by: Roberto Sassu roberto.sassu@huawei.com --- security/integrity/ima/ima_main.c | 46 ++++++++++++++++++++++--------- 1 file changed, 33 insertions(+), 13 deletions(-)
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 946ba8a12eab..ed1a82f1def3 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -520,20 +520,38 @@ int ima_file_check(struct file *file, int mask) } EXPORT_SYMBOL_GPL(ima_file_check);
-static int __ima_inode_hash(struct inode *inode, char *buf, size_t buf_size) +static int __ima_inode_hash(struct inode *inode, struct file *file, char *buf, + size_t buf_size) { - struct integrity_iint_cache *iint; - int hash_algo; + struct integrity_iint_cache *iint = NULL, tmp_iint; + int rc, hash_algo;
- if (!ima_policy_flag) - return -EOPNOTSUPP; + if (ima_policy_flag) { + iint = integrity_iint_find(inode); + if (iint) + mutex_lock(&iint->mutex); + } + + if ((!iint || !(iint->flags & IMA_COLLECTED)) && file) { + if (iint) + mutex_unlock(&iint->mutex); + + memset(&tmp_iint, 0, sizeof(tmp_iint)); + tmp_iint.inode = inode; + mutex_init(&tmp_iint.mutex); + + rc = ima_collect_measurement(&tmp_iint, file, NULL, 0, + ima_hash_algo, NULL); + if (rc < 0) + return -EOPNOTSUPP; + + iint = &tmp_iint; + mutex_lock(&iint->mutex); + }
- iint = integrity_iint_find(inode); if (!iint) return -EOPNOTSUPP;
- mutex_lock(&iint->mutex); - /* * ima_file_hash can be called when ima_collect_measurement has still * not been called, we might not always have a hash. @@ -552,12 +570,14 @@ static int __ima_inode_hash(struct inode *inode, char *buf, size_t buf_size) hash_algo = iint->ima_hash->algo; mutex_unlock(&iint->mutex);
+ if (iint == &tmp_iint) + kfree(iint->ima_hash); + return hash_algo; }
/** - * ima_file_hash - return the stored measurement if a file has been hashed and - * is in the iint cache. + * ima_file_hash - return a measurement of the file * @file: pointer to the file * @buf: buffer in which to store the hash * @buf_size: length of the buffer @@ -570,7 +590,7 @@ static int __ima_inode_hash(struct inode *inode, char *buf, size_t buf_size) * The file hash returned is based on the entire file, including the appended * signature. * - * If IMA is disabled or if no measurement is available, return -EOPNOTSUPP. + * If the measurement cannot be performed, return -EOPNOTSUPP. * If the parameters are incorrect, return -EINVAL. */ int ima_file_hash(struct file *file, char *buf, size_t buf_size) @@ -578,7 +598,7 @@ int ima_file_hash(struct file *file, char *buf, size_t buf_size) if (!file) return -EINVAL;
- return __ima_inode_hash(file_inode(file), buf, buf_size); + return __ima_inode_hash(file_inode(file), file, buf, buf_size); } EXPORT_SYMBOL_GPL(ima_file_hash);
@@ -605,7 +625,7 @@ int ima_inode_hash(struct inode *inode, char *buf, size_t buf_size) if (!inode) return -EINVAL;
- return __ima_inode_hash(inode, buf, buf_size); + return __ima_inode_hash(inode, NULL, buf, buf_size); } EXPORT_SYMBOL_GPL(ima_inode_hash);
On Wed, 2022-03-02 at 12:13 +0100, Roberto Sassu wrote:
__ima_inode_hash() checks if a digest has been already calculated by looking for the integrity_iint_cache structure associated to the passed inode.
Users of ima_file_hash() (e.g. eBPF) might be interested in obtaining the information without having to setup an IMA policy so that the digest is always available at the time they call this function.
In addition, they likely expect the digest to be fresh, e.g. recalculated by IMA after a file write. Although getting the digest from the bprm_committed_creds hook (as in the eBPF test) ensures that the digest is fresh, as the IMA hook is executed before that hook, this is not always the case (e.g. for the mmap_file hook).
Call ima_collect_measurement() in __ima_inode_hash(), if the file descriptor is available (passed by ima_file_hash()) and the digest is not available/not fresh, and store the file measurement in a temporary integrity_iint_cache structure.
This change does not cause memory usage increase, due to using the temporary integrity_iint_cache structure, and due to freeing the ima_digest_data structure inside integrity_iint_cache before exiting from __ima_inode_hash().
For compatibility reasons, the behavior of ima_inode_hash() remains unchanged.
Signed-off-by: Roberto Sassu roberto.sassu@huawei.com
The patch itself is fine, but with great hesitancy due to the existing eBPF integrity gaps and how these functions are planned to be used,
Reviewed-by: Mimi Zohar zohar@linux.ibm.com
ima_file_hash() has been modified to calculate the measurement of a file on demand, if it has not been already performed by IMA or the measurement is not fresh. For compatibility reasons, ima_inode_hash() remains unchanged.
Keep the same approach in eBPF and introduce the new helper bpf_ima_file_hash() to take advantage of the modified behavior of ima_file_hash().
Signed-off-by: Roberto Sassu roberto.sassu@huawei.com --- include/uapi/linux/bpf.h | 11 +++++++++++ kernel/bpf/bpf_lsm.c | 20 ++++++++++++++++++++ tools/include/uapi/linux/bpf.h | 11 +++++++++++ 3 files changed, 42 insertions(+)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index b0383d371b9a..87d996e954d4 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -5018,6 +5018,16 @@ union bpf_attr { * * Return * The number of arguments of the traced function. + * + * long bpf_ima_file_hash(struct file *file, void *dst, u32 size) + * Description + * Returns a calculated IMA hash of the *file*. + * If the hash is larger than *size*, then only *size* + * bytes will be copied to *dst* + * Return + * The **hash_algo** is returned on success, + * **-EOPNOTSUP** if the hash calculation failed or **-EINVAL** if + * invalid arguments are passed. */ #define __BPF_FUNC_MAPPER(FN) \ FN(unspec), \ @@ -5206,6 +5216,7 @@ union bpf_attr { FN(get_func_arg), \ FN(get_func_ret), \ FN(get_func_arg_cnt), \ + FN(ima_file_hash), \ /* */
/* integer value in 'imm' field of BPF_CALL instruction selects which helper diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c index 9e4ecc990647..e8d27af5bbcc 100644 --- a/kernel/bpf/bpf_lsm.c +++ b/kernel/bpf/bpf_lsm.c @@ -99,6 +99,24 @@ static const struct bpf_func_proto bpf_ima_inode_hash_proto = { .allowed = bpf_ima_inode_hash_allowed, };
+BPF_CALL_3(bpf_ima_file_hash, struct file *, file, void *, dst, u32, size) +{ + return ima_file_hash(file, dst, size); +} + +BTF_ID_LIST_SINGLE(bpf_ima_file_hash_btf_ids, struct, file) + +static const struct bpf_func_proto bpf_ima_file_hash_proto = { + .func = bpf_ima_file_hash, + .gpl_only = false, + .ret_type = RET_INTEGER, + .arg1_type = ARG_PTR_TO_BTF_ID, + .arg1_btf_id = &bpf_ima_file_hash_btf_ids[0], + .arg2_type = ARG_PTR_TO_UNINIT_MEM, + .arg3_type = ARG_CONST_SIZE, + .allowed = bpf_ima_inode_hash_allowed, +}; + static const struct bpf_func_proto * bpf_lsm_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) { @@ -121,6 +139,8 @@ bpf_lsm_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) return &bpf_bprm_opts_set_proto; case BPF_FUNC_ima_inode_hash: return prog->aux->sleepable ? &bpf_ima_inode_hash_proto : NULL; + case BPF_FUNC_ima_file_hash: + return prog->aux->sleepable ? &bpf_ima_file_hash_proto : NULL; default: return tracing_prog_func_proto(func_id, prog); } diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h index b0383d371b9a..87d996e954d4 100644 --- a/tools/include/uapi/linux/bpf.h +++ b/tools/include/uapi/linux/bpf.h @@ -5018,6 +5018,16 @@ union bpf_attr { * * Return * The number of arguments of the traced function. + * + * long bpf_ima_file_hash(struct file *file, void *dst, u32 size) + * Description + * Returns a calculated IMA hash of the *file*. + * If the hash is larger than *size*, then only *size* + * bytes will be copied to *dst* + * Return + * The **hash_algo** is returned on success, + * **-EOPNOTSUP** if the hash calculation failed or **-EINVAL** if + * invalid arguments are passed. */ #define __BPF_FUNC_MAPPER(FN) \ FN(unspec), \ @@ -5206,6 +5216,7 @@ union bpf_attr { FN(get_func_arg), \ FN(get_func_ret), \ FN(get_func_arg_cnt), \ + FN(ima_file_hash), \ /* */
/* integer value in 'imm' field of BPF_CALL instruction selects which helper
Move sample generator code to ima_test_common() so that the new function can be called by multiple LSM hooks.
Signed-off-by: Roberto Sassu roberto.sassu@huawei.com --- tools/testing/selftests/bpf/progs/ima.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/bpf/progs/ima.c b/tools/testing/selftests/bpf/progs/ima.c index 96060ff4ffc6..b5a0de50d1b4 100644 --- a/tools/testing/selftests/bpf/progs/ima.c +++ b/tools/testing/selftests/bpf/progs/ima.c @@ -18,8 +18,7 @@ struct {
char _license[] SEC("license") = "GPL";
-SEC("lsm.s/bprm_committed_creds") -void BPF_PROG(ima, struct linux_binprm *bprm) +static void ima_test_common(struct file *file) { u64 ima_hash = 0; u64 *sample; @@ -28,7 +27,7 @@ void BPF_PROG(ima, struct linux_binprm *bprm)
pid = bpf_get_current_pid_tgid() >> 32; if (pid == monitored_pid) { - ret = bpf_ima_inode_hash(bprm->file->f_inode, &ima_hash, + ret = bpf_ima_inode_hash(file->f_inode, &ima_hash, sizeof(ima_hash)); if (ret < 0 || ima_hash == 0) return; @@ -43,3 +42,9 @@ void BPF_PROG(ima, struct linux_binprm *bprm)
return; } + +SEC("lsm.s/bprm_committed_creds") +void BPF_PROG(bprm_committed_creds, struct linux_binprm *bprm) +{ + ima_test_common(bprm->file); +}
Add new test to ensure that bpf_ima_file_hash() returns the digest of the executed files.
Signed-off-by: Roberto Sassu roberto.sassu@huawei.com --- .../selftests/bpf/prog_tests/test_ima.c | 43 +++++++++++++++++-- tools/testing/selftests/bpf/progs/ima.c | 10 ++++- 2 files changed, 47 insertions(+), 6 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/test_ima.c b/tools/testing/selftests/bpf/prog_tests/test_ima.c index 97d8a6f84f4a..acc911ba63fd 100644 --- a/tools/testing/selftests/bpf/prog_tests/test_ima.c +++ b/tools/testing/selftests/bpf/prog_tests/test_ima.c @@ -13,6 +13,8 @@
#include "ima.skel.h"
+#define MAX_SAMPLES 2 + static int run_measured_process(const char *measured_dir, u32 *monitored_pid) { int child_pid, child_status; @@ -32,14 +34,25 @@ static int run_measured_process(const char *measured_dir, u32 *monitored_pid) return -EINVAL; }
-static u64 ima_hash_from_bpf; +static u64 ima_hash_from_bpf[MAX_SAMPLES]; +static int ima_hash_from_bpf_idx;
static int process_sample(void *ctx, void *data, size_t len) { - ima_hash_from_bpf = *((u64 *)data); + if (ima_hash_from_bpf_idx >= MAX_SAMPLES) + return -ENOSPC; + + ima_hash_from_bpf[ima_hash_from_bpf_idx++] = *((u64 *)data); return 0; }
+static void test_init(struct ima__bss *bss) +{ + ima_hash_from_bpf_idx = 0; + + bss->use_ima_file_hash = false; +} + void test_test_ima(void) { char measured_dir_template[] = "/tmp/ima_measuredXXXXXX"; @@ -72,13 +85,35 @@ void test_test_ima(void) if (CHECK(err, "failed to run command", "%s, errno = %d\n", cmd, errno)) goto close_clean;
+ /* + * Test #1 + * - Goal: obtain a sample with the bpf_ima_inode_hash() helper + * - Expected result: 1 sample (/bin/true) + */ + test_init(skel->bss); err = run_measured_process(measured_dir, &skel->bss->monitored_pid); - if (CHECK(err, "run_measured_process", "err = %d\n", err)) + if (CHECK(err, "run_measured_process #1", "err = %d\n", err)) goto close_clean;
err = ring_buffer__consume(ringbuf); ASSERT_EQ(err, 1, "num_samples_or_err"); - ASSERT_NEQ(ima_hash_from_bpf, 0, "ima_hash"); + ASSERT_NEQ(ima_hash_from_bpf[0], 0, "ima_hash"); + + /* + * Test #2 + * - Goal: obtain samples with the bpf_ima_file_hash() helper + * - Expected result: 2 samples (./ima_setup.sh, /bin/true) + */ + test_init(skel->bss); + skel->bss->use_ima_file_hash = true; + err = run_measured_process(measured_dir, &skel->bss->monitored_pid); + if (CHECK(err, "run_measured_process #2", "err = %d\n", err)) + goto close_clean; + + err = ring_buffer__consume(ringbuf); + ASSERT_EQ(err, 2, "num_samples_or_err"); + ASSERT_NEQ(ima_hash_from_bpf[0], 0, "ima_hash"); + ASSERT_NEQ(ima_hash_from_bpf[1], 0, "ima_hash");
close_clean: snprintf(cmd, sizeof(cmd), "./ima_setup.sh cleanup %s", measured_dir); diff --git a/tools/testing/selftests/bpf/progs/ima.c b/tools/testing/selftests/bpf/progs/ima.c index b5a0de50d1b4..e0b073dcfb5d 100644 --- a/tools/testing/selftests/bpf/progs/ima.c +++ b/tools/testing/selftests/bpf/progs/ima.c @@ -18,6 +18,8 @@ struct {
char _license[] SEC("license") = "GPL";
+bool use_ima_file_hash; + static void ima_test_common(struct file *file) { u64 ima_hash = 0; @@ -27,8 +29,12 @@ static void ima_test_common(struct file *file)
pid = bpf_get_current_pid_tgid() >> 32; if (pid == monitored_pid) { - ret = bpf_ima_inode_hash(file->f_inode, &ima_hash, - sizeof(ima_hash)); + if (!use_ima_file_hash) + ret = bpf_ima_inode_hash(file->f_inode, &ima_hash, + sizeof(ima_hash)); + else + ret = bpf_ima_file_hash(file, &ima_hash, + sizeof(ima_hash)); if (ret < 0 || ima_hash == 0) return;
Verify that bpf_ima_inode_hash() returns a non-fresh digest after a file write, and that bpf_ima_file_hash() returns a fresh digest. Verification is done by requesting the digest from the bprm_creds_for_exec hook, called before ima_bprm_check().
Signed-off-by: Roberto Sassu roberto.sassu@huawei.com --- tools/testing/selftests/bpf/ima_setup.sh | 24 ++++++- .../selftests/bpf/prog_tests/test_ima.c | 72 ++++++++++++++++++- tools/testing/selftests/bpf/progs/ima.c | 11 +++ 3 files changed, 103 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/bpf/ima_setup.sh b/tools/testing/selftests/bpf/ima_setup.sh index 8e62581113a3..a3de1cd43ba0 100755 --- a/tools/testing/selftests/bpf/ima_setup.sh +++ b/tools/testing/selftests/bpf/ima_setup.sh @@ -12,7 +12,7 @@ LOG_FILE="$(mktemp /tmp/ima_setup.XXXX.log)"
usage() { - echo "Usage: $0 <setup|cleanup|run> <existing_tmp_dir>" + echo "Usage: $0 <setup|cleanup|run|modify-bin|restore-bin> <existing_tmp_dir>" exit 1 }
@@ -77,6 +77,24 @@ run() exec "${copied_bin_path}" }
+modify_bin() +{ + local tmp_dir="$1" + local mount_dir="${tmp_dir}/mnt" + local copied_bin_path="${mount_dir}/$(basename ${TEST_BINARY})" + + echo "mod" >> "${copied_bin_path}" +} + +restore_bin() +{ + local tmp_dir="$1" + local mount_dir="${tmp_dir}/mnt" + local copied_bin_path="${mount_dir}/$(basename ${TEST_BINARY})" + + truncate -s -4 "${copied_bin_path}" +} + catch() { local exit_code="$1" @@ -105,6 +123,10 @@ main() cleanup "${tmp_dir}" elif [[ "${action}" == "run" ]]; then run "${tmp_dir}" + elif [[ "${action}" == "modify-bin" ]]; then + modify_bin "${tmp_dir}" + elif [[ "${action}" == "restore-bin" ]]; then + restore_bin "${tmp_dir}" else echo "Unknown action: ${action}" exit 1 diff --git a/tools/testing/selftests/bpf/prog_tests/test_ima.c b/tools/testing/selftests/bpf/prog_tests/test_ima.c index acc911ba63fd..a0cfba0b4273 100644 --- a/tools/testing/selftests/bpf/prog_tests/test_ima.c +++ b/tools/testing/selftests/bpf/prog_tests/test_ima.c @@ -13,16 +13,17 @@
#include "ima.skel.h"
-#define MAX_SAMPLES 2 +#define MAX_SAMPLES 4
-static int run_measured_process(const char *measured_dir, u32 *monitored_pid) +static int _run_measured_process(const char *measured_dir, u32 *monitored_pid, + const char *cmd) { int child_pid, child_status;
child_pid = fork(); if (child_pid == 0) { *monitored_pid = getpid(); - execlp("./ima_setup.sh", "./ima_setup.sh", "run", measured_dir, + execlp("./ima_setup.sh", "./ima_setup.sh", cmd, measured_dir, NULL); exit(errno);
@@ -34,6 +35,11 @@ static int run_measured_process(const char *measured_dir, u32 *monitored_pid) return -EINVAL; }
+static int run_measured_process(const char *measured_dir, u32 *monitored_pid) +{ + return _run_measured_process(measured_dir, monitored_pid, "run"); +} + static u64 ima_hash_from_bpf[MAX_SAMPLES]; static int ima_hash_from_bpf_idx;
@@ -51,6 +57,7 @@ static void test_init(struct ima__bss *bss) ima_hash_from_bpf_idx = 0;
bss->use_ima_file_hash = false; + bss->enable_bprm_creds_for_exec = false; }
void test_test_ima(void) @@ -58,6 +65,7 @@ void test_test_ima(void) char measured_dir_template[] = "/tmp/ima_measuredXXXXXX"; struct ring_buffer *ringbuf = NULL; const char *measured_dir; + u64 bin_true_sample; char cmd[256];
int err, duration = 0; @@ -114,6 +122,64 @@ void test_test_ima(void) ASSERT_EQ(err, 2, "num_samples_or_err"); ASSERT_NEQ(ima_hash_from_bpf[0], 0, "ima_hash"); ASSERT_NEQ(ima_hash_from_bpf[1], 0, "ima_hash"); + bin_true_sample = ima_hash_from_bpf[1]; + + /* + * Test #3 + * - Goal: confirm that bpf_ima_inode_hash() returns a non-fresh digest + * - Expected result: 2 samples (/bin/true: non-fresh, fresh) + */ + test_init(skel->bss); + + err = _run_measured_process(measured_dir, &skel->bss->monitored_pid, + "modify-bin"); + if (CHECK(err, "modify-bin #3", "err = %d\n", err)) + goto close_clean; + + skel->bss->enable_bprm_creds_for_exec = true; + err = run_measured_process(measured_dir, &skel->bss->monitored_pid); + if (CHECK(err, "run_measured_process #3", "err = %d\n", err)) + goto close_clean; + + err = ring_buffer__consume(ringbuf); + ASSERT_EQ(err, 2, "num_samples_or_err"); + ASSERT_NEQ(ima_hash_from_bpf[0], 0, "ima_hash"); + ASSERT_NEQ(ima_hash_from_bpf[1], 0, "ima_hash"); + ASSERT_EQ(ima_hash_from_bpf[0], bin_true_sample, "sample_equal_or_err"); + /* IMA refreshed the digest. */ + ASSERT_NEQ(ima_hash_from_bpf[1], bin_true_sample, + "sample_different_or_err"); + + /* + * Test #4 + * - Goal: verify that bpf_ima_file_hash() returns a fresh digest + * - Expected result: 4 samples (./ima_setup.sh: fresh, fresh; + * /bin/true: fresh, fresh) + */ + test_init(skel->bss); + skel->bss->use_ima_file_hash = true; + skel->bss->enable_bprm_creds_for_exec = true; + err = run_measured_process(measured_dir, &skel->bss->monitored_pid); + if (CHECK(err, "run_measured_process #4", "err = %d\n", err)) + goto close_clean; + + err = ring_buffer__consume(ringbuf); + ASSERT_EQ(err, 4, "num_samples_or_err"); + ASSERT_NEQ(ima_hash_from_bpf[0], 0, "ima_hash"); + ASSERT_NEQ(ima_hash_from_bpf[1], 0, "ima_hash"); + ASSERT_NEQ(ima_hash_from_bpf[2], 0, "ima_hash"); + ASSERT_NEQ(ima_hash_from_bpf[3], 0, "ima_hash"); + ASSERT_NEQ(ima_hash_from_bpf[2], bin_true_sample, + "sample_different_or_err"); + ASSERT_EQ(ima_hash_from_bpf[3], ima_hash_from_bpf[2], + "sample_equal_or_err"); + + skel->bss->use_ima_file_hash = false; + skel->bss->enable_bprm_creds_for_exec = false; + err = _run_measured_process(measured_dir, &skel->bss->monitored_pid, + "restore-bin"); + if (CHECK(err, "restore-bin #3", "err = %d\n", err)) + goto close_clean;
close_clean: snprintf(cmd, sizeof(cmd), "./ima_setup.sh cleanup %s", measured_dir); diff --git a/tools/testing/selftests/bpf/progs/ima.c b/tools/testing/selftests/bpf/progs/ima.c index e0b073dcfb5d..9633e5f2453d 100644 --- a/tools/testing/selftests/bpf/progs/ima.c +++ b/tools/testing/selftests/bpf/progs/ima.c @@ -19,6 +19,7 @@ struct { char _license[] SEC("license") = "GPL";
bool use_ima_file_hash; +bool enable_bprm_creds_for_exec;
static void ima_test_common(struct file *file) { @@ -54,3 +55,13 @@ void BPF_PROG(bprm_committed_creds, struct linux_binprm *bprm) { ima_test_common(bprm->file); } + +SEC("lsm.s/bprm_creds_for_exec") +int BPF_PROG(bprm_creds_for_exec, struct linux_binprm *bprm) +{ + if (!enable_bprm_creds_for_exec) + return 0; + + ima_test_common(bprm->file); + return 0; +}
Make bpf_lsm_kernel_read_file() as sleepable, so that bpf_ima_inode_hash() or bpf_ima_file_hash() can be called inside the implementation of this hook.
Signed-off-by: Roberto Sassu roberto.sassu@huawei.com --- kernel/bpf/bpf_lsm.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c index e8d27af5bbcc..064eccba641d 100644 --- a/kernel/bpf/bpf_lsm.c +++ b/kernel/bpf/bpf_lsm.c @@ -187,6 +187,7 @@ BTF_ID(func, bpf_lsm_inode_setxattr) BTF_ID(func, bpf_lsm_inode_symlink) BTF_ID(func, bpf_lsm_inode_unlink) BTF_ID(func, bpf_lsm_kernel_module_request) +BTF_ID(func, bpf_lsm_kernel_read_file) BTF_ID(func, bpf_lsm_kernfs_init_security)
#ifdef CONFIG_KEYS
Test the ability of bpf_lsm_kernel_read_file() to call the sleepable functions bpf_ima_inode_hash() or bpf_ima_file_hash() to obtain a measurement of a loaded IMA policy.
Signed-off-by: Roberto Sassu roberto.sassu@huawei.com --- tools/testing/selftests/bpf/ima_setup.sh | 13 ++++++++++++- .../selftests/bpf/prog_tests/test_ima.c | 19 +++++++++++++++++++ tools/testing/selftests/bpf/progs/ima.c | 18 ++++++++++++++++++ 3 files changed, 49 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/bpf/ima_setup.sh b/tools/testing/selftests/bpf/ima_setup.sh index a3de1cd43ba0..8ecead4ccad0 100755 --- a/tools/testing/selftests/bpf/ima_setup.sh +++ b/tools/testing/selftests/bpf/ima_setup.sh @@ -12,7 +12,7 @@ LOG_FILE="$(mktemp /tmp/ima_setup.XXXX.log)"
usage() { - echo "Usage: $0 <setup|cleanup|run|modify-bin|restore-bin> <existing_tmp_dir>" + echo "Usage: $0 <setup|cleanup|run|modify-bin|restore-bin|load-policy> <existing_tmp_dir>" exit 1 }
@@ -51,6 +51,7 @@ setup()
ensure_mount_securityfs echo "measure func=BPRM_CHECK fsuuid=${mount_uuid}" > ${IMA_POLICY_FILE} + echo "measure func=BPRM_CHECK fsuuid=${mount_uuid}" > ${mount_dir}/policy_test }
cleanup() { @@ -95,6 +96,14 @@ restore_bin() truncate -s -4 "${copied_bin_path}" }
+load_policy() +{ + local tmp_dir="$1" + local mount_dir="${tmp_dir}/mnt" + + echo ${mount_dir}/policy_test > ${IMA_POLICY_FILE} 2> /dev/null +} + catch() { local exit_code="$1" @@ -127,6 +136,8 @@ main() modify_bin "${tmp_dir}" elif [[ "${action}" == "restore-bin" ]]; then restore_bin "${tmp_dir}" + elif [[ "${action}" == "load-policy" ]]; then + load_policy "${tmp_dir}" else echo "Unknown action: ${action}" exit 1 diff --git a/tools/testing/selftests/bpf/prog_tests/test_ima.c b/tools/testing/selftests/bpf/prog_tests/test_ima.c index a0cfba0b4273..b13a141c4220 100644 --- a/tools/testing/selftests/bpf/prog_tests/test_ima.c +++ b/tools/testing/selftests/bpf/prog_tests/test_ima.c @@ -58,6 +58,7 @@ static void test_init(struct ima__bss *bss)
bss->use_ima_file_hash = false; bss->enable_bprm_creds_for_exec = false; + bss->enable_kernel_read_file = false; }
void test_test_ima(void) @@ -181,6 +182,24 @@ void test_test_ima(void) if (CHECK(err, "restore-bin #3", "err = %d\n", err)) goto close_clean;
+ /* + * Test #5 + * - Goal: obtain a sample from the kernel_read_file hook + * - Expected result: 2 samples (./ima_setup.sh, policy_test) + */ + test_init(skel->bss); + skel->bss->use_ima_file_hash = true; + skel->bss->enable_kernel_read_file = true; + err = _run_measured_process(measured_dir, &skel->bss->monitored_pid, + "load-policy"); + if (CHECK(err, "run_measured_process #5", "err = %d\n", err)) + goto close_clean; + + err = ring_buffer__consume(ringbuf); + ASSERT_EQ(err, 2, "num_samples_or_err"); + ASSERT_NEQ(ima_hash_from_bpf[0], 0, "ima_hash"); + ASSERT_NEQ(ima_hash_from_bpf[1], 0, "ima_hash"); + close_clean: snprintf(cmd, sizeof(cmd), "./ima_setup.sh cleanup %s", measured_dir); err = system(cmd); diff --git a/tools/testing/selftests/bpf/progs/ima.c b/tools/testing/selftests/bpf/progs/ima.c index 9633e5f2453d..e3ce943c5c3d 100644 --- a/tools/testing/selftests/bpf/progs/ima.c +++ b/tools/testing/selftests/bpf/progs/ima.c @@ -20,6 +20,7 @@ char _license[] SEC("license") = "GPL";
bool use_ima_file_hash; bool enable_bprm_creds_for_exec; +bool enable_kernel_read_file;
static void ima_test_common(struct file *file) { @@ -65,3 +66,20 @@ int BPF_PROG(bprm_creds_for_exec, struct linux_binprm *bprm) ima_test_common(bprm->file); return 0; } + +SEC("lsm.s/kernel_read_file") +int BPF_PROG(kernel_read_file, struct file *file, enum kernel_read_file_id id, + bool contents) +{ + if (!enable_kernel_read_file) + return 0; + + if (!contents) + return 0; + + if (id != READING_POLICY) + return 0; + + ima_test_common(file); + return 0; +}
Check that bpf_kernel_read_file() denies the reading of an IMA policy, by ensuring that ima_setup.sh exits with an error.
Signed-off-by: Roberto Sassu roberto.sassu@huawei.com --- .../selftests/bpf/prog_tests/test_ima.c | 17 +++++++++++++++++ tools/testing/selftests/bpf/progs/ima.c | 18 ++++++++++++++++++ 2 files changed, 35 insertions(+)
diff --git a/tools/testing/selftests/bpf/prog_tests/test_ima.c b/tools/testing/selftests/bpf/prog_tests/test_ima.c index b13a141c4220..b13feceb38f1 100644 --- a/tools/testing/selftests/bpf/prog_tests/test_ima.c +++ b/tools/testing/selftests/bpf/prog_tests/test_ima.c @@ -59,6 +59,7 @@ static void test_init(struct ima__bss *bss) bss->use_ima_file_hash = false; bss->enable_bprm_creds_for_exec = false; bss->enable_kernel_read_file = false; + bss->test_deny = false; }
void test_test_ima(void) @@ -200,6 +201,22 @@ void test_test_ima(void) ASSERT_NEQ(ima_hash_from_bpf[0], 0, "ima_hash"); ASSERT_NEQ(ima_hash_from_bpf[1], 0, "ima_hash");
+ /* + * Test #6 + * - Goal: ensure that the kernel_read_file hook denies an operation + * - Expected result: 0 samples + */ + test_init(skel->bss); + skel->bss->enable_kernel_read_file = true; + skel->bss->test_deny = true; + err = _run_measured_process(measured_dir, &skel->bss->monitored_pid, + "load-policy"); + if (CHECK(!err, "run_measured_process #6", "err = %d\n", err)) + goto close_clean; + + err = ring_buffer__consume(ringbuf); + ASSERT_EQ(err, 0, "num_samples_or_err"); + close_clean: snprintf(cmd, sizeof(cmd), "./ima_setup.sh cleanup %s", measured_dir); err = system(cmd); diff --git a/tools/testing/selftests/bpf/progs/ima.c b/tools/testing/selftests/bpf/progs/ima.c index e3ce943c5c3d..e16a2c208481 100644 --- a/tools/testing/selftests/bpf/progs/ima.c +++ b/tools/testing/selftests/bpf/progs/ima.c @@ -21,6 +21,7 @@ char _license[] SEC("license") = "GPL"; bool use_ima_file_hash; bool enable_bprm_creds_for_exec; bool enable_kernel_read_file; +bool test_deny;
static void ima_test_common(struct file *file) { @@ -51,6 +52,17 @@ static void ima_test_common(struct file *file) return; }
+static int ima_test_deny(void) +{ + u32 pid; + + pid = bpf_get_current_pid_tgid() >> 32; + if (pid == monitored_pid && test_deny) + return -EPERM; + + return 0; +} + SEC("lsm.s/bprm_committed_creds") void BPF_PROG(bprm_committed_creds, struct linux_binprm *bprm) { @@ -71,6 +83,8 @@ SEC("lsm.s/kernel_read_file") int BPF_PROG(kernel_read_file, struct file *file, enum kernel_read_file_id id, bool contents) { + int ret; + if (!enable_kernel_read_file) return 0;
@@ -80,6 +94,10 @@ int BPF_PROG(kernel_read_file, struct file *file, enum kernel_read_file_id id, if (id != READING_POLICY) return 0;
+ ret = ima_test_deny(); + if (ret < 0) + return ret; + ima_test_common(file); return 0; }
On Wed, Mar 02, 2022 at 12:13:55PM +0100, Roberto Sassu wrote:
Extend the interoperability with IMA, to give wider flexibility for the implementation of integrity-focused LSMs based on eBPF.
Patch 1 fixes some style issues.
Patches 2-6 give the ability to eBPF-based LSMs to take advantage of the measurement capability of IMA without needing to setup a policy in IMA (those LSMs might implement the policy capability themselves).
Patches 7-9 allow eBPF-based LSMs to evaluate files read by the kernel.
Changelog
v2:
- Add better description to patch 1 (suggested by Shuah)
- Recalculate digest if it is not fresh (when IMA_COLLECTED flag not set)
- Move declaration of bpf_ima_file_hash() at the end (suggested by Yonghong)
- Add tests to check if the digest has been recalculated
- Add deny test for bpf_kernel_read_file()
- Add description to tests
v1:
- Modify ima_file_hash() only and allow the usage of the function with the modified behavior by eBPF-based LSMs through the new function bpf_ima_file_hash() (suggested by Mimi)
- Make bpf_lsm_kernel_read_file() sleepable so that bpf_ima_inode_hash() and bpf_ima_file_hash() can be called inside the implementation of eBPF-based LSMs for this hook
Roberto Sassu (9): ima: Fix documentation-related warnings in ima_main.c ima: Always return a file measurement in ima_file_hash() bpf-lsm: Introduce new helper bpf_ima_file_hash() selftests/bpf: Move sample generation code to ima_test_common() selftests/bpf: Add test for bpf_ima_file_hash() selftests/bpf: Check if the digest is refreshed after a file write bpf-lsm: Make bpf_lsm_kernel_read_file() as sleepable selftests/bpf: Add test for bpf_lsm_kernel_read_file() selftests/bpf: Check that bpf_kernel_read_file() denies reading IMA policy
We have to land this set through bpf-next. Please get the Acks for patches 1 and 2, so we can proceed.
From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com] Sent: Wednesday, March 2, 2022 11:21 PM On Wed, Mar 02, 2022 at 12:13:55PM +0100, Roberto Sassu wrote:
Extend the interoperability with IMA, to give wider flexibility for the implementation of integrity-focused LSMs based on eBPF.
Patch 1 fixes some style issues.
Patches 2-6 give the ability to eBPF-based LSMs to take advantage of the measurement capability of IMA without needing to setup a policy in IMA (those LSMs might implement the policy capability themselves).
Patches 7-9 allow eBPF-based LSMs to evaluate files read by the kernel.
Changelog
v2:
- Add better description to patch 1 (suggested by Shuah)
- Recalculate digest if it is not fresh (when IMA_COLLECTED flag not set)
- Move declaration of bpf_ima_file_hash() at the end (suggested by Yonghong)
- Add tests to check if the digest has been recalculated
- Add deny test for bpf_kernel_read_file()
- Add description to tests
v1:
- Modify ima_file_hash() only and allow the usage of the function with the modified behavior by eBPF-based LSMs through the new function bpf_ima_file_hash() (suggested by Mimi)
- Make bpf_lsm_kernel_read_file() sleepable so that bpf_ima_inode_hash() and bpf_ima_file_hash() can be called inside the implementation of eBPF-based LSMs for this hook
Roberto Sassu (9): ima: Fix documentation-related warnings in ima_main.c ima: Always return a file measurement in ima_file_hash() bpf-lsm: Introduce new helper bpf_ima_file_hash() selftests/bpf: Move sample generation code to ima_test_common() selftests/bpf: Add test for bpf_ima_file_hash() selftests/bpf: Check if the digest is refreshed after a file write bpf-lsm: Make bpf_lsm_kernel_read_file() as sleepable selftests/bpf: Add test for bpf_lsm_kernel_read_file() selftests/bpf: Check that bpf_kernel_read_file() denies reading IMA policy
We have to land this set through bpf-next. Please get the Acks for patches 1 and 2, so we can proceed.
Ok. Mimi, do you have time to have a look at those patches?
Thanks
Roberto
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Zhong Ronghua
[Cc'ing Florent, Kees]
Hi Alexei,
On Wed, 2022-03-02 at 14:20 -0800, Alexei Starovoitov wrote:
On Wed, Mar 02, 2022 at 12:13:55PM +0100, Roberto Sassu wrote:
Extend the interoperability with IMA, to give wider flexibility for the implementation of integrity-focused LSMs based on eBPF.
Patch 1 fixes some style issues.
Patches 2-6 give the ability to eBPF-based LSMs to take advantage of the measurement capability of IMA without needing to setup a policy in IMA (those LSMs might implement the policy capability themselves).
Patches 7-9 allow eBPF-based LSMs to evaluate files read by the kernel.
Changelog
v2:
- Add better description to patch 1 (suggested by Shuah)
- Recalculate digest if it is not fresh (when IMA_COLLECTED flag not set)
- Move declaration of bpf_ima_file_hash() at the end (suggested by Yonghong)
- Add tests to check if the digest has been recalculated
- Add deny test for bpf_kernel_read_file()
- Add description to tests
v1:
- Modify ima_file_hash() only and allow the usage of the function with the modified behavior by eBPF-based LSMs through the new function bpf_ima_file_hash() (suggested by Mimi)
- Make bpf_lsm_kernel_read_file() sleepable so that bpf_ima_inode_hash() and bpf_ima_file_hash() can be called inside the implementation of eBPF-based LSMs for this hook
Roberto Sassu (9): ima: Fix documentation-related warnings in ima_main.c ima: Always return a file measurement in ima_file_hash() bpf-lsm: Introduce new helper bpf_ima_file_hash() selftests/bpf: Move sample generation code to ima_test_common() selftests/bpf: Add test for bpf_ima_file_hash() selftests/bpf: Check if the digest is refreshed after a file write bpf-lsm: Make bpf_lsm_kernel_read_file() as sleepable selftests/bpf: Add test for bpf_lsm_kernel_read_file() selftests/bpf: Check that bpf_kernel_read_file() denies reading IMA policy
We have to land this set through bpf-next. Please get the Acks for patches 1 and 2, so we can proceed.
Each year in the LSS integrity status update talk, I've mentioned the eBPF integrity gaps. I finally reached out to KP, Florent Revest, Kees and others, letting them know that I'm concerned about the eBPF module integrity gaps. True there is a difference between signing the eBPF source modules versus the eBPF generated output, but IMA could at least verify the integrity of the source eBPF modules making sure they are measured, the module hash audited, and are properly signed.
Before expanding the ima_file_hash() or ima_inode_hash() usage, I'd appreciate someone adding the IMA support to measure, appraise, and audit eBPF modules. I realize that closing the eBPF integrity gaps is orthogonal to this patch set, but this patch set is not only extending the ima_file_hash()/ima_inode_hash() usage, but will be used to circumvent IMA. As per usual, IMA is policy based, allowing those interested in eBPF module integrity to define IMA policy rules.
thanks,
Mimi
On Thu, Mar 3, 2022 at 5:05 PM Mimi Zohar zohar@linux.ibm.com wrote:
[Cc'ing Florent, Kees]
Hi Alexei,
On Wed, 2022-03-02 at 14:20 -0800, Alexei Starovoitov wrote:
On Wed, Mar 02, 2022 at 12:13:55PM +0100, Roberto Sassu wrote:
Extend the interoperability with IMA, to give wider flexibility for the implementation of integrity-focused LSMs based on eBPF.
Patch 1 fixes some style issues.
Patches 2-6 give the ability to eBPF-based LSMs to take advantage of the measurement capability of IMA without needing to setup a policy in IMA (those LSMs might implement the policy capability themselves).
Patches 7-9 allow eBPF-based LSMs to evaluate files read by the kernel.
Changelog
v2:
- Add better description to patch 1 (suggested by Shuah)
- Recalculate digest if it is not fresh (when IMA_COLLECTED flag not set)
- Move declaration of bpf_ima_file_hash() at the end (suggested by Yonghong)
- Add tests to check if the digest has been recalculated
- Add deny test for bpf_kernel_read_file()
- Add description to tests
v1:
- Modify ima_file_hash() only and allow the usage of the function with the modified behavior by eBPF-based LSMs through the new function bpf_ima_file_hash() (suggested by Mimi)
- Make bpf_lsm_kernel_read_file() sleepable so that bpf_ima_inode_hash() and bpf_ima_file_hash() can be called inside the implementation of eBPF-based LSMs for this hook
Roberto Sassu (9): ima: Fix documentation-related warnings in ima_main.c ima: Always return a file measurement in ima_file_hash() bpf-lsm: Introduce new helper bpf_ima_file_hash() selftests/bpf: Move sample generation code to ima_test_common() selftests/bpf: Add test for bpf_ima_file_hash() selftests/bpf: Check if the digest is refreshed after a file write bpf-lsm: Make bpf_lsm_kernel_read_file() as sleepable selftests/bpf: Add test for bpf_lsm_kernel_read_file() selftests/bpf: Check that bpf_kernel_read_file() denies reading IMA policy
We have to land this set through bpf-next. Please get the Acks for patches 1 and 2, so we can proceed.
Hi Mimi,
Each year in the LSS integrity status update talk, I've mentioned the eBPF integrity gaps. I finally reached out to KP, Florent Revest, Kees
Thanks for bringing this up and it's very timely because we have been having discussion around eBPF program signing and delineating that from eBPF program integrity use-cases.
My plan is to travel to LSS (travel and visa permitting) and we can discuss it more there.
If you prefer we can also discuss it before in one of the BPF office hours:
https://docs.google.com/spreadsheets/d/1LfrDXZ9-fdhvPEp_LHkxAMYyxxpwBXjywWa0...
and others, letting them know that I'm concerned about the eBPF module integrity gaps. True there is a difference between signing the eBPF source modules versus the eBPF generated output, but IMA could at least verify the integrity of the source eBPF modules making sure they are measured, the module hash audited, and are properly signed.
Before expanding the ima_file_hash() or ima_inode_hash() usage, I'd appreciate someone adding the IMA support to measure, appraise, and audit eBPF modules. I realize that closing the eBPF integrity gaps is orthogonal to this patch set, but this patch set is not only extending
This really is orthogonal and IMHO it does not seem rational to block this patchset on it.
the ima_file_hash()/ima_inode_hash() usage, but will be used to circumvent IMA. As per usual, IMA is policy based, allowing those
I don't think they are being used to circumvent IMA but for totally different use-cases (e.g. as a data point for detecting attacks).
- KP
interested in eBPF module integrity to define IMA policy rules.
thanks,
Mimi
On Thu, 2022-03-03 at 17:17 +0100, KP Singh wrote:
On Thu, Mar 3, 2022 at 5:05 PM Mimi Zohar zohar@linux.ibm.com wrote:
[Cc'ing Florent, Kees]
Hi Alexei,
On Wed, 2022-03-02 at 14:20 -0800, Alexei Starovoitov wrote:
On Wed, Mar 02, 2022 at 12:13:55PM +0100, Roberto Sassu wrote:
Extend the interoperability with IMA, to give wider flexibility for the implementation of integrity-focused LSMs based on eBPF.
Patch 1 fixes some style issues.
Patches 2-6 give the ability to eBPF-based LSMs to take advantage of the measurement capability of IMA without needing to setup a policy in IMA (those LSMs might implement the policy capability themselves).
Patches 7-9 allow eBPF-based LSMs to evaluate files read by the kernel.
Changelog
v2:
- Add better description to patch 1 (suggested by Shuah)
- Recalculate digest if it is not fresh (when IMA_COLLECTED flag not set)
- Move declaration of bpf_ima_file_hash() at the end (suggested by Yonghong)
- Add tests to check if the digest has been recalculated
- Add deny test for bpf_kernel_read_file()
- Add description to tests
v1:
- Modify ima_file_hash() only and allow the usage of the function with the modified behavior by eBPF-based LSMs through the new function bpf_ima_file_hash() (suggested by Mimi)
- Make bpf_lsm_kernel_read_file() sleepable so that bpf_ima_inode_hash() and bpf_ima_file_hash() can be called inside the implementation of eBPF-based LSMs for this hook
Roberto Sassu (9): ima: Fix documentation-related warnings in ima_main.c ima: Always return a file measurement in ima_file_hash() bpf-lsm: Introduce new helper bpf_ima_file_hash() selftests/bpf: Move sample generation code to ima_test_common() selftests/bpf: Add test for bpf_ima_file_hash() selftests/bpf: Check if the digest is refreshed after a file write bpf-lsm: Make bpf_lsm_kernel_read_file() as sleepable selftests/bpf: Add test for bpf_lsm_kernel_read_file() selftests/bpf: Check that bpf_kernel_read_file() denies reading IMA policy
We have to land this set through bpf-next. Please get the Acks for patches 1 and 2, so we can proceed.
Hi Mimi,
Each year in the LSS integrity status update talk, I've mentioned the eBPF integrity gaps. I finally reached out to KP, Florent Revest, Kees
Thanks for bringing this up and it's very timely because we have been having discussion around eBPF program signing and delineating that from eBPF program integrity use-cases.
My plan is to travel to LSS (travel and visa permitting) and we can discuss it more there.
If you prefer we can also discuss it before in one of the BPF office hours:
https://docs.google.com/spreadsheets/d/1LfrDXZ9-fdhvPEp_LHkxAMYyxxpwBXjywWa0...
Sounds good.
and others, letting them know that I'm concerned about the eBPF module integrity gaps. True there is a difference between signing the eBPF source modules versus the eBPF generated output, but IMA could at least verify the integrity of the source eBPF modules making sure they are measured, the module hash audited, and are properly signed.
Before expanding the ima_file_hash() or ima_inode_hash() usage, I'd appreciate someone adding the IMA support to measure, appraise, and audit eBPF modules. I realize that closing the eBPF integrity gaps is orthogonal to this patch set, but this patch set is not only extending
This really is orthogonal and IMHO it does not seem rational to block this patchset on it.
the ima_file_hash()/ima_inode_hash() usage, but will be used to circumvent IMA. As per usual, IMA is policy based, allowing those
I don't think they are being used to circumvent IMA but for totally different use-cases (e.g. as a data point for detecting attacks).
interested in eBPF module integrity to define IMA policy rules.
That might be true for your usecase, but not Roberto's. From the cover letter above, Roberto was honest in saying:
Patches 2-6 give the ability to eBPF-based LSMs to take advantage of the measurement capability of IMA without needing to setup a policy in IMA (those LSMs might implement the policy capability themselves).
On Thu, Mar 3, 2022 at 5:30 PM Mimi Zohar zohar@linux.ibm.com wrote:
On Thu, 2022-03-03 at 17:17 +0100, KP Singh wrote:
On Thu, Mar 3, 2022 at 5:05 PM Mimi Zohar zohar@linux.ibm.com wrote:
[Cc'ing Florent, Kees]
Hi Alexei,
On Wed, 2022-03-02 at 14:20 -0800, Alexei Starovoitov wrote:
On Wed, Mar 02, 2022 at 12:13:55PM +0100, Roberto Sassu wrote:
Extend the interoperability with IMA, to give wider flexibility for the implementation of integrity-focused LSMs based on eBPF.
Patch 1 fixes some style issues.
Patches 2-6 give the ability to eBPF-based LSMs to take advantage of the measurement capability of IMA without needing to setup a policy in IMA (those LSMs might implement the policy capability themselves).
Patches 7-9 allow eBPF-based LSMs to evaluate files read by the kernel.
Changelog
v2:
- Add better description to patch 1 (suggested by Shuah)
- Recalculate digest if it is not fresh (when IMA_COLLECTED flag not set)
- Move declaration of bpf_ima_file_hash() at the end (suggested by Yonghong)
- Add tests to check if the digest has been recalculated
- Add deny test for bpf_kernel_read_file()
- Add description to tests
v1:
- Modify ima_file_hash() only and allow the usage of the function with the modified behavior by eBPF-based LSMs through the new function bpf_ima_file_hash() (suggested by Mimi)
- Make bpf_lsm_kernel_read_file() sleepable so that bpf_ima_inode_hash() and bpf_ima_file_hash() can be called inside the implementation of eBPF-based LSMs for this hook
Roberto Sassu (9): ima: Fix documentation-related warnings in ima_main.c ima: Always return a file measurement in ima_file_hash() bpf-lsm: Introduce new helper bpf_ima_file_hash() selftests/bpf: Move sample generation code to ima_test_common() selftests/bpf: Add test for bpf_ima_file_hash() selftests/bpf: Check if the digest is refreshed after a file write bpf-lsm: Make bpf_lsm_kernel_read_file() as sleepable selftests/bpf: Add test for bpf_lsm_kernel_read_file() selftests/bpf: Check that bpf_kernel_read_file() denies reading IMA policy
We have to land this set through bpf-next. Please get the Acks for patches 1 and 2, so we can proceed.
Hi Mimi,
Each year in the LSS integrity status update talk, I've mentioned the eBPF integrity gaps. I finally reached out to KP, Florent Revest, Kees
Thanks for bringing this up and it's very timely because we have been having discussion around eBPF program signing and delineating that from eBPF program integrity use-cases.
My plan is to travel to LSS (travel and visa permitting) and we can discuss it more there.
If you prefer we can also discuss it before in one of the BPF office hours:
https://docs.google.com/spreadsheets/d/1LfrDXZ9-fdhvPEp_LHkxAMYyxxpwBXjywWa0...
Sounds good.
and others, letting them know that I'm concerned about the eBPF module integrity gaps. True there is a difference between signing the eBPF source modules versus the eBPF generated output, but IMA could at least verify the integrity of the source eBPF modules making sure they are measured, the module hash audited, and are properly signed.
Before expanding the ima_file_hash() or ima_inode_hash() usage, I'd appreciate someone adding the IMA support to measure, appraise, and audit eBPF modules. I realize that closing the eBPF integrity gaps is orthogonal to this patch set, but this patch set is not only extending
This really is orthogonal and IMHO it does not seem rational to block this patchset on it.
the ima_file_hash()/ima_inode_hash() usage, but will be used to circumvent IMA. As per usual, IMA is policy based, allowing those
I don't think they are being used to circumvent IMA but for totally different use-cases (e.g. as a data point for detecting attacks).
interested in eBPF module integrity to define IMA policy rules.
That might be true for your usecase, but not Roberto's. From the cover letter above, Roberto was honest in saying:
Patches 2-6 give the ability to eBPF-based LSMs to take advantage of the measurement capability of IMA without needing to setup a policy in IMA (those LSMs might implement the policy capability themselves).
Currently to use the helper bpf_ima_inode_hash in LSM progs one needs to have a basic IMA policy in place just to get a hash, even if one does not need to use the whole feature-set provided by IMA. These patches would be quite beneficial for this user-journey.
Even Robert's use case is to implement IMA policies in BPF this is still fundamentally different from IMA doing integrity measurement for BPF and blocking this patch-set on the latter does not seem rational and I don't see how implementing integrity for BPF would avoid your concerns.
-- thanks,
Mimi
On Thu, 2022-03-03 at 19:14 +0100, KP Singh wrote:
Even Robert's use case is to implement IMA policies in BPF this is still fundamentally different from IMA doing integrity measurement for BPF and blocking this patch-set on the latter does not seem rational and I don't see how implementing integrity for BPF would avoid your concerns.
eBPF modules are an entire class of files currently not being measured, audited, or appraised. This is an integrity gap that needs to be closed. The purpose would be to at least measure and verify the integrity of the eBPF module that is going to be used in lieu of traditional IMA.
On Thu, Mar 3, 2022 at 11:13 AM Mimi Zohar zohar@linux.ibm.com wrote:
On Thu, 2022-03-03 at 19:14 +0100, KP Singh wrote:
Even Robert's use case is to implement IMA policies in BPF this is still fundamentally different from IMA doing integrity measurement for BPF and blocking this patch-set on the latter does not seem rational and I don't see how implementing integrity for BPF would avoid your concerns.
eBPF modules are an entire class of files currently not being measured, audited, or appraised. This is an integrity gap that needs to be closed. The purpose would be to at least measure and verify the integrity of the eBPF module that is going to be used in lieu of traditional IMA.
Mimi,
. There is no such thing as "eBPF modules". There are BPF programs. They cannot be signed the same way as kernel modules. We've been working on providing a way to sign them for more than a year now. That work is still ongoing.
. IMA cannot be used for integrity check of BPF programs for the same reasons why kernel module like signing cannot be used.
. This patch set is orthogonal.
On Thu, 2022-03-03 at 14:39 -0800, Alexei Starovoitov wrote:
. There is no such thing as "eBPF modules". There are BPF programs. They cannot be signed the same way as kernel modules. We've been working on providing a way to sign them for more than a year now. That work is still ongoing.
. IMA cannot be used for integrity check of BPF programs for the same reasons why kernel module like signing cannot be used.
I assume the issue isn't where the signature is stored (e.g. appended, xattr), but of calculating the hash. Where is the discussion taking place? Are there any summaries of what has been discussed?
FYI, IMA isn't limited to measuring files. Support was added for buffer measurements (e.g kexec boot command line, certificates) and measuring kernel critical data (e.g. SELinux in memory policy & state, device mapper).
thanks,
Mimi
On Mon, Mar 7, 2022 at 3:57 AM Mimi Zohar zohar@linux.ibm.com wrote:
On Thu, 2022-03-03 at 14:39 -0800, Alexei Starovoitov wrote:
. There is no such thing as "eBPF modules". There are BPF programs. They cannot be signed the same way as kernel modules. We've been working on providing a way to sign them for more than a year now. That work is still ongoing.
. IMA cannot be used for integrity check of BPF programs for the same reasons why kernel module like signing cannot be used.
I assume the issue isn't where the signature is stored (e.g. appended, xattr), but of calculating the hash. Where is the discussion taking
This has the relevant background: https://lwn.net/Articles/853489/
We had some more discussions in one of our BSC meeting:
https://github.com/ebpf-io/bsc/blob/master/minutes.md
and we expect the discussions to continue over conferences this year (e.g. LSF/MM/BPF, Linux Plumbers). As I mentioned on another thread we don't have to wait for conferences and we can discuss this in the BPF office hours. Please feel free to add an agenda at:
https://docs.google.com/spreadsheets/d/1LfrDXZ9-fdhvPEp_LHkxAMYyxxpwBXjywWa0...
(best is to give some notice so that interested folks can join).
place? Are there any summaries of what has been discussed?
FYI, IMA isn't limited to measuring files. Support was added for buffer measurements (e.g kexec boot command line, certificates) and measuring kernel critical data (e.g. SELinux in memory policy & state, device mapper).
Nice. I need to look at how this is implemented.
- KP
thanks,
Mimi
On Mon, 2022-03-07 at 14:17 +0100, KP Singh wrote:
On Mon, Mar 7, 2022 at 3:57 AM Mimi Zohar zohar@linux.ibm.com wrote:
On Thu, 2022-03-03 at 14:39 -0800, Alexei Starovoitov wrote:
. There is no such thing as "eBPF modules". There are BPF programs. They cannot be signed the same way as kernel modules. We've been working on providing a way to sign them for more than a year now. That work is still ongoing.
. IMA cannot be used for integrity check of BPF programs for the same reasons why kernel module like signing cannot be used.
I assume the issue isn't where the signature is stored (e.g. appended, xattr), but of calculating the hash. Where is the discussion taking
This has the relevant background: https://lwn.net/Articles/853489/
Thanks, Jon!
We had some more discussions in one of our BSC meeting:
https://github.com/ebpf-io/bsc/blob/master/minutes.md
and we expect the discussions to continue over conferences this year (e.g. LSF/MM/BPF, Linux Plumbers). As I mentioned on another thread we don't have to wait for conferences and we can discuss this in the BPF office hours. Please feel free to add an agenda at:
https://docs.google.com/spreadsheets/d/1LfrDXZ9-fdhvPEp_LHkxAMYyxxpwBXjywWa0...
(best is to give some notice so that interested folks can join).
Right, but probably a good idea to understand the issues at least at a high level, before a meeting.
place? Are there any summaries of what has been discussed?
FYI, IMA isn't limited to measuring files. Support was added for buffer measurements (e.g kexec boot command line, certificates) and measuring kernel critical data (e.g. SELinux in memory policy & state, device mapper).
Nice. I need to look at how this is implemented.
ima_measure_critical_data() is of kernel state info, so signature verification is currently not needed or supported, only measurement.
thanks,
Mimi
On Wed, 2022-03-02 at 12:13 +0100, Roberto Sassu wrote:
Extend the interoperability with IMA, to give wider flexibility for the implementation of integrity-focused LSMs based on eBPF.
Patch 1 fixes some style issues.
Patches 2-6 give the ability to eBPF-based LSMs to take advantage of the measurement capability of IMA without needing to setup a policy in IMA (those LSMs might implement the policy capability themselves).
Patches 7-9 allow eBPF-based LSMs to evaluate files read by the kernel.
The tests seem to only work when neither a builtin IMA policy or a custom policy is previously loaded.
thanks,
Mimi
From: Mimi Zohar [mailto:zohar@linux.ibm.com] Sent: Sunday, March 6, 2022 8:24 PM On Wed, 2022-03-02 at 12:13 +0100, Roberto Sassu wrote:
Extend the interoperability with IMA, to give wider flexibility for the implementation of integrity-focused LSMs based on eBPF.
Patch 1 fixes some style issues.
Patches 2-6 give the ability to eBPF-based LSMs to take advantage of the measurement capability of IMA without needing to setup a policy in IMA (those LSMs might implement the policy capability themselves).
Patches 7-9 allow eBPF-based LSMs to evaluate files read by the kernel.
The tests seem to only work when neither a builtin IMA policy or a custom policy is previously loaded.
Hi Mimi
unfortunately yes. If there are more generic rules, the number of samples differs from that expected.
For example, if you have an existing rule like:
measure func=BPRM_CHECK mask=MAY_EXEC
you will have:
test_test_ima:PASS:run_measured_process #1 0 nsec test_test_ima:FAIL:num_samples_or_err unexpected num_samples_or_err: actual 2 != expected 1
Test #1 fails because also ima_setup.sh is measured.
Thanks
Roberto
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Zhong Ronghua
Hello:
This series was applied to bpf/bpf-next.git (master) by Alexei Starovoitov ast@kernel.org:
On Wed, 2 Mar 2022 12:13:55 +0100 you wrote:
Extend the interoperability with IMA, to give wider flexibility for the implementation of integrity-focused LSMs based on eBPF.
Patch 1 fixes some style issues.
Patches 2-6 give the ability to eBPF-based LSMs to take advantage of the measurement capability of IMA without needing to setup a policy in IMA (those LSMs might implement the policy capability themselves).
[...]
Here is the summary with links: - [v3,1/9] ima: Fix documentation-related warnings in ima_main.c https://git.kernel.org/bpf/bpf-next/c/bae60eefb95c - [v3,2/9] ima: Always return a file measurement in ima_file_hash() https://git.kernel.org/bpf/bpf-next/c/280fe8367b0d - [v3,3/9] bpf-lsm: Introduce new helper bpf_ima_file_hash() https://git.kernel.org/bpf/bpf-next/c/174b16946e39 - [v3,4/9] selftests/bpf: Move sample generation code to ima_test_common() https://git.kernel.org/bpf/bpf-next/c/2746de3c53d6 - [v3,5/9] selftests/bpf: Add test for bpf_ima_file_hash() https://git.kernel.org/bpf/bpf-next/c/27a77d0d460c - [v3,6/9] selftests/bpf: Check if the digest is refreshed after a file write https://git.kernel.org/bpf/bpf-next/c/91e8fa254dbd - [v3,7/9] bpf-lsm: Make bpf_lsm_kernel_read_file() as sleepable https://git.kernel.org/bpf/bpf-next/c/df6b3039fa11 - [v3,8/9] selftests/bpf: Add test for bpf_lsm_kernel_read_file() https://git.kernel.org/bpf/bpf-next/c/e6dcf7bbf37c - [v3,9/9] selftests/bpf: Check that bpf_kernel_read_file() denies reading IMA policy https://git.kernel.org/bpf/bpf-next/c/7bae42b68d7f
You are awesome, thank you!
linux-kselftest-mirror@lists.linaro.org
-
Alexei Starovoitov -
KP Singh -
Mimi Zohar -
patchwork-bot+netdevbpf＠kernel.org -
Roberto Sassu