From: Andy Lutomirski <luto@amacapital.net> To: "Mickaël Salaün" <mic@digikod.net> Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, Alexei Starovoitov <ast@kernel.org>, Arnaldo Carvalho de Melo <acme@kernel.org>, Casey Schaufler <casey@schaufler-ca.com>, Daniel Borkmann <daniel@iogearbox.net>, David Drysdale <drysdale@google.com>, "David S . Miller" <davem@davemloft.net>, "Eric W . Biederman" <ebiederm@xmission.com>, James Morris <james.l.morris@oracle.com>, Jann Horn <jann@thejh.net>, Jonathan Corbet <corbet@lwn.net>, Matthew Garrett <mjg59@srcf.ucam.org>, Michael Kerrisk <mtk.manpages@gmail.com>, Kees Cook <keescook@chromium.org>, Paul Moore <paul@paul-moore.com>, Sargun Dhillon <sargun@sargun.me>, "Serge E . Hallyn" <serge@hallyn.com>, Shuah Khan <shuah@kernel.org>, Tejun Heo <tj@kernel.org>, Thomas Graf <tgraf@suug.ch>, Will Drewry <wad@chromium.org>, "kernel-hardening@lists.openwall.com" <kernel-hardening@lists.openwall.com>, Linux API <linux-api@vger.kernel.org>, LSM List <linux-security-module@vger.kernel.org>, Network Development <netdev@vger.kernel.org>, Andrew Morton <akpm@linux-foundation.org> Subject: Re: [PATCH v5 06/10] seccomp,landlock: Handle Landlock events per process hierarchy Date: Thu, 2 Mar 2017 08:36:27 -0800 Message-ID: <CALCETrXpaz3Sw8Q28PKi2LGB956bZ0mfhoNg59Pe5EY+kTmqZg@mail.gmail.com> (raw) In-Reply-To: <db8cbaea-d4e5-3123-0b14-857737b753bc@digikod.net> On Wed, Mar 1, 2017 at 3:28 PM, Mickaël Salaün <mic@digikod.net> wrote: > > > On 01/03/2017 23:20, Andy Lutomirski wrote: >> On Wed, Mar 1, 2017 at 2:14 PM, Mickaël Salaün <mic@digikod.net> wrote: >>> >>> On 28/02/2017 21:01, Andy Lutomirski wrote: >>>> On Tue, Feb 21, 2017 at 5:26 PM, Mickaël Salaün <mic@digikod.net> wrote: >>> This design makes it possible for a process to add more constraints to >>> its children on the fly. I think it is a good feature to have and a >>> safer default inheritance mechanism, but it could be guarded by an >>> option flag if we want both mechanism to be available. The same design >>> could be used by seccomp filter too. >>> >> >> Then let's do it right. >> >> Currently each task has an array of seccomp filter layers. When a >> task forks, the child inherits the layers. All the layers are >> presently immutable. With Landlock, a layer can logically be a >> syscall fitler layer or a Landlock layer. This fits in to the >> existing model just fine. >> >> If we want to have an interface to allow modification of an existing >> layer, let's make it so that, when a layer is added, you have to >> specify a flag to make the layer modifiable (by current, presumably, >> although I can imagine other policies down the road). Then have a >> separate API that modifies a layer. >> >> IOW, I think your patch is bad for three reasons, all fixable: >> >> 1. The default is wrong. A layer should be immutable to avoid an easy >> attack in which you try to sandbox *yourself* and then you just modify >> the layer to weaken it. > > This is not possible, there is only an operation for now: > SECCOMP_ADD_LANDLOCK_RULE. You can only add more rules to the list (as > for seccomp filter). There is no way to weaken a sandbox. The question > is: how do we want to handle the rules *tree* (from the kernel point of > view)? > Fair enough. But I still think that immutability (like regular seccomp) should be the default. For security, simplicity is important. I guess there could be two ways to relax immutability: allowing making the layer stricter and allowing any change at all. As a default, though, programs should be able to expect that: seccomp(SECCOMP_ADD_WHATEVER, ...); fork(); [parent gets compromised] [in parent]seccomp(anything whatsoever); will not affect the child, If the parent wants to relax that, that's fine, but I think it should be explicit. >> >> 2. The API that adds a layer should be different from the API that >> modifies a layer. > > Right, but it doesn't apply now because we can only add rules. That's not what the code appears to do, though. Sometimes it makes a new layer without modifying tasks that share the layer and sometimes it modifies the layer. Both operations are probably okay, but they're not the same operation and they shouldn't pretend to be. > >> >> 3. The whole modification mechanism should be a separate patch to be >> reviewed on its own merits. > > For a rule *replacement*, sure! And for modification of policy for non-current tasks. That's a big departure from normal seccomp and should be reviewed as such.
next prev parent reply index Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-02-22 1:26 [PATCH v5 00/10] Landlock LSM: Toward unprivileged sandboxing Mickaël Salaün 2017-02-22 1:26 ` [PATCH v5 01/10] bpf: Add eBPF program subtype and is_valid_subtype() verifier Mickaël Salaün 2017-02-22 1:26 ` [PATCH v5 02/10] bpf,landlock: Define an eBPF program type for Landlock Mickaël Salaün 2017-02-22 1:26 ` [PATCH v5 03/10] bpf: Define handle_fs and add a new helper bpf_handle_fs_get_mode() Mickaël Salaün 2017-03-01 9:32 ` James Morris 2017-03-01 22:20 ` Mickaël Salaün 2017-02-22 1:26 ` [PATCH v5 04/10] landlock: Add LSM hooks related to filesystem Mickaël Salaün 2017-02-22 1:26 ` [PATCH v5 05/10] seccomp: Split put_seccomp_filter() with put_seccomp() Mickaël Salaün 2017-02-22 1:26 ` [PATCH v5 06/10] seccomp,landlock: Handle Landlock events per process hierarchy Mickaël Salaün 2017-02-28 20:01 ` Andy Lutomirski 2017-03-01 22:14 ` Mickaël Salaün 2017-03-01 22:20 ` Andy Lutomirski 2017-03-01 23:28 ` Mickaël Salaün 2017-03-02 16:36 ` Andy Lutomirski [this message] 2017-03-03 0:48 ` Mickaël Salaün 2017-03-03 0:55 ` Andy Lutomirski 2017-03-03 1:05 ` Mickaël Salaün 2017-03-02 10:22 ` [kernel-hardening] " Djalal Harouni 2017-03-03 0:54 ` Mickaël Salaün 2017-02-22 1:26 ` [PATCH v5 07/10] bpf: Add a Landlock sandbox example Mickaël Salaün 2017-02-23 22:13 ` Mickaël Salaün 2017-02-22 1:26 ` [PATCH v5 08/10] seccomp: Enhance test_harness with an assert step mechanism Mickaël Salaün 2017-02-22 1:26 ` [PATCH v5 09/10] bpf,landlock: Add tests for Landlock Mickaël Salaün 2017-02-22 1:26 ` [PATCH v5 10/10] landlock: Add user and kernel documentation " Mickaël Salaün 2017-02-22 5:21 ` Andy Lutomirski 2017-02-22 7:43 ` Mickaël Salaün
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CALCETrXpaz3Sw8Q28PKi2LGB956bZ0mfhoNg59Pe5EY+kTmqZg@mail.gmail.com \ --to=luto@amacapital.net \ --cc=acme@kernel.org \ --cc=akpm@linux-foundation.org \ --cc=ast@kernel.org \ --cc=casey@schaufler-ca.com \ --cc=corbet@lwn.net \ --cc=daniel@iogearbox.net \ --cc=davem@davemloft.net \ --cc=drysdale@google.com \ --cc=ebiederm@xmission.com \ --cc=james.l.morris@oracle.com \ --cc=jann@thejh.net \ --cc=keescook@chromium.org \ --cc=kernel-hardening@lists.openwall.com \ --cc=linux-api@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-security-module@vger.kernel.org \ --cc=mic@digikod.net \ --cc=mjg59@srcf.ucam.org \ --cc=mtk.manpages@gmail.com \ --cc=netdev@vger.kernel.org \ --cc=paul@paul-moore.com \ --cc=sargun@sargun.me \ --cc=serge@hallyn.com \ --cc=shuah@kernel.org \ --cc=tgraf@suug.ch \ --cc=tj@kernel.org \ --cc=wad@chromium.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
LKML Archive on lore.kernel.org Archives are clonable: git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \ linux-kernel@vger.kernel.org public-inbox-index lkml Example config snippet for mirrors Newsgroup available over NNTP: nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel AGPL code for this site: git clone https://public-inbox.org/public-inbox.git