integrity: Add errno field in audit message

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.

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 <[email protected]>

Suggested-by Mimi Zohar <[email protected]>

---
 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(-)

security/integrity/ima/ima.h +20 -16

index df93ac258e01..e42101eebd69 100644
@@ -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)),

security/integrity/ima/ima.h +7 -0

+{
+	if (func >= MAX_CHECK)
+		return ima_hooks_measure_str[NONE];
+
+	return ima_hooks_measure_str[func];
+}
+
 extern const char *const func_tokens[];

Error code is not included in the audit messages logged by the integrity subsystem. Add a new field namely "errno" in the audit message and set the value to the error code passed to integrity_audit_msg() in the "result" parameter.

Sample audit message:

[    6.284329] audit: type=1804 audit(1591756723.627:2): pid=1 uid=0 auid=4294967295 ses=4294967295 subj=kernel op=add_boot_aggregate cause=alloc_entry errno=-12 comm="swapper/0" name="boot_aggregate" res=0

Signed-off-by Lakshmi Ramasubramanian <[email protected]>

Suggested-by Steve Grubb <[email protected]>

---
 security/integrity/integrity_audit.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

security/integrity/integrity_audit.c +0 -0

index 5109173839cc..8cbf415bb977 100644

security/integrity/integrity_audit.c +0 -1

        audit_log_untrustedstring(ab, get_task_comm(name, current));
        if (fname) {
                audit_log_format(ab, " name=");
--

2.27.0

I'm sorry I didn't get a chance to mention this before you posted this patch, but for the past several years we have been sticking with a policy of only adding new fields to the end of existing records; please adjust this patch accordingly. Otherwise, this looks fine to me.

Quoted 4 lines

         audit_log_untrustedstring(ab, get_task_comm(name, current));
         if (fname) {
                 audit_log_format(ab, " name=");

--

On 6/10/20 6:45 PM, Paul Moore wrote:

Hi Paul,

Quoted 6 lines

I'm sorry I didn't get a chance to mention this before you posted this patch, but for the past several years we have been sticking with a policy of only adding new fields to the end of existing records; please adjust this patch accordingly. Otherwise, this looks fine to me.

Quoted 4 lines

Quoted 4 lines

         audit_log_untrustedstring(ab, get_task_comm(name, current));
         if (fname) {
                 audit_log_format(ab, " name=");

--

Steve mentioned that since this new field "errno" is not a searchable entry, it can be added anywhere in the audit log message.

But I have no problem moving this to the end of the audit record.

Error code is not included in the audit messages logged by the integrity subsystem. Add a new field namely "errno" in the audit message and set the value to the error code passed to integrity_audit_msg() in the "result" parameter.

Sample audit message:

[    6.284329] audit: type=1804 audit(1591756723.627:2): pid=1 uid=0 auid=4294967295 ses=4294967295 subj=kernel op=add_boot_aggregate cause=alloc_entry errno=-12 comm="swapper/0" name="boot_aggregate" res=0

Signed-off-by Lakshmi Ramasubramanian <[email protected]>

Suggested-by Steve Grubb <[email protected]>

 security/integrity/integrity_audit.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

security/integrity/integrity_audit.c +0 -0

index 5109173839cc..8cbf415bb977 100644

security/integrity/integrity_audit.c +0 -0

 	audit_log_untrustedstring(ab, get_task_comm(name, current));
 	if (fname) {
 		audit_log_format(ab, " name=");

The idea is a good idea, but you're assuming that "result" is always errno.  That was probably true originally, but isn't now.  For example, ima_appraise_measurement() calls xattr_verify(), which compares the security.ima hash with the calculated file hash.  On failure, it returns the result of memcmp().  Each and every code path will need to be checked.

Quoted 6 lines · Mimi Zohar

The idea is a good idea, but you're assuming that "result" is always errno. That was probably true originally, but isn't now. For example, ima_appraise_measurement() calls xattr_verify(), which compares the security.ima hash with the calculated file hash. On failure, it returns the result of memcmp(). Each and every code path will need to be checked.

Good catch Mimi.

Instead of "errno" should we just use "result" and log the value given in the result parameter?

From the audit field dictionary (link given below) "result" is already
a known field that is used to indicate the result of the audited operation.

@Steve\Paul:
Like "res" is "result" also expected to have only values "0" or "1", or can it be any result code?

https://github.com/linux-audit/audit-documentation/blob/master/specs/fields
/field-dictionary.csv

res alphanumeric result of the audited operation(success/fail)

result alphanumeric result of the audited operation(success/fail)

Quoted 8 lines · EDT Lakshmi Ramasubramanian

Quoted 6 lines · Mimi Zohar

The idea is a good idea, but you're assuming that "result" is always errno. That was probably true originally, but isn't now. For example, ima_appraise_measurement() calls xattr_verify(), which compares the security.ima hash with the calculated file hash. On failure, it returns the result of memcmp(). Each and every code path will need to be checked.

Quoted 4 lines

Good catch Mimi.

Instead of "errno" should we just use "result" and log the value given in the result parameter?

That would likely collide with another field of the same name which is the operation's results. If it really is errno, the name is fine. It's generic enough that it can be reused on other events if that mattered.

Quoted 11 lines · Steve Grubb

Quoted 10 lines · EDT Lakshmi Ramasubramanian

Quoted 6 lines · Mimi Zohar

The idea is a good idea, but you're assuming that "result" is always errno. That was probably true originally, but isn't now. For example, ima_appraise_measurement() calls xattr_verify(), which compares the security.ima hash with the calculated file hash. On failure, it returns the result of memcmp(). Each and every code path will need to be checked.

Good catch Mimi.

Quoted 8 lines

Quoted 2 lines

Instead of "errno" should we just use "result" and log the value given in the result parameter?

That would likely collide with another field of the same name which is the
operation's results. If it really is errno, the name is fine. It's generic
enough that it can be reused on other events if that mattered.

Steve, what is the historical reason why we have both "res" and "result" for indicating a boolean success/fail? I'm just curious how we ended up this way, and who may still be using "result".

Quoted 18 lines

Quoted 17 lines

Quoted 11 lines

Quoted 6 lines

The idea is a good idea, but you're assuming that "result" is always errno. That was probably true originally, but isn't now. For example, ima_appraise_measurement() calls xattr_verify(), which compares the security.ima hash with the calculated file hash. On failure, it returns the result of memcmp(). Each and every code path will need to be checked.

Good catch Mimi.

Instead of "errno" should we just use "result" and log the value given in the result parameter?

That would likely collide with another field of the same name which is the
operation's results. If it really is errno, the name is fine. It's generic
enough that it can be reused on other events if that mattered.

Quoted 3 lines

Steve, what is the historical reason why we have both "res" and "result" for indicating a boolean success/fail? I'm just curious how we ended up this way, and who may still be using "result".

I think its pam and some other user space things did this. But because of mixed machines in datacenters supporting multiple versions of OS, we have to leave result alone. It has to be 0,1 or success/fail. We cannot use it for errno.

Quoted 18 lines · Steve Grubb

Quoted 18 lines

Quoted 17 lines

Quoted 11 lines

Quoted 6 lines

The idea is a good idea, but you're assuming that "result" is always errno. That was probably true originally, but isn't now. For example, ima_appraise_measurement() calls xattr_verify(), which compares the security.ima hash with the calculated file hash. On failure, it returns the result of memcmp(). Each and every code path will need to be checked.

Good catch Mimi.

Instead of "errno" should we just use "result" and log the value given in the result parameter?

That would likely collide with another field of the same name which is the
operation's results. If it really is errno, the name is fine. It's generic
enough that it can be reused on other events if that mattered.

Quoted 8 lines

Quoted 3 lines

Steve, what is the historical reason why we have both "res" and "result" for indicating a boolean success/fail? I'm just curious how we ended up this way, and who may still be using "result".

I think its pam and some other user space things did this. But because of mixed machines in datacenters supporting multiple versions of OS, we have to leave result alone. It has to be 0,1 or success/fail. We cannot use it for errno.

As Mimi had pointed out, since the value passed in result parameter is not always an error code, "errno" is not an appropriate name.

Can we add a new field, say, "op_result" to report the result of the specified operation?

Quoted 28 lines · EDT Lakshmi Ramasubramanian

Quoted 26 lines · Steve Grubb

Quoted 18 lines

Quoted 9 lines

Quoted 9 lines

Quoted 6 lines

The idea is a good idea, but you're assuming that "result" is always errno. That was probably true originally, but isn't now. For example, ima_appraise_measurement() calls xattr_verify(), which compares the security.ima hash with the calculated file hash. On failure, it returns the result of memcmp(). Each and every code path will need to be checked.

Good catch Mimi.

Quoted 8 lines

Quoted 2 lines

Instead of "errno" should we just use "result" and log the value given in the result parameter?

That would likely collide with another field of the same name which is the
operation's results. If it really is errno, the name is fine. It's generic
enough that it can be reused on other events if that mattered.

Quoted 3 lines

Steve, what is the historical reason why we have both "res" and "result" for indicating a boolean success/fail? I'm just curious how we ended up this way, and who may still be using "result".

I think its pam and some other user space things did this. But because of mixed machines in datacenters supporting multiple versions of OS, we have to leave result alone. It has to be 0,1 or success/fail. We cannot use it for errno.

Quoted 5 lines

As Mimi had pointed out, since the value passed in result parameter is not always an error code, "errno" is not an appropriate name.

Can we add a new field, say, "op_result" to report the result of the specified operation?

Sure. But since it is errno sometimes, how would we know when to translate it?

Quoted 31 lines · Steve Grubb

Quoted 30 lines · EDT Lakshmi Ramasubramanian

Quoted 17 lines · Steve Grubb

Quoted 17 lines

Quoted 16 lines

Quoted 11 lines

Quoted 6 lines

The idea is a good idea, but you're assuming that "result" is always errno. That was probably true originally, but isn't now. For example, ima_appraise_measurement() calls xattr_verify(), which compares the security.ima hash with the calculated file hash. On failure, it returns the result of memcmp(). Each and every code path will need to be checked.

Good catch Mimi.

Instead of "errno" should we just use "result" and log the value given in the result parameter?

That would likely collide with another field of the same name which is the operation's results. If it really is errno, the name is fine. It's generic enough that it can be reused on other events if that mattered.

Quoted 8 lines

Quoted 3 lines

Steve, what is the historical reason why we have both "res" and "result" for indicating a boolean success/fail? I'm just curious how we ended up this way, and who may still be using "result".

I think its pam and some other user space things did this. But because of mixed machines in datacenters supporting multiple versions of OS, we have to leave result alone. It has to be 0,1 or success/ fail. We cannot use it for errno.

As Mimi had pointed out, since the value passed in result parameter is not always an error code, "errno" is not an appropriate name.

Quoted 5 lines

Quoted 2 lines

Can we add a new field, say, "op_result" to report the result of the specified operation?

Sure. But since it is errno sometimes, how would we know when to translate it?

Perhaps the solution is not to imply "res" is "errno", but pass it as a separate "errno" field.

Then only include "errno" in the audit message when it isn't zero. This assumes that some audit messages for the same audit number include errno, while others do not.

With this solution, the existing integrity_audit_msg() could become a wrapper for the new function.

Mimi