Received: by 10.223.185.116 with SMTP id b49csp5564845wrg; Tue, 27 Feb 2018 16:02:39 -0800 (PST) X-Google-Smtp-Source: AH8x227Bpza3XJbZQziOXbceZrgO6qih8S6q1nGiKHFs37xOyB78Syqfy+qDcD+KAzqSJBmZl0fq X-Received: by 10.99.124.91 with SMTP id l27mr12389559pgn.298.1519776159818; Tue, 27 Feb 2018 16:02:39 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1519776159; cv=none; d=google.com; s=arc-20160816; b=XUuzrXgUtYCEjW/3SL99SlOZGhV2YAI75k0BHkJcnWizpC1SNOtSLWafDGG93OqfCj 7lT3C+HeKYL2Tnya5awo5XdW50V+BnaRtWDziegZW/+Vjs1XqlybWmqoh7VYvuSXVJyD rhVPjAxGew2awtrY7QURP3GRnMMt6c76ItOkNlbEJhNzESpjxVnhjm0jZSiHwyp+z9Pv uBksaNPyIj2+D9HgtlwJsSBS3fr57D1haYWSe3dkY2iR1fyE+GIV0n6KGIzncIwTCOXg 460xJ1PQlZ21cd/Gh+Ib1iK9jDzTG9DCuI1K/IOi57plY8vnhUewOapzP9Pnt+qZbOad TNGQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:in-reply-to:mime-version:user-agent:date :message-id:from:references:cc:to:subject:arc-authentication-results; bh=Q8S+1Q/7i4d7Shodz9Isg4fDaL4vVfgJOVJwwhwy/Aw=; b=wJtPdQG/BccY7VHtEhNyiop3rs/WXtMER1ucZ86VZUzzngj7or1hMdhfYw3tqKNeID Qr5gylRRjs+UYkU3XvgIRvpLSM/mEN4r1HMpEasf4vqZ8uetYOY0L8MV1RSo39DS5KT5 KyswKA20adzyzK6Wmay/uEjiUPuIYOjWrq0wcdlcxZVI5jAbKrvdfRic/1ZEwFdifgO5 I1KWGJn5Um6GOrqxQN1rzyRNV6nFr6XFSxVlgVRfwZLrz4F3CnOI1CLI5UvHEe/ihyI/ 53dsVFW09ppdrFH3TOrnrGHaj1iJwphsJhcsDMFsrOJCDQ67qlXioq6cRr8v9X4M0Gom xxHA== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id w1si187075pgm.799.2018.02.27.16.02.24; Tue, 27 Feb 2018 16:02:39 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751721AbeB1ABn (ORCPT + 99 others); Tue, 27 Feb 2018 19:01:43 -0500 Received: from smtp-sh2.infomaniak.ch ([128.65.195.6]:37536 "EHLO smtp-sh2.infomaniak.ch" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751126AbeB1ABk (ORCPT ); Tue, 27 Feb 2018 19:01:40 -0500 Received: from smtp5.infomaniak.ch (smtp5.infomaniak.ch [83.166.132.18]) by smtp-sh.infomaniak.ch (8.14.5/8.14.5) with ESMTP id w1S00IoG000370 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 28 Feb 2018 01:00:19 +0100 Received: from ns3096276.ip-94-23-54.eu (ns3096276.ip-94-23-54.eu [94.23.54.103]) (authenticated bits=0) by smtp5.infomaniak.ch (8.14.5/8.14.5) with ESMTP id w1S00Fm3048685 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Wed, 28 Feb 2018 01:00:16 +0100 Subject: Re: [PATCH bpf-next v8 08/11] landlock: Add ptrace restrictions To: Andy Lutomirski Cc: LKML , Alexei Starovoitov , Arnaldo Carvalho de Melo , Casey Schaufler , Daniel Borkmann , David Drysdale , "David S . Miller" , "Eric W . Biederman" , James Morris , Jann Horn , Jonathan Corbet , Michael Kerrisk , Kees Cook , Paul Moore , Sargun Dhillon , "Serge E . Hallyn" , Shuah Khan , Tejun Heo , Thomas Graf , Tycho Andersen , Will Drewry , Kernel Hardening , Linux API , LSM List , Network Development References: <20180227004121.3633-1-mic@digikod.net> <20180227004121.3633-9-mic@digikod.net> <0e7d0512-12a3-568d-aa55-3def4b91c6d0@digikod.net> From: =?UTF-8?Q?Micka=c3=abl_Sala=c3=bcn?= Message-ID: Date: Wed, 28 Feb 2018 01:00:10 +0100 User-Agent: MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="2eRD9swR8P6ZLiKYgLnCeG5fVYgVzeZED" X-Antivirus: Dr.Web (R) for Unix mail servers drweb plugin ver.6.0.2.8 X-Antivirus-Code: 0x100000 Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --2eRD9swR8P6ZLiKYgLnCeG5fVYgVzeZED Content-Type: multipart/mixed; boundary="RzDzFHzLXzjtV1wK3XaAt7NETvtCmIfrn"; protected-headers="v1" From: =?UTF-8?Q?Micka=c3=abl_Sala=c3=bcn?= To: Andy Lutomirski Cc: LKML , Alexei Starovoitov , Arnaldo Carvalho de Melo , Casey Schaufler , Daniel Borkmann , David Drysdale , "David S . Miller" , "Eric W . Biederman" , James Morris , Jann Horn , Jonathan Corbet , Michael Kerrisk , Kees Cook , Paul Moore , Sargun Dhillon , "Serge E . Hallyn" , Shuah Khan , Tejun Heo , Thomas Graf , Tycho Andersen , Will Drewry , Kernel Hardening , Linux API , LSM List , Network Development Message-ID: Subject: Re: [PATCH bpf-next v8 08/11] landlock: Add ptrace restrictions References: <20180227004121.3633-1-mic@digikod.net> <20180227004121.3633-9-mic@digikod.net> <0e7d0512-12a3-568d-aa55-3def4b91c6d0@digikod.net> In-Reply-To: --RzDzFHzLXzjtV1wK3XaAt7NETvtCmIfrn Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 28/02/2018 00:23, Andy Lutomirski wrote: > On Tue, Feb 27, 2018 at 11:02 PM, Andy Lutomirski wro= te: >> On Tue, Feb 27, 2018 at 10:14 PM, Micka=C3=ABl Sala=C3=BCn wrote: >>> >>> On 27/02/2018 06:01, Andy Lutomirski wrote: >>>> >>>> >>>>> On Feb 26, 2018, at 8:17 PM, Andy Lutomirski = wrote: >>>>> >>>>>> On Tue, Feb 27, 2018 at 12:41 AM, Micka=C3=ABl Sala=C3=BCn wrote: >>>>>> A landlocked process has less privileges than a non-landlocked pro= cess >>>>>> and must then be subject to additional restrictions when manipulat= ing >>>>>> processes. To be allowed to use ptrace(2) and related syscalls on = a >>>>>> target process, a landlocked process must have a subset of the tar= get >>>>>> process' rules. >>>>>> >>>>>> Signed-off-by: Micka=C3=ABl Sala=C3=BCn >>>>>> Cc: Alexei Starovoitov >>>>>> Cc: Andy Lutomirski >>>>>> Cc: Daniel Borkmann >>>>>> Cc: David S. Miller >>>>>> Cc: James Morris >>>>>> Cc: Kees Cook >>>>>> Cc: Serge E. Hallyn >>>>>> --- >>>>>> >>>>>> Changes since v6: >>>>>> * factor out ptrace check >>>>>> * constify pointers >>>>>> * cleanup headers >>>>>> * use the new security_add_hooks() >>>>>> --- >>>>>> security/landlock/Makefile | 2 +- >>>>>> security/landlock/hooks_ptrace.c | 124 +++++++++++++++++++++++++++= ++++++++++++ >>>>>> security/landlock/hooks_ptrace.h | 11 ++++ >>>>>> security/landlock/init.c | 2 + >>>>>> 4 files changed, 138 insertions(+), 1 deletion(-) >>>>>> create mode 100644 security/landlock/hooks_ptrace.c >>>>>> create mode 100644 security/landlock/hooks_ptrace.h >>>>>> >>>>>> diff --git a/security/landlock/Makefile b/security/landlock/Makefi= le >>>>>> index d0f532a93b4e..605504d852d3 100644 >>>>>> --- a/security/landlock/Makefile >>>>>> +++ b/security/landlock/Makefile >>>>>> @@ -3,4 +3,4 @@ obj-$(CONFIG_SECURITY_LANDLOCK) :=3D landlock.o >>>>>> landlock-y :=3D init.o chain.o task.o \ >>>>>> tag.o tag_fs.o \ >>>>>> enforce.o enforce_seccomp.o \ >>>>>> - hooks.o hooks_cred.o hooks_fs.o >>>>>> + hooks.o hooks_cred.o hooks_fs.o hooks_ptrace.o >>>>>> diff --git a/security/landlock/hooks_ptrace.c b/security/landlock/= hooks_ptrace.c >>>>>> new file mode 100644 >>>>>> index 000000000000..f1b977b9c808 >>>>>> --- /dev/null >>>>>> +++ b/security/landlock/hooks_ptrace.c >>>>>> @@ -0,0 +1,124 @@ >>>>>> +/* >>>>>> + * Landlock LSM - ptrace hooks >>>>>> + * >>>>>> + * Copyright =C2=A9 2017 Micka=C3=ABl Sala=C3=BCn >>>>>> + * >>>>>> + * This program is free software; you can redistribute it and/or = modify >>>>>> + * it under the terms of the GNU General Public License version 2= , as >>>>>> + * published by the Free Software Foundation. >>>>>> + */ >>>>>> + >>>>>> +#include >>>>>> +#include >>>>>> +#include /* ARRAY_SIZE */ >>>>>> +#include >>>>>> +#include /* struct task_struct */ >>>>>> +#include >>>>>> + >>>>>> +#include "common.h" /* struct landlock_prog_set */ >>>>>> +#include "hooks.h" /* landlocked() */ >>>>>> +#include "hooks_ptrace.h" >>>>>> + >>>>>> +static bool progs_are_subset(const struct landlock_prog_set *pare= nt, >>>>>> + const struct landlock_prog_set *child) >>>>>> +{ >>>>>> + size_t i; >>>>>> + >>>>>> + if (!parent || !child) >>>>>> + return false; >>>>>> + if (parent =3D=3D child) >>>>>> + return true; >>>>>> + >>>>>> + for (i =3D 0; i < ARRAY_SIZE(child->programs); i++) { >>>>> >>>>> ARRAY_SIZE(child->programs) seems misleading. Is there no define >>>>> NUM_LANDLOCK_PROG_TYPES or similar? >>>>> >>>>>> + struct landlock_prog_list *walker; >>>>>> + bool found_parent =3D false; >>>>>> + >>>>>> + if (!parent->programs[i]) >>>>>> + continue; >>>>>> + for (walker =3D child->programs[i]; walker; >>>>>> + walker =3D walker->prev) { >>>>>> + if (walker =3D=3D parent->programs[i]) { >>>>>> + found_parent =3D true; >>>>>> + break; >>>>>> + } >>>>>> + } >>>>>> + if (!found_parent) >>>>>> + return false; >>>>>> + } >>>>>> + return true; >>>>>> +} >>>>> >>>>> If you used seccomp, you'd get this type of check for free, and it >>>>> would be a lot easier to comprehend. AFAICT the only extra lenienc= y >>>>> you're granting is that you're agnostic to the order in which the >>>>> rules associated with different program types were applied, which >>>>> could easily be added to seccomp. >>>> >>>> On second thought, this is all way too complicated. I think the cor= rect logic is either "if you are filtered by Landlock, you cannot ptrace = anything" or to delete this patch entirely. >>> >>> This does not fit a lot of use cases like running a container >>> constrained with some Landlock programs. We should not deny users the= >>> ability to debug their stuff. >>> >>> This patch add the minimal protection which are needed to have >>> meaningful Landlock security policy. Without it, they may be easily >>> bypassable, hence useless. >>> >> >> I think you're wrong here. Any sane container trying to use Landlock >> like this would also create a PID namespace. Problem solved. I still= >> think you should drop this patch. Containers is one use case, another is build-in sandboxing (e.g. for web browser=E2=80=A6) and another one is for sandbox managers (e.g. Firejail,= Bubblewrap, Flatpack=E2=80=A6). In some of these use cases, especially fr= om a developer point of view, you may want/need to debug your applications (without requiring to be root). For nested Landlock access-controls (e.g. container + user session + web browser), it may not be allowed to create a PID namespace, but you still want to have a meaningful access-control. >> >>> >>>> If something like Tycho's notifiers goes in, then it's not obvious t= hat, just because you have the same set of filters, you have the same pri= vilege. Similarly, if a feature that lets a filter query its cgroup goes= in (and you proposed this once!) then the logic you implemented here is = wrong. >>> >>> I don't get your point. Please take a look at the tests (patch 10). >> >> I don't know what you want me to look at. >> >> What I'm saying is: suppose I write a filter like this: >> >> bool allow_some_action(void) >> { >> int value_from_container_manager =3D call_out_to_user_notifier(); >> return value_from_container_manager =3D=3D 42; >> } >> >> or >> >> bool allow_some_action(void) >> { >> return my_cgroup_is("/foo/bar"); >> } >> >> In both of these cases, your code will do the wrong thing. You are right about the fact that the same filters/programs may not be equivalent if they use external data (other than from the eBPF context) to take a decision. This is why using a function my_cgroup_is("/foo/bar") should not be allowed. If we want to enforce a security policy according to a cgroup, the Landlock programs should be pinned on this cgroup. This way, the kernel knows if this programs should be called or not. It is the same argument I used in the thread [PATCH bpf-next v8 05/11] about the cache. The only way a Landlock program may change its behavior is because of an eBPF map. However, in this case the map is common to all the instances of this program. To say it another way, the Landlock's enforce API (currently only seccomp) is in charge of defining what is a subject. By using seccomp to enforce a policy, the subject is a hierarchy of processes. By pinning a Landlock program to a cgroup, the subject is the set of processes under this cgroup. This is much more efficient than letting a program define its one subjects. This also allows to audit which processes are restricted by a set of Landlock programs. Because of that, calls to functions like bpf_get_current_pid_tgid() should not be allowed (or limited) for a Landlock program. Let's make this programs as pure as possible. :) >> >>> >>>> >>>> Or you could just say that it's the responsibility of a Landlock use= r to properly filter ptrace() just like it's the responsibility of seccom= p users to filter ptrace if needed. >>> >>> A user should be able to enforce a security policy on ptrace as well,= >>> but this patch enforce a minimal set of security boundaries. It will = be >>> easy to add a new Landlock program type to get this kind of access co= ntrol. >> >> It sounds like you want Landlock to be a complete container system all= >> by itself. I disagree with that design goal. >=20 > Having actually read your series more correctly now (oops!), I still > think that this patch should be dropped. I can see an argument for > having a flag that one can set when adding a seccomp filter that says > "prevent ptrace of any child that doesn't have this exact stack > installed", but I think that could be added later and should not be > part of an initial submission. For now, Landlock users can block > ptrace() entirely or use PID namespaces. >=20 I also though about using a flag, but we should encourage sane/safe default behavior, which means at least to not have trivially bypassable access-control rules, to not shoot yourself in the foot. However, a flag could be added to disable this safe behavior. --RzDzFHzLXzjtV1wK3XaAt7NETvtCmIfrn-- --2eRD9swR8P6ZLiKYgLnCeG5fVYgVzeZED Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAEBCgAdFiEEUysCyY8er9Axt7hqIt7+33O9apUFAlqV8QsACgkQIt7+33O9 apW5ywf+N5ntwm3Th6YCmqP1vqHEBxKpwBrQT0x76w0VtxEDk1CNg3D5fNLLCPDX 6rzLeyzQAH6XBqI/5NJSZjMa788677mBPVbGGyqfbXEBJpblR+EG7qGpG74HQwsh 4Hm26cpvyVWZN7uKdkb2+/Z0k7hu78dLkGCbkmmbt3gqpca1x2mV3nFDrtU4iutR nIYPwXJZFmPx6H1Gg+fiKfrFNj1+YwKN3jhN/7EMzRD7ikO2DtisX8qgq8jknwx6 e8xBkpBSvYKzw8Ek1C6nlDYL3Pz6kO052cP6/iSSD2c9blT7B9JXw3K4sqW3Qwws orl8N2b/kYSioG0b8v1S1zYilAYAfQ== =WcaF -----END PGP SIGNATURE----- --2eRD9swR8P6ZLiKYgLnCeG5fVYgVzeZED--