From: "Mickaël Salaün" <firstname.lastname@example.org>
To: Andy Lutomirski <email@example.com>
Cc: "firstname.lastname@example.org" <email@example.com>,
Alexei Starovoitov <firstname.lastname@example.org>,
Arnaldo Carvalho de Melo <email@example.com>,
Casey Schaufler <firstname.lastname@example.org>,
Daniel Borkmann <email@example.com>,
David Drysdale <firstname.lastname@example.org>,
"David S . Miller" <email@example.com>,
"Eric W . Biederman" <firstname.lastname@example.org>,
James Morris <email@example.com>,
Jann Horn <firstname.lastname@example.org>, Jonathan Corbet <email@example.com>,
Matthew Garrett <firstname.lastname@example.org>,
Michael Kerrisk <email@example.com>,
Kees Cook <firstname.lastname@example.org>,
Paul Moore <email@example.com>,
Sargun Dhillon <firstname.lastname@example.org>,
"Serge E . Hallyn" <email@example.com>,
Shuah Khan <firstname.lastname@example.org>, Tejun Heo <email@example.com>,
Thomas Graf <firstname.lastname@example.org>, Will Drewry <email@example.com>,
Linux API <firstname.lastname@example.org>,
LSM List <email@example.com>,
Network Development <firstname.lastname@example.org>,
Andrew Morton <email@example.com>
Subject: Re: [PATCH v5 06/10] seccomp,landlock: Handle Landlock events per process hierarchy
Date: Fri, 3 Mar 2017 02:05:00 +0100 [thread overview]
Message-ID: <firstname.lastname@example.org> (raw)
[-- Attachment #1.1: Type: text/plain, Size: 6165 bytes --]
On 03/03/2017 01:55, Andy Lutomirski wrote:
> On Thu, Mar 2, 2017 at 4:48 PM, Mickaël Salaün <email@example.com> wrote:
>> On 02/03/2017 17:36, Andy Lutomirski wrote:
>>> On Wed, Mar 1, 2017 at 3:28 PM, Mickaël Salaün <firstname.lastname@example.org> wrote:
>>>> On 01/03/2017 23:20, Andy Lutomirski wrote:
>>>>> On Wed, Mar 1, 2017 at 2:14 PM, Mickaël Salaün <email@example.com> wrote:
>>>>>> On 28/02/2017 21:01, Andy Lutomirski wrote:
>>>>>>> On Tue, Feb 21, 2017 at 5:26 PM, Mickaël Salaün <firstname.lastname@example.org> 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
>>> 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, ...);
>>> [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.
>> Good point. However the term "immutability" doesn't fit right because
>> the process is still allowed to add more rules to itself (as for
>> seccomp). The difference lays in the way a rule may be "appended" (by
>> the current process) or "inserted" (by a parent process).
>> I think three or four kind of operations (through the seccomp syscall)
>> make sense:
>> * append a rule (for the current process and its future children)
> Sure, but this operation should *never* affect existing children,
> existing seccomp layers, existing nodes, etc. It should affect
> current and future children only. Or it could simply not exist for
> Landlock and instead you'd have to add a layer (see below) and then
> program that layer.
>> * add a node (insert point), from which the inserted rules will be tied
>> * insert a rule in the node, which will be inherited by futures children
> I would advocate calling this a "seccomp layer" and making creation
> and manipulation of them generic.
>> * (maybe a "lock" command to make a layer immutable for the current
>> process and its children)
> Hmm, maybe.
>> Doing so, a process is only allowed to insert a rule if a node was
>> previously added. To forbid itself to insert new rules to one of its
>> children, a process just need to not add a node before forking. Like
>> this, there is no need for special rule flags nor default behavior,
>> everything is explicit.
> This is still slightly too complicated. If you really want an
> operation that adds a layer (please don't call it a node in the ABI)
> and adds a rule to that layer in a single operation, it should be a
> separate operation. Please make everything explicit.
> (I don't like exposing the word "node" to userspace because it means
> nothing. Having more than one layer of filter makes sense to me,
> which is why I like "layer". I'm sure that other good choices exist.)
I keep that for a future discussion, I'm now convinced the simple
inheritance (seccomp-like) doesn't block future extension.
>> For this series, I will stick to the same behavior as seccomp filter:
>> only append rules to the current process (and its future children).
>>>>> 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.
>> It should be OK with my previous proposal. The other details could be
>> discussed in a separate future patch series.
> NAK, or at least NAK pending better docs and justification. The
> operations of "add a layer and put a rule in it" and "add a rule to an
> existing layer" are logically different and should not be the same
> SECCOMP operation.
We are agree.
> "Do what I mean" is a nice paradigm for a language
> like Perl, but for security (and for kernel interfaces in general),
> "do what I say and error out if I said nonsense" is much safer.
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
next prev parent reply other threads:[~2017-03-03 1:17 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
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 [this message]
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
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 \
* 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).