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 <keesc...@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;
> }
[...]
signature.asc
Description: Digital signature
