[bpf-next,v8,08/11] landlock: Add ptrace restrictions
diff mbox series

Message ID 20180227004121.3633-9-mic@digikod.net
State New
Headers show
Series
  • Landlock LSM: Toward unprivileged sandboxing
Related show

Commit Message

Mickaël Salaün Feb. 27, 2018, 12:41 a.m. UTC
A landlocked process has less privileges than a non-landlocked process
and must then be subject to additional restrictions when manipulating
processes. To be allowed to use ptrace(2) and related syscalls on a
target process, a landlocked process must have a subset of the target
process' rules.

Signed-off-by: Mickaël Salaün <mic@digikod.net>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: David S. Miller <davem@davemloft.net>
Cc: James Morris <james.l.morris@oracle.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Serge E. Hallyn <serge@hallyn.com>
---

Changes since v6:
* factor out ptrace check
* constify pointers
* cleanup headers
* use the new security_add_hooks()
---
 security/landlock/Makefile       |   2 +-
 security/landlock/hooks_ptrace.c | 124 +++++++++++++++++++++++++++++++++++++++
 security/landlock/hooks_ptrace.h |  11 ++++
 security/landlock/init.c         |   2 +
 4 files changed, 138 insertions(+), 1 deletion(-)
 create mode 100644 security/landlock/hooks_ptrace.c
 create mode 100644 security/landlock/hooks_ptrace.h

Comments

Andy Lutomirski Feb. 27, 2018, 4:17 a.m. UTC | #1
On Tue, Feb 27, 2018 at 12:41 AM, Mickaël Salaün <mic@digikod.net> wrote:
> A landlocked process has less privileges than a non-landlocked process
> and must then be subject to additional restrictions when manipulating
> processes. To be allowed to use ptrace(2) and related syscalls on a
> target process, a landlocked process must have a subset of the target
> process' rules.
>
> Signed-off-by: Mickaël Salaün <mic@digikod.net>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Andy Lutomirski <luto@amacapital.net>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: James Morris <james.l.morris@oracle.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Serge E. Hallyn <serge@hallyn.com>
> ---
>
> Changes since v6:
> * factor out ptrace check
> * constify pointers
> * cleanup headers
> * use the new security_add_hooks()
> ---
>  security/landlock/Makefile       |   2 +-
>  security/landlock/hooks_ptrace.c | 124 +++++++++++++++++++++++++++++++++++++++
>  security/landlock/hooks_ptrace.h |  11 ++++
>  security/landlock/init.c         |   2 +
>  4 files changed, 138 insertions(+), 1 deletion(-)
>  create mode 100644 security/landlock/hooks_ptrace.c
>  create mode 100644 security/landlock/hooks_ptrace.h
>
> diff --git a/security/landlock/Makefile b/security/landlock/Makefile
> index d0f532a93b4e..605504d852d3 100644
> --- a/security/landlock/Makefile
> +++ b/security/landlock/Makefile
> @@ -3,4 +3,4 @@ obj-$(CONFIG_SECURITY_LANDLOCK) := landlock.o
>  landlock-y := init.o chain.o task.o \
>         tag.o tag_fs.o \
>         enforce.o enforce_seccomp.o \
> -       hooks.o hooks_cred.o hooks_fs.o
> +       hooks.o hooks_cred.o hooks_fs.o hooks_ptrace.o
> diff --git a/security/landlock/hooks_ptrace.c b/security/landlock/hooks_ptrace.c
> new file mode 100644
> index 000000000000..f1b977b9c808
> --- /dev/null
> +++ b/security/landlock/hooks_ptrace.c
> @@ -0,0 +1,124 @@
> +/*
> + * Landlock LSM - ptrace hooks
> + *
> + * Copyright © 2017 Mickaël Salaün <mic@digikod.net>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2, as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <asm/current.h>
> +#include <linux/errno.h>
> +#include <linux/kernel.h> /* ARRAY_SIZE */
> +#include <linux/lsm_hooks.h>
> +#include <linux/sched.h> /* struct task_struct */
> +#include <linux/seccomp.h>
> +
> +#include "common.h" /* struct landlock_prog_set */
> +#include "hooks.h" /* landlocked() */
> +#include "hooks_ptrace.h"
> +
> +static bool progs_are_subset(const struct landlock_prog_set *parent,
> +               const struct landlock_prog_set *child)
> +{
> +       size_t i;
> +
> +       if (!parent || !child)
> +               return false;
> +       if (parent == child)
> +               return true;
> +
> +       for (i = 0; i < ARRAY_SIZE(child->programs); i++) {

ARRAY_SIZE(child->programs) seems misleading.  Is there no define
NUM_LANDLOCK_PROG_TYPES or similar?

> +               struct landlock_prog_list *walker;
> +               bool found_parent = false;
> +
> +               if (!parent->programs[i])
> +                       continue;
> +               for (walker = child->programs[i]; walker;
> +                               walker = walker->prev) {
> +                       if (walker == parent->programs[i]) {
> +                               found_parent = true;
> +                               break;
> +                       }
> +               }
> +               if (!found_parent)
> +                       return false;
> +       }
> +       return true;
> +}

