[PATCH] seccomp: dump core when using SECCOMP_RET_KILL

[PATCH] seccomp: dump core when using SECCOMP_RET_KILL

[Mike Frysinger]

From: Mike Frysinger <vapier@chromium.org>

The SECCOMP_RET_KILL mode is documented as immediately killing the
process as if a SIGSYS had been sent and not caught (similar to a
SIGKILL).  However, a SIGSYS is documented as triggering a coredump
which does not happen today.

This has the advantage of being able to more easily debug a process
that fails a seccomp filter.  Today, most apps need to recompile and
change their filter in order to get detailed info out, or manually run
things through strace, or enable detailed kernel auditing.  Now we get
coredumps that fit into existing system-wide crash reporting setups.

>From a security pov, this shouldn't be a problem.  Unhandled signals
can already be sent externally which trigger a coredump independent of
the status of the seccomp filter.  The act of dumping core itself does
not cause change in execution of the program.

URL: https://crbug.com/676357
Signed-off-by: Mike Frysinger <vapier@chromium.org>
Acked-by: Jorge Lucangeli Obes <jorgelo@chromium.org>
Acked-by: Kees Cook <keescook@chromium.org>
---
 kernel/seccomp.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index f7ce79a46050..f8f88ebcb3ba 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -16,6 +16,7 @@
 #include <linux/atomic.h>
 #include <linux/audit.h>
 #include <linux/compat.h>
+#include <linux/coredump.h>
 #include <linux/sched.h>
 #include <linux/seccomp.h>
 #include <linux/slab.h>
@@ -486,6 +487,17 @@ void put_seccomp_filter(struct task_struct *tsk)
 	}
 }
 
+static void seccomp_init_siginfo(siginfo_t *info, int syscall, int reason)
+{
+	memset(info, 0, sizeof(*info));
+	info->si_signo = SIGSYS;
+	info->si_code = SYS_SECCOMP;
+	info->si_call_addr = (void __user *)KSTK_EIP(current);
+	info->si_errno = reason;
+	info->si_arch = syscall_get_arch();
+	info->si_syscall = syscall;
+}
+
 /**
  * seccomp_send_sigsys - signals the task to allow in-process syscall emulation
  * @syscall: syscall number to send to userland
@@ -496,13 +508,7 @@ void put_seccomp_filter(struct task_struct *tsk)
 static void seccomp_send_sigsys(int syscall, int reason)
 {
 	struct siginfo info;
-	memset(&info, 0, sizeof(info));
-	info.si_signo = SIGSYS;
-	info.si_code = SYS_SECCOMP;
-	info.si_call_addr = (void __user *)KSTK_EIP(current);
-	info.si_errno = reason;
-	info.si_arch = syscall_get_arch();
-	info.si_syscall = syscall;
+	seccomp_init_siginfo(&info, syscall, reason);
 	force_sig_info(SIGSYS, &info, current);
 }
 #endif	/* CONFIG_SECCOMP_FILTER */
@@ -634,10 +640,17 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
 		return 0;
 
 	case SECCOMP_RET_KILL:
-	default:
+	default: {
+		siginfo_t info;
 		audit_seccomp(this_syscall, SIGSYS, action);
+		/* Show the original registers in the dump. */
+		syscall_rollback(current, task_pt_regs(current));
+		/* Trigger a manual coredump since do_exit skips it. */
+		seccomp_init_siginfo(&info, this_syscall, data);
+		do_coredump(&info);
 		do_exit(SIGSYS);
 	}
+	}
 
 	unreachable();
 
-- 
2.11.0

Re: [PATCH] seccomp: dump core when using SECCOMP_RET_KILL

[Kees Cook]

On Thu, Jan 19, 2017 at 8:28 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> From: Mike Frysinger <vapier@chromium.org>
>
> The SECCOMP_RET_KILL mode is documented as immediately killing the
> process as if a SIGSYS had been sent and not caught (similar to a
> SIGKILL).  However, a SIGSYS is documented as triggering a coredump
> which does not happen today.
>
> This has the advantage of being able to more easily debug a process
> that fails a seccomp filter.  Today, most apps need to recompile and
> change their filter in order to get detailed info out, or manually run
> things through strace, or enable detailed kernel auditing.  Now we get
> coredumps that fit into existing system-wide crash reporting setups.
>
> From a security pov, this shouldn't be a problem.  Unhandled signals
> can already be sent externally which trigger a coredump independent of
> the status of the seccomp filter.  The act of dumping core itself does
> not cause change in execution of the program.
>
> URL: https://crbug.com/676357
> Signed-off-by: Mike Frysinger <vapier@chromium.org>
> Acked-by: Jorge Lucangeli Obes <jorgelo@chromium.org>

Yup, I think this is fine. The additional kernel code executed before
the do_exit() is relatively limited, and is equivalent to leaving
kill(self, SIGSEGV) exposed in a seccomp filter. Setting an RLIMIT is
also sufficient to block the core generation, so really paranoid
environments can still do that.

The forwarded ack stands:

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

James, can you add this to your tree?

-Kees

-- 
Kees Cook
Nexus Security

Re: [PATCH] seccomp: dump core when using SECCOMP_RET_KILL

[James Morris]

On Fri, 20 Jan 2017, Kees Cook wrote:

