On Thu, Mar 2, 2017 at 4:48 PM, Mickaël Salaün [off-list ref] wrote:

On 02/03/2017 17:36, Andy Lutomirski wrote:

quoted

On Wed, Mar 1, 2017 at 3:28 PM, Mickaël Salaün [off-list ref] wrote:

quoted

On 01/03/2017 23:20, Andy Lutomirski wrote:

quoted

On Wed, Mar 1, 2017 at 2:14 PM, Mickaël Salaün [off-list ref] wrote:

quoted

On 28/02/2017 21:01, Andy Lutomirski wrote:

quoted

On Tue, Feb 21, 2017 at 5:26 PM, Mickaël Salaün [off-list ref] 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.)

For this series, I will stick to the same behavior as seccomp filter:
only append rules to the current process (and its future children).

quoted

quoted

quoted

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