From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755400AbcEZW45 (ORCPT ); Thu, 26 May 2016 18:56:57 -0400 Received: from thejh.net ([37.221.195.125]:47515 "EHLO thejh.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754898AbcEZW44 (ORCPT ); Thu, 26 May 2016 18:56:56 -0400 Date: Fri, 27 May 2016 00:56:44 +0200 From: Jann Horn To: Kees Cook Cc: Andy Lutomirski , Stephane Graber , Will Drewry , linux-kernel@vger.kernel.org Subject: Re: [PATCH] seccomp: plug syscall-dodging ptrace hole Message-ID: <20160526225644.GA6137@pc.thejh.net> References: <20160526210450.GA31203@www.outflux.net> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="BOKacYhQ+x31HxR3" Content-Disposition: inline In-Reply-To: <20160526210450.GA31203@www.outflux.net> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --BOKacYhQ+x31HxR3 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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. >=20 > 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 > --- > 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(-) >=20 > 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_stru= ct *tsk) > { > return; > } > + > +static inline int seccomp_phase1_recheck(void) > +{ > + return 0; > +} > #endif /* CONFIG_SECCOMP_FILTER */ > =20 > #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 @@ > =20 > #include > #include > +#include > #include > #include > #include > @@ -100,7 +101,12 @@ static inline int ptrace_report_syscall(struct pt_re= gs *regs) > static inline __must_check int tracehook_report_syscall_entry( > struct pt_regs *regs) > { > - return ptrace_report_syscall(regs); > + int skip; > + > + skip =3D ptrace_report_syscall(regs); > + if (skip) > + return skip; > + return seccomp_phase1_recheck(); > } > =20 > /** > 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) > } > =20 > /** > + * 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 syscal= l. > + */ > +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 =3D 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) !=3D 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 sysc= all > * @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; > =20 > return 0; > } [...] --BOKacYhQ+x31HxR3 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJXR38rAAoJED4KNFJOeCOoHeEQAKu9w+u5E9NYo95Ci+qMTyYv Ch5ut5CEZgLeX40TqL+1Cq1OcYR44XZSkFAjovFgjZabi5gSpTKpI4A+OyagNheI OxBq1oTfup6ObJOCUx/wgjV1G2kM65d+JIZELFHJOwmKaDHI/j4XOeHOjAjjQSRG iGsbJQim/3ExuWrS5knWC8ADcYc1n9jMNMDm1BP2d4sc+dNqy5PqW0rOIV3zrkJ2 lQnwOavUR4PrxRUaoYUJvJzK3vXuLEZdNkFNkDMLnJADTadnOBrfob3mygFG3Ara aGoWQpq+vQbMNiOa4PPcvsg/cRDUn3ZxrZFr0MqNmw+J5vGTeyOy9zvj3Cw82nNq GZQiIxNp5NSJ/B8nobQLCqSZXzRHHa97PEC12sZMVjPLmPtS5lU2kSP7dKEpd+5k 8wut3wXJi6TkPV9sj4zMYNUrUFQdWOU5YIpfsHHulSZk6NVo/KHDkAhBk/xNUQA5 WTLq//+IzSaO0YnWuFude0wmdJu/XXtOpHoyTkbipm1EDUO+erf/mS6rRmviX92w 2+22hIc+jGn68p48ShC600rfd0lnDyEdakpH6rub1w+CHqYkkK9qH5svTo/L9Yuq ai45o8gq1qLecGjmfQGNnWkM1/8zurU4jFM98FoixsdBAVZDNfozPwPGWnj1fb38 AlsTxda+RaNOM5zeWTsY =azsy -----END PGP SIGNATURE----- --BOKacYhQ+x31HxR3--