Kernel-hardening archive on lore.kernel.org
 help / color / Atom feed
From: "Mickaël Salaün" <mic@digikod.net>
To: Andy Lutomirski <luto@amacapital.net>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Alexei Starovoitov <ast@kernel.org>,
	Arnaldo Carvalho de Melo <acme@kernel.org>,
	Casey Schaufler <casey@schaufler-ca.com>,
	Daniel Borkmann <daniel@iogearbox.net>,
	David Drysdale <drysdale@google.com>,
	"David S . Miller" <davem@davemloft.net>,
	"Eric W . Biederman" <ebiederm@xmission.com>,
	James Morris <james.l.morris@oracle.com>,
	Jann Horn <jann@thejh.net>, Jonathan Corbet <corbet@lwn.net>,
	Matthew Garrett <mjg59@srcf.ucam.org>,
	Michael Kerrisk <mtk.manpages@gmail.com>,
	Kees Cook <keescook@chromium.org>,
	Paul Moore <paul@paul-moore.com>,
	Sargun Dhillon <sargun@sargun.me>,
	"Serge E . Hallyn" <serge@hallyn.com>,
	Shuah Khan <shuah@kernel.org>, Tejun Heo <tj@kernel.org>,
	Thomas Graf <tgraf@suug.ch>, Will Drewry <wad@chromium.org>,
	"kernel-hardening@lists.openwall.com"
	<kernel-hardening@lists.openwall.com>,
	Linux API <linux-api@vger.kernel.org>,
	LSM List <linux-security-module@vger.kernel.org>,
	Network Development <netdev@vger.kernel.org>,
	Andrew Morton <akpm@linux-foundation.org>
Subject: [kernel-hardening] Re: [PATCH v5 06/10] seccomp,landlock: Handle Landlock events per process hierarchy
Date: Fri, 3 Mar 2017 02:05:00 +0100
Message-ID: <c10a503d-5e35-7785-2f3d-25ed8dd63fab@digikod.net> (raw)
In-Reply-To: <CALCETrW8Pmap7CgdtE+XL39fPR1tOM4d8BBA=S4jot-ocAcw3g@mail.gmail.com>

[-- 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 <mic@digikod.net> wrote:
>>
>> On 02/03/2017 17:36, Andy Lutomirski wrote:
>>> On Wed, Mar 1, 2017 at 3:28 PM, Mickaël Salaün <mic@digikod.net> wrote:
>>>>
>>>>
>>>> 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:
>>>>>> 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.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

  reply index

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-02-22  1:26 [kernel-hardening] [PATCH v5 00/10] Landlock LSM: Toward unprivileged sandboxing Mickaël Salaün
2017-02-22  1:26 ` [kernel-hardening] [PATCH v5 01/10] bpf: Add eBPF program subtype and is_valid_subtype() verifier Mickaël Salaün
2017-02-22  1:26 ` [kernel-hardening] [PATCH v5 02/10] bpf,landlock: Define an eBPF program type for Landlock Mickaël Salaün
2017-02-22  1:26 ` [kernel-hardening] [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   ` [kernel-hardening] " James Morris
2017-03-01 22:20     ` Mickaël Salaün
2017-02-22  1:26 ` [kernel-hardening] [PATCH v5 04/10] landlock: Add LSM hooks related to filesystem Mickaël Salaün
2017-02-22  1:26 ` [kernel-hardening] [PATCH v5 05/10] seccomp: Split put_seccomp_filter() with put_seccomp() Mickaël Salaün
2017-02-22  1:26 ` [kernel-hardening] [PATCH v5 06/10] seccomp,landlock: Handle Landlock events per process hierarchy Mickaël Salaün
2017-02-28 20:01   ` [kernel-hardening] " 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 ` [kernel-hardening] [PATCH v5 07/10] bpf: Add a Landlock sandbox example Mickaël Salaün
2017-02-23 22:13   ` [kernel-hardening] " Mickaël Salaün
2017-02-22  1:26 ` [kernel-hardening] [PATCH v5 08/10] seccomp: Enhance test_harness with an assert step mechanism Mickaël Salaün
2017-02-22  1:26 ` [kernel-hardening] [PATCH v5 09/10] bpf,landlock: Add tests for Landlock Mickaël Salaün
2017-02-22  1:26 ` [kernel-hardening] [PATCH v5 10/10] landlock: Add user and kernel documentation " Mickaël Salaün
2017-02-22  5:21   ` [kernel-hardening] " Andy Lutomirski
2017-02-22  7:43     ` Mickaël Salaün

Reply instructions:

You may reply publically 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:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

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

  git send-email \
    --in-reply-to=c10a503d-5e35-7785-2f3d-25ed8dd63fab@digikod.net \
    --to=mic@digikod.net \
    --cc=acme@kernel.org \
    --cc=akpm@linux-foundation.org \
    --cc=ast@kernel.org \
    --cc=casey@schaufler-ca.com \
    --cc=corbet@lwn.net \
    --cc=daniel@iogearbox.net \
    --cc=davem@davemloft.net \
    --cc=drysdale@google.com \
    --cc=ebiederm@xmission.com \
    --cc=james.l.morris@oracle.com \
    --cc=jann@thejh.net \
    --cc=keescook@chromium.org \
    --cc=kernel-hardening@lists.openwall.com \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=luto@amacapital.net \
    --cc=mjg59@srcf.ucam.org \
    --cc=mtk.manpages@gmail.com \
    --cc=netdev@vger.kernel.org \
    --cc=paul@paul-moore.com \
    --cc=sargun@sargun.me \
    --cc=serge@hallyn.com \
    --cc=shuah@kernel.org \
    --cc=tgraf@suug.ch \
    --cc=tj@kernel.org \
    --cc=wad@chromium.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Kernel-hardening archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/kernel-hardening/0 kernel-hardening/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 kernel-hardening kernel-hardening/ https://lore.kernel.org/kernel-hardening \
		kernel-hardening@lists.openwall.com kernel-hardening@archiver.kernel.org
	public-inbox-index kernel-hardening


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/com.openwall.lists.kernel-hardening


AGPL code for this site: git clone https://public-inbox.org/ public-inbox