[PATCH 1/2] integrity: Add errno field in audit message

[PATCH 1/2] integrity: Add errno field in audit message

[Lakshmi Ramasubramanian]

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 <nramas@linux.microsoft.com>
Suggested-by: Steve Grubb <sgrubb@redhat.com>
---
 security/integrity/integrity_audit.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/security/integrity/integrity_audit.c b/security/integrity/integrity_audit.c
index 5109173839cc..8cbf415bb977 100644
--- a/security/integrity/integrity_audit.c
+++ b/security/integrity/integrity_audit.c
@@ -42,7 +42,8 @@ void integrity_audit_msg(int audit_msgno, struct inode *inode,
 			 from_kuid(&init_user_ns, audit_get_loginuid(current)),
 			 audit_get_sessionid(current));
 	audit_log_task_context(ab);
-	audit_log_format(ab, " op=%s cause=%s comm=", op, cause);
+	audit_log_format(ab, " op=%s cause=%s errno=%d comm=",
+			 op, cause, result);
 	audit_log_untrustedstring(ab, get_task_comm(name, current));
 	if (fname) {
 		audit_log_format(ab, " name=");
-- 
2.27.0


--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH 2/2] IMA: Add audit log for failure conditions

[Lakshmi Ramasubramanian]

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 <nramas@linux.microsoft.com>
Suggested-by: Mimi Zohar <zohar@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)
+{
+	if (func >= MAX_CHECK)
+		return ima_hooks_measure_str[NONE];
+
+	return ima_hooks_measure_str[func];
+}
+
 extern const char *const func_tokens[];
 
 struct modsig;
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 800fb3bba418..114b0699fb02 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -739,6 +739,7 @@ void process_buffer_measurement(const void *buf, int size,
 				int pcr, const char *keyring)
 {
 	int ret = 0;
+	const char *audit_cause = "ENOMEM";
 	struct ima_template_entry *entry = NULL;
 	struct integrity_iint_cache iint = {};
 	struct ima_event_data event_data = {.iint = &iint,
@@ -793,21 +794,28 @@ void process_buffer_measurement(const void *buf, int size,
 	iint.ima_hash->length = hash_digest_size[ima_hash_algo];
 
 	ret = ima_calc_buffer_hash(buf, size, iint.ima_hash);
-	if (ret < 0)
+	if (ret < 0) {
+		audit_cause = "hashing_error";
 		goto out;
+	}
 
 	ret = ima_alloc_init_template(&event_data, &entry, template);
-	if (ret < 0)
+	if (ret < 0) {
+		audit_cause = "alloc_entry";
 		goto out;
+	}
 
 	ret = ima_store_template(entry, violation, NULL, buf, pcr);
-
-	if (ret < 0)
+	if (ret < 0) {
+		audit_cause = "store_entry";
 		ima_free_template_entry(entry);
+	}
 
 out:
 	if (ret < 0)
-		pr_devel("%s: failed, result: %d\n", __func__, ret);
+		integrity_audit_msg(AUDIT_INTEGRITY_PCR, NULL, eventname,
+				    ima_hooks_func_measure_str(func),
+				    audit_cause, ret, 0);
 
 	return;
 }
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index e493063a3c34..66aa3e17a888 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -1414,7 +1414,7 @@ void ima_delete_rules(void)
 	}
 }
 
