Hi Lakshmi,
I haven't yet tested the patch. Below are a couple of comments.
On Wed, 2020-06-10 at 17:04 -0700, Lakshmi Ramasubramanian wrote:
The final log statement in process_buffer_measurement() for failure
condition is at debug level. This does not log the message unless
the system log level is raised which would significantly increase
the messages in the system log. Change this to an audit message to
audit integrity failures with the "op" field of the audit message
set to indicate the measurement operation that failed.
The problem with the existing "pr" level is kind of irrelevant. You
could keep the existing pr_debug() statement, if you wanted to. The
reason for auditing a failure is because it is "integrity" relevant or
more generically "security" relevant. The first patch addresses the
change in the audit message format.
Also, add an audit message for failures in ima_alloc_key_entry().
Sample audit messages:
[ 6.284329] audit: type=1804 audit(1591756723.627:2): pid=1 uid=0
auid=4294967295 ses=4294967295 subj=kernel
op=measuring_kexec_cmdline cause=alloc_entry errno=-12
comm="swapper/0" name="kexec-cmdline" res=0
[ 8.017126] audit: type=1804 audit(1591756725.360:10): pid=1
uid=0 auid=4294967295 ses=4294967295
subj=system_u:system_r:init_t:s0 op=measuring_key
cause=hashing_error errno=-22 comm="systemd"
name=".builtin_trusted_keys" res=0
Signed-off-by: Lakshmi Ramasubramanian <nramas(a)linux.microsoft.com>
Suggested-by: Mimi Zohar <zohar(a)linux.ibm.com>
---
security/integrity/ima/ima.h | 48 ++++++++++++++++---------
security/integrity/ima/ima_main.c | 18 +++++++---
security/integrity/ima/ima_policy.c | 2 +-
security/integrity/ima/ima_queue_keys.c | 5 +++
4 files changed, 51 insertions(+), 22 deletions(-)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index df93ac258e01..e42101eebd69 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -186,27 +186,43 @@ static inline unsigned int ima_hash_key(u8 *digest)
return (digest[0] | digest[1] << 8) % IMA_MEASURE_HTABLE_SIZE;
}
-#define __ima_hooks(hook) \
- hook(NONE) \
- hook(FILE_CHECK) \
- hook(MMAP_CHECK) \
- hook(BPRM_CHECK) \
- hook(CREDS_CHECK) \
- hook(POST_SETATTR) \
- hook(MODULE_CHECK) \
- hook(FIRMWARE_CHECK) \
- hook(KEXEC_KERNEL_CHECK) \
- hook(KEXEC_INITRAMFS_CHECK) \
- hook(POLICY_CHECK) \
- hook(KEXEC_CMDLINE) \
- hook(KEY_CHECK) \
- hook(MAX_CHECK)
-#define __ima_hook_enumify(ENUM) ENUM,
+#define __ima_hooks(hook) \
+ hook(NONE, none) \
+ hook(FILE_CHECK, file) \
+ hook(MMAP_CHECK, mmap) \
+ hook(BPRM_CHECK, bprm) \
+ hook(CREDS_CHECK, creds) \
+ hook(POST_SETATTR, post_setattr) \
+ hook(MODULE_CHECK, module) \
+ hook(FIRMWARE_CHECK, firmware) \
+ hook(KEXEC_KERNEL_CHECK, kexec_kernel) \
+ hook(KEXEC_INITRAMFS_CHECK, kexec_initramfs) \
+ hook(POLICY_CHECK, policy) \
+ hook(KEXEC_CMDLINE, kexec_cmdline) \
+ hook(KEY_CHECK, key) \
+ hook(MAX_CHECK, none)
+
+#define __ima_hook_enumify(ENUM, str) ENUM,
+#define __ima_stringify(arg) (#arg)
+#define __ima_hook_measuring_stringify(ENUM, str) \
+ (__ima_stringify(measuring_ ##str)),
enum ima_hooks {
__ima_hooks(__ima_hook_enumify)
};
+static const char * const ima_hooks_measure_str[] = {
+ __ima_hooks(__ima_hook_measuring_stringify)
+};
+
+static inline const char *ima_hooks_func_measure_str(enum ima_hooks func)
"ima_hooks_func_measure_str" is a bit long. There's no reason for
having both "hooks" and "func" in the name. Also this is a static
function, so it doesn't really need to be prefixed with "ima_". Maybe
truncate it to "func_measure_str()", similar to "func_token".
Mimi
+{
+ if (func >= MAX_CHECK)
+ return ima_hooks_measure_str[NONE];
+
+ return ima_hooks_measure_str[func];
+}
+
extern const char *const func_tokens[];