archive mirror
 help / color / mirror / Atom feed
From: Andy Lutomirski <>
To: "Mickaël Salaün" <>
Cc: "" <>,
	Alexei Starovoitov <>,
	Arnaldo Carvalho de Melo <>,
	Casey Schaufler <>,
	Daniel Borkmann <>,
	David Drysdale <>,
	"David S . Miller" <>,
	"Eric W . Biederman" <>,
	James Morris <>,
	Jann Horn <>, Jonathan Corbet <>,
	Matthew Garrett <>,
	Michael Kerrisk <>,
	Kees Cook <>,
	Paul Moore <>,
	Sargun Dhillon <>,
	"Serge E . Hallyn" <>,
	Shuah Khan <>, Tejun Heo <>,
	Thomas Graf <>, Will Drewry <>,
	Linux API <>,
	LSM List <>,
	Network Development <>,
	Andrew Morton <>
Subject: Re: [PATCH v5 06/10] seccomp,landlock: Handle Landlock events per process hierarchy
Date: Wed, 1 Mar 2017 14:20:44 -0800	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

On Wed, Mar 1, 2017 at 2:14 PM, Mickaël Salaün <> wrote:
> On 28/02/2017 21:01, Andy Lutomirski wrote:
>> On Tue, Feb 21, 2017 at 5:26 PM, Mickaël Salaün <> wrote:
>>> The seccomp(2) syscall can be use to apply a Landlock rule to the
>>> current process. As with a seccomp filter, the Landlock rule is enforced
>>> for all its future children. An inherited rule tree can be updated
>>> (append-only) by the owner of inherited Landlock nodes (e.g. a parent
>>> process that create a new rule)
>> Can you clarify exaclty what this type of update does?  Is it
>> something that should be supported by normal seccomp rules as well?
> There is two main structures involved here: struct landlock_node and
> struct landlock_rule, both defined in include/linux/landlock.h [02/10].
> Let's take an example with seccomp filter and then Landlock:
> * seccomp filter: Process P1 creates and applies a seccomp filter F1 to
> itself. Then it forks and creates a child P2, which inherits P1's
> filters, hence F1. Now, if P1 add a new seccomp filter F2 to itself, P2
> *won't get it*. The P2's filter list will still only contains F1 but not
> F2. If P2 sets up and applies a new filter F3 to itself, its filter list
> will contains F1 and F3.
> * Landlock: Process P1 creates and applies a Landlock rule R1 to itself.
> Underneath the kernel creates a new node N1 dedicated to P1, which
> contains all its rules. Then P1 forks and creates a child P2, which
> inherits P1's rules, hence R1. Underneath P2 inherited N1. Now, if P1
> add a new Landlock rule R2 to itself, P2 *will get it* as well (because
> R2 is part of N1). If P2 creates and applies a new rule R3 to itself,
> its rules will contains R1, R2 and R3. Underneath the kernel created a
> new node N2 for P2, which only contains R3 but inherits/links to N1.
> 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.

2. The API that adds a layer should be different from the API that
modifies a layer.

3. The whole modification mechanism should be a separate patch to be
reviewed on its own merits.

> The current inheritance mechanism doesn't enable to only add a rule to
> the current process. The rule will be inherited by its children
> (starting from the children created after the first applied rule). An
> option flag NEW_RULE_HIERARCHY (or maybe another seccomp operation)
> could enable to create a new node for the current process, and then
> makes it not inherited by the previous children.

I like my proposal above much better.  "Add a layer" and "change a
layer" should be different operations.


  reply	other threads:[~2017-03-01 23:29 UTC|newest]

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 [this message]
2017-03-01 23:28         ` Mickaël Salaün
2017-03-02 16:36           ` Andy Lutomirski
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:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='' \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).