From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752158AbdBKATi (ORCPT ); Fri, 10 Feb 2017 19:19:38 -0500 Received: from youngberry.canonical.com ([91.189.89.112]:48511 "EHLO youngberry.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751060AbdBKATh (ORCPT ); Fri, 10 Feb 2017 19:19:37 -0500 Subject: Re: [PATCH v2 3/4] seccomp: Create an action to log before allowing To: Kees Cook References: <1486100262-32391-1-git-send-email-tyhicks@canonical.com> <1486100262-32391-4-git-send-email-tyhicks@canonical.com> Cc: Paul Moore , Eric Paris , Andy Lutomirski , Will Drewry , linux-audit@redhat.com, LKML From: Tyler Hicks Message-ID: Date: Fri, 10 Feb 2017 18:01:07 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="0effuG85QlHh1m8Wmx5FdFHrXhaVP9sqB" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --0effuG85QlHh1m8Wmx5FdFHrXhaVP9sqB Content-Type: multipart/mixed; boundary="1eGRkrJfVb2BxxgRn7I1FArM1OdKuNFK0"; protected-headers="v1" From: Tyler Hicks To: Kees Cook Cc: Paul Moore , Eric Paris , Andy Lutomirski , Will Drewry , linux-audit@redhat.com, LKML Message-ID: Subject: Re: [PATCH v2 3/4] seccomp: Create an action to log before allowing References: <1486100262-32391-1-git-send-email-tyhicks@canonical.com> <1486100262-32391-4-git-send-email-tyhicks@canonical.com> In-Reply-To: --1eGRkrJfVb2BxxgRn7I1FArM1OdKuNFK0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 02/07/2017 06:33 PM, Kees Cook wrote: > On Thu, Feb 2, 2017 at 9:37 PM, Tyler Hicks wro= te: >> Add a new action, SECCOMP_RET_LOG, that logs a syscall before allowing= >> the syscall. At the implementation level, this action is identical to >> the existing SECCOMP_RET_ALLOW action. However, it can be very useful = when >> initially developing a seccomp filter for an application. The develope= r >> can set the default action to be SECCOMP_RET_LOG, maybe mark any >> obviously needed syscalls with SECCOMP_RET_ALLOW, and then put the >> application through its paces. A list of syscalls that triggered the >> default action (SECCOMP_RET_LOG) can be easily gleaned from the logs a= nd >> that list can be used to build the syscall whitelist. Finally, the >> developer can change the default action to the desired value. >> >> This provides a more friendly experience than seeing the application g= et >> killed, then updating the filter and rebuilding the app, seeing the >> application get killed due to a different syscall, then updating the >> filter and rebuilding the app, etc. >> >> The functionality is similar to what's supported by the various LSMs. >> SELinux has permissive mode, AppArmor has complain mode, SMACK has >> bring-up mode, etc. >> >> SECCOMP_RET_LOG is given a lower value than SECCOMP_RET_ALLOW so that >> "allow" can be written to the max_action_to_log sysctl in order to get= a >> list of logged actions without the, potentially larger, set of allowed= >> actions. >> >> Signed-off-by: Tyler Hicks >> --- >> Documentation/prctl/seccomp_filter.txt | 6 ++++++ >> include/uapi/linux/seccomp.h | 1 + >> kernel/seccomp.c | 4 ++++ >> 3 files changed, 11 insertions(+) >> >> diff --git a/Documentation/prctl/seccomp_filter.txt b/Documentation/pr= ctl/seccomp_filter.txt >> index 1e469ef..ba55a91 100644 >> --- a/Documentation/prctl/seccomp_filter.txt >> +++ b/Documentation/prctl/seccomp_filter.txt >> @@ -138,6 +138,12 @@ SECCOMP_RET_TRACE: >> allow use of ptrace, even of other sandboxed processes, withou= t >> extreme care; ptracers can use this mechanism to escape.) >> >> +SECCOMP_RET_LOG: >> + Results in the system call being executed after it is logged. = This >> + should be used by application developers to learn which syscal= ls their >> + application needs without having to iterate through multiple t= est and >> + development cycles to build the list. >> + >> SECCOMP_RET_ALLOW: >> Results in the system call being executed. >> >> diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp= =2Eh >> index 0f238a4..67f72cd 100644 >> --- a/include/uapi/linux/seccomp.h >> +++ b/include/uapi/linux/seccomp.h >> @@ -29,6 +29,7 @@ >> #define SECCOMP_RET_TRAP 0x00030000U /* disallow and force a SI= GSYS */ >> #define SECCOMP_RET_ERRNO 0x00050000U /* returns an errno */ >> #define SECCOMP_RET_TRACE 0x7ff00000U /* pass to a tracer or dis= allow */ >> +#define SECCOMP_RET_LOG 0x7ffe0000U /* allow after log= ging */ >=20 > This adds to UAPI, so it'd be good to think for a moment about how > this would work on older kernels: right now, if someone tried to use > this RET_LOG on an old kernel, it'll get treated like RET_KILL. Is > this sane? It is not sane for userspace code to blindly attempt to use a new feature on an old kernel. One of the main motivations of the actions_avail sysctl is to allow userspace to be smart about what the current kernel supports. I'll be adding logic (requested by Paul) to libseccomp that checks this sysctl when SECOMP_RET_LOG is attempted to be used. Programs that don't use libseccomp will have to do something similar. >=20 > I'm also trying to figure out if there is some other solution to this, > but they all involve tests against an otherwise RET_ALLOW case, which > I want to avoid. :) >=20 > So, I think, for now, this looks good, but I'd prefer this be > 0x7ffc0000U, just to make sure we have not painted ourselves into a > numerical corner if we for some reason ever need to put something > between RET_ALLOW and RET_LOG. That makes sense. I'll do that in v3. >=20 >> #define SECCOMP_RET_ALLOW 0x7fff0000U /* allow */ >> >> /* Masks for the return value sections. */ >> diff --git a/kernel/seccomp.c b/kernel/seccomp.c >> index 548fb89..8627481 100644 >> --- a/kernel/seccomp.c >> +++ b/kernel/seccomp.c >> @@ -650,6 +650,7 @@ static int __seccomp_filter(int this_syscall, cons= t struct seccomp_data *sd, >> >> return 0; >> >> + case SECCOMP_RET_LOG: >=20 > Given my protective feelings about the RET_ALLOW case, can you make > this a fully separate case statement? I'd rather have RET_ALLOW be > distinctly separate. Sure! It actually has to be two different cases now that we're doing the hot path approach for SECCOMP_RET_ALLOW. Tyler >=20 >> case SECCOMP_RET_ALLOW: >> seccomp_log(this_syscall, 0, action); >> return 0; >> @@ -934,6 +935,7 @@ long seccomp_get_filter(struct task_struct *task, = unsigned long filter_off, >> #define SECCOMP_RET_TRAP_NAME "trap" >> #define SECCOMP_RET_ERRNO_NAME "errno" >> #define SECCOMP_RET_TRACE_NAME "trace" >> +#define SECCOMP_RET_LOG_NAME "log" >> #define SECCOMP_RET_ALLOW_NAME "allow" >> >> /* Largest strlen() of all action names */ >> @@ -943,6 +945,7 @@ static char seccomp_actions_avail[] =3D SECCOMP_RE= T_KILL_NAME " " >> SECCOMP_RET_TRAP_NAME " " >> SECCOMP_RET_ERRNO_NAME " " >> SECCOMP_RET_TRACE_NAME " " >> + SECCOMP_RET_LOG_NAME " " >> SECCOMP_RET_ALLOW_NAME; >> >> struct seccomp_action_name { >> @@ -955,6 +958,7 @@ static struct seccomp_action_name seccomp_action_n= ames[] =3D { >> { SECCOMP_RET_TRAP, SECCOMP_RET_TRAP_NAME }, >> { SECCOMP_RET_ERRNO, SECCOMP_RET_ERRNO_NAME }, >> { SECCOMP_RET_TRACE, SECCOMP_RET_TRACE_NAME }, >> + { SECCOMP_RET_LOG, SECCOMP_RET_LOG_NAME }, >> { SECCOMP_RET_ALLOW, SECCOMP_RET_ALLOW_NAME }, >> { } >> }; >> -- >> 2.7.4 >> >=20 > -Kees >=20 --1eGRkrJfVb2BxxgRn7I1FArM1OdKuNFK0-- --0effuG85QlHh1m8Wmx5FdFHrXhaVP9sqB Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAEBCgAGBQJYnlRDAAoJENaSAD2qAscK+9MP/0f4h4lYrVLshEOAcMlKwuQc TzOL5rS7zifGvuU1WR1SJRbX7pCsB7g4R3BjtQetyPmmBdHxsRbWsWixC9noG7Ez Gi16X5fXVWr+aj5XTL/Vf8b+g8KWgQzrYNUbSHIAFp0PZPdGCi2mJf3koRsLDhES NOMW3LZmumDCVzJSqutpS50het/7PyMgGk7luWKV8vDjh4GCbBE/1xRtqN9jrmzO FmkbvlV8MphqwOTuGX6wFkvKrAkHjj0rMT5vwuZ/K+u5pP/ayTa6bVmjJ1xfBzh8 ApH4GUS6qmj+WzhPKPcuzonfoqjHUVBwDa+gEB7zyrdqLe5z6W+FjLuW2AkUdEJ8 OPS48PKC8t4494lrqFpAx3VOGyLbCRwT1QZLJhRbe2d5nRfj7eg8us1LLLPELPzs q7P93f/2VaKhbF46VdWonYBinfMD3UvTP/c+lQIdtZDEv58cn1ZFZmyvsqU1/+Eh VdsnZCyIhgnUkvaDnWyLaUFKP95/RzmrUVNYa+Z+A+tsiGKbZ7D+j7eEjMMErblK 0PTyzV7rtSBJ0UW6ttWjXVs2SPavblaZlCBgb9wi0DpbFIhM16tpBL/0uNlWICOL 32HXHnyp4RN8kmqzKkEvYbkJ0/H6TT9hm+DknBCkg2Fe4CCQrOWv7k8vn+fqA8LB BOAz2+P7Oq/zEo0jujSD =zf5I -----END PGP SIGNATURE----- --0effuG85QlHh1m8Wmx5FdFHrXhaVP9sqB--