Discussion:
patch suggested by rgb for fixing auditd logs for clone syscall shows exit code as container namespace pid of child process instead of host namespace
(too old to reply)
madz car
2018-01-05 11:00:01 UTC
Permalink
Raw Message
Hi Guys,

Please refer to the issue details at github :
https://github.com/linux-audit/audit-kernel/issues/68

Here is a patch as suggested by rgb, i can confirm that it works.


diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index ecc23e2..9a78ecb 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1557,6 +1557,11 @@ void __audit_syscall_exit(int success, long
return_code)
{
struct task_struct *tsk = current;
struct audit_context *context;
+
+ rcu_read_lock();
+ return_code = pid_nr(find_vpid((int) return_code));
+ rcu_read_unlock();
+

if (success)
success = AUDITSC_SUCCESS;


Kindly review.

Regards,
Madzcar
Steve Grubb
2018-01-05 18:07:16 UTC
Permalink
Raw Message
Post by madz car
Hi Guys,
https://github.com/linux-audit/audit-kernel/issues/68
Here is a patch as suggested by rgb, i can confirm that it works.
By hooking this function, doesn't this change the return code for all
syscalls?

-Steve
Post by madz car
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index ecc23e2..9a78ecb 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1557,6 +1557,11 @@ void __audit_syscall_exit(int success, long
return_code)
{
struct task_struct *tsk = current;
struct audit_context *context;
+
+ rcu_read_lock();
+ return_code = pid_nr(find_vpid((int) return_code));
+ rcu_read_unlock();
+
if (success)
success = AUDITSC_SUCCESS;
Kindly review.
Regards,
Madzcar
Richard Guy Briggs
2018-01-08 12:53:08 UTC
Permalink
Raw Message
Post by Steve Grubb
Post by madz car
Hi Guys,
https://github.com/linux-audit/audit-kernel/issues/68
Here is a patch as suggested by rgb, i can confirm that it works.
By hooking this function, doesn't this change the return code for all
syscalls?
Yes, you are right, Steve. This would give bogus return values for all
other syscalls.

Madzcar, I assume you can confirm that this patch will give incorrect
results for all other syscalls for the "exit" field.

So, that should be in kernel/fork.c:_do_fork(), or rather, just replace
the pid_vnr() call with pid_nr(). However, this will mess up all
callers (clone(2), fork(2), vfork(2) kernel_thread(), do_fork()), who
expect the return value in the caller's PID namespace, so that won't
work. The return value is technically correct for the PID namespace
from which it was called and reported correctly in the audit record.

Madzcar, the way you are trying to interpret the results from the audit
record is clever, but not going to work without another way to translate
that value lifted out of the audit record.

I don't know if there is a userspace tool or call to translate PIDs
between namespaces.
Post by Steve Grubb
-Steve
Post by madz car
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index ecc23e2..9a78ecb 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1557,6 +1557,11 @@ void __audit_syscall_exit(int success, long
return_code)
{
struct task_struct *tsk = current;
struct audit_context *context;
+
+ rcu_read_lock();
+ return_code = pid_nr(find_vpid((int) return_code));
+ rcu_read_unlock();
+
if (success)
success = AUDITSC_SUCCESS;
Kindly review.
Regards,
Madzcar
- RGB

--
Richard Guy Briggs <***@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
madz car
2018-01-08 17:49:48 UTC
Permalink
Raw Message
Rgb/Steve,

Appreciate your response on this issue. However, if you would notice the
pid and ppid values in the same log is in the initial namespace, while the
exit code is in a different namespace. Doesnt this make the audit log
inconsistent? How is an application supposed to analyse the logs when a
single log spans multiple namespaces ? How does an application parsing the
logs get to know if its actually a different pid namespace other than
relying on audit to provide consistent information in the log?

I agree it can mess up other system calls, but atleast in this case for
clone, this issue should be fixed somehow. The audit logs having pid and
ppids in different namespace and exit codes in different namespace is just
making the log impossible to parse. Some of these connections(ssh etc) can
be extremely shortlived and as such audit logs are the only way to track
such events which is not possible without fixing this issue.

Shouldnt we use one namespace in the log and as such all pid, ppids and
exit codes should be translated to that namespace for the log ?

Regards,
Madzcar
Post by Richard Guy Briggs
Post by Steve Grubb
Post by madz car
Hi Guys,
https://github.com/linux-audit/audit-kernel/issues/68
Here is a patch as suggested by rgb, i can confirm that it works.
By hooking this function, doesn't this change the return code for all
syscalls?
Yes, you are right, Steve. This would give bogus return values for all
other syscalls.
Madzcar, I assume you can confirm that this patch will give incorrect
results for all other syscalls for the "exit" field.
So, that should be in kernel/fork.c:_do_fork(), or rather, just replace
the pid_vnr() call with pid_nr(). However, this will mess up all
callers (clone(2), fork(2), vfork(2) kernel_thread(), do_fork()), who
expect the return value in the caller's PID namespace, so that won't
work. The return value is technically correct for the PID namespace
from which it was called and reported correctly in the audit record.
Madzcar, the way you are trying to interpret the results from the audit
record is clever, but not going to work without another way to translate
that value lifted out of the audit record.
I don't know if there is a userspace tool or call to translate PIDs
between namespaces.
Post by Steve Grubb
-Steve
Post by madz car
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index ecc23e2..9a78ecb 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1557,6 +1557,11 @@ void __audit_syscall_exit(int success, long
return_code)
{
struct task_struct *tsk = current;
struct audit_context *context;
+
+ rcu_read_lock();
+ return_code = pid_nr(find_vpid((int) return_code));
+ rcu_read_unlock();
+
if (success)
success = AUDITSC_SUCCESS;
Kindly review.
Regards,
Madzcar
- RGB
--
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
--
Linux-audit mailing list
https://www.redhat.com/mailman/listinfo/linux-audit
Steve Grubb
2018-01-08 19:03:14 UTC
Permalink
Raw Message
Hello,
Post by madz car
Appreciate your response on this issue. However, if you would notice the
pid and ppid values in the same log is in the initial namespace, while the
exit code is in a different namespace. Doesnt this make the audit log
inconsistent?
I would think that pid and ppid should be from the caller/parent's pov. The
exit code, on a successful call, should be the pid that the new process knows
itself by. However, that alone is not enough.
Post by madz car
How is an application supposed to analyse the logs when a single log spans
multiple namespaces ?
That very problem is being worked on. Richard has circulated several drafts
of how the audit subsystem will track processes through namespaces. He is
revising another draft and should circulate it again in the near future.
Post by madz car
How does an application parsing the logs get to know if its actually a
different pid namespace other than relying on audit to provide consistent
information in the log?
There needs to be a tracker ID added to audit records.
Post by madz car
I agree it can mess up other system calls, but at least in this case for
clone, this issue should be fixed somehow.
I think everyone agrees on this. But the details of how it should be done are
still being pinned down.
Post by madz car
The audit logs having pid and ppids in different namespace and exit codes
in different namespace is just making the log impossible to parse.
I think you're trying to do something that we're currently not capable of
doing. When all stakeholders are in agreement with the specification, I am
sure that if anyone has free time to code up the specification or parts of it
so that it lands in the upstream kernel sooner than later, it would be
greatly appreciated.
Post by madz car
Some of these connections(ssh etc) can be extremely shortlived and as such
audit logs are the only way to track such events which is not possible
without fixing this issue.
Right. But the fix is much bigger. The last draft was aired here:
https://www.redhat.com/archives/linux-audit/2017-October/msg00037.html

But a v3 draft should be out at some point.

-Steve
Post by madz car
Shouldnt we use one namespace in the log and as such all pid, ppids and
exit codes should be translated to that namespace for the log ?
Regards,
Madzcar
Post by Richard Guy Briggs
Post by Steve Grubb
Post by madz car
Hi Guys,
https://github.com/linux-audit/audit-kernel/issues/68
Here is a patch as suggested by rgb, i can confirm that it works.
By hooking this function, doesn't this change the return code for all
syscalls?
Yes, you are right, Steve. This would give bogus return values for all
other syscalls.
Madzcar, I assume you can confirm that this patch will give incorrect
results for all other syscalls for the "exit" field.
So, that should be in kernel/fork.c:_do_fork(), or rather, just replace
the pid_vnr() call with pid_nr(). However, this will mess up all
callers (clone(2), fork(2), vfork(2) kernel_thread(), do_fork()), who
expect the return value in the caller's PID namespace, so that won't
work. The return value is technically correct for the PID namespace
from which it was called and reported correctly in the audit record.
Madzcar, the way you are trying to interpret the results from the audit
record is clever, but not going to work without another way to translate
that value lifted out of the audit record.
I don't know if there is a userspace tool or call to translate PIDs
between namespaces.
Post by Steve Grubb
-Steve
Post by madz car
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index ecc23e2..9a78ecb 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1557,6 +1557,11 @@ void __audit_syscall_exit(int success, long
return_code)
{
struct task_struct *tsk = current;
struct audit_context *context;
+
+ rcu_read_lock();
+ return_code = pid_nr(find_vpid((int) return_code));
+ rcu_read_unlock();
+
if (success)
success = AUDITSC_SUCCESS;
Kindly review.
Regards,
Madzcar
- RGB
--
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
--
Linux-audit mailing list
https://www.redhat.com/mailman/listinfo/linux-audit
Richard Guy Briggs
2018-01-09 06:01:10 UTC
Permalink
Raw Message
Post by Steve Grubb
Hello,
Post by madz car
Appreciate your response on this issue. However, if you would notice the
pid and ppid values in the same log is in the initial namespace, while the
exit code is in a different namespace. Doesnt this make the audit log
inconsistent?
I would think that pid and ppid should be from the caller/parent's pov. The
exit code, on a successful call, should be the pid that the new process knows
itself by. However, that alone is not enough.
pid and ppid are fields in an audit record that are reported relative to
the namespace of the audit daemon that receives the message.

The exit code is the pid of the process as it knows itself in the PID
namespace in which it is running. Audit should not be trying to
interpret it.
Post by Steve Grubb
Post by madz car
How is an application supposed to analyse the logs when a single log spans
multiple namespaces ?
That very problem is being worked on. Richard has circulated several drafts
of how the audit subsystem will track processes through namespaces. He is
revising another draft and should circulate it again in the near future.
I have a V3 ready to post.
Post by Steve Grubb
Post by madz car
How does an application parsing the logs get to know if its actually a
different pid namespace other than relying on audit to provide consistent
information in the log?
There needs to be a tracker ID added to audit records.
The main proposal is for container IDs, but part of the proposal also
outlines records to document the namespaces implicated.
Post by Steve Grubb
Post by madz car
I agree it can mess up other system calls, but at least in this case for
clone, this issue should be fixed somehow.
I think everyone agrees on this. But the details of how it should be done are
still being pinned down.
I should have been more clear that I agree it is an issue that needs to
be solved, but not the way proposed. That's why clearly separating the
statement of problem from the proposed solution is helpful and not
getting too attached to the proposed solution as long as the eventual
solution solves the problem.
Post by Steve Grubb
Post by madz car
The audit logs having pid and ppids in different namespace and exit codes
in different namespace is just making the log impossible to parse.
I think you're trying to do something that we're currently not capable of
doing. When all stakeholders are in agreement with the specification, I am
sure that if anyone has free time to code up the specification or parts of it
so that it lands in the upstream kernel sooner than later, it would be
greatly appreciated.
At first I was tempted to add a field to that record that labels that
value as a "cpid" (Child PID) or something like that to make it clear
that value is only valid or available for certain contexts or syscalls,
but on more reflection, seems better placed in an auxilliary record to
avoid it polluting the syscall record, the vast majority for whom that
field won't exist.
Post by Steve Grubb
Post by madz car
Some of these connections(ssh etc) can be extremely shortlived and as such
audit logs are the only way to track such events which is not possible
without fixing this issue.
https://www.redhat.com/archives/linux-audit/2017-October/msg00037.html
But a v3 draft should be out at some point.
Should be very soon.

Madzcar, thank you for reporting this issue and coding up and testing a
solution. I'll review my V3 draft with this issue in mind. Please
review it and provide feedback to the list.

(Note: For kernel patches, please see the kernel patch style guide:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html
In particular, cut/paste mangles tabs.
)
Post by Steve Grubb
-Steve
Post by madz car
Shouldnt we use one namespace in the log and as such all pid, ppids and
exit codes should be translated to that namespace for the log ?
Regards,
Madzcar
Post by Richard Guy Briggs
Post by Steve Grubb
Post by madz car
Hi Guys,
https://github.com/linux-audit/audit-kernel/issues/68
Here is a patch as suggested by rgb, i can confirm that it works.
By hooking this function, doesn't this change the return code for all
syscalls?
Yes, you are right, Steve. This would give bogus return values for all
other syscalls.
Madzcar, I assume you can confirm that this patch will give incorrect
results for all other syscalls for the "exit" field.
So, that should be in kernel/fork.c:_do_fork(), or rather, just replace
the pid_vnr() call with pid_nr(). However, this will mess up all
callers (clone(2), fork(2), vfork(2) kernel_thread(), do_fork()), who
expect the return value in the caller's PID namespace, so that won't
work. The return value is technically correct for the PID namespace
from which it was called and reported correctly in the audit record.
Madzcar, the way you are trying to interpret the results from the audit
record is clever, but not going to work without another way to translate
that value lifted out of the audit record.
I don't know if there is a userspace tool or call to translate PIDs
between namespaces.
Post by Steve Grubb
-Steve
Post by madz car
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index ecc23e2..9a78ecb 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1557,6 +1557,11 @@ void __audit_syscall_exit(int success, long
return_code)
{
struct task_struct *tsk = current;
struct audit_context *context;
+
+ rcu_read_lock();
+ return_code = pid_nr(find_vpid((int) return_code));
+ rcu_read_unlock();
+
if (success)
success = AUDITSC_SUCCESS;
Kindly review.
Regards,
Madzcar
- RGB
--
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
--
Linux-audit mailing list
https://www.redhat.com/mailman/listinfo/linux-audit
- RGB

--
Richard Guy Briggs <***@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
Richard Guy Briggs
2018-01-09 09:54:31 UTC
Permalink
Raw Message
Post by Richard Guy Briggs
Post by Steve Grubb
Hello,
Post by madz car
Appreciate your response on this issue. However, if you would notice the
pid and ppid values in the same log is in the initial namespace, while the
exit code is in a different namespace. Doesnt this make the audit log
inconsistent?
I would think that pid and ppid should be from the caller/parent's pov. The
exit code, on a successful call, should be the pid that the new process knows
itself by. However, that alone is not enough.
pid and ppid are fields in an audit record that are reported relative to
the namespace of the audit daemon that receives the message.
The exit code is the pid of the process as it knows itself in the PID
namespace in which it is running. Audit should not be trying to
interpret it.
Post by Steve Grubb
Post by madz car
How is an application supposed to analyse the logs when a single log spans
multiple namespaces ?
That very problem is being worked on. Richard has circulated several drafts
of how the audit subsystem will track processes through namespaces. He is
revising another draft and should circulate it again in the near future.
I have a V3 ready to post.
Post by Steve Grubb
Post by madz car
How does an application parsing the logs get to know if its actually a
different pid namespace other than relying on audit to provide consistent
information in the log?
There needs to be a tracker ID added to audit records.
The main proposal is for container IDs, but part of the proposal also
outlines records to document the namespaces implicated.
Post by Steve Grubb
Post by madz car
I agree it can mess up other system calls, but at least in this case for
clone, this issue should be fixed somehow.
I think everyone agrees on this. But the details of how it should be done are
still being pinned down.
I should have been more clear that I agree it is an issue that needs to
be solved, but not the way proposed. That's why clearly separating the
statement of problem from the proposed solution is helpful and not
getting too attached to the proposed solution as long as the eventual
solution solves the problem.
Post by Steve Grubb
Post by madz car
The audit logs having pid and ppids in different namespace and exit codes
in different namespace is just making the log impossible to parse.
I think you're trying to do something that we're currently not capable of
doing. When all stakeholders are in agreement with the specification, I am
sure that if anyone has free time to code up the specification or parts of it
so that it lands in the upstream kernel sooner than later, it would be
greatly appreciated.
At first I was tempted to add a field to that record that labels that
value as a "cpid" (Child PID) or something like that to make it clear
that value is only valid or available for certain contexts or syscalls,
but on more reflection, seems better placed in an auxilliary record to
avoid it polluting the syscall record, the vast majority for whom that
field won't exist.
Reviewing my draft V3 proposal again there isn't anywhere I could
justify fitting it in, but that proposal would be the foundation for the
following idea:

What about an auxilliary record to the fork/clone syscall records, say
"AUDIT_FORK" or "AUDIT_CLONE", that records the returned PID in a "pid="
or "cpid=" field that is only generated on success if the audit daemon
is not in the same PID namespace as the caller, or even generate it
unconditionally? (I'm also noticing "opid" or "vm-pid" that might
work.)

Since we are at it, is there any other information that would be really
useful?

I was tempted to add an auxilliary record for each PID namespace up to
the initial PID namespace, but the only one that really matters here it
appears is the one hosting the audit daemon (since that is the one to
which the existing pid and ppid fields are relative).
Post by Richard Guy Briggs
Post by Steve Grubb
Post by madz car
Some of these connections(ssh etc) can be extremely shortlived and as such
audit logs are the only way to track such events which is not possible
without fixing this issue.
https://www.redhat.com/archives/linux-audit/2017-October/msg00037.html
But a v3 draft should be out at some point.
Should be very soon.
Madzcar, thank you for reporting this issue and coding up and testing a
solution. I'll review my V3 draft with this issue in mind. Please
review it and provide feedback to the list.
https://www.kernel.org/doc/html/latest/process/submitting-patches.html
In particular, cut/paste mangles tabs.
)
Post by Steve Grubb
-Steve
Post by madz car
Shouldnt we use one namespace in the log and as such all pid, ppids and
exit codes should be translated to that namespace for the log ?
Regards,
Madzcar
Post by Richard Guy Briggs
Post by Steve Grubb
Post by madz car
Hi Guys,
https://github.com/linux-audit/audit-kernel/issues/68
Here is a patch as suggested by rgb, i can confirm that it works.
By hooking this function, doesn't this change the return code for all
syscalls?
Yes, you are right, Steve. This would give bogus return values for all
other syscalls.
Madzcar, I assume you can confirm that this patch will give incorrect
results for all other syscalls for the "exit" field.
So, that should be in kernel/fork.c:_do_fork(), or rather, just replace
the pid_vnr() call with pid_nr(). However, this will mess up all
callers (clone(2), fork(2), vfork(2) kernel_thread(), do_fork()), who
expect the return value in the caller's PID namespace, so that won't
work. The return value is technically correct for the PID namespace
from which it was called and reported correctly in the audit record.
Madzcar, the way you are trying to interpret the results from the audit
record is clever, but not going to work without another way to translate
that value lifted out of the audit record.
I don't know if there is a userspace tool or call to translate PIDs
between namespaces.
Post by Steve Grubb
-Steve
Post by madz car
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index ecc23e2..9a78ecb 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1557,6 +1557,11 @@ void __audit_syscall_exit(int success, long
return_code)
{
struct task_struct *tsk = current;
struct audit_context *context;
+
+ rcu_read_lock();
+ return_code = pid_nr(find_vpid((int) return_code));
+ rcu_read_unlock();
+
if (success)
success = AUDITSC_SUCCESS;
Kindly review.
Regards,
Madzcar
- RGB
- RGB
- RGB

--
Richard Guy Briggs <***@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
Paul Moore
2018-01-10 16:19:49 UTC
Permalink
Raw Message
Post by Richard Guy Briggs
Post by Steve Grubb
Post by madz car
Hi Guys,
https://github.com/linux-audit/audit-kernel/issues/68
Here is a patch as suggested by rgb, i can confirm that it works.
By hooking this function, doesn't this change the return code for all
syscalls?
Yes, you are right, Steve. This would give bogus return values for all
other syscalls.
Yes, this patch is not something we want to merge.
Post by Richard Guy Briggs
Madzcar, I assume you can confirm that this patch will give incorrect
results for all other syscalls for the "exit" field.
So, that should be in kernel/fork.c:_do_fork(), or rather, just replace
the pid_vnr() call with pid_nr(). However, this will mess up all
callers (clone(2), fork(2), vfork(2) kernel_thread(), do_fork()), who
expect the return value in the caller's PID namespace, so that won't
work. The return value is technically correct for the PID namespace
from which it was called and reported correctly in the audit record.
I think we should just leave the current behavior intact for the time
being; the information being reported is correct, even if it is a bit
confusing outside of the initial PID namespace. Yes, I understand it
may be a bit awkward, but there are plenty of things that are
currently awkward when audit is used with the various
namespaces/containers. The good news is that we are currently working
on trying to solve these issues; it make take some time to get
everything sorted, but solving this as part of the larger, multi-step
effort makes much more sense than a quick and dirty hack now.
Post by Richard Guy Briggs
Madzcar, the way you are trying to interpret the results from the audit
record is clever, but not going to work without another way to translate
that value lifted out of the audit record.
I don't know if there is a userspace tool or call to translate PIDs
between namespaces.
--
paul moore
www.paul-moore.com
Loading...