> Yup, I think this is fine. The additional kernel code executed before
> the do_exit() is relatively limited, and is equivalent to leaving
> kill(self, SIGSEGV) exposed in a seccomp filter. Setting an RLIMIT is
> also sufficient to block the core generation, so really paranoid
> environments can still do that.
> 
> The forwarded ack stands:
> 
> Acked-by: Kees Cook <keescook@chromium.org>
> 
> James, can you add this to your tree?

Mike, please resend the patch, I don't have it.


-- 
James Morris
<jmorris@namei.org>

Re: [PATCH] seccomp: dump core when using SECCOMP_RET_KILL

[Mike Frysinger]

On Sun, Jan 22, 2017 at 4:10 PM, James Morris wrote:
> On Fri, 20 Jan 2017, Kees Cook wrote:
> > Yup, I think this is fine. The additional kernel code executed before
> > the do_exit() is relatively limited, and is equivalent to leaving
> > kill(self, SIGSEGV) exposed in a seccomp filter. Setting an RLIMIT is
> > also sufficient to block the core generation, so really paranoid
> > environments can still do that.
> >
> > The forwarded ack stands:
> >
> > Acked-by: Kees Cook <keescook@chromium.org>
> >
> > James, can you add this to your tree?
>
> Mike, please resend the patch, I don't have it.

looks like patchwork grabbed it:
  https://patchwork.kernel.org/patch/9527359/

has a mbox link which should get you what you need ?
-mike

Re: [PATCH] seccomp: dump core when using SECCOMP_RET_KILL

[James Morris]

On Mon, 23 Jan 2017, Mike Frysinger wrote:

> On Sun, Jan 22, 2017 at 4:10 PM, James Morris wrote:
> > On Fri, 20 Jan 2017, Kees Cook wrote:
> > > Yup, I think this is fine. The additional kernel code executed before
> > > the do_exit() is relatively limited, and is equivalent to leaving
> > > kill(self, SIGSEGV) exposed in a seccomp filter. Setting an RLIMIT is
> > > also sufficient to block the core generation, so really paranoid
> > > environments can still do that.
> > >
> > > The forwarded ack stands:
> > >
> > > Acked-by: Kees Cook <keescook@chromium.org>
> > >
> > > James, can you add this to your tree?
> >
> > Mike, please resend the patch, I don't have it.
> 
> looks like patchwork grabbed it:
>   https://patchwork.kernel.org/patch/9527359/
> 
> has a mbox link which should get you what you need ?

Thanks.

Applied to
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git next


-- 
James Morris
<jmorris@namei.org>

Re: seccomp: dump core when using SECCOMP_RET_KILL

[Andrei Vagin]

Hi,

One of CRIU tests fails with this patch:
https://github.com/xemul/criu/blob/master/test/zdtm/static/seccomp_filter_tsync.c

Before this patch only a thread which called a "wrong" syscall is killed.
Now a whole process is killed if one of threads called a "wrong" syscall.

Before this patch only one thread is killed:

512   seccomp(SECCOMP_SET_MODE_FILTER, SECCOMP_FILTER_FLAG_TSYNC, {len=4, filter=0x7fffe111fb10} <unfinished ...>
484   kill(30, SIG_0)                   = 0
484   write(1, "Wait for zdtm/static/seccomp_filter_tsync(30) to die for 0.100000\n", 66 <unfinished ...>
512   <... seccomp resumed> )           = 0 
512   futex(0x606420, FUTEX_WAKE_PRIVATE, 1 <unfinished ...>                   
484   <... write resumed> )             = 66                                   
512   <... futex resumed> )             = 1 
484   select(0, NULL, NULL, NULL, {tv_sec=0, tv_usec=100000} <unfinished ...>
512   futex(0x7f9e894a19d0, FUTEX_WAIT, 32, NULL <unfinished ...>              
513   <... futex resumed> )             = 0
513   futex(0x606420, FUTEX_WAKE_PRIVATE, 1) = 0
513   ptrace(PTRACE_TRACEME)            = ?
513   +++ killed by SIGSYS +++
512   <... futex resumed> )             = 0

After this patch a whole process is killed:

767   seccomp(SECCOMP_SET_MODE_FILTER, SECCOMP_FILTER_FLAG_TSYNC, {len=4, filter=0x7ffdeedbcd20}) = 0
767   futex(0x606420, FUTEX_WAKE_PRIVATE, 1) = 1
768   <... futex resumed> )             = 0 
767   futex(0x7fab05b229d0, FUTEX_WAIT, 32, NULL <unfinished ...>              
768   futex(0x606420, FUTEX_WAKE_PRIVATE, 1) = 0
768   ptrace(PTRACE_TRACEME <unfinished ...> 
767   <... futex resumed>)              = ? 
768   <... ptrace resumed>)             = ?
768   +++ killed by SIGSYS (core dumped) +++ 
767   +++ killed by SIGSYS (core dumped) +++
766   <... wait4 resumed> [{WIFSIGNALED(s) && WTERMSIG(s) == SIGSYS && WCOREDUMP(s)}], 0, NULL) = 31
766   --- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_KILLED, si_pid=31, si_

Steps to reproduce:
$ git clone git://github.com/xemul/criu
$ cd criu/test/zdtm/static/
$ make seccomp_filter_tsync.out
$ cat seccomp_filter_tsync.out

