As Andy pointed out that there are races between force_sig_info_to_task and sigaction[1] when force_sig_info_task. As Kees discovered[2] ptrace is also able to change these signals. In the case of seeccomp killing a process with a signal it is a security violation to allow the signal to be caught or manipulated. Solve this problem by introducing a new flag SA_IMMUTABLE that prevents sigaction and ptrace from modifying these forced signals. This flag is carefully made kernel internal so that no new ABI is introduced. Longer term I think this can be solved by guaranteeing short circuit delivery of signals in this case. Unfortunately reliable and guaranteed short circuit delivery of these signals is still a ways off from being implemented, tested, and merged. So I have implemented a much simpler alternative for now. [1] https://lkml.kernel.org/r/b5d52d25-7bde-4030-a7b1-7c6f8ab90660@www.fastmail.com [2] https://lkml.kernel.org/r/202110281136.5CE65399A7@keescook Cc: stable@vger.kernel.org Fixes: 307d522f5eb8 ("signal/seccomp: Refactor seccomp signal and coredump generation") Tested-by: Andrea Righi <andrea.righi@canonical.com> Tested-by: Kees Cook <keescook@chromium.org> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
Unified · Split
| @@ -70,6 +70,9 @@ struct ksignal { · 69 unmodified lines | ||
| 70 | 70 | int sig; |
| 71 | 71 | }; |
| 72 | 72 | |
| 73 | +/* Used to kill the race between sigaction and forced signals */ | |
| 74 | +#define SA_IMMUTABLE 0x00800000 | |
| 75 | + | |
| 73 | 76 | #ifndef __ARCH_UAPI_SA_FLAGS |
| 74 | 77 | #ifdef SA_RESTORER |
| 75 | 78 | #define __ARCH_UAPI_SA_FLAGS SA_RESTORER |
| @@ -45,6 +45,7 @@ · 44 unmodified lines | ||
| 45 | 45 | #define SA_UNSUPPORTED 0x00000400 |
| 46 | 46 | #define SA_EXPOSE_TAGBITS 0x00000800 |
| 47 | 47 | /* 0x00010000 used on mips */ |
| 48 | +/* 0x00800000 used for internal SA_IMMUTABLE */ | |
| 48 | 49 | /* 0x01000000 used on x86 */ |
| 49 | 50 | /* 0x02000000 used on x86 */ |
| 50 | 51 | /* |
| @@ -1336,6 +1336,7 @@ force_sig_info_to_task(struct kernel_siginfo *info, struct task_struct *t, bool · 1335 unmodified lines | ||
| 1336 | 1336 | blocked = sigismember(&t->blocked, sig); |
| 1337 | 1337 | if (blocked || ignored || sigdfl) { |
| 1338 | 1338 | action->sa.sa_handler = SIG_DFL; |
| 1339 | + action->sa.sa_flags |= SA_IMMUTABLE; | |
| 1339 | 1340 | if (blocked) { |
| 1340 | 1341 | sigdelset(&t->blocked, sig); |
| 1341 | 1342 | recalc_sigpending_and_wake(t); |
| @@ -2760,7 +2761,8 @@ relock: · 1418 unmodified lines | ||
| 2760 | 2761 | if (!signr) |
| 2761 | 2762 | break; /* will return 0 */ |
| 2762 | 2763 | |
| 2763 | − if (unlikely(current->ptrace) && signr != SIGKILL) { | |
| 2764 | + if (unlikely(current->ptrace) && (signr != SIGKILL) && | |
| 2765 | + !(sighand->action[signr -1].sa.sa_flags & SA_IMMUTABLE)) { | |
| 2764 | 2766 | signr = ptrace_signal(signr, &ksig->info); |
| 2765 | 2767 | if (!signr) |
| 2766 | 2768 | continue; |
| @@ -4110,6 +4112,10 @@ int do_sigaction(int sig, struct k_sigaction *act, struct k_sigaction *oact) · 1343 unmodified lines | ||
| 4110 | 4112 | k = &p->sighand->action[sig-1]; |
| 4111 | 4113 | |
| 4112 | 4114 | spin_lock_irq(&p->sighand->siglock); |
| 4115 | + if (k->sa.sa_flags & SA_IMMUTABLE) { | |
| 4116 | + spin_unlock_irq(&p->sighand->siglock); | |
| 4117 | + return -EINVAL; | |
| 4118 | + } | |
| 4113 | 4119 | if (oact) |
| 4114 | 4120 | *oact = *k; |
| 4115 | 4121 | |