* [PATCH 1/2] integrity: Add errno field in audit message
@ 2020-06-11 0:03 Lakshmi Ramasubramanian
2020-06-11 0:04 ` [PATCH 2/2] IMA: Add audit log for failure conditions Lakshmi Ramasubramanian
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Lakshmi Ramasubramanian @ 2020-06-11 0:03 UTC (permalink / raw)
To: zohar, sgrubb, paul; +Cc: rgb, linux-integrity, linux-audit, linux-kernel
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
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 2/2] IMA: Add audit log for failure conditions 2020-06-11 0:03 [PATCH 1/2] integrity: Add errno field in audit message Lakshmi Ramasubramanian @ 2020-06-11 0:04 ` Lakshmi Ramasubramanian 2020-06-12 20:23 ` Mimi Zohar 2020-06-11 1:45 ` [PATCH 1/2] integrity: Add errno field in audit message Paul Moore 2020-06-12 19:25 ` Mimi Zohar 2 siblings, 1 reply; 15+ messages in thread From: Lakshmi Ramasubramanian @ 2020-06-11 0:04 UTC (permalink / raw) To: zohar, sgrubb, paul; +Cc: rgb, linux-integrity, linux-audit, linux-kernel 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] IMA: Add audit log for failure conditions 2020-06-11 0:04 ` [PATCH 2/2] IMA: Add audit log for failure conditions Lakshmi Ramasubramanian @ 2020-06-12 20:23 ` Mimi Zohar 0 siblings, 0 replies; 15+ messages in thread From: Mimi Zohar @ 2020-06-12 20:23 UTC (permalink / raw) To: Lakshmi Ramasubramanian, sgrubb, paul Cc: rgb, linux-integrity, linux-audit, linux-kernel 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] integrity: Add errno field in audit message 2020-06-11 0:03 [PATCH 1/2] integrity: Add errno field in audit message Lakshmi Ramasubramanian 2020-06-11 0:04 ` [PATCH 2/2] IMA: Add audit log for failure conditions Lakshmi Ramasubramanian @ 2020-06-11 1:45 ` Paul Moore 2020-06-11 1:58 ` Lakshmi Ramasubramanian 2020-06-12 19:25 ` Mimi Zohar 2 siblings, 1 reply; 15+ messages in thread From: Paul Moore @ 2020-06-11 1:45 UTC (permalink / raw) To: Lakshmi Ramasubramanian Cc: rgb, linux-kernel, zohar, linux-audit, linux-integrity 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] integrity: Add errno field in audit message 2020-06-11 1:45 ` [PATCH 1/2] integrity: Add errno field in audit message Paul Moore @ 2020-06-11 1:58 ` Lakshmi Ramasubramanian 2020-06-11 2:19 ` Paul Moore 0 siblings, 1 reply; 15+ messages in thread From: Lakshmi Ramasubramanian @ 2020-06-11 1:58 UTC (permalink / raw) To: Paul Moore; +Cc: rgb, linux-kernel, zohar, linux-audit, linux-integrity 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] integrity: Add errno field in audit message 2020-06-11 1:58 ` Lakshmi Ramasubramanian @ 2020-06-11 2:19 ` Paul Moore 0 siblings, 0 replies; 15+ messages in thread From: Paul Moore @ 2020-06-11 2:19 UTC (permalink / raw) To: Lakshmi Ramasubramanian Cc: rgb, linux-kernel, zohar, linux-audit, linux-integrity 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] integrity: Add errno field in audit message 2020-06-11 0:03 [PATCH 1/2] integrity: Add errno field in audit message Lakshmi Ramasubramanian 2020-06-11 0:04 ` [PATCH 2/2] IMA: Add audit log for failure conditions Lakshmi Ramasubramanian 2020-06-11 1:45 ` [PATCH 1/2] integrity: Add errno field in audit message Paul Moore @ 2020-06-12 19:25 ` Mimi Zohar 2020-06-12 19:50 ` Lakshmi Ramasubramanian 2 siblings, 1 reply; 15+ messages in thread From: Mimi Zohar @ 2020-06-12 19:25 UTC (permalink / raw) To: Lakshmi Ramasubramanian, sgrubb, paul Cc: rgb, linux-integrity, linux-audit, linux-kernel 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] integrity: Add errno field in audit message 2020-06-12 19:25 ` Mimi Zohar @ 2020-06-12 19:50 ` Lakshmi Ramasubramanian 2020-06-15 22:23 ` Steve Grubb 0 siblings, 1 reply; 15+ messages in thread From: Lakshmi Ramasubramanian @ 2020-06-12 19:50 UTC (permalink / raw) To: Mimi Zohar, sgrubb, paul; +Cc: rgb, linux-integrity, linux-audit, linux-kernel 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] integrity: Add errno field in audit message 2020-06-12 19:50 ` Lakshmi Ramasubramanian @ 2020-06-15 22:23 ` Steve Grubb 2020-06-15 22:58 ` Paul Moore 0 siblings, 1 reply; 15+ messages in thread From: Steve Grubb @ 2020-06-15 22:23 UTC (permalink / raw) To: Lakshmi Ramasubramanian Cc: rgb, linux-kernel, Mimi Zohar, linux-audit, linux-integrity 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] integrity: Add errno field in audit message 2020-06-15 22:23 ` Steve Grubb @ 2020-06-15 22:58 ` Paul Moore 2020-06-16 15:29 ` Steve Grubb 0 siblings, 1 reply; 15+ messages in thread From: Paul Moore @ 2020-06-15 22:58 UTC (permalink / raw) To: Steve Grubb Cc: rgb, linux-kernel, Mimi Zohar, Lakshmi Ramasubramanian, linux-audit, linux-integrity 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] integrity: Add errno field in audit message 2020-06-15 22:58 ` Paul Moore @ 2020-06-16 15:29 ` Steve Grubb 2020-06-16 15:43 ` Lakshmi Ramasubramanian 0 siblings, 1 reply; 15+ messages in thread From: Steve Grubb @ 2020-06-16 15:29 UTC (permalink / raw) To: Paul Moore Cc: rgb, linux-kernel, Mimi Zohar, Lakshmi Ramasubramanian, linux-audit, linux-integrity 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] integrity: Add errno field in audit message 2020-06-16 15:29 ` Steve Grubb @ 2020-06-16 15:43 ` Lakshmi Ramasubramanian 2020-06-16 15:55 ` Steve Grubb 0 siblings, 1 reply; 15+ messages in thread From: Lakshmi Ramasubramanian @ 2020-06-16 15:43 UTC (permalink / raw) To: Steve Grubb, Paul Moore Cc: rgb, linux-integrity, linux-audit, linux-kernel, Mimi Zohar 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] integrity: Add errno field in audit message 2020-06-16 15:43 ` Lakshmi Ramasubramanian @ 2020-06-16 15:55 ` Steve Grubb 2020-06-16 19:53 ` Mimi Zohar 0 siblings, 1 reply; 15+ messages in thread From: Steve Grubb @ 2020-06-16 15:55 UTC (permalink / raw) To: Lakshmi Ramasubramanian Cc: rgb, linux-kernel, Mimi Zohar, linux-audit, linux-integrity 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] integrity: Add errno field in audit message 2020-06-16 15:55 ` Steve Grubb @ 2020-06-16 19:53 ` Mimi Zohar 2020-06-16 20:28 ` Steve Grubb 0 siblings, 1 reply; 15+ messages in thread From: Mimi Zohar @ 2020-06-16 19:53 UTC (permalink / raw) To: Steve Grubb, Lakshmi Ramasubramanian Cc: rgb, linux-integrity, linux-audit, linux-kernel 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] integrity: Add errno field in audit message 2020-06-16 19:53 ` Mimi Zohar @ 2020-06-16 20:28 ` Steve Grubb 0 siblings, 0 replies; 15+ messages in thread From: Steve Grubb @ 2020-06-16 20:28 UTC (permalink / raw) To: Mimi Zohar Cc: rgb, linux-kernel, Lakshmi Ramasubramanian, linux-audit, linux-integrity 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2020-06-16 20:29 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-06-11 0:03 [PATCH 1/2] integrity: Add errno field in audit message Lakshmi Ramasubramanian 2020-06-11 0:04 ` [PATCH 2/2] IMA: Add audit log for failure conditions Lakshmi Ramasubramanian 2020-06-12 20:23 ` Mimi Zohar 2020-06-11 1:45 ` [PATCH 1/2] integrity: Add errno field in audit message Paul Moore 2020-06-11 1:58 ` Lakshmi Ramasubramanian 2020-06-11 2:19 ` Paul Moore 2020-06-12 19:25 ` Mimi Zohar 2020-06-12 19:50 ` Lakshmi Ramasubramanian 2020-06-15 22:23 ` Steve Grubb 2020-06-15 22:58 ` Paul Moore 2020-06-16 15:29 ` Steve Grubb 2020-06-16 15:43 ` Lakshmi Ramasubramanian 2020-06-16 15:55 ` Steve Grubb 2020-06-16 19:53 ` Mimi Zohar 2020-06-16 20:28 ` Steve Grubb
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).