On Thu, Jan 19, 2017 at 10:28:57PM -0600, Mike Frysinger wrote:
> From: Mike Frysinger <vapier@chromium.org>
> 
> The SECCOMP_RET_KILL mode is documented as immediately killing the
> process as if a SIGSYS had been sent and not caught (similar to a
> SIGKILL).  However, a SIGSYS is documented as triggering a coredump
> which does not happen today.
> 
> This has the advantage of being able to more easily debug a process
> that fails a seccomp filter.  Today, most apps need to recompile and
> change their filter in order to get detailed info out, or manually run
> things through strace, or enable detailed kernel auditing.  Now we get
> coredumps that fit into existing system-wide crash reporting setups.
> 
> >From a security pov, this shouldn't be a problem.  Unhandled signals
> can already be sent externally which trigger a coredump independent of
> the status of the seccomp filter.  The act of dumping core itself does
> not cause change in execution of the program.
> 
> URL: https://crbug.com/676357
> Signed-off-by: Mike Frysinger <vapier@chromium.org>
> Acked-by: Jorge Lucangeli Obes <jorgelo@chromium.org>
> Acked-by: Kees Cook <keescook@chromium.org>
> Acked-by: Kees Cook <keescook@chromium.org>
> ---
>  kernel/seccomp.c | 29 +++++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index f7ce79a46050..f8f88ebcb3ba 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -16,6 +16,7 @@
>  #include <linux/atomic.h>
>  #include <linux/audit.h>
>  #include <linux/compat.h>
> +#include <linux/coredump.h>
>  #include <linux/sched.h>
>  #include <linux/seccomp.h>
>  #include <linux/slab.h>
> @@ -486,6 +487,17 @@ void put_seccomp_filter(struct task_struct *tsk)
>  	}
>  }
>  
> +static void seccomp_init_siginfo(siginfo_t *info, int syscall, int reason)
> +{
> +	memset(info, 0, sizeof(*info));
> +	info->si_signo = SIGSYS;
> +	info->si_code = SYS_SECCOMP;
> +	info->si_call_addr = (void __user *)KSTK_EIP(current);
> +	info->si_errno = reason;
> +	info->si_arch = syscall_get_arch();
> +	info->si_syscall = syscall;
> +}
> +
>  /**
>   * seccomp_send_sigsys - signals the task to allow in-process syscall emulation
>   * @syscall: syscall number to send to userland
> @@ -496,13 +508,7 @@ void put_seccomp_filter(struct task_struct *tsk)
>  static void seccomp_send_sigsys(int syscall, int reason)
>  {
>  	struct siginfo info;
> -	memset(&info, 0, sizeof(info));
> -	info.si_signo = SIGSYS;
> -	info.si_code = SYS_SECCOMP;
> -	info.si_call_addr = (void __user *)KSTK_EIP(current);
> -	info.si_errno = reason;
> -	info.si_arch = syscall_get_arch();
> -	info.si_syscall = syscall;
> +	seccomp_init_siginfo(&info, syscall, reason);
>  	force_sig_info(SIGSYS, &info, current);
>  }
>  #endif	/* CONFIG_SECCOMP_FILTER */
> @@ -634,10 +640,17 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
>  		return 0;
>  
>  	case SECCOMP_RET_KILL:
> -	default:
> +	default: {
> +		siginfo_t info;
>  		audit_seccomp(this_syscall, SIGSYS, action);
> +		/* Show the original registers in the dump. */
> +		syscall_rollback(current, task_pt_regs(current));
> +		/* Trigger a manual coredump since do_exit skips it. */
> +		seccomp_init_siginfo(&info, this_syscall, data);
> +		do_coredump(&info);
>  		do_exit(SIGSYS);
>  	}
> +	}
>  
>  	unreachable();
>  

Re: seccomp: dump core when using SECCOMP_RET_KILL

[Mike Frysinger]

thank for the testcase.  i'll take a look.  when i went through the code
visually, i didn't think it was killing, just suspending+resuming for the
sake of snapshotting, but i must have misread.
-mike