If you used seccomp, you'd get this type of check for free, and it
would be a lot easier to comprehend.  AFAICT the only extra leniency
you're granting is that you're agnostic to the order in which the
rules associated with different program types were applied, which
could easily be added to seccomp.
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andy Lutomirski Feb. 27, 2018, 5:01 a.m. UTC | #2
> On Feb 26, 2018, at 8:17 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> 
>> On Tue, Feb 27, 2018 at 12:41 AM, Mickaël Salaün <mic@digikod.net> wrote:
>> A landlocked process has less privileges than a non-landlocked process
>> and must then be subject to additional restrictions when manipulating
>> processes. To be allowed to use ptrace(2) and related syscalls on a
>> target process, a landlocked process must have a subset of the target
>> process' rules.
>> 
>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
>> Cc: Alexei Starovoitov <ast@kernel.org>
>> Cc: Andy Lutomirski <luto@amacapital.net>
>> Cc: Daniel Borkmann <daniel@iogearbox.net>
>> Cc: David S. Miller <davem@davemloft.net>
>> Cc: James Morris <james.l.morris@oracle.com>
>> Cc: Kees Cook <keescook@chromium.org>
>> Cc: Serge E. Hallyn <serge@hallyn.com>
>> ---
>> 
>> Changes since v6:
>> * factor out ptrace check
>> * constify pointers
>> * cleanup headers
>> * use the new security_add_hooks()
>> ---
>> security/landlock/Makefile       |   2 +-
>> security/landlock/hooks_ptrace.c | 124 +++++++++++++++++++++++++++++++++++++++
>> security/landlock/hooks_ptrace.h |  11 ++++
>> security/landlock/init.c         |   2 +
>> 4 files changed, 138 insertions(+), 1 deletion(-)
>> create mode 100644 security/landlock/hooks_ptrace.c
>> create mode 100644 security/landlock/hooks_ptrace.h
>> 
>> diff --git a/security/landlock/Makefile b/security/landlock/Makefile
>> index d0f532a93b4e..605504d852d3 100644
>> --- a/security/landlock/Makefile
>> +++ b/security/landlock/Makefile
>> @@ -3,4 +3,4 @@ obj-$(CONFIG_SECURITY_LANDLOCK) := landlock.o
>> landlock-y := init.o chain.o task.o \
>>        tag.o tag_fs.o \
>>        enforce.o enforce_seccomp.o \
>> -       hooks.o hooks_cred.o hooks_fs.o
>> +       hooks.o hooks_cred.o hooks_fs.o hooks_ptrace.o
>> diff --git a/security/landlock/hooks_ptrace.c b/security/landlock/hooks_ptrace.c
>> new file mode 100644
>> index 000000000000..f1b977b9c808
>> --- /dev/null
>> +++ b/security/landlock/hooks_ptrace.c
>> @@ -0,0 +1,124 @@
>> +/*
>> + * Landlock LSM - ptrace hooks
>> + *
>> + * Copyright © 2017 Mickaël Salaün <mic@digikod.net>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2, as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#include <asm/current.h>
>> +#include <linux/errno.h>
>> +#include <linux/kernel.h> /* ARRAY_SIZE */
>> +#include <linux/lsm_hooks.h>
>> +#include <linux/sched.h> /* struct task_struct */
>> +#include <linux/seccomp.h>
>> +
>> +#include "common.h" /* struct landlock_prog_set */
>> +#include "hooks.h" /* landlocked() */
>> +#include "hooks_ptrace.h"
>> +
>> +static bool progs_are_subset(const struct landlock_prog_set *parent,
>> +               const struct landlock_prog_set *child)
>> +{
>> +       size_t i;
>> +
>> +       if (!parent || !child)
>> +               return false;
>> +       if (parent == child)
>> +               return true;
>> +
>> +       for (i = 0; i < ARRAY_SIZE(child->programs); i++) {
> 
> ARRAY_SIZE(child->programs) seems misleading.  Is there no define
> NUM_LANDLOCK_PROG_TYPES or similar?
> 
>> +               struct landlock_prog_list *walker;
>> +               bool found_parent = false;
>> +
>> +               if (!parent->programs[i])
>> +                       continue;
>> +               for (walker = child->programs[i]; walker;
>> +                               walker = walker->prev) {
>> +                       if (walker == parent->programs[i]) {
>> +                               found_parent = true;
>> +                               break;
>> +                       }
>> +               }
>> +               if (!found_parent)
>> +                       return false;
>> +       }
>> +       return true;
>> +}
> 
> If you used seccomp, you'd get this type of check for free, and it
> would be a lot easier to comprehend.  AFAICT the only extra leniency
> you're granting is that you're agnostic to the order in which the
> rules associated with different program types were applied, which
> could easily be added to seccomp.

On second thought, this is all way too complicated.  I think the correct logic is either "if you are filtered by Landlock, you cannot ptrace anything" or to delete this patch entirely. If something like Tycho's notifiers goes in, then it's not obvious that, just because you have the same set of filters, you have the same privilege.  Similarly, if a feature that lets a filter query its cgroup goes in (and you proposed this once!) then the logic you implemented here is wrong.

Or you could just say that it's the responsibility of a Landlock user to properly filter ptrace() just like it's the responsibility of seccomp users to filter ptrace if needed.

I take this as further evidence that Landlock makes much more sense as part of seccomp than as a totally separate thing.  We've very carefully reviewed these things for seccomp.  Please don't make us do it again from scratch.--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mickaël Salaün Feb. 27, 2018, 10:14 p.m. UTC | #3
On 27/02/2018 06:01, Andy Lutomirski wrote:
> 
> 
>> On Feb 26, 2018, at 8:17 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>>
>>> On Tue, Feb 27, 2018 at 12:41 AM, Mickaël Salaün <mic@digikod.net> wrote:
>>> A landlocked process has less privileges than a non-landlocked process
>>> and must then be subject to additional restrictions when manipulating
>>> processes. To be allowed to use ptrace(2) and related syscalls on a
>>> target process, a landlocked process must have a subset of the target
>>> process' rules.
>>>
>>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
>>> Cc: Alexei Starovoitov <ast@kernel.org>
>>> Cc: Andy Lutomirski <luto@amacapital.net>
>>> Cc: Daniel Borkmann <daniel@iogearbox.net>
>>> Cc: David S. Miller <davem@davemloft.net>
>>> Cc: James Morris <james.l.morris@oracle.com>
>>> Cc: Kees Cook <keescook@chromium.org>
>>> Cc: Serge E. Hallyn <serge@hallyn.com>
>>> ---
>>>
>>> Changes since v6:
>>> * factor out ptrace check
>>> * constify pointers
>>> * cleanup headers
>>> * use the new security_add_hooks()
>>> ---
>>> security/landlock/Makefile       |   2 +-
>>> security/landlock/hooks_ptrace.c | 124 +++++++++++++++++++++++++++++++++++++++
>>> security/landlock/hooks_ptrace.h |  11 ++++
>>> security/landlock/init.c         |   2 +
>>> 4 files changed, 138 insertions(+), 1 deletion(-)
>>> create mode 100644 security/landlock/hooks_ptrace.c
>>> create mode 100644 security/landlock/hooks_ptrace.h
>>>
>>> diff --git a/security/landlock/Makefile b/security/landlock/Makefile
>>> index d0f532a93b4e..605504d852d3 100644
>>> --- a/security/landlock/Makefile
>>> +++ b/security/landlock/Makefile
>>> @@ -3,4 +3,4 @@ obj-$(CONFIG_SECURITY_LANDLOCK) := landlock.o
>>> landlock-y := init.o chain.o task.o \
>>>        tag.o tag_fs.o \
>>>        enforce.o enforce_seccomp.o \
>>> -       hooks.o hooks_cred.o hooks_fs.o
>>> +       hooks.o hooks_cred.o hooks_fs.o hooks_ptrace.o
>>> diff --git a/security/landlock/hooks_ptrace.c b/security/landlock/hooks_ptrace.c
>>> new file mode 100644
>>> index 000000000000..f1b977b9c808
>>> --- /dev/null
>>> +++ b/security/landlock/hooks_ptrace.c
>>> @@ -0,0 +1,124 @@
>>> +/*
>>> + * Landlock LSM - ptrace hooks
>>> + *
>>> + * Copyright © 2017 Mickaël Salaün <mic@digikod.net>
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License version 2, as
>>> + * published by the Free Software Foundation.
>>> + */
>>> +
>>> +#include <asm/current.h>
>>> +#include <linux/errno.h>
>>> +#include <linux/kernel.h> /* ARRAY_SIZE */
>>> +#include <linux/lsm_hooks.h>
>>> +#include <linux/sched.h> /* struct task_struct */
>>> +#include <linux/seccomp.h>
>>> +
>>> +#include "common.h" /* struct landlock_prog_set */
>>> +#include "hooks.h" /* landlocked() */
>>> +#include "hooks_ptrace.h"
>>> +
>>> +static bool progs_are_subset(const struct landlock_prog_set *parent,
>>> +               const struct landlock_prog_set *child)
>>> +{
>>> +       size_t i;
>>> +
>>> +       if (!parent || !child)
>>> +               return false;
>>> +       if (parent == child)
>>> +               return true;
>>> +
>>> +       for (i = 0; i < ARRAY_SIZE(child->programs); i++) {
>>
>> ARRAY_SIZE(child->programs) seems misleading.  Is there no define
>> NUM_LANDLOCK_PROG_TYPES or similar?
>>
>>> +               struct landlock_prog_list *walker;
>>> +               bool found_parent = false;
>>> +
>>> +               if (!parent->programs[i])
>>> +                       continue;
>>> +               for (walker = child->programs[i]; walker;
>>> +                               walker = walker->prev) {
>>> +                       if (walker == parent->programs[i]) {
>>> +                               found_parent = true;
>>> +                               break;
>>> +                       }
>>> +               }
>>> +               if (!found_parent)
>>> +                       return false;
>>> +       }
>>> +       return true;
>>> +}
>>
>> If you used seccomp, you'd get this type of check for free, and it
>> would be a lot easier to comprehend.  AFAICT the only extra leniency
>> you're granting is that you're agnostic to the order in which the
>> rules associated with different program types were applied, which
>> could easily be added to seccomp.
> 
> On second thought, this is all way too complicated.  I think the correct logic is either "if you are filtered by Landlock, you cannot ptrace anything" or to delete this patch entirely.

