Re: [PATCH] seccomp: plug syscall-dodging ptrace hole

[Jann Horn <jann@thejh.net>]

[-- Attachment #1: Type: text/plain, Size: 4781 bytes --]

On Thu, May 26, 2016 at 02:04:50PM -0700, Kees Cook wrote:
> One problem with seccomp was that ptrace could be used to change a
> syscall after seccomp filtering had completed. This was a well documented
> limitation, and it was recommended to block ptrace when defining a filter
> to avoid this problem. This can be quite a limitation for containers or
> other places where ptrace is desired even under seccomp filters.
> 
> Since seccomp filtering has been split into pre-trace and trace phases
> (phase1 and phase2 respectively), it's possible to re-run phase1 seccomp
> after ptrace. This makes that change, and updates the test suite for
> both SECCOMP_RET_TRACE and PTRACE_SYSCALL manipulation.

Looks good to me. As far as I can tell, there are no codepaths that allow
manipulation of syscall arguments via ptrace register modification without
going through tracehook_report_syscall_entry() or seccomp_phase2(), and
the checks look good, too.


> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  include/linux/seccomp.h                       |   6 +
>  include/linux/tracehook.h                     |   8 +-
>  kernel/seccomp.c                              |  42 ++++++
>  tools/testing/selftests/seccomp/seccomp_bpf.c | 176 ++++++++++++++++++++++++--
>  4 files changed, 220 insertions(+), 12 deletions(-)
> 
> diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h
> index 2296e6b2f690..e2b72394c200 100644
> --- a/include/linux/seccomp.h
> +++ b/include/linux/seccomp.h
> @@ -85,6 +85,7 @@ static inline int seccomp_mode(struct seccomp *s)
>  #ifdef CONFIG_SECCOMP_FILTER
>  extern void put_seccomp_filter(struct task_struct *tsk);
>  extern void get_seccomp_filter(struct task_struct *tsk);
> +extern int seccomp_phase1_recheck(void);
>  #else  /* CONFIG_SECCOMP_FILTER */
>  static inline void put_seccomp_filter(struct task_struct *tsk)
>  {
> @@ -94,6 +95,11 @@ static inline void get_seccomp_filter(struct task_struct *tsk)
>  {
>  	return;
>  }
> +
> +static inline int seccomp_phase1_recheck(void)
> +{
> +	return 0;
> +}
>  #endif /* CONFIG_SECCOMP_FILTER */
>  
>  #if defined(CONFIG_SECCOMP_FILTER) && defined(CONFIG_CHECKPOINT_RESTORE)
> diff --git a/include/linux/tracehook.h b/include/linux/tracehook.h
> index 26c152122a42..69b584d88508 100644
> --- a/include/linux/tracehook.h
> +++ b/include/linux/tracehook.h
> @@ -48,6 +48,7 @@
>  
>  #include <linux/sched.h>
>  #include <linux/ptrace.h>
> +#include <linux/seccomp.h>
>  #include <linux/security.h>
>  #include <linux/task_work.h>
>  #include <linux/memcontrol.h>
> @@ -100,7 +101,12 @@ static inline int ptrace_report_syscall(struct pt_regs *regs)
>  static inline __must_check int tracehook_report_syscall_entry(
>  	struct pt_regs *regs)
>  {
> -	return ptrace_report_syscall(regs);
> +	int skip;
> +
> +	skip = ptrace_report_syscall(regs);
> +	if (skip)
> +		return skip;
> +	return seccomp_phase1_recheck();
>  }
>  
>  /**
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index 7002796f14a4..6eaa3a1c5edb 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -665,6 +665,46 @@ u32 seccomp_phase1(struct seccomp_data *sd)
>  }
>  
>  /**
> + * seccomp_phase1_recheck() - recheck phase1 in the context of ptrace
> + *
> + * This re-runs phase 1 seccomp checks in the case where ptrace may have
> + * just changed things out from under us.
> + *
> + * Returns 0 if the syscall should be processed or -1 to skip the syscall.
> + */
> +int seccomp_phase1_recheck(void)
> +{
> +	u32 action;
> +
> +	/* If we're not under seccomp, continue normally. */
> +	if (!test_thread_flag(TIF_SECCOMP))
> +		return 0;
> +
> +	/* Pass NULL struct seccomp_data to force reload after ptrace. */
> +	action = seccomp_phase1(NULL);
> +	switch (action) {
> +	case SECCOMP_PHASE1_OK:
> +		/* Passes seccomp, continue normally. */
> +		break;
> +	case SECCOMP_PHASE1_SKIP:
> +		/* Skip the syscall. */
> +		return -1;
> +	default:
> +		if ((action & SECCOMP_RET_ACTION) != SECCOMP_RET_TRACE) {
> +			/* Impossible return value: kill the process. */
> +			do_exit(SIGSYS);
> +		}
> +		/*
> +		 * We've hit a trace request, but ptrace already put us
> +		 * into this state, so just continue.
> +		 */
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
>   * seccomp_phase2() - finish slow path seccomp work for the current syscall
>   * @phase1_result: The return value from seccomp_phase1()
>   *
> @@ -701,6 +741,8 @@ int seccomp_phase2(u32 phase1_result)
>  		do_exit(SIGSYS);
>  	if (syscall_get_nr(current, regs) < 0)
>  		return -1;  /* Explicit request to skip. */
> +	if (seccomp_phase1_recheck() < 0)
> +		return -1;
>  
>  	return 0;
>  }
[...]

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]