On Wed, Nov 17, 2021 at 03:04:31PM -0600, Eric W. Biederman wrote:
Kyle Huey me@kylehuey.com writes:
On Wed, Nov 17, 2021 at 11:05 AM Kyle Huey me@kylehuey.com wrote:
On Wed, Nov 17, 2021 at 10:51 AM Kees Cook keescook@chromium.org wrote:
On Wed, Nov 17, 2021 at 10:47:13AM -0800, Kyle Huey wrote:
rr, a userspace record and replay debugger[0], is completely broken on 5.16rc1. I bisected this to 00b06da29cf9dc633cdba87acd3f57f4df3fd5c7.
That patch makes two changes, it blocks sigaction from changing signal handlers once the kernel has decided to force the program to take a signal and it also stops notifying ptracers of the signal in the same circumstances. The latter behavior is just wrong. There's no reason that ptrace should not be able to observe and even change (non-SIGKILL) forced signals. It should be reverted.
This behavior change is also observable in gdb. If you take a program that sets SIGSYS to SIG_IGN and then raises a SIGSYS via SECCOMP_RET_TRAP and run it under gdb on a good kernel gdb will stop when the SIGSYS is raised, let you inspect program state, etc. After the SA_IMMUTABLE change gdb won't stop until the program has already died of SIGSYS.
Ah, hm, this was trying to fix the case where a program trips SECCOMP_RET_KILL (which is a "fatal SIGSYS"), and had been unobservable before. I guess the fix was too broad...
Perhaps I don't understand precisely what you mean by this, but gdb's behavior for a program that is SECCOMP_RET_KILLed was not changed by this patch (the SIGSYS is not observed until after program exit before or after this change).
The SA_IMMUTABLE change was to deal with failures seen in the seccomp test suite after the recent fatal signal refactoring. Mainly that a process that should have effectively performed do_exit() was suddenly visible to the tracer.
Ah, maybe that behavior changed in 5.15 (my "before" here is a 5.14 kernel). I would argue that the debugger seeing the SIGSYS for SECCOMP_RET_KILL is desirable though ...
This is definitely worth discussing, and probably in need of fixing (aka something in rr seems to have broken).
We definitely need protection against the race with sigaction.
The fundamental question becomes does it make sense and is it safe to allow a debugger to stop at, and possibly change these signals.
I have no problem with a debugger getting notified about a fatal (SECCOMP_RET_KILL*-originated) SIGSYS. But whatever happens, the kernel needs to make sure the process does not continue. (i.e. signal can't be changed/removed/etc.)
Stopping at something SA_IMMUTABLE as long as the signal is allowed to continue and kill the process when PTRACE_CONT happens seems harmless.
Allowing the debugger to change the signal, or change it's handling I don't know.
Right -- I'm fine with a visibility change (the seccomp test suite is just checking for various expected state machine changes across the various signal/death cases: as long as it _dies_, that's what we want. If a extra notification appears before it dies, that's okay, it just needs the test suite to change).
[...] Kees I am back to asking the question I had before I figured out SA_IMMUTABLE. Are there security concerns with debuggers intercepting SECCOMP_RET_KILL.
I see no problem with allowing a tracer to observe the signal, but the signalled process must have no way to continue running. If we end up in such a state, then a seccomp process with access to clone() and ptrace() can escape the seccomp sandbox. This is why seccomp had been using the big do_exit() hammer -- I really want to absolutely never have a bug manifest with a bypassed SECCOMP_RET_KILL: having a completely unavoidable "dying" state is needed.