Error code is not included in the audit messages logged by
the integrity subsystem. Add a new field namely "errno" in
the audit message and set the value to the error code passed
to integrity_audit_msg() in the "result" parameter.
Sample audit message:
[ 6.284329] audit: type=1804 audit(1591756723.627:2): pid=1 uid=0 auid=4294967295 ses=4294967295 subj=kernel op=add_boot_aggregate cause=alloc_entry errno=-12 comm="swapper/0" name="boot_aggregate" res=0
Signed-off-by: Lakshmi Ramasubramanian <[email protected]>
Suggested-by: Steve Grubb <[email protected]>
---
security/integrity/integrity_audit.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
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
On Wed, Jun 10, 2020 at 8:04 PM Lakshmi Ramasubramanian
<[email protected]> 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 <[email protected]>
> Suggested-by: Steve Grubb <[email protected]>
> ---
> 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
http://www.paul-moore.com
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
On Wed, Jun 10, 2020 at 9:58 PM Lakshmi Ramasubramanian
<[email protected]> 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
http://www.paul-moore.com
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 <[email protected]>
> Suggested-by: Steve Grubb <[email protected]>
---
> 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=");
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
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)
On Mon, Jun 15, 2020 at 6:23 PM Steve Grubb <[email protected]> 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
http://www.paul-moore.com
On Monday, June 15, 2020 6:58:13 PM EDT Paul Moore wrote:
> On Mon, Jun 15, 2020 at 6:23 PM Steve Grubb <[email protected]> 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
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
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
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
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