Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date: Fri, 3 Mar 2017 02:05:00 +0100
From: Mickaël Salaün <>
To: Andy Lutomirski <>
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

On 03/03/2017 01:55, Andy Lutomirski wrote:
> On Thu, Mar 2, 2017 at 4:48 PM, Mickaël Salaün <> wrote:
>> On 02/03/2017 17:36, Andy Lutomirski wrote:
>>> On Wed, Mar 1, 2017 at 3:28 PM, Mickaël Salaün <> wrote:
>>>> On 01/03/2017 23:20, Andy Lutomirski wrote:
>>>>> 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:
>>>>>> 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.
>> 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.

Totally agree.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.