audit: correct reporting of *execve(2) errors

[kevans]

Summary

rS326145 corrected do_execve() to return EJUSTRETURN upon success so that important registers are not clobbered. This had the side effect of tapping out 'failures' for all *execve(2) audit records, which is less than useful for auditing purposes.

PR: 249179

Test Plan

Before:

header_ex,132,11,execve(2),0,10.1.10.10,Fri Oct 23 10:17:36 2020, + 944 msec
exec arg,./t.sh
path,/tmp/./t.sh
attribute,755,root,0,968485463,32027,2160726950
subject,ltning,root,0,root,0,83815,83621,61751,192.168.127.2
return,failure: Unknown error: 201,4294967295
trailer,132

After:

header,132,11,execve(2),0,Fri Oct 23 23:47:51 2020, + 384 msec
exec arg,./a.out
path,/root/grep/./a.out
attribute,755,root,0,897360606,160136,3214960080
subject,-1,root,0,root,0,1584,0,0,0.0.0.0
return,success,0
trailer,132
header,104,11,execve(2),0,Fri Oct 23 23:47:51 2020, + 384 msec
exec arg,./a.out
path,/nonexistent/binary
subject,-1,root,0,root,0,1584,0,0,0.0.0.0
return,failure : No such file or directory,4294967295
trailer,104
header,121,11,fexecve(2),0,Fri Oct 23 23:47:51 2020, + 385 msec
argument,1,0x3,fd
exec arg,./a.out
attribute,555,root,0,897360606,1113686,2160950516
subject,-1,root,0,root,0,1584,0,0,0.0.0.0
return,success,0
trailer,121

[kevans]

sys/kern/subr_syscall.c

171 ↗

(On Diff #78658)

This might actually need to just unconditionally be td->td_errno so that bsm tooling doesn't interpret posix_* syscalls as successful returns.

[kib]

sys/kern/subr_syscall.c

169 ↗

(On Diff #78658)

I am not sure that I follow the part of the comment about kern_posix_error(). I believe you always do AUDIT_SYSCALL_EXIT(0, td) for kern_posix_error() using some errno > 0.

[kib]

Instead of trying to outguess all syscalls, might be add a TDP flag to indicate that AUDIT on exit does not need to be done. Then, in the special-case syscalls like *execve() and perhaps all users of kern_posix_exit(), call AUDIT_SYSCALL_EXIT() manually when you know the correct errno.

[kevans]

In D26922#600534, @kib wrote:

Instead of trying to outguess all syscalls, might be add a TDP flag to indicate that AUDIT on exit does not need to be done. Then, in the special-case syscalls like *execve() and perhaps all users of kern_posix_exit(), call AUDIT_SYSCALL_EXIT() manually when you know the correct errno.

Hmm... I like it. I will do this.

[kevans]

Audit *execve(2) earlier than syscallenter(). We don't actually need another TDP flag, AUDIT_SYSCALL_EXIT() will unset TDP_AUDITREC making any future AUDIT_SYSCALL_EXIT() a nop anyways. I considered just putting it in do_execve(), but decided that it felt better to put it in the sys_*() methods.

[kib]

What about compat32 ?

[kevans]

compat32 as well, whoops.

[csjp]

In D26922#600559, @kevans wrote:

In D26922#600534, @kib wrote:

Instead of trying to outguess all syscalls, might be add a TDP flag to indicate that AUDIT on exit does not need to be done. Then, in the special-case syscalls like *execve() and perhaps all users of kern_posix_exit(), call AUDIT_SYSCALL_EXIT() manually when you know the correct errno.

Hmm... I like it. I will do this.

Although I think there is still opportunity for things to go wrong, specifically people adding code in between

error = (sa->callp->sy_call)(td, sa->args);

and

AUDIT_SYSCALL_EXIT()

In subr_syscall.c

This is probably the cleanest path forward. I would suggest that we add a comment in subr_syscall.c stating that it's possible certain syscalls may have committed their record early pointing out execve() and that some of that might require re-assessment if code that could change the return value/errnos in between these code blocks. This way people will be aware that they could be breaking some of our assumptions here. Specifically that basically nothing noteworthy would be happening outside of some thread property manipulation.

[csjp]

Otherwise +1 from me

[kevans]

How about:

diff --git a/sys/kern/subr_syscall.c b/sys/kern/subr_syscall.c
index 5ed9a402caa..381756b3732 100644
--- a/sys/kern/subr_syscall.c
+++ b/sys/kern/subr_syscall.c
@@ -154,7 +154,18 @@ syscallenter(struct thread *td)
                        td->td_pflags &= ~TDP_NERRNO;
                else
                        td->td_errno = error;
+
+               /*
+                * Note that some syscall implementations (e.g., sys_execve)
+                * will commit the audit record just before their final return.
+                * These were done under the assumption that nothing of interest
+                * would happen between their return and here, where we would
+                * normally commit the audit record.  These assumptions will
+                * need to be revisited should any substantial logic be added
+                * above.
+                */
                AUDIT_SYSCALL_EXIT(error, td);
+
 #ifdef KDTRACE_HOOKS
                /* Give the syscall:::return DTrace probe a chance to fire. */
                if (__predict_false(sa->callp->sy_return != 0))

[csjp]

+1