This does not fit a lot of use cases like running a container
constrained with some Landlock programs. We should not deny users the
ability to debug their stuff.

This patch add the minimal protection which are needed to have
meaningful Landlock security policy. Without it, they may be easily
bypassable, hence useless.


> If something like Tycho's notifiers goes in, then it's not obvious that, just because you have the same set of filters, you have the same privilege.  Similarly, if a feature that lets a filter query its cgroup goes in (and you proposed this once!) then the logic you implemented here is wrong.

I don't get your point. Please take a look at the tests (patch 10).

> 
> Or you could just say that it's the responsibility of a Landlock user to properly filter ptrace() just like it's the responsibility of seccomp users to filter ptrace if needed.

A user should be able to enforce a security policy on ptrace as well,
but this patch enforce a minimal set of security boundaries. It will be
easy to add a new Landlock program type to get this kind of access control.

> 
> I take this as further evidence that Landlock makes much more sense as part of seccomp than as a totally separate thing.  We've very carefully reviewed these things for seccomp.  Please don't make us do it again from scratch.
> 

Landlock is more complex than seccomp, because of its different goal.
seccomp is less restrictive because it is more simple, but this patch
follow the same logic with the knowledge of the Landlock internals.
Mickaël Salaün Feb. 27, 2018, 10:18 p.m. UTC | #4
On 27/02/2018 05:17, Andy Lutomirski wrote:
> On Tue, Feb 27, 2018 at 12:41 AM, Mickaël Salaün <mic@digikod.net> wrote:
>> A landlocked process has less privileges than a non-landlocked process
>> and must then be subject to additional restrictions when manipulating
>> processes. To be allowed to use ptrace(2) and related syscalls on a
>> target process, a landlocked process must have a subset of the target
>> process' rules.
>>
>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
>> Cc: Alexei Starovoitov <ast@kernel.org>
>> Cc: Andy Lutomirski <luto@amacapital.net>
>> Cc: Daniel Borkmann <daniel@iogearbox.net>
>> Cc: David S. Miller <davem@davemloft.net>
>> Cc: James Morris <james.l.morris@oracle.com>
>> Cc: Kees Cook <keescook@chromium.org>
>> Cc: Serge E. Hallyn <serge@hallyn.com>
>> ---
>>
>> Changes since v6:
>> * factor out ptrace check
>> * constify pointers
>> * cleanup headers
>> * use the new security_add_hooks()
>> ---
>>  security/landlock/Makefile       |   2 +-
>>  security/landlock/hooks_ptrace.c | 124 +++++++++++++++++++++++++++++++++++++++
>>  security/landlock/hooks_ptrace.h |  11 ++++
>>  security/landlock/init.c         |   2 +
>>  4 files changed, 138 insertions(+), 1 deletion(-)
>>  create mode 100644 security/landlock/hooks_ptrace.c
>>  create mode 100644 security/landlock/hooks_ptrace.h
>>
>> diff --git a/security/landlock/Makefile b/security/landlock/Makefile
>> index d0f532a93b4e..605504d852d3 100644
>> --- a/security/landlock/Makefile
>> +++ b/security/landlock/Makefile
>> @@ -3,4 +3,4 @@ obj-$(CONFIG_SECURITY_LANDLOCK) := landlock.o
>>  landlock-y := init.o chain.o task.o \
>>         tag.o tag_fs.o \
>>         enforce.o enforce_seccomp.o \
>> -       hooks.o hooks_cred.o hooks_fs.o
>> +       hooks.o hooks_cred.o hooks_fs.o hooks_ptrace.o
>> diff --git a/security/landlock/hooks_ptrace.c b/security/landlock/hooks_ptrace.c
>> new file mode 100644
>> index 000000000000..f1b977b9c808
>> --- /dev/null
>> +++ b/security/landlock/hooks_ptrace.c
>> @@ -0,0 +1,124 @@
>> +/*
>> + * Landlock LSM - ptrace hooks
>> + *
>> + * Copyright © 2017 Mickaël Salaün <mic@digikod.net>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2, as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#include <asm/current.h>
>> +#include <linux/errno.h>
>> +#include <linux/kernel.h> /* ARRAY_SIZE */
>> +#include <linux/lsm_hooks.h>
>> +#include <linux/sched.h> /* struct task_struct */
>> +#include <linux/seccomp.h>
>> +
>> +#include "common.h" /* struct landlock_prog_set */
>> +#include "hooks.h" /* landlocked() */
>> +#include "hooks_ptrace.h"
>> +
>> +static bool progs_are_subset(const struct landlock_prog_set *parent,
>> +               const struct landlock_prog_set *child)
>> +{
>> +       size_t i;
>> +
>> +       if (!parent || !child)
>> +               return false;
>> +       if (parent == child)
>> +               return true;
>> +
>> +       for (i = 0; i < ARRAY_SIZE(child->programs); i++) {
> 
> ARRAY_SIZE(child->programs) seems misleading.  Is there no define
> NUM_LANDLOCK_PROG_TYPES or similar?

Yes, there is _LANDLOCK_HOOK_LAST, but this code seems more readable
exactly because it does not require the developer (or the code checking
tools) to know about this static value.
Andy Lutomirski Feb. 27, 2018, 11:02 p.m. UTC | #5
On Tue, Feb 27, 2018 at 10:14 PM, Mickaël Salaün <mic@digikod.net> wrote:
>
> On 27/02/2018 06:01, Andy Lutomirski wrote:
>>
>>
>>> On Feb 26, 2018, at 8:17 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>>>
>>>> On Tue, Feb 27, 2018 at 12:41 AM, Mickaël Salaün <mic@digikod.net> wrote:
>>>> A landlocked process has less privileges than a non-landlocked process
>>>> and must then be subject to additional restrictions when manipulating
>>>> processes. To be allowed to use ptrace(2) and related syscalls on a
>>>> target process, a landlocked process must have a subset of the target
>>>> process' rules.
>>>>
>>>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
>>>> Cc: Alexei Starovoitov <ast@kernel.org>
>>>> Cc: Andy Lutomirski <luto@amacapital.net>
>>>> Cc: Daniel Borkmann <daniel@iogearbox.net>
>>>> Cc: David S. Miller <davem@davemloft.net>
>>>> Cc: James Morris <james.l.morris@oracle.com>
>>>> Cc: Kees Cook <keescook@chromium.org>
>>>> Cc: Serge E. Hallyn <serge@hallyn.com>
>>>> ---
>>>>
>>>> Changes since v6:
>>>> * factor out ptrace check
>>>> * constify pointers
>>>> * cleanup headers
>>>> * use the new security_add_hooks()
>>>> ---
>>>> security/landlock/Makefile       |   2 +-
>>>> security/landlock/hooks_ptrace.c | 124 +++++++++++++++++++++++++++++++++++++++
>>>> security/landlock/hooks_ptrace.h |  11 ++++
>>>> security/landlock/init.c         |   2 +
>>>> 4 files changed, 138 insertions(+), 1 deletion(-)
>>>> create mode 100644 security/landlock/hooks_ptrace.c
>>>> create mode 100644 security/landlock/hooks_ptrace.h
>>>>
>>>> diff --git a/security/landlock/Makefile b/security/landlock/Makefile
>>>> index d0f532a93b4e..605504d852d3 100644
>>>> --- a/security/landlock/Makefile
>>>> +++ b/security/landlock/Makefile
>>>> @@ -3,4 +3,4 @@ obj-$(CONFIG_SECURITY_LANDLOCK) := landlock.o
>>>> landlock-y := init.o chain.o task.o \
>>>>        tag.o tag_fs.o \
>>>>        enforce.o enforce_seccomp.o \
>>>> -       hooks.o hooks_cred.o hooks_fs.o
>>>> +       hooks.o hooks_cred.o hooks_fs.o hooks_ptrace.o
>>>> diff --git a/security/landlock/hooks_ptrace.c b/security/landlock/hooks_ptrace.c
>>>> new file mode 100644
>>>> index 000000000000..f1b977b9c808
>>>> --- /dev/null
>>>> +++ b/security/landlock/hooks_ptrace.c
>>>> @@ -0,0 +1,124 @@
>>>> +/*
>>>> + * Landlock LSM - ptrace hooks
>>>> + *
>>>> + * Copyright © 2017 Mickaël Salaün <mic@digikod.net>
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or modify
>>>> + * it under the terms of the GNU General Public License version 2, as
>>>> + * published by the Free Software Foundation.
>>>> + */
>>>> +
>>>> +#include <asm/current.h>
>>>> +#include <linux/errno.h>
>>>> +#include <linux/kernel.h> /* ARRAY_SIZE */
>>>> +#include <linux/lsm_hooks.h>
>>>> +#include <linux/sched.h> /* struct task_struct */
>>>> +#include <linux/seccomp.h>
>>>> +
>>>> +#include "common.h" /* struct landlock_prog_set */
>>>> +#include "hooks.h" /* landlocked() */
>>>> +#include "hooks_ptrace.h"
>>>> +
>>>> +static bool progs_are_subset(const struct landlock_prog_set *parent,
>>>> +               const struct landlock_prog_set *child)
>>>> +{
>>>> +       size_t i;
>>>> +
>>>> +       if (!parent || !child)
>>>> +               return false;
>>>> +       if (parent == child)
>>>> +               return true;
>>>> +
>>>> +       for (i = 0; i < ARRAY_SIZE(child->programs); i++) {
>>>
>>> ARRAY_SIZE(child->programs) seems misleading.  Is there no define
>>> NUM_LANDLOCK_PROG_TYPES or similar?
>>>
>>>> +               struct landlock_prog_list *walker;
>>>> +               bool found_parent = false;
>>>> +
>>>> +               if (!parent->programs[i])
>>>> +                       continue;
>>>> +               for (walker = child->programs[i]; walker;
>>>> +                               walker = walker->prev) {
>>>> +                       if (walker == parent->programs[i]) {
>>>> +                               found_parent = true;
>>>> +                               break;
>>>> +                       }
>>>> +               }
>>>> +               if (!found_parent)
>>>> +                       return false;
>>>> +       }
>>>> +       return true;
>>>> +}
>>>
>>> If you used seccomp, you'd get this type of check for free, and it
>>> would be a lot easier to comprehend.  AFAICT the only extra leniency
>>> you're granting is that you're agnostic to the order in which the
>>> rules associated with different program types were applied, which
>>> could easily be added to seccomp.
>>
>> On second thought, this is all way too complicated.  I think the correct logic is either "if you are filtered by Landlock, you cannot ptrace anything" or to delete this patch entirely.
>
> This does not fit a lot of use cases like running a container
> constrained with some Landlock programs. We should not deny users the
> ability to debug their stuff.
>
> This patch add the minimal protection which are needed to have
> meaningful Landlock security policy. Without it, they may be easily
> bypassable, hence useless.
>

I think you're wrong here.  Any sane container trying to use Landlock
like this would also create a PID namespace.  Problem solved.  I still
think you should drop this patch.

>
>> If something like Tycho's notifiers goes in, then it's not obvious that, just because you have the same set of filters, you have the same privilege.  Similarly, if a feature that lets a filter query its cgroup goes in (and you proposed this once!) then the logic you implemented here is wrong.
>
> I don't get your point. Please take a look at the tests (patch 10).

I don't know what you want me to look at.

What I'm saying is: suppose I write a filter like this:

bool allow_some_action(void)
{
  int value_from_container_manager = call_out_to_user_notifier();
  return value_from_container_manager == 42;
}

or

bool allow_some_action(void)
{
  return my_cgroup_is("/foo/bar");
}

In both of these cases, your code will do the wrong thing.

>
>>
>> Or you could just say that it's the responsibility of a Landlock user to properly filter ptrace() just like it's the responsibility of seccomp users to filter ptrace if needed.
>
> A user should be able to enforce a security policy on ptrace as well,
> but this patch enforce a minimal set of security boundaries. It will be
> easy to add a new Landlock program type to get this kind of access control.

It sounds like you want Landlock to be a complete container system all
by itself.  I disagree with that design goal.
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andy Lutomirski Feb. 27, 2018, 11:23 p.m. UTC | #6
On Tue, Feb 27, 2018 at 11:02 PM, Andy Lutomirski <luto@kernel.org> wrote:
> On Tue, Feb 27, 2018 at 10:14 PM, Mickaël Salaün <mic@digikod.net> wrote:
>>
>> On 27/02/2018 06:01, Andy Lutomirski wrote:
>>>
>>>
>>>> On Feb 26, 2018, at 8:17 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>>>>
>>>>> On Tue, Feb 27, 2018 at 12:41 AM, Mickaël Salaün <mic@digikod.net> wrote:
>>>>> A landlocked process has less privileges than a non-landlocked process
>>>>> and must then be subject to additional restrictions when manipulating
>>>>> processes. To be allowed to use ptrace(2) and related syscalls on a
>>>>> target process, a landlocked process must have a subset of the target
>>>>> process' rules.
>>>>>
>>>>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
>>>>> Cc: Alexei Starovoitov <ast@kernel.org>
>>>>> Cc: Andy Lutomirski <luto@amacapital.net>
>>>>> Cc: Daniel Borkmann <daniel@iogearbox.net>
>>>>> Cc: David S. Miller <davem@davemloft.net>
>>>>> Cc: James Morris <james.l.morris@oracle.com>
>>>>> Cc: Kees Cook <keescook@chromium.org>
>>>>> Cc: Serge E. Hallyn <serge@hallyn.com>
>>>>> ---
>>>>>
>>>>> Changes since v6:
>>>>> * factor out ptrace check
>>>>> * constify pointers
>>>>> * cleanup headers
>>>>> * use the new security_add_hooks()
>>>>> ---
>>>>> security/landlock/Makefile       |   2 +-
>>>>> security/landlock/hooks_ptrace.c | 124 +++++++++++++++++++++++++++++++++++++++
>>>>> security/landlock/hooks_ptrace.h |  11 ++++
>>>>> security/landlock/init.c         |   2 +
>>>>> 4 files changed, 138 insertions(+), 1 deletion(-)
>>>>> create mode 100644 security/landlock/hooks_ptrace.c
>>>>> create mode 100644 security/landlock/hooks_ptrace.h
>>>>>
>>>>> diff --git a/security/landlock/Makefile b/security/landlock/Makefile
>>>>> index d0f532a93b4e..605504d852d3 100644
>>>>> --- a/security/landlock/Makefile
>>>>> +++ b/security/landlock/Makefile
>>>>> @@ -3,4 +3,4 @@ obj-$(CONFIG_SECURITY_LANDLOCK) := landlock.o
>>>>> landlock-y := init.o chain.o task.o \
>>>>>        tag.o tag_fs.o \
>>>>>        enforce.o enforce_seccomp.o \
>>>>> -       hooks.o hooks_cred.o hooks_fs.o
>>>>> +       hooks.o hooks_cred.o hooks_fs.o hooks_ptrace.o
>>>>> diff --git a/security/landlock/hooks_ptrace.c b/security/landlock/hooks_ptrace.c
>>>>> new file mode 100644
>>>>> index 000000000000..f1b977b9c808
>>>>> --- /dev/null
>>>>> +++ b/security/landlock/hooks_ptrace.c
>>>>> @@ -0,0 +1,124 @@
>>>>> +/*
>>>>> + * Landlock LSM - ptrace hooks
>>>>> + *
>>>>> + * Copyright © 2017 Mickaël Salaün <mic@digikod.net>
>>>>> + *
>>>>> + * This program is free software; you can redistribute it and/or modify
>>>>> + * it under the terms of the GNU General Public License version 2, as
>>>>> + * published by the Free Software Foundation.
>>>>> + */
>>>>> +
>>>>> +#include <asm/current.h>
>>>>> +#include <linux/errno.h>
>>>>> +#include <linux/kernel.h> /* ARRAY_SIZE */
>>>>> +#include <linux/lsm_hooks.h>
>>>>> +#include <linux/sched.h> /* struct task_struct */
>>>>> +#include <linux/seccomp.h>
>>>>> +
>>>>> +#include "common.h" /* struct landlock_prog_set */
>>>>> +#include "hooks.h" /* landlocked() */
>>>>> +#include "hooks_ptrace.h"
>>>>> +
>>>>> +static bool progs_are_subset(const struct landlock_prog_set *parent,
>>>>> +               const struct landlock_prog_set *child)
>>>>> +{
>>>>> +       size_t i;
>>>>> +
>>>>> +       if (!parent || !child)
>>>>> +               return false;
>>>>> +       if (parent == child)
>>>>> +               return true;
>>>>> +
>>>>> +       for (i = 0; i < ARRAY_SIZE(child->programs); i++) {
>>>>
>>>> ARRAY_SIZE(child->programs) seems misleading.  Is there no define
>>>> NUM_LANDLOCK_PROG_TYPES or similar?
>>>>
>>>>> +               struct landlock_prog_list *walker;
>>>>> +               bool found_parent = false;
>>>>> +
>>>>> +               if (!parent->programs[i])
>>>>> +                       continue;
>>>>> +               for (walker = child->programs[i]; walker;
>>>>> +                               walker = walker->prev) {
>>>>> +                       if (walker == parent->programs[i]) {
>>>>> +                               found_parent = true;
>>>>> +                               break;
>>>>> +                       }
>>>>> +               }
>>>>> +               if (!found_parent)
>>>>> +                       return false;
>>>>> +       }
>>>>> +       return true;
>>>>> +}
>>>>
>>>> If you used seccomp, you'd get this type of check for free, and it
>>>> would be a lot easier to comprehend.  AFAICT the only extra leniency
>>>> you're granting is that you're agnostic to the order in which the
>>>> rules associated with different program types were applied, which
>>>> could easily be added to seccomp.
>>>
>>> On second thought, this is all way too complicated.  I think the correct logic is either "if you are filtered by Landlock, you cannot ptrace anything" or to delete this patch entirely.
>>
>> This does not fit a lot of use cases like running a container
>> constrained with some Landlock programs. We should not deny users the
>> ability to debug their stuff.
>>
>> This patch add the minimal protection which are needed to have
>> meaningful Landlock security policy. Without it, they may be easily
>> bypassable, hence useless.
>>
>
> I think you're wrong here.  Any sane container trying to use Landlock
> like this would also create a PID namespace.  Problem solved.  I still
> think you should drop this patch.
>
>>
>>> If something like Tycho's notifiers goes in, then it's not obvious that, just because you have the same set of filters, you have the same privilege.  Similarly, if a feature that lets a filter query its cgroup goes in (and you proposed this once!) then the logic you implemented here is wrong.
>>
>> I don't get your point. Please take a look at the tests (patch 10).
>
> I don't know what you want me to look at.
>
> What I'm saying is: suppose I write a filter like this:
>
> bool allow_some_action(void)
> {
>   int value_from_container_manager = call_out_to_user_notifier();
>   return value_from_container_manager == 42;
> }
>
> or
>
> bool allow_some_action(void)
> {
>   return my_cgroup_is("/foo/bar");
> }
>
> In both of these cases, your code will do the wrong thing.
>
>>
>>>
>>> Or you could just say that it's the responsibility of a Landlock user to properly filter ptrace() just like it's the responsibility of seccomp users to filter ptrace if needed.
>>
>> A user should be able to enforce a security policy on ptrace as well,
>> but this patch enforce a minimal set of security boundaries. It will be
>> easy to add a new Landlock program type to get this kind of access control.
>
> It sounds like you want Landlock to be a complete container system all
> by itself.  I disagree with that design goal.

Having actually read your series more correctly now (oops!), I still
think that this patch should be dropped.  I can see an argument for
having a flag that one can set when adding a seccomp filter that says
"prevent ptrace of any child that doesn't have this exact stack
installed", but I think that could be added later and should not be
part of an initial submission.  For now, Landlock users can block
ptrace() entirely or use PID namespaces.
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mickaël Salaün Feb. 28, 2018, midnight UTC | #7
On 28/02/2018 00:23, Andy Lutomirski wrote:
> On Tue, Feb 27, 2018 at 11:02 PM, Andy Lutomirski <luto@kernel.org> wrote:
>> On Tue, Feb 27, 2018 at 10:14 PM, Mickaël Salaün <mic@digikod.net> wrote:
>>>
>>> On 27/02/2018 06:01, Andy Lutomirski wrote:
>>>>
>>>>
>>>>> On Feb 26, 2018, at 8:17 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>>>>>
>>>>>> On Tue, Feb 27, 2018 at 12:41 AM, Mickaël Salaün <mic@digikod.net> wrote:
>>>>>> A landlocked process has less privileges than a non-landlocked process
>>>>>> and must then be subject to additional restrictions when manipulating
>>>>>> processes. To be allowed to use ptrace(2) and related syscalls on a
>>>>>> target process, a landlocked process must have a subset of the target
>>>>>> process' rules.
>>>>>>
>>>>>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
>>>>>> Cc: Alexei Starovoitov <ast@kernel.org>
>>>>>> Cc: Andy Lutomirski <luto@amacapital.net>
>>>>>> Cc: Daniel Borkmann <daniel@iogearbox.net>
>>>>>> Cc: David S. Miller <davem@davemloft.net>
>>>>>> Cc: James Morris <james.l.morris@oracle.com>
>>>>>> Cc: Kees Cook <keescook@chromium.org>
>>>>>> Cc: Serge E. Hallyn <serge@hallyn.com>
>>>>>> ---
>>>>>>
>>>>>> Changes since v6:
>>>>>> * factor out ptrace check
>>>>>> * constify pointers
>>>>>> * cleanup headers
>>>>>> * use the new security_add_hooks()
>>>>>> ---
>>>>>> security/landlock/Makefile       |   2 +-
>>>>>> security/landlock/hooks_ptrace.c | 124 +++++++++++++++++++++++++++++++++++++++
>>>>>> security/landlock/hooks_ptrace.h |  11 ++++
>>>>>> security/landlock/init.c         |   2 +
>>>>>> 4 files changed, 138 insertions(+), 1 deletion(-)
>>>>>> create mode 100644 security/landlock/hooks_ptrace.c
>>>>>> create mode 100644 security/landlock/hooks_ptrace.h
>>>>>>
>>>>>> diff --git a/security/landlock/Makefile b/security/landlock/Makefile
>>>>>> index d0f532a93b4e..605504d852d3 100644
>>>>>> --- a/security/landlock/Makefile
>>>>>> +++ b/security/landlock/Makefile
>>>>>> @@ -3,4 +3,4 @@ obj-$(CONFIG_SECURITY_LANDLOCK) := landlock.o
>>>>>> landlock-y := init.o chain.o task.o \
>>>>>>        tag.o tag_fs.o \
>>>>>>        enforce.o enforce_seccomp.o \
>>>>>> -       hooks.o hooks_cred.o hooks_fs.o
>>>>>> +       hooks.o hooks_cred.o hooks_fs.o hooks_ptrace.o
>>>>>> diff --git a/security/landlock/hooks_ptrace.c b/security/landlock/hooks_ptrace.c
>>>>>> new file mode 100644
>>>>>> index 000000000000..f1b977b9c808
>>>>>> --- /dev/null
>>>>>> +++ b/security/landlock/hooks_ptrace.c
>>>>>> @@ -0,0 +1,124 @@
>>>>>> +/*
>>>>>> + * Landlock LSM - ptrace hooks
>>>>>> + *
>>>>>> + * Copyright © 2017 Mickaël Salaün <mic@digikod.net>
>>>>>> + *
>>>>>> + * This program is free software; you can redistribute it and/or modify
>>>>>> + * it under the terms of the GNU General Public License version 2, as
>>>>>> + * published by the Free Software Foundation.
>>>>>> + */
>>>>>> +
>>>>>> +#include <asm/current.h>
>>>>>> +#include <linux/errno.h>
>>>>>> +#include <linux/kernel.h> /* ARRAY_SIZE */
>>>>>> +#include <linux/lsm_hooks.h>
>>>>>> +#include <linux/sched.h> /* struct task_struct */
>>>>>> +#include <linux/seccomp.h>
>>>>>> +
>>>>>> +#include "common.h" /* struct landlock_prog_set */
>>>>>> +#include "hooks.h" /* landlocked() */
>>>>>> +#include "hooks_ptrace.h"
>>>>>> +
>>>>>> +static bool progs_are_subset(const struct landlock_prog_set *parent,
>>>>>> +               const struct landlock_prog_set *child)
>>>>>> +{
>>>>>> +       size_t i;
>>>>>> +
>>>>>> +       if (!parent || !child)
>>>>>> +               return false;
>>>>>> +       if (parent == child)
>>>>>> +               return true;
>>>>>> +
>>>>>> +       for (i = 0; i < ARRAY_SIZE(child->programs); i++) {
>>>>>
>>>>> ARRAY_SIZE(child->programs) seems misleading.  Is there no define
>>>>> NUM_LANDLOCK_PROG_TYPES or similar?
>>>>>
>>>>>> +               struct landlock_prog_list *walker;
>>>>>> +               bool found_parent = false;
>>>>>> +
>>>>>> +               if (!parent->programs[i])
>>>>>> +                       continue;
>>>>>> +               for (walker = child->programs[i]; walker;
>>>>>> +                               walker = walker->prev) {
>>>>>> +                       if (walker == parent->programs[i]) {
>>>>>> +                               found_parent = true;
>>>>>> +                               break;
>>>>>> +                       }
>>>>>> +               }
>>>>>> +               if (!found_parent)
>>>>>> +                       return false;
>>>>>> +       }
>>>>>> +       return true;
>>>>>> +}
>>>>>
>>>>> If you used seccomp, you'd get this type of check for free, and it
>>>>> would be a lot easier to comprehend.  AFAICT the only extra leniency
>>>>> you're granting is that you're agnostic to the order in which the
>>>>> rules associated with different program types were applied, which
>>>>> could easily be added to seccomp.
>>>>
>>>> On second thought, this is all way too complicated.  I think the correct logic is either "if you are filtered by Landlock, you cannot ptrace anything" or to delete this patch entirely.
>>>
>>> This does not fit a lot of use cases like running a container
>>> constrained with some Landlock programs. We should not deny users the
>>> ability to debug their stuff.
>>>
>>> This patch add the minimal protection which are needed to have
>>> meaningful Landlock security policy. Without it, they may be easily
>>> bypassable, hence useless.
>>>
>>
>> I think you're wrong here.  Any sane container trying to use Landlock
>> like this would also create a PID namespace.  Problem solved.  I still
>> think you should drop this patch.

Containers is one use case, another is build-in sandboxing (e.g. for web
browser…) and another one is for sandbox managers (e.g. Firejail,
Bubblewrap, Flatpack…). In some of these use cases, especially from a
developer point of view, you may want/need to debug your applications
(without requiring to be root). For nested Landlock access-controls
(e.g. container + user session + web browser), it may not be allowed to
create a PID namespace, but you still want to have a meaningful
access-control.

>>
>>>
>>>> If something like Tycho's notifiers goes in, then it's not obvious that, just because you have the same set of filters, you have the same privilege.  Similarly, if a feature that lets a filter query its cgroup goes in (and you proposed this once!) then the logic you implemented here is wrong.
>>>
>>> I don't get your point. Please take a look at the tests (patch 10).
>>
>> I don't know what you want me to look at.
>>
>> What I'm saying is: suppose I write a filter like this:
>>
>> bool allow_some_action(void)
>> {
>>   int value_from_container_manager = call_out_to_user_notifier();
>>   return value_from_container_manager == 42;
>> }
>>
>> or
>>
>> bool allow_some_action(void)
>> {
>>   return my_cgroup_is("/foo/bar");
>> }
>>
>> In both of these cases, your code will do the wrong thing.

You are right about the fact that the same filters/programs may not be
equivalent if they use external data (other than from the eBPF context)
to take a decision. This is why using a function
my_cgroup_is("/foo/bar") should not be allowed. If we want to enforce a
security policy according to a cgroup, the Landlock programs should be
pinned on this cgroup. This way, the kernel knows if this programs
should be called or not. It is the same argument I used in the thread
[PATCH bpf-next v8 05/11] about the cache.

The only way a Landlock program may change its behavior is because of an
eBPF map. However, in this case the map is common to all the instances
of this program.

To say it another way, the Landlock's enforce API (currently only
seccomp) is in charge of defining what is a subject. By using seccomp to
enforce a policy, the subject is a hierarchy of processes. By pinning a
Landlock program to a cgroup, the subject is the set of processes under
this cgroup. This is much more efficient than letting a program define
its one subjects. This also allows to audit which processes are
restricted by a set of Landlock programs. Because of that, calls to
functions like bpf_get_current_pid_tgid() should not be allowed (or
limited) for a Landlock program. Let's make this programs as pure as
possible. :)


