lkml.org 
[lkml]   [2017]   [Mar]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v5 06/10] seccomp,landlock: Handle Landlock events per process hierarchy
    From
    Date


    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:
    >>>> 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.

    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)?

    >
    > 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.

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

    For a rule *replacement*, sure!

    >
    >> 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.

    I agree, but for now it's about how to handle immutable (but growing)
    inherited rules.

    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2017-03-02 00:32    [W:3.737 / U:0.612 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site