On Tue, Jan 24, 2017 at 2:53 PM, Andrei Vagin <avagin@virtuozzo.com> wrote:
> Hi,
>
> One of CRIU tests fails with this patch:
> https://github.com/xemul/criu/blob/master/test/zdtm/static/seccomp_filter_tsync.c
>
> Before this patch only a thread which called a "wrong" syscall is killed.
> Now a whole process is killed if one of threads called a "wrong" syscall.
>
> Before this patch only one thread is killed:
>
> 512   seccomp(SECCOMP_SET_MODE_FILTER, SECCOMP_FILTER_FLAG_TSYNC, {len=4, filter=0x7fffe111fb10} <unfinished ...>
> 484   kill(30, SIG_0)                   = 0
> 484   write(1, "Wait for zdtm/static/seccomp_filter_tsync(30) to die for 0.100000\n", 66 <unfinished ...>
> 512   <... seccomp resumed> )           = 0
> 512   futex(0x606420, FUTEX_WAKE_PRIVATE, 1 <unfinished ...>
> 484   <... write resumed> )             = 66
> 512   <... futex resumed> )             = 1
> 484   select(0, NULL, NULL, NULL, {tv_sec=0, tv_usec=100000} <unfinished ...>
> 512   futex(0x7f9e894a19d0, FUTEX_WAIT, 32, NULL <unfinished ...>
> 513   <... futex resumed> )             = 0
> 513   futex(0x606420, FUTEX_WAKE_PRIVATE, 1) = 0
> 513   ptrace(PTRACE_TRACEME)            = ?
> 513   +++ killed by SIGSYS +++
> 512   <... futex resumed> )             = 0
>
> After this patch a whole process is killed:
>
> 767   seccomp(SECCOMP_SET_MODE_FILTER, SECCOMP_FILTER_FLAG_TSYNC, {len=4, filter=0x7ffdeedbcd20}) = 0
> 767   futex(0x606420, FUTEX_WAKE_PRIVATE, 1) = 1
> 768   <... futex resumed> )             = 0
> 767   futex(0x7fab05b229d0, FUTEX_WAIT, 32, NULL <unfinished ...>
> 768   futex(0x606420, FUTEX_WAKE_PRIVATE, 1) = 0
> 768   ptrace(PTRACE_TRACEME <unfinished ...>
> 767   <... futex resumed>)              = ?
> 768   <... ptrace resumed>)             = ?
> 768   +++ killed by SIGSYS (core dumped) +++
> 767   +++ killed by SIGSYS (core dumped) +++
> 766   <... wait4 resumed> [{WIFSIGNALED(s) && WTERMSIG(s) == SIGSYS && WCOREDUMP(s)}], 0, NULL) = 31
> 766   --- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_KILLED, si_pid=31, si_
>
> Steps to reproduce:
> $ git clone git://github.com/xemul/criu
> $ cd criu/test/zdtm/static/
> $ make seccomp_filter_tsync.out
> $ cat seccomp_filter_tsync.out
>
> On Thu, Jan 19, 2017 at 10:28:57PM -0600, Mike Frysinger wrote:
>> From: Mike Frysinger <vapier@chromium.org>
>>
>> The SECCOMP_RET_KILL mode is documented as immediately killing the
>> process as if a SIGSYS had been sent and not caught (similar to a
>> SIGKILL).  However, a SIGSYS is documented as triggering a coredump
>> which does not happen today.
>>
>> This has the advantage of being able to more easily debug a process
>> that fails a seccomp filter.  Today, most apps need to recompile and
>> change their filter in order to get detailed info out, or manually run
>> things through strace, or enable detailed kernel auditing.  Now we get
>> coredumps that fit into existing system-wide crash reporting setups.
>>
>> >From a security pov, this shouldn't be a problem.  Unhandled signals
>> can already be sent externally which trigger a coredump independent of
>> the status of the seccomp filter.  The act of dumping core itself does
>> not cause change in execution of the program.
>>
>> URL: https://crbug.com/676357
>> Signed-off-by: Mike Frysinger <vapier@chromium.org>
>> Acked-by: Jorge Lucangeli Obes <jorgelo@chromium.org>
>> Acked-by: Kees Cook <keescook@chromium.org>
>> Acked-by: Kees Cook <keescook@chromium.org>
>> ---
>>  kernel/seccomp.c | 29 +++++++++++++++++++++--------
>>  1 file changed, 21 insertions(+), 8 deletions(-)
>>
>> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
>> index f7ce79a46050..f8f88ebcb3ba 100644
>> --- a/kernel/seccomp.c
>> +++ b/kernel/seccomp.c
>> @@ -16,6 +16,7 @@
>>  #include <linux/atomic.h>
>>  #include <linux/audit.h>
>>  #include <linux/compat.h>
>> +#include <linux/coredump.h>
>>  #include <linux/sched.h>
>>  #include <linux/seccomp.h>
>>  #include <linux/slab.h>
>> @@ -486,6 +487,17 @@ void put_seccomp_filter(struct task_struct *tsk)
>>       }
>>  }
>>
>> +static void seccomp_init_siginfo(siginfo_t *info, int syscall, int reason)
>> +{
>> +     memset(info, 0, sizeof(*info));
>> +     info->si_signo = SIGSYS;
>> +     info->si_code = SYS_SECCOMP;
>> +     info->si_call_addr = (void __user *)KSTK_EIP(current);
>> +     info->si_errno = reason;
>> +     info->si_arch = syscall_get_arch();
>> +     info->si_syscall = syscall;
>> +}
>> +
>>  /**
>>   * seccomp_send_sigsys - signals the task to allow in-process syscall emulation
>>   * @syscall: syscall number to send to userland
>> @@ -496,13 +508,7 @@ void put_seccomp_filter(struct task_struct *tsk)
>>  static void seccomp_send_sigsys(int syscall, int reason)
>>  {
>>       struct siginfo info;
>> -     memset(&info, 0, sizeof(info));
>> -     info.si_signo = SIGSYS;
>> -     info.si_code = SYS_SECCOMP;
>> -     info.si_call_addr = (void __user *)KSTK_EIP(current);
>> -     info.si_errno = reason;
>> -     info.si_arch = syscall_get_arch();
>> -     info.si_syscall = syscall;
>> +     seccomp_init_siginfo(&info, syscall, reason);
>>       force_sig_info(SIGSYS, &info, current);
>>  }
>>  #endif       /* CONFIG_SECCOMP_FILTER */
>> @@ -634,10 +640,17 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
>>               return 0;
>>
>>       case SECCOMP_RET_KILL:
>> -     default:
>> +     default: {
>> +             siginfo_t info;
>>               audit_seccomp(this_syscall, SIGSYS, action);
>> +             /* Show the original registers in the dump. */
>> +             syscall_rollback(current, task_pt_regs(current));
>> +             /* Trigger a manual coredump since do_exit skips it. */
>> +             seccomp_init_siginfo(&info, this_syscall, data);
>> +             do_coredump(&info);
>>               do_exit(SIGSYS);
>>       }
>> +     }
>>
>>       unreachable();
>>