>>
>>>
>>>>
>>>> Or you could just say that it's the responsibility of a Landlock user to properly filter ptrace() just like it's the responsibility of seccomp users to filter ptrace if needed.
>>>
>>> A user should be able to enforce a security policy on ptrace as well,
>>> but this patch enforce a minimal set of security boundaries. It will be
>>> easy to add a new Landlock program type to get this kind of access control.
>>
>> It sounds like you want Landlock to be a complete container system all
>> by itself.  I disagree with that design goal.
> 
> Having actually read your series more correctly now (oops!), I still
> think that this patch should be dropped.  I can see an argument for
> having a flag that one can set when adding a seccomp filter that says
> "prevent ptrace of any child that doesn't have this exact stack
> installed", but I think that could be added later and should not be
> part of an initial submission.  For now, Landlock users can block
> ptrace() entirely or use PID namespaces.
> 

I also though about using a flag, but we should encourage sane/safe
default behavior, which means at least to not have trivially bypassable
access-control rules, to not shoot yourself in the foot. However, a flag
could be added to disable this safe behavior.
Andy Lutomirski Feb. 28, 2018, 12:09 a.m. UTC | #8
On Wed, Feb 28, 2018 at 12:00 AM, Mickaël Salaün <mic@digikod.net> wrote:
>
> On 28/02/2018 00:23, Andy Lutomirski wrote:
>> On Tue, Feb 27, 2018 at 11:02 PM, Andy Lutomirski <luto@kernel.org> wrote:
>>> On Tue, Feb 27, 2018 at 10:14 PM, Mickaël Salaün <mic@digikod.net> wrote:
>>>>
>>>
>>> I think you're wrong here.  Any sane container trying to use Landlock
>>> like this would also create a PID namespace.  Problem solved.  I still
>>> think you should drop this patch.
>
> Containers is one use case, another is build-in sandboxing (e.g. for web
> browser…) and another one is for sandbox managers (e.g. Firejail,
> Bubblewrap, Flatpack…). In some of these use cases, especially from a
> developer point of view, you may want/need to debug your applications
> (without requiring to be root). For nested Landlock access-controls
> (e.g. container + user session + web browser), it may not be allowed to
> create a PID namespace, but you still want to have a meaningful
> access-control.
>

