Messages in this thread | | | Date | Fri, 27 May 2016 00:56:44 +0200 | From | Jann Horn <> | Subject | Re: [PATCH] seccomp: plug syscall-dodging ptrace hole |
| |
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; > } [...] [unhandled content-type:application/pgp-signature] | |