Re: seccomp: dump core when using SECCOMP_RET_KILL

[Kees Cook]

On Tue, Jan 24, 2017 at 4:53 PM, Andrei Vagin <avagin@virtuozzo.com> wrote:
> Hi,
>
> One of CRIU tests fails with this patch:
> https://github.com/xemul/criu/blob/master/test/zdtm/static/seccomp_filter_tsync.c
>
> Before this patch only a thread which called a "wrong" syscall is killed.
> Now a whole process is killed if one of threads called a "wrong" syscall.

Oh ew. I wonder what is causing this? In other do_coredump() callers,
they explicitly call do_group_exit(). Hmmm

-Kees

>
> Before this patch only one thread is killed:
>
> 512   seccomp(SECCOMP_SET_MODE_FILTER, SECCOMP_FILTER_FLAG_TSYNC, {len=4, filter=0x7fffe111fb10} <unfinished ...>
> 484   kill(30, SIG_0)                   = 0
> 484   write(1, "Wait for zdtm/static/seccomp_filter_tsync(30) to die for 0.100000\n", 66 <unfinished ...>
> 512   <... seccomp resumed> )           = 0
> 512   futex(0x606420, FUTEX_WAKE_PRIVATE, 1 <unfinished ...>
> 484   <... write resumed> )             = 66
> 512   <... futex resumed> )             = 1
> 484   select(0, NULL, NULL, NULL, {tv_sec=0, tv_usec=100000} <unfinished ...>
> 512   futex(0x7f9e894a19d0, FUTEX_WAIT, 32, NULL <unfinished ...>
> 513   <... futex resumed> )             = 0
> 513   futex(0x606420, FUTEX_WAKE_PRIVATE, 1) = 0
> 513   ptrace(PTRACE_TRACEME)            = ?
> 513   +++ killed by SIGSYS +++
> 512   <... futex resumed> )             = 0
>
> After this patch a whole process is killed:
>
> 767   seccomp(SECCOMP_SET_MODE_FILTER, SECCOMP_FILTER_FLAG_TSYNC, {len=4, filter=0x7ffdeedbcd20}) = 0
> 767   futex(0x606420, FUTEX_WAKE_PRIVATE, 1) = 1
> 768   <... futex resumed> )             = 0
> 767   futex(0x7fab05b229d0, FUTEX_WAIT, 32, NULL <unfinished ...>
> 768   futex(0x606420, FUTEX_WAKE_PRIVATE, 1) = 0
> 768   ptrace(PTRACE_TRACEME <unfinished ...>
> 767   <... futex resumed>)              = ?
> 768   <... ptrace resumed>)             = ?
> 768   +++ killed by SIGSYS (core dumped) +++
> 767   +++ killed by SIGSYS (core dumped) +++
> 766   <... wait4 resumed> [{WIFSIGNALED(s) && WTERMSIG(s) == SIGSYS && WCOREDUMP(s)}], 0, NULL) = 31
> 766   --- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_KILLED, si_pid=31, si_
>
> Steps to reproduce:
> $ git clone git://github.com/xemul/criu
> $ cd criu/test/zdtm/static/
> $ make seccomp_filter_tsync.out
> $ cat seccomp_filter_tsync.out
>
> On Thu, Jan 19, 2017 at 10:28:57PM -0600, Mike Frysinger wrote:
>> From: Mike Frysinger <vapier@chromium.org>
>>
>> The SECCOMP_RET_KILL mode is documented as immediately killing the
>> process as if a SIGSYS had been sent and not caught (similar to a
>> SIGKILL).  However, a SIGSYS is documented as triggering a coredump
>> which does not happen today.
>>
>> This has the advantage of being able to more easily debug a process
>> that fails a seccomp filter.  Today, most apps need to recompile and
>> change their filter in order to get detailed info out, or manually run
>> things through strace, or enable detailed kernel auditing.  Now we get
>> coredumps that fit into existing system-wide crash reporting setups.
>>
>> >From a security pov, this shouldn't be a problem.  Unhandled signals
>> can already be sent externally which trigger a coredump independent of
>> the status of the seccomp filter.  The act of dumping core itself does
>> not cause change in execution of the program.
>>
>> URL: https://crbug.com/676357
>> Signed-off-by: Mike Frysinger <vapier@chromium.org>
>> Acked-by: Jorge Lucangeli Obes <jorgelo@chromium.org>
>> Acked-by: Kees Cook <keescook@chromium.org>
>> Acked-by: Kees Cook <keescook@chromium.org>
>> ---
>>  kernel/seccomp.c | 29 +++++++++++++++++++++--------
>>  1 file changed, 21 insertions(+), 8 deletions(-)
>>
>> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
>> index f7ce79a46050..f8f88ebcb3ba 100644
>> --- a/kernel/seccomp.c
>> +++ b/kernel/seccomp.c
>> @@ -16,6 +16,7 @@
>>  #include <linux/atomic.h>
>>  #include <linux/audit.h>
>>  #include <linux/compat.h>
>> +#include <linux/coredump.h>
>>  #include <linux/sched.h>
>>  #include <linux/seccomp.h>
>>  #include <linux/slab.h>
>> @@ -486,6 +487,17 @@ void put_seccomp_filter(struct task_struct *tsk)
>>       }
>>  }
>>
>> +static void seccomp_init_siginfo(siginfo_t *info, int syscall, int reason)
>> +{
>> +     memset(info, 0, sizeof(*info));
>> +     info->si_signo = SIGSYS;
>> +     info->si_code = SYS_SECCOMP;
>> +     info->si_call_addr = (void __user *)KSTK_EIP(current);
>> +     info->si_errno = reason;
>> +     info->si_arch = syscall_get_arch();
>> +     info->si_syscall = syscall;
>> +}
>> +
>>  /**
>>   * seccomp_send_sigsys - signals the task to allow in-process syscall emulation
>>   * @syscall: syscall number to send to userland
>> @@ -496,13 +508,7 @@ void put_seccomp_filter(struct task_struct *tsk)
>>  static void seccomp_send_sigsys(int syscall, int reason)
>>  {
>>       struct siginfo info;
>> -     memset(&info, 0, sizeof(info));
>> -     info.si_signo = SIGSYS;
>> -     info.si_code = SYS_SECCOMP;
>> -     info.si_call_addr = (void __user *)KSTK_EIP(current);
>> -     info.si_errno = reason;
>> -     info.si_arch = syscall_get_arch();
>> -     info.si_syscall = syscall;
>> +     seccomp_init_siginfo(&info, syscall, reason);
>>       force_sig_info(SIGSYS, &info, current);
>>  }
>>  #endif       /* CONFIG_SECCOMP_FILTER */
>> @@ -634,10 +640,17 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
>>               return 0;
>>
>>       case SECCOMP_RET_KILL:
>> -     default:
>> +     default: {
>> +             siginfo_t info;
>>               audit_seccomp(this_syscall, SIGSYS, action);
>> +             /* Show the original registers in the dump. */
>> +             syscall_rollback(current, task_pt_regs(current));
>> +             /* Trigger a manual coredump since do_exit skips it. */
>> +             seccomp_init_siginfo(&info, this_syscall, data);
>> +             do_coredump(&info);
>>               do_exit(SIGSYS);
>>       }
>> +     }
>>
>>       unreachable();
>>



