[PATCH] Don't allow blocking of signals using sigreturn.

[PATCH] Don't allow blocking of signals using sigreturn.

[Jann Horn]

This prevents processes running under seccomp, including
SECCOMP_MODE_STRICT, from abusing sigreturn() to block
signals other than SIGKILL intended to kill them.
(For example, someone might decide to use alarm() to
restrict the wall-clock runtime of a seccomp-restricted
process.)

A side effect is that after this change, unblocking
signals in a signal handler is a persistent change, not
a change that can be reversed by returning from the
signal handler.

Is that side effect tolerable, or does the patch have to
be rewritten so that removing signals from the signal
mask in a signal handler stays temporary? (That would
probably make it more complicated.)

Or should I throw this patch away and write a patch
for the prctl() manpage instead that documents that
being able to call sigreturn() implies being able to
effectively call sigprocmask(), at least on some
architectures like X86?

(I hope I got the list of recipients right?)

Signed-off-by: Jann Horn <jann@thejh.net>
---
 arch/arm/kernel/signal.c     |  2 +-
 arch/arm64/kernel/signal.c   |  2 +-
 arch/arm64/kernel/signal32.c |  2 +-
 arch/x86/ia32/ia32_signal.c  |  4 ++--
 arch/x86/kernel/signal.c     |  6 +++---
 arch/x86/um/signal.c         |  4 ++--
 include/linux/signal.h       |  1 +
 kernel/signal.c              | 16 ++++++++++++++++
 8 files changed, 27 insertions(+), 10 deletions(-)

diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
index 023ac90..0659e7e 100644
--- a/arch/arm/kernel/signal.c
+++ b/arch/arm/kernel/signal.c
@@ -147,7 +147,7 @@ static int restore_sigframe(struct pt_regs *regs, struct sigframe __user *sf)
 
 	err = __copy_from_user(&set, &sf->uc.uc_sigmask, sizeof(set));
 	if (err == 0)
-		set_current_blocked(&set);
+		unblock_current(&set);
 
 	__get_user_error(regs->ARM_r0, &sf->uc.uc_mcontext.arm_r0, err);
 	__get_user_error(regs->ARM_r1, &sf->uc.uc_mcontext.arm_r1, err);
diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index 660ccf9..f2f035a 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -101,7 +101,7 @@ static int restore_sigframe(struct pt_regs *regs,
 
 	err = __copy_from_user(&set, &sf->uc.uc_sigmask, sizeof(set));
 	if (err == 0)
-		set_current_blocked(&set);
+		unblock_current(&set);
 
 	for (i = 0; i < 31; i++)
 		__get_user_error(regs->regs[i], &sf->uc.uc_mcontext.regs[i],
diff --git a/arch/arm64/kernel/signal32.c b/arch/arm64/kernel/signal32.c
index d26fcd4..28332ed 100644
--- a/arch/arm64/kernel/signal32.c
+++ b/arch/arm64/kernel/signal32.c
@@ -306,7 +306,7 @@ static int compat_restore_sigframe(struct pt_regs *regs,
 	err = get_sigset_t(&set, &sf->uc.uc_sigmask);
 	if (err == 0) {
 		sigdelsetmask(&set, ~_BLOCKABLE);
-		set_current_blocked(&set);
+		unblock_current(&set);
 	}
 
 	__get_user_error(regs->regs[0], &sf->uc.uc_mcontext.arm_r0, err);
diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c
index d0165c9..09be79c 100644
--- a/arch/x86/ia32/ia32_signal.c
+++ b/arch/x86/ia32/ia32_signal.c
@@ -222,7 +222,7 @@ asmlinkage long sys32_sigreturn(void)
 				    sizeof(frame->extramask))))
 		goto badframe;
 
-	set_current_blocked(&set);
+	unblock_current(&set);
 
 	if (ia32_restore_sigcontext(regs, &frame->sc, &ax))
 		goto badframe;
@@ -247,7 +247,7 @@ asmlinkage long sys32_rt_sigreturn(void)
 	if (__copy_from_user(&set, &frame->uc.uc_sigmask, sizeof(set)))
 		goto badframe;
 
