Re: [PATCH 1/2] integrity: Add errno field in audit message
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
Re: [PATCH 1/2] integrity: Add errno field in audit message
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
Re: [PATCH 1/2] integrity: Add errno field in audit message
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
Re: [PATCH 1/2] integrity: Add errno field in audit message
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
Re: [PATCH 1/2] integrity: Add errno field in audit message
On Monday, June 15, 2020 6:58:13 PM EDT Paul Moore wrote: > On Mon, Jun 15, 2020 at 6:23 PM Steve Grubb 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
Re: [PATCH 1/2] integrity: Add errno field in audit message
On Mon, Jun 15, 2020 at 6:23 PM Steve Grubb 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
Re: [PATCH 1/2] integrity: Add errno field in audit message
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 alphanumericresult of the audited operation(success/fail) > > resultalphanumericresult of the audited operation(success/fail)
Re: [PATCH 1/2] integrity: Add errno field in audit message
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 alphanumericresult of the audited operation(success/fail) result alphanumericresult of the audited operation(success/fail) thanks, -lakshmi
Re: [PATCH 1/2] integrity: Add errno field in audit message
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
> Suggested-by: Steve Grubb
---
> 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(_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=");
Re: [PATCH 1/2] integrity: Add errno field in audit message
On Wed, Jun 10, 2020 at 9:58 PM Lakshmi Ramasubramanian
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
Re: [PATCH 1/2] integrity: Add errno field in audit message
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
Re: [PATCH 1/2] integrity: Add errno field in audit message
On Wed, Jun 10, 2020 at 8:04 PM 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
> Suggested-by: Steve Grubb
> ---
> 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(_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
[PATCH 1/2] integrity: Add errno field in audit message
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
Suggested-by: Steve Grubb
---
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(_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