-- 
Kees Cook
Nexus Security

Re: seccomp: dump core when using SECCOMP_RET_KILL

[Kees Cook]

On Wed, Jan 25, 2017 at 12:05 PM, Kees Cook <keescook@chromium.org> wrote:
> On Tue, Jan 24, 2017 at 4:53 PM, Andrei Vagin <avagin@virtuozzo.com> wrote:
>> Hi,
>>
>> One of CRIU tests fails with this patch:
>> https://github.com/xemul/criu/blob/master/test/zdtm/static/seccomp_filter_tsync.c
>>
>> Before this patch only a thread which called a "wrong" syscall is killed.
>> Now a whole process is killed if one of threads called a "wrong" syscall.
>
> Oh ew. I wonder what is causing this? In other do_coredump() callers,
> they explicitly call do_group_exit(). Hmmm

We need to find a way to fix this or remove the coredump change from
-next. We have a few things that have come up recently (coincident
with the coredump change):

- some folks would like seccomp kills to kill the entire process not
just the thread
- on a full-process kill, there needs to be a way to get a coredump
- on a kill, it would be nice to have reliable logging

Getting a coredump requires a full-process kill. It is possible to do
this already with RET_TRAP and just not catch the SIGSYS. However,
this isn't sufficient if you want to be _sure_ the entire process gets
killed since RET_TRAP depends on cooperation from the process.

Getting reliable logging out of seccomp for non-RET_KILL is
non-trivial because syscall-audit doesn't track forks.

The RET_* values are part of the UAPI, so changing or adding to them
requires care.

Right now we have very little room in the RET_* values (the lower
bytes are for the RET_DATA which is ignored for RET_KILL and the
semantics of changing that is very difficult):

#define SECCOMP_RET_KILL        0x00000000U /* kill the task immediately */
#define SECCOMP_RET_TRAP        0x00030000U /* disallow and force a SIGSYS */

Killing the entire process is more aggressive than RET_KILL currently,
so the question becomes, should we upgrade RET_KILL to
RET_KILL_PROCESS and add RET_KILL_THREAD? Are there people that WANT
only a thread to be killed? Andrei, does CRIU depend on this behavior,
or is it "just" a regression test detail?

We can add two more RET_* values... however, it seems like only thread
vs process is a filter-level concept. Whether or not to dump core
seems to be an external aspect of the process (think ulimit -c), and
logging seems to be a system-level thing.

For logging, I think audit needs to grow fork-tracking, and/or have a
new "is under seccomp" test that can be exposed to auditctl. Then the
system owner can issue either "tell me about all seccomp kills" or
"tell me about seccomp kills in this process tree". As such, I don't
think we should be making filter-level changes to deal with the needs
of seccomp logging.

For coredumping, I remain a bit stumped. Strictly speaking,
coredumping exposes more kernel code to the running process, so it is
"less safe" than the instant kill RET_KILL performs. Though the
current patch is actually more aggressive in that it causes the entire
process to die as part of the coredumping.