-	set_current_blocked(&set);
+	unblock_current(&set);
 
 	if (ia32_restore_sigcontext(regs, &frame->uc.uc_mcontext, &ax))
 		goto badframe;
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index e504246..dcc50c51 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -551,7 +551,7 @@ asmlinkage unsigned long sys_sigreturn(void)
 				    sizeof(frame->extramask))))
 		goto badframe;
 
-	set_current_blocked(&set);
+	unblock_current(&set);
 
 	if (restore_sigcontext(regs, &frame->sc, &ax))
 		goto badframe;
@@ -577,7 +577,7 @@ asmlinkage long sys_rt_sigreturn(void)
 	if (__copy_from_user(&set, &frame->uc.uc_sigmask, sizeof(set)))
 		goto badframe;
 
-	set_current_blocked(&set);
+	unblock_current(&set);
 
 	if (restore_sigcontext(regs, &frame->uc.uc_mcontext, &ax))
 		goto badframe;
@@ -789,7 +789,7 @@ asmlinkage long sys32_x32_rt_sigreturn(void)
 	if (__copy_from_user(&set, &frame->uc.uc_sigmask, sizeof(set)))
 		goto badframe;
 
-	set_current_blocked(&set);
+	unblock_current(&set);
 
 	if (restore_sigcontext(regs, &frame->uc.uc_mcontext, &ax))
 		goto badframe;
diff --git a/arch/x86/um/signal.c b/arch/x86/um/signal.c
index 0c8c32b..15bb054 100644
--- a/arch/x86/um/signal.c
+++ b/arch/x86/um/signal.c
@@ -476,7 +476,7 @@ long sys_sigreturn(void)
 	    copy_from_user(&set.sig[1], extramask, sig_size))
 		goto segfault;
 
-	set_current_blocked(&set);
+	unblock_current(&set);
 
 	if (copy_sc_from_user(&current->thread.regs, sc))
 		goto segfault;
@@ -584,7 +584,7 @@ long sys_rt_sigreturn(void)
 	if (copy_from_user(&set, &uc->uc_sigmask, sizeof(set)))
 		goto segfault;
 
-	set_current_blocked(&set);
+	unblock_current(&set);
 
 	if (copy_sc_from_user(&current->thread.regs, &uc->uc_mcontext))
 		goto segfault;
diff --git a/include/linux/signal.h b/include/linux/signal.h
index ab1e039..371dd19 100644
--- a/include/linux/signal.h
+++ b/include/linux/signal.h
@@ -238,6 +238,7 @@ extern int do_sigtimedwait(const sigset_t *, siginfo_t *,
 extern int sigprocmask(int, sigset_t *, sigset_t *);
 extern void set_current_blocked(sigset_t *);
 extern void __set_current_blocked(const sigset_t *);
+extern void unblock_current(const sigset_t *);
 extern int show_unhandled_signals;
 extern int sigsuspend(sigset_t *);
 
diff --git a/kernel/signal.c b/kernel/signal.c
index a390499..48be2ca 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2544,6 +2544,22 @@ void __set_current_blocked(const sigset_t *newset)
 	spin_unlock_irq(&tsk->sighand->siglock);
 }
 