The consideration should be exactly the same as for normal seccomp.
If I'm in a container (using PID namespaces + seccomp) and a run a web
browser, I can debug the browser.

If there's a real use case for adding this type of automatic ptrace
protection, then by all means, let's add it as a general seccomp
feature.
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mickaël Salaün March 6, 2018, 10:28 p.m. UTC | #9
On 28/02/2018 01:09, Andy Lutomirski wrote:
> On Wed, Feb 28, 2018 at 12:00 AM, Mickaël Salaün <mic@digikod.net> wrote:
>>
>> On 28/02/2018 00:23, Andy Lutomirski wrote:
>>> On Tue, Feb 27, 2018 at 11:02 PM, Andy Lutomirski <luto@kernel.org> wrote:
>>>> On Tue, Feb 27, 2018 at 10:14 PM, Mickaël Salaün <mic@digikod.net> wrote:
>>>>>
>>>>
>>>> I think you're wrong here.  Any sane container trying to use Landlock
>>>> like this would also create a PID namespace.  Problem solved.  I still
>>>> think you should drop this patch.
>>
>> Containers is one use case, another is build-in sandboxing (e.g. for web
>> browser…) and another one is for sandbox managers (e.g. Firejail,
>> Bubblewrap, Flatpack…). In some of these use cases, especially from a
>> developer point of view, you may want/need to debug your applications
>> (without requiring to be root). For nested Landlock access-controls
>> (e.g. container + user session + web browser), it may not be allowed to
>> create a PID namespace, but you still want to have a meaningful
>> access-control.
>>
> 
> The consideration should be exactly the same as for normal seccomp.
> If I'm in a container (using PID namespaces + seccomp) and a run a web
> browser, I can debug the browser.
> 
> If there's a real use case for adding this type of automatic ptrace
> protection, then by all means, let's add it as a general seccomp
> feature.
> 