I think that if there is a move to make RET_KILL kill the process,
then we can add coredumping. If not, I'm less sure how to proceed...

-Kees

-- 
Kees Cook
Nexus Security

Re: seccomp: dump core when using SECCOMP_RET_KILL

[Paul Moore]

On Fri, Jan 27, 2017 at 4:48 PM, Kees Cook <keescook@chromium.org> wrote:
> For logging, I think audit needs to grow fork-tracking, and/or have a
> new "is under seccomp" test that can be exposed to auditctl. Then the
> system owner can issue either "tell me about all seccomp kills" or
> "tell me about seccomp kills in this process tree". As such, I don't
> think we should be making filter-level changes to deal with the needs
> of seccomp logging.

I really don't want to see seccomp logging relying on special audit
functionality simply because there are people using seccomp that don't
use audit.  Whatever we do with seccomp logging we need to make sure
it works okay~ish regardless of audit.

See some of the discussion around Tyler's last patchset.

-- 
paul moore
www.paul-moore.com

Re: seccomp: dump core when using SECCOMP_RET_KILL

[Andrei Vagin]

On Fri, Jan 27, 2017 at 01:48:30PM -0800, Kees Cook wrote:
> On Wed, Jan 25, 2017 at 12:05 PM, Kees Cook <keescook@chromium.org> wrote:
> > On Tue, Jan 24, 2017 at 4:53 PM, Andrei Vagin <avagin@virtuozzo.com> wrote:
> >> Hi,
> >>
> >> One of CRIU tests fails with this patch:
> >> https://github.com/xemul/criu/blob/master/test/zdtm/static/seccomp_filter_tsync.c
> >>
> >> Before this patch only a thread which called a "wrong" syscall is killed.
> >> Now a whole process is killed if one of threads called a "wrong" syscall.
> >
> > Oh ew. I wonder what is causing this? In other do_coredump() callers,
> > they explicitly call do_group_exit(). Hmmm
> 
> We need to find a way to fix this or remove the coredump change from
> -next. We have a few things that have come up recently (coincident
> with the coredump change):
> 
> - some folks would like seccomp kills to kill the entire process not
> just the thread
> - on a full-process kill, there needs to be a way to get a coredump
> - on a kill, it would be nice to have reliable logging
> 
> Getting a coredump requires a full-process kill. It is possible to do
> this already with RET_TRAP and just not catch the SIGSYS. However,
> this isn't sufficient if you want to be _sure_ the entire process gets
> killed since RET_TRAP depends on cooperation from the process.
> 
> Getting reliable logging out of seccomp for non-RET_KILL is
> non-trivial because syscall-audit doesn't track forks.
> 
> The RET_* values are part of the UAPI, so changing or adding to them
> requires care.
> 
> Right now we have very little room in the RET_* values (the lower
> bytes are for the RET_DATA which is ignored for RET_KILL and the
> semantics of changing that is very difficult):
> 
> #define SECCOMP_RET_KILL        0x00000000U /* kill the task immediately */
> #define SECCOMP_RET_TRAP        0x00030000U /* disallow and force a SIGSYS */
> 
> Killing the entire process is more aggressive than RET_KILL currently,
> so the question becomes, should we upgrade RET_KILL to
> RET_KILL_PROCESS and add RET_KILL_THREAD? Are there people that WANT
> only a thread to be killed? Andrei, does CRIU depend on this behavior,
> or is it "just" a regression test detail?

It is just a regression test detail. We can change the test if you
decide to kill a whole process.

> 
> We can add two more RET_* values... however, it seems like only thread
> vs process is a filter-level concept. Whether or not to dump core
> seems to be an external aspect of the process (think ulimit -c), and
> logging seems to be a system-level thing.
> 
> For logging, I think audit needs to grow fork-tracking, and/or have a
> new "is under seccomp" test that can be exposed to auditctl. Then the
> system owner can issue either "tell me about all seccomp kills" or
> "tell me about seccomp kills in this process tree". As such, I don't
> think we should be making filter-level changes to deal with the needs
> of seccomp logging.
> 
> For coredumping, I remain a bit stumped. Strictly speaking,
> coredumping exposes more kernel code to the running process, so it is
> "less safe" than the instant kill RET_KILL performs. Though the
> current patch is actually more aggressive in that it causes the entire
> process to die as part of the coredumping.
> 
> I think that if there is a move to make RET_KILL kill the process,
> then we can add coredumping. If not, I'm less sure how to proceed...
> 
> -Kees
> 
> -- 
> Kees Cook
> Nexus Security

Re: seccomp: dump core when using SECCOMP_RET_KILL

[Andy Lutomirski]