+/**
+ * unblock_current - change current->blocked mask, but only allow
+ * unblocking of signals.
+ * @newset: new mask
+ *
+ * This method should be used in sigreturn implementations to prevent
+ * seccomp-isolated processes from blocking signals using sigreturn.
+ */
+void unblock_current(const sigset_t *newset)
+{
+	spin_lock_irq(&current->sighand->siglock);
+	sigandsets(&current->blocked, &current->blocked, newset);
+	recalc_sigpending();
+	spin_unlock_irq(&current->sighand->siglock);
+}
+
 /*
  * This is also useful for kernel threads that want to temporarily
  * (or permanently) block certain signals.
-- 
2.1.4

Re: [PATCH] Don't allow blocking of signals using sigreturn.

[Mikael Pettersson]

Jann Horn writes:
 > Or should I throw this patch away and write a patch
 > for the prctl() manpage instead that documents that
 > being able to call sigreturn() implies being able to
 > effectively call sigprocmask(), at least on some
 > architectures like X86?

Well, that is the semantics of sigreturn().  It is essentially
setcontext() [which includes the actions of sigprocmask()], but
with restrictions on parameter placement (at least on x86).

You could introduce some setting to restrict that aspect for
seccomp processes, but you can't change this for normal processes
without breaking things.

Re: [PATCH] Don't allow blocking of signals using sigreturn.

[Andy Lutomirski]

On Wed, Mar 11, 2015 at 2:43 PM, Mikael Pettersson <mikpelinux@gmail.com> wrote:
> Jann Horn writes:
>  > Or should I throw this patch away and write a patch
>  > for the prctl() manpage instead that documents that
>  > being able to call sigreturn() implies being able to
>  > effectively call sigprocmask(), at least on some
>  > architectures like X86?
>
> Well, that is the semantics of sigreturn().  It is essentially
> setcontext() [which includes the actions of sigprocmask()], but
> with restrictions on parameter placement (at least on x86).
>
> You could introduce some setting to restrict that aspect for
> seccomp processes, but you can't change this for normal processes
> without breaking things.

Which leads to the interesting question: does anyone ever call
sigreturn with a different signal mask than the kernel put there
during signal delivery or, even more strangely, with a totally made up
context?  I suspect that the former does happen, even if the latter
may be rare or completely implausible.

I certainly have code that modifies GPRs in the context prior to sigreturn.

--Andy

-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [PATCH] Don't allow blocking of signals using sigreturn.

[Mikael Pettersson]

Andy Lutomirski writes:
 > On Wed, Mar 11, 2015 at 2:43 PM, Mikael Pettersson <mikpelinux@gmail.com> wrote:
 > > Jann Horn writes:
 > >  > Or should I throw this patch away and write a patch
 > >  > for the prctl() manpage instead that documents that
 > >  > being able to call sigreturn() implies being able to
 > >  > effectively call sigprocmask(), at least on some
 > >  > architectures like X86?
 > >
 > > Well, that is the semantics of sigreturn().  It is essentially
 > > setcontext() [which includes the actions of sigprocmask()], but
 > > with restrictions on parameter placement (at least on x86).
 > >
 > > You could introduce some setting to restrict that aspect for
 > > seccomp processes, but you can't change this for normal processes
 > > without breaking things.
 > 
 > Which leads to the interesting question: does anyone ever call
 > sigreturn with a different signal mask than the kernel put there
 > during signal delivery

Yes.  Either a sigfillset();sigdelset(SIGSEGV), or a copy of the
thread's sigmask from a previous sigframe.

 > or, even more strangely, with a totally made up
 > context?

Not "totally made up", but certainly with adjustments(*) made to
both GPRs and PC.  In a different piece of SW: FPU controls.

(*) Rolling back or force-committing a micro-transaction until
PC+GPRs represent the state at an original instruction boundary.
This was in a product using dynamic binary instrumentation.

[PATCH] seccomp.2: Add note about alarm(2) not being sufficient to limit runtime

[Jann Horn]

On Wed, Mar 11, 2015 at 10:43:50PM +0100, Mikael Pettersson wrote:
> Jann Horn writes:
>  > Or should I throw this patch away and write a patch
>  > for the prctl() manpage instead that documents that
>  > being able to call sigreturn() implies being able to
>  > effectively call sigprocmask(), at least on some
>  > architectures like X86?
> 
> Well, that is the semantics of sigreturn().  It is essentially
> setcontext() [which includes the actions of sigprocmask()], but
> with restrictions on parameter placement (at least on x86).
> 
> You could introduce some setting to restrict that aspect for
> seccomp processes, but you can't change this for normal processes
> without breaking things.

Then I think it's probably better and easier to just document the existing
behavior? If a new setting would have to be introduced and developers would
need to be aware of that, it's probably easier to just tell everyone to use
SIGKILL.

Does this manpage patch look good?

---
 man2/seccomp.2 | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/man2/seccomp.2 b/man2/seccomp.2
index 702ceb8..f762d07 100644
--- a/man2/seccomp.2
+++ b/man2/seccomp.2
@@ -64,6 +64,31 @@ Strict secure computing mode is useful for number-crunching
 applications that may need to execute untrusted byte code, perhaps
 obtained by reading from a pipe or socket.
 
+Note that although the calling thread can no longer call
+.BR sigprocmask (2),
+it can use
+.BR sigreturn (2)
+to block all signals apart from
+.BR SIGKILL
+and
+.BR SIGSTOP .
+Therefore, to reliably terminate it,
+.BR SIGKILL
+has to be used, meaning that e.g.
+.BR alarm (2)
+is not sufficient for restricting its runtime. Instead, use
+.BR timer_create (2)
+with
+.BR SIGEV_SIGNAL
+and
+.BR sigev_signo
+set to
+.BR SIGKILL
+or use
+.BR setrlimit (2)
+to set the hard limit for
+.BR RLIMIT_CPU .
+
 This operation is available only if the kernel is configured with
 .BR CONFIG_SECCOMP
 enabled.
-- 
2.1.4

Re: [PATCH] seccomp.2: Add note about alarm(2) not being sufficient to limit runtime

[Andy Lutomirski]

On Thu, Mar 12, 2015 at 6:07 AM, Jann Horn <jann@thejh.net> wrote:
> On Wed, Mar 11, 2015 at 10:43:50PM +0100, Mikael Pettersson wrote:
>> Jann Horn writes:
>>  > Or should I throw this patch away and write a patch
>>  > for the prctl() manpage instead that documents that
>>  > being able to call sigreturn() implies being able to
>>  > effectively call sigprocmask(), at least on some
>>  > architectures like X86?
>>
>> Well, that is the semantics of sigreturn().  It is essentially
>> setcontext() [which includes the actions of sigprocmask()], but
>> with restrictions on parameter placement (at least on x86).
>>
>> You could introduce some setting to restrict that aspect for
>> seccomp processes, but you can't change this for normal processes
>> without breaking things.
>
> Then I think it's probably better and easier to just document the existing
> behavior? If a new setting would have to be introduced and developers would
> need to be aware of that, it's probably easier to just tell everyone to use
> SIGKILL.
>
> Does this manpage patch look good?

Looks good to me.

FWIW, if we wanted to fix this in the kernel, I think it could be
easier to add SIG_KILL which would be just like SIG_DFL except always
fatal even if masked rather than coming up with complicated changes to
sigreturn.

--Andy

>
> ---
>  man2/seccomp.2 | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>
> diff --git a/man2/seccomp.2 b/man2/seccomp.2
> index 702ceb8..f762d07 100644
> --- a/man2/seccomp.2
> +++ b/man2/seccomp.2
> @@ -64,6 +64,31 @@ Strict secure computing mode is useful for number-crunching
>  applications that may need to execute untrusted byte code, perhaps
>  obtained by reading from a pipe or socket.
>
> +Note that although the calling thread can no longer call
> +.BR sigprocmask (2),
> +it can use
> +.BR sigreturn (2)
> +to block all signals apart from
> +.BR SIGKILL
> +and
> +.BR SIGSTOP .
> +Therefore, to reliably terminate it,
> +.BR SIGKILL
> +has to be used, meaning that e.g.
> +.BR alarm (2)
> +is not sufficient for restricting its runtime. Instead, use
> +.BR timer_create (2)
> +with
> +.BR SIGEV_SIGNAL
> +and
> +.BR sigev_signo
> +set to
> +.BR SIGKILL
> +or use
> +.BR setrlimit (2)
> +to set the hard limit for
> +.BR RLIMIT_CPU .
> +
>  This operation is available only if the kernel is configured with
>  .BR CONFIG_SECCOMP
>  enabled.
> --
> 2.1.4
>



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [PATCH] seccomp.2: Add note about alarm(2) not being sufficient to limit runtime

[Kees Cook]

On Thu, Mar 12, 2015 at 6:07 AM, Jann Horn <jann@thejh.net> wrote:
> On Wed, Mar 11, 2015 at 10:43:50PM +0100, Mikael Pettersson wrote:
>> Jann Horn writes:
>>  > Or should I throw this patch away and write a patch
>>  > for the prctl() manpage instead that documents that
>>  > being able to call sigreturn() implies being able to
>>  > effectively call sigprocmask(), at least on some
>>  > architectures like X86?
>>
>> Well, that is the semantics of sigreturn().  It is essentially
>> setcontext() [which includes the actions of sigprocmask()], but
>> with restrictions on parameter placement (at least on x86).
>>
>> You could introduce some setting to restrict that aspect for
>> seccomp processes, but you can't change this for normal processes
>> without breaking things.
>
> Then I think it's probably better and easier to just document the existing
> behavior? If a new setting would have to be introduced and developers would
> need to be aware of that, it's probably easier to just tell everyone to use
> SIGKILL.
>
> Does this manpage patch look good?
>
> ---
>  man2/seccomp.2 | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>
> diff --git a/man2/seccomp.2 b/man2/seccomp.2
> index 702ceb8..f762d07 100644
> --- a/man2/seccomp.2
> +++ b/man2/seccomp.2
> @@ -64,6 +64,31 @@ Strict secure computing mode is useful for number-crunching
>  applications that may need to execute untrusted byte code, perhaps
>  obtained by reading from a pipe or socket.
>
> +Note that although the calling thread can no longer call
> +.BR sigprocmask (2),
> +it can use
> +.BR sigreturn (2)
> +to block all signals apart from
> +.BR SIGKILL
> +and
> +.BR SIGSTOP .
> +Therefore, to reliably terminate it,
> +.BR SIGKILL
> +has to be used, meaning that e.g.
> +.BR alarm (2)
> +is not sufficient for restricting its runtime. Instead, use
> +.BR timer_create (2)
> +with
> +.BR SIGEV_SIGNAL
> +and
> +.BR sigev_signo
> +set to
> +.BR SIGKILL
> +or use
> +.BR setrlimit (2)
> +to set the hard limit for
> +.BR RLIMIT_CPU .
> +
>  This operation is available only if the kernel is configured with
>  .BR CONFIG_SECCOMP
>  enabled.
> --
> 2.1.4
>

Thanks! This looks good.

Acked-by: Kees Cook <keescook@chromium.org>

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH] seccomp.2: Add note about alarm(2) not being sufficient to limit runtime

[Mikael Pettersson]

Jann Horn writes:
 > On Wed, Mar 11, 2015 at 10:43:50PM +0100, Mikael Pettersson wrote:
 > > Jann Horn writes:
 > >  > Or should I throw this patch away and write a patch
 > >  > for the prctl() manpage instead that documents that
 > >  > being able to call sigreturn() implies being able to
 > >  > effectively call sigprocmask(), at least on some
 > >  > architectures like X86?
 > > 
 > > Well, that is the semantics of sigreturn().  It is essentially
 > > setcontext() [which includes the actions of sigprocmask()], but
 > > with restrictions on parameter placement (at least on x86).
 > > 
 > > You could introduce some setting to restrict that aspect for
 > > seccomp processes, but you can't change this for normal processes
 > > without breaking things.
 > 
 > Then I think it's probably better and easier to just document the existing
 > behavior? If a new setting would have to be introduced and developers would
 > need to be aware of that, it's probably easier to just tell everyone to use
 > SIGKILL.
 > 
 > Does this manpage patch look good?

LGTM

Acked-by: Mikael Pettersson <mikpelinux@gmail.com>

 > 
 > ---
 >  man2/seccomp.2 | 25 +++++++++++++++++++++++++
 >  1 file changed, 25 insertions(+)
 > 
 > diff --git a/man2/seccomp.2 b/man2/seccomp.2
 > index 702ceb8..f762d07 100644
 > --- a/man2/seccomp.2
 > +++ b/man2/seccomp.2
 > @@ -64,6 +64,31 @@ Strict secure computing mode is useful for number-crunching
 >  applications that may need to execute untrusted byte code, perhaps
 >  obtained by reading from a pipe or socket.
 >  
 > +Note that although the calling thread can no longer call
 > +.BR sigprocmask (2),
 > +it can use
 > +.BR sigreturn (2)
 > +to block all signals apart from
 > +.BR SIGKILL
 > +and
 > +.BR SIGSTOP .
 > +Therefore, to reliably terminate it,
 > +.BR SIGKILL
 > +has to be used, meaning that e.g.
 > +.BR alarm (2)
 > +is not sufficient for restricting its runtime. Instead, use
 > +.BR timer_create (2)
 > +with
 > +.BR SIGEV_SIGNAL
 > +and
 > +.BR sigev_signo
 > +set to
 > +.BR SIGKILL
 > +or use
 > +.BR setrlimit (2)
 > +to set the hard limit for
 > +.BR RLIMIT_CPU .
 > +
 >  This operation is available only if the kernel is configured with
 >  .BR CONFIG_SECCOMP
 >  enabled.
 > -- 
 > 2.1.4

-- 

Re: [PATCH] seccomp.2: Add note about alarm(2) not being sufficient to limit runtime

[Michael Kerrisk (man-pages)]

Hello Jann,

On 03/12/2015 02:07 PM, Jann Horn wrote:
> On Wed, Mar 11, 2015 at 10:43:50PM +0100, Mikael Pettersson wrote:
>> Jann Horn writes:
>>  > Or should I throw this patch away and write a patch
>>  > for the prctl() manpage instead that documents that
>>  > being able to call sigreturn() implies being able to
>>  > effectively call sigprocmask(), at least on some
>>  > architectures like X86?
>>
>> Well, that is the semantics of sigreturn().  It is essentially
>> setcontext() [which includes the actions of sigprocmask()], but
>> with restrictions on parameter placement (at least on x86).
>>
>> You could introduce some setting to restrict that aspect for
>> seccomp processes, but you can't change this for normal processes
>> without breaking things.
> 
> Then I think it's probably better and easier to just document the existing
> behavior? If a new setting would have to be introduced and developers would
> need to be aware of that, it's probably easier to just tell everyone to use
> SIGKILL.
> 
> Does this manpage patch look good?

Patch applied, with Acks from Andy, Mikael, and Kees (I don't
usually get patches whose pedigree is that good. Thanks!)

I tweaked a few wordings. You can find the changes in Git [1]

Cheers,

Michael

[1] 
http://git.kernel.org/cgit/docs/man-pages/man-pages.git/commit/?id=65be1b46fb88e14f0af494ac6b53a2d6a63bb860

> ---
>  man2/seccomp.2 | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/man2/seccomp.2 b/man2/seccomp.2
> index 702ceb8..f762d07 100644
> --- a/man2/seccomp.2
> +++ b/man2/seccomp.2
> @@ -64,6 +64,31 @@ Strict secure computing mode is useful for number-crunching
>  applications that may need to execute untrusted byte code, perhaps
>  obtained by reading from a pipe or socket.
>  
> +Note that although the calling thread can no longer call
> +.BR sigprocmask (2),
> +it can use
> +.BR sigreturn (2)
> +to block all signals apart from
> +.BR SIGKILL
> +and
> +.BR SIGSTOP .
> +Therefore, to reliably terminate it,
> +.BR SIGKILL
> +has to be used, meaning that e.g.
> +.BR alarm (2)
> +is not sufficient for restricting its runtime. Instead, use
> +.BR timer_create (2)
> +with
> +.BR SIGEV_SIGNAL
> +and
> +.BR sigev_signo
> +set to
> +.BR SIGKILL
> +or use
> +.BR setrlimit (2)
> +to set the hard limit for
> +.BR RLIMIT_CPU .
> +
>  This operation is available only if the kernel is configured with
>  .BR CONFIG_SECCOMP
>  enabled.
> 


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/