Right, it makes sense to add this feature to seccomp filters as well.
What do you think Kees?

Patch
diff mbox series

diff --git a/security/landlock/Makefile b/security/landlock/Makefile
index d0f532a93b4e..605504d852d3 100644
--- a/security/landlock/Makefile
+++ b/security/landlock/Makefile
@@ -3,4 +3,4 @@  obj-$(CONFIG_SECURITY_LANDLOCK) := landlock.o
 landlock-y := init.o chain.o task.o \
 	tag.o tag_fs.o \
 	enforce.o enforce_seccomp.o \
-	hooks.o hooks_cred.o hooks_fs.o
+	hooks.o hooks_cred.o hooks_fs.o hooks_ptrace.o
diff --git a/security/landlock/hooks_ptrace.c b/security/landlock/hooks_ptrace.c
new file mode 100644
index 000000000000..f1b977b9c808
--- /dev/null
+++ b/security/landlock/hooks_ptrace.c
@@ -0,0 +1,124 @@ 
+/*
+ * Landlock LSM - ptrace hooks
+ *
+ * Copyright © 2017 Mickaël Salaün <mic@digikod.net>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2, as
+ * published by the Free Software Foundation.
+ */
+
+#include <asm/current.h>
+#include <linux/errno.h>
+#include <linux/kernel.h> /* ARRAY_SIZE */
+#include <linux/lsm_hooks.h>
+#include <linux/sched.h> /* struct task_struct */
+#include <linux/seccomp.h>
+
+#include "common.h" /* struct landlock_prog_set */
+#include "hooks.h" /* landlocked() */
+#include "hooks_ptrace.h"
+
+static bool progs_are_subset(const struct landlock_prog_set *parent,
+		const struct landlock_prog_set *child)
+{
+	size_t i;
+
+	if (!parent || !child)
+		return false;
+	if (parent == child)
+		return true;
+
+	for (i = 0; i < ARRAY_SIZE(child->programs); i++) {
+		struct landlock_prog_list *walker;
+		bool found_parent = false;
+
+		if (!parent->programs[i])
+			continue;
+		for (walker = child->programs[i]; walker;
+				walker = walker->prev) {
+			if (walker == parent->programs[i]) {
+				found_parent = true;
+				break;
+			}
+		}
+		if (!found_parent)
+			return false;
+	}
+	return true;
+}
+
+static bool task_has_subset_progs(const struct task_struct *parent,
+		const struct task_struct *child)
+{
+#ifdef CONFIG_SECCOMP_FILTER
+	if (progs_are_subset(parent->seccomp.landlock_prog_set,
+				child->seccomp.landlock_prog_set))
+		/* must be ANDed with other providers (i.e. cgroup) */
+		return true;
+#endif /* CONFIG_SECCOMP_FILTER */
+	return false;
+}
+
+static int task_ptrace(const struct task_struct *parent,
+		const struct task_struct *child)
+{
+	if (!landlocked(parent))
+		return 0;
+
+	if (!landlocked(child))
+		return -EPERM;
+
+	if (task_has_subset_progs(parent, child))
+		return 0;
+
+	return -EPERM;
+}
+
+/**
+ * hook_ptrace_access_check - determine whether the current process may access
+ *			      another
+ *
+ * @child: the process to be accessed
+ * @mode: the mode of attachment
+ *
+ * If the current task has Landlock programs, then the child must have at least
+ * the same programs.  Else denied.
+ *
+ * Determine whether a process may access another, returning 0 if permission
+ * granted, -errno if denied.
+ */
+static int hook_ptrace_access_check(struct task_struct *child,
+		unsigned int mode)
+{
+	return task_ptrace(current, child);
+}
+
+/**
+ * hook_ptrace_traceme - determine whether another process may trace the
+ *			 current one
+ *
+ * @parent: the task proposed to be the tracer
+ *
+ * If the parent has Landlock programs, then the current task must have the
+ * same or more programs.
+ * Else denied.
+ *
+ * Determine whether the nominated task is permitted to trace the current
+ * process, returning 0 if permission is granted, -errno if denied.
+ */
+static int hook_ptrace_traceme(struct task_struct *parent)
+{
+	return task_ptrace(parent, current);
+}
+
+static struct security_hook_list landlock_hooks[] = {
+	LSM_HOOK_INIT(ptrace_access_check, hook_ptrace_access_check),
+	LSM_HOOK_INIT(ptrace_traceme, hook_ptrace_traceme),
+};
+
+__init void landlock_add_hooks_ptrace(void)
+{
+	security_add_hooks(landlock_hooks, ARRAY_SIZE(landlock_hooks),
+			LANDLOCK_NAME);
+}
diff --git a/security/landlock/hooks_ptrace.h b/security/landlock/hooks_ptrace.h
new file mode 100644
index 000000000000..15b1f3479e0e
--- /dev/null
+++ b/security/landlock/hooks_ptrace.h
@@ -0,0 +1,11 @@ 
+/*
+ * Landlock LSM - ptrace hooks
+ *
+ * Copyright © 2017 Mickaël Salaün <mic@digikod.net>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2, as
+ * published by the Free Software Foundation.
+ */
+
+__init void landlock_add_hooks_ptrace(void);
diff --git a/security/landlock/init.c b/security/landlock/init.c
index 3486272d17b2..0f16848f5ad1 100644
--- a/security/landlock/init.c
+++ b/security/landlock/init.c
@@ -17,6 +17,7 @@ 
 #include "common.h" /* LANDLOCK_* */
 #include "hooks_fs.h"
 #include "hooks_cred.h"
+#include "hooks_ptrace.h"
 
 static bool bpf_landlock_is_valid_access(int off, int size,
 		enum bpf_access_type type, struct bpf_insn_access_aux *info,
@@ -232,5 +233,6 @@  void __init landlock_add_hooks(void)
 {
 	pr_info(LANDLOCK_NAME ": Ready to sandbox with seccomp\n");
 	landlock_add_hooks_cred();
+	landlock_add_hooks_ptrace();
 	landlock_add_hooks_fs();
 }