On Fri, Jan 27, 2017 at 1:48 PM, Kees Cook <keescook@chromium.org> wrote:
> On Wed, Jan 25, 2017 at 12:05 PM, Kees Cook <keescook@chromium.org> wrote:
>> On Tue, Jan 24, 2017 at 4:53 PM, Andrei Vagin <avagin@virtuozzo.com> wrote:
>>> Hi,
>>>
>>> One of CRIU tests fails with this patch:
>>> https://github.com/xemul/criu/blob/master/test/zdtm/static/seccomp_filter_tsync.c
>>>
>>> Before this patch only a thread which called a "wrong" syscall is killed.
>>> Now a whole process is killed if one of threads called a "wrong" syscall.
>>
>> Oh ew. I wonder what is causing this? In other do_coredump() callers,
>> they explicitly call do_group_exit(). Hmmm
>
> We need to find a way to fix this or remove the coredump change from
> -next. We have a few things that have come up recently (coincident
> with the coredump change):
>
> - some folks would like seccomp kills to kill the entire process not
> just the thread
> - on a full-process kill, there needs to be a way to get a coredump
> - on a kill, it would be nice to have reliable logging
>
> Getting a coredump requires a full-process kill. It is possible to do
> this already with RET_TRAP and just not catch the SIGSYS. However,
> this isn't sufficient if you want to be _sure_ the entire process gets
> killed since RET_TRAP depends on cooperation from the process.
>
> Getting reliable logging out of seccomp for non-RET_KILL is
> non-trivial because syscall-audit doesn't track forks.
>
> The RET_* values are part of the UAPI, so changing or adding to them
> requires care.
>
> Right now we have very little room in the RET_* values (the lower
> bytes are for the RET_DATA which is ignored for RET_KILL and the
> semantics of changing that is very difficult):
>
> #define SECCOMP_RET_KILL        0x00000000U /* kill the task immediately */
> #define SECCOMP_RET_TRAP        0x00030000U /* disallow and force a SIGSYS */
>
> Killing the entire process is more aggressive than RET_KILL currently,
> so the question becomes, should we upgrade RET_KILL to
> RET_KILL_PROCESS and add RET_KILL_THREAD? Are there people that WANT
> only a thread to be killed? Andrei, does CRIU depend on this behavior,
> or is it "just" a regression test detail?

I think we actually have more flexibility: we have all the low bits --
they're currently ignored.  We could plausibly have one code for "kill
this thread, no dump", one code for "kill this process, dump core",
and maybe one code for "kill this process, don't dump core".

I would be a bit surprised if anyone uses codes between 0x1 and 0xffff
inclusive, so changing the semantics of those codes ought to be safe.
(Or we could gate them behind a flag.)  I'm not sure how I feel about
changing the semantics of 0.

--Andy

Re: seccomp: dump core when using SECCOMP_RET_KILL

[Kees Cook]

On Tue, Jan 31, 2017 at 1:59 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Fri, Jan 27, 2017 at 1:48 PM, Kees Cook <keescook@chromium.org> wrote:
>> On Wed, Jan 25, 2017 at 12:05 PM, Kees Cook <keescook@chromium.org> wrote:
>>> On Tue, Jan 24, 2017 at 4:53 PM, Andrei Vagin <avagin@virtuozzo.com> wrote:
>>>> Hi,
>>>>
>>>> One of CRIU tests fails with this patch:
>>>> https://github.com/xemul/criu/blob/master/test/zdtm/static/seccomp_filter_tsync.c
>>>>
>>>> Before this patch only a thread which called a "wrong" syscall is killed.
>>>> Now a whole process is killed if one of threads called a "wrong" syscall.
>>>
>>> Oh ew. I wonder what is causing this? In other do_coredump() callers,
>>> they explicitly call do_group_exit(). Hmmm
>>
>> We need to find a way to fix this or remove the coredump change from
>> -next. We have a few things that have come up recently (coincident
>> with the coredump change):
>>
>> - some folks would like seccomp kills to kill the entire process not
>> just the thread
>> - on a full-process kill, there needs to be a way to get a coredump
>> - on a kill, it would be nice to have reliable logging
>>
>> Getting a coredump requires a full-process kill. It is possible to do
>> this already with RET_TRAP and just not catch the SIGSYS. However,
>> this isn't sufficient if you want to be _sure_ the entire process gets
>> killed since RET_TRAP depends on cooperation from the process.
>>
>> Getting reliable logging out of seccomp for non-RET_KILL is
>> non-trivial because syscall-audit doesn't track forks.
>>
>> The RET_* values are part of the UAPI, so changing or adding to them
>> requires care.
>>
>> Right now we have very little room in the RET_* values (the lower
>> bytes are for the RET_DATA which is ignored for RET_KILL and the
>> semantics of changing that is very difficult):
>>
>> #define SECCOMP_RET_KILL        0x00000000U /* kill the task immediately */
>> #define SECCOMP_RET_TRAP        0x00030000U /* disallow and force a SIGSYS */
>>
>> Killing the entire process is more aggressive than RET_KILL currently,
>> so the question becomes, should we upgrade RET_KILL to
>> RET_KILL_PROCESS and add RET_KILL_THREAD? Are there people that WANT
>> only a thread to be killed? Andrei, does CRIU depend on this behavior,
>> or is it "just" a regression test detail?
>
> I think we actually have more flexibility: we have all the low bits --
> they're currently ignored.  We could plausibly have one code for "kill
> this thread, no dump", one code for "kill this process, dump core",
> and maybe one code for "kill this process, don't dump core".
>
> I would be a bit surprised if anyone uses codes between 0x1 and 0xffff
> inclusive, so changing the semantics of those codes ought to be safe.
> (Or we could gate them behind a flag.)  I'm not sure how I feel about
> changing the semantics of 0.

The most recently installed filter controls the value of the low bits,
so this isn't quite useful for this case, I don't think.

-Kees

-- 
Kees Cook
Pixel Security