-#define __ima_hook_stringify(str)	(#str),
+#define __ima_hook_stringify(func, str)	(#func),
 
 const char *const func_tokens[] = {
 	__ima_hooks(__ima_hook_stringify)
diff --git a/security/integrity/ima/ima_queue_keys.c b/security/integrity/ima/ima_queue_keys.c
index cb3e3f501593..f1105e0b8eef 100644
--- a/security/integrity/ima/ima_queue_keys.c
+++ b/security/integrity/ima/ima_queue_keys.c
@@ -68,6 +68,7 @@ static struct ima_key_entry *ima_alloc_key_entry(struct key *keyring,
 						 size_t payload_len)
 {
 	int rc = 0;
+	const char *audit_cause = "ENOMEM";
 	struct ima_key_entry *entry;
 
 	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
@@ -88,6 +89,10 @@ static struct ima_key_entry *ima_alloc_key_entry(struct key *keyring,
 
 out:
 	if (rc) {
+		integrity_audit_msg(AUDIT_INTEGRITY_PCR, NULL,
+				    keyring->description,
+				    ima_hooks_func_measure_str(KEY_CHECK),
+				    audit_cause, rc, 0);
 		ima_free_key_entry(entry);
 		entry = NULL;
 	}
-- 
2.27.0


--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH 1/2] integrity: Add errno field in audit message

[Paul Moore]

On Wed, Jun 10, 2020 at 8:04 PM Lakshmi Ramasubramanian
<nramas@linux.microsoft.com> wrote:
>
> 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 <nramas@linux.microsoft.com>
> Suggested-by: Steve Grubb <sgrubb@redhat.com>
> ---
>  security/integrity/integrity_audit.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/security/integrity/integrity_audit.c b/security/integrity/integrity_audit.c
> index 5109173839cc..8cbf415bb977 100644
> --- a/security/integrity/integrity_audit.c
> +++ b/security/integrity/integrity_audit.c
> @@ -42,7 +42,8 @@ void integrity_audit_msg(int audit_msgno, struct inode *inode,
>                          from_kuid(&init_user_ns, audit_get_loginuid(current)),
>                          audit_get_sessionid(current));
>         audit_log_task_context(ab);
> -       audit_log_format(ab, " op=%s cause=%s comm=", op, cause);
> +       audit_log_format(ab, " op=%s cause=%s errno=%d comm=",
> +                        op, cause, result);

Hi Lakshmi,

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.

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

-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH 1/2] integrity: Add errno field in audit message

[Lakshmi Ramasubramanian]

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

Hi Paul,

> 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.
> 
>>          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.

thanks,
  -lakshmi


--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH 1/2] integrity: Add errno field in audit message

[Paul Moore]

On Wed, Jun 10, 2020 at 9:58 PM Lakshmi Ramasubramanian
<nramas@linux.microsoft.com> wrote:
> On 6/10/20 6:45 PM, Paul Moore wrote:
>
> Hi Paul,
>
> > 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.
> >
> >>          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.

Steve and I have a different opinion on this issue.  I won't rehash
the long argument or drag you into it, but I will just say that the
*kernel* has had a policy of only adding fields to the end of existing
records unless under extreme cases (this is not an extreme case).

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

Great, please do that.  Thank you.

-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH 1/2] integrity: Add errno field in audit message

[Mimi Zohar]

On Wed, 2020-06-10 at 17:03 -0700, Lakshmi Ramasubramanian wrote:
> 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 <nramas@linux.microsoft.com>
> Suggested-by: Steve Grubb <sgrubb@redhat.com>
---
>  security/integrity/integrity_audit.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/security/integrity/integrity_audit.c b/security/integrity/integrity_audit.c
> index 5109173839cc..8cbf415bb977 100644
> --- a/security/integrity/integrity_audit.c
> +++ b/security/integrity/integrity_audit.c
> @@ -42,7 +42,8 @@ void integrity_audit_msg(int audit_msgno, struct inode *inode,
>  			 from_kuid(&init_user_ns, audit_get_loginuid(current)),
>  			 audit_get_sessionid(current));
>  	audit_log_task_context(ab);
> -	audit_log_format(ab, " op=%s cause=%s comm=", op, cause);
> +	audit_log_format(ab, " op=%s cause=%s errno=%d comm=",
> +			 op, cause, result);

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.

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


--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH 1/2] integrity: Add errno field in audit message

[Lakshmi Ramasubramanian]

On 6/12/20 12:25 PM, Mimi Zohar wrote:

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

thanks,
  -lakshmi


--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH 2/2] IMA: Add audit log for failure conditions

[Mimi Zohar]

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@linux.microsoft.com>
> Suggested-by: Mimi Zohar <zohar@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[];


--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH 1/2] integrity: Add errno field in audit message

[Steve Grubb]

On Friday, June 12, 2020 3:50:14 PM EDT Lakshmi Ramasubramanian wrote:
> On 6/12/20 12:25 PM, Mimi Zohar wrote:
> > 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.


>  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?

It should only be 0 or 1. Sometime in the past it may have been the words 
success/fail. But we've been converting those as we find them.

-Steve

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




--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH 1/2] integrity: Add errno field in audit message

[Paul Moore]

On Mon, Jun 15, 2020 at 6:23 PM Steve Grubb <sgrubb@redhat.com> wrote:
> On Friday, June 12, 2020 3:50:14 PM EDT Lakshmi Ramasubramanian wrote:
> > On 6/12/20 12:25 PM, Mimi Zohar wrote:
> > > 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.

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".

-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH 1/2] integrity: Add errno field in audit message

[Steve Grubb]

On Monday, June 15, 2020 6:58:13 PM EDT Paul Moore wrote:
> On Mon, Jun 15, 2020 at 6:23 PM Steve Grubb <sgrubb@redhat.com> wrote:
> > On Friday, June 12, 2020 3:50:14 PM EDT Lakshmi Ramasubramanian wrote:
> > > On 6/12/20 12:25 PM, Mimi Zohar wrote:
> > > > 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.
> 
> 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.

-Steve


--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH 1/2] integrity: Add errno field in audit message

[Lakshmi Ramasubramanian]

On 6/16/20 8:29 AM, Steve Grubb wrote:

>>>>> 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.
>>
>> 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?

thanks,
  -lakshmi


--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH 1/2] integrity: Add errno field in audit message

[Steve Grubb]

On Tuesday, June 16, 2020 11:43:31 AM EDT Lakshmi Ramasubramanian wrote:
> On 6/16/20 8:29 AM, Steve Grubb wrote:
> >>>>> 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.
> >> 
> >> 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?

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

-Steve



--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH 1/2] integrity: Add errno field in audit message

[Mimi Zohar]

On Tue, 2020-06-16 at 11:55 -0400, Steve Grubb wrote:
> On Tuesday, June 16, 2020 11:43:31 AM EDT Lakshmi Ramasubramanian wrote:
> > On 6/16/20 8:29 AM, Steve Grubb wrote:
> > >>>>> 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.
> > >> 
> > >> 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?
> 
> 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


--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH 1/2] integrity: Add errno field in audit message

[Steve Grubb]

On Tuesday, June 16, 2020 3:53:40 PM EDT Mimi Zohar wrote:
> On Tue, 2020-06-16 at 11:55 -0400, Steve Grubb wrote:
> > On Tuesday, June 16, 2020 11:43:31 AM EDT Lakshmi Ramasubramanian wrote:
> > > On 6/16/20 8:29 AM, Steve Grubb wrote:
> > > >>>>> 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.
> > > >> 
> > > >> 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?
> > 
> > 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.

That's what is done on syscalls. There is success and exit where they both 
have different meaning sometimes but otherwise they agree.

> 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.

We normally do not like to have fields that swing in and out because then its 
hard to know exactly what's in the event. When an event has different fields 
under special conditions, then we just say call it a new event. Split it into 
2 or 3 instead forcing it all into 1. And we also do not like fields that 
change meaning. Because then intepretation becomes hard. Or other people 
wishing to record the same info in another event have to follow the same 
pattern.

So, if you really need this field, the give some name like err_code or errno 
or anything not taken. And just fill it out every time. Its OK to be 0. If 
this only happens under some special operation, then make it a new event and 
fill it out only for that operation/event.

Best Regards,
-Steve


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




--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit