Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1625669 > unrolled thread

Re: [PATCH net-next v6 04/11] landlock: Add LSM hooks related to filesystem

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH net-next v6 04/11] landlock: Add LSM hooks related to filesystem Kees Cook <keescook@chromium.org> - 2017-04-19 00:20 +0200
    Re: [PATCH net-next v6 04/11] landlock: Add LSM hooks related to  filesystem Mickaël Salaün <mic@digikod.net> - 2017-04-19 00:50 +0200
      Re: [PATCH net-next v6 04/11] landlock: Add LSM hooks related to  filesystem Casey Schaufler <casey@schaufler-ca.com> - 2017-04-19 01:20 +0200
        Re: [PATCH net-next v6 04/11] landlock: Add LSM hooks related to filesystem Kees Cook <keescook@chromium.org> - 2017-04-19 01:50 +0200
          Re: [PATCH net-next v6 04/11] landlock: Add LSM hooks related to  filesystem Mickaël Salaün <mic@digikod.net> - 2017-04-20 00:10 +0200
            Re: [kernel-hardening] Re: [PATCH net-next v6 04/11] landlock: Add  LSM hooks related to filesystem Casey Schaufler <casey@schaufler-ca.com> - 2017-04-20 02:00 +0200
            Re: [PATCH net-next v6 04/11] landlock: Add LSM hooks related to filesystem Kees Cook <keescook@chromium.org> - 2017-04-20 03:50 +0200
      Re: [PATCH net-next v6 04/11] landlock: Add LSM hooks related to filesystem Kees Cook <keescook@chromium.org> - 2017-04-19 01:40 +0200

#1625669 — Re: [PATCH net-next v6 04/11] landlock: Add LSM hooks related to filesystem

On Tue, Mar 28, 2017 at 4:46 PM, Mickaël Salaün <mic@digikod.net> wrote:
> Handle 33 filesystem-related LSM hooks for the Landlock filesystem
> event: LANDLOCK_SUBTYPE_EVENT_FS.
>
> A Landlock event wrap LSM hooks for similar kernel object types (e.g.
> struct file, struct path...). Multiple LSM hooks can trigger the same
> Landlock event.
>
> Landlock handle nine coarse-grained actions: read, write, execute, new,
> get, remove, ioctl, lock and fcntl. Each of them abstract LSM hook
> access control in a way that can be extended in the future.
>
> The Landlock LSM hook registration is done after other LSM to only run
> actions from user-space, via eBPF programs, if the access was granted by
> major (privileged) LSMs.
>
> Changes since v5:
> * split hooks.[ch] into hooks.[ch] and hooks_fs.[ch]
> * add more documentation
> * cosmetic fixes
>
> Changes since v4:
> * add LSM hook abstraction called Landlock event
>   * use the compiler type checking to verify hooks use by an event
>   * handle all filesystem related LSM hooks (e.g. file_permission,
>     mmap_file, sb_mount...)
> * register BPF programs for Landlock just after LSM hooks registration
> * move hooks registration after other LSMs
> * add failsafes to check if a hook is not used by the kernel
> * allow partial raw value access form the context (needed for programs
>   generated by LLVM)
>
> Changes since v3:
> * split commit
> * add hooks dealing with struct inode and struct path pointers:
>   inode_permission and inode_getattr
> * add abstraction over eBPF helper arguments thanks to wrapping structs
>
> 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>
> ---
>  include/linux/lsm_hooks.h    |   5 +
>  security/landlock/Makefile   |   4 +-
>  security/landlock/hooks.c    | 115 +++++++++
>  security/landlock/hooks.h    | 177 ++++++++++++++
>  security/landlock/hooks_fs.c | 563 +++++++++++++++++++++++++++++++++++++++++++
>  security/landlock/hooks_fs.h |  19 ++
>  security/landlock/init.c     |  13 +
>  security/security.c          |   7 +-
>  8 files changed, 901 insertions(+), 2 deletions(-)
>  create mode 100644 security/landlock/hooks.c
>  create mode 100644 security/landlock/hooks.h
>  create mode 100644 security/landlock/hooks_fs.c
>  create mode 100644 security/landlock/hooks_fs.h
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index e29d4c62a3c8..884289166a0e 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -1920,5 +1920,10 @@ void __init loadpin_add_hooks(void);
>  #else
>  static inline void loadpin_add_hooks(void) { };
>  #endif
> +#ifdef CONFIG_SECURITY_LANDLOCK
> +extern void __init landlock_add_hooks(void);
> +#else
> +static inline void __init landlock_add_hooks(void) { }
> +#endif /* CONFIG_SECURITY_LANDLOCK */
>
>  #endif /* ! __LINUX_LSM_HOOKS_H */
> diff --git a/security/landlock/Makefile b/security/landlock/Makefile
> index 7205f9a7a2ee..c0db504a6335 100644
> --- a/security/landlock/Makefile
> +++ b/security/landlock/Makefile
> @@ -1,3 +1,5 @@
> +ccflags-$(CONFIG_SECURITY_LANDLOCK) += -Werror=unused-function

Why is this needed? If it can't be avoided, a comment should exist
here explaining why.

> [...]
> @@ -127,3 +132,11 @@ static struct bpf_prog_type_list bpf_landlock_type __ro_after_init = {
>         .ops = &bpf_landlock_ops,
>         .type = BPF_PROG_TYPE_LANDLOCK,
>  };
> +
> +void __init landlock_add_hooks(void)
> +{
> +       pr_info("landlock: Version %u", LANDLOCK_VERSION);
> +       landlock_add_hooks_fs();
> +       security_add_hooks(NULL, 0, "landlock");
> +       bpf_register_prog_type(&bpf_landlock_type);

I'm confused by the separation of hook registration here. The call to
security_add_hooks is with count=0 is especially weird. Why isn't this
just a single call with security_add_hooks(landlock_hooks,
ARRAY_SIZE(landlock_hooks), "landlock")?

> +}
> diff --git a/security/security.c b/security/security.c
> index d0e07f269b2d..a3e9f4625991 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -64,10 +64,15 @@ int __init security_init(void)
>         loadpin_add_hooks();
>
>         /*
> -        * Load all the remaining security modules.
> +        * Load all remaining privileged security modules.
>          */
>         do_security_initcalls();
>
> +       /*
> +        * Load potentially-unprivileged security modules at the end.
> +        */
> +       landlock_add_hooks();

Oh, is this to make it last in the list? Is there a reason it has to be last?

-Kees

-- 
Kees Cook
Pixel Security

[toc] | [next] | [standalone]

#1625680 — Re: [PATCH net-next v6 04/11] landlock: Add LSM hooks related to filesystem

[Multipart message — attachments visible in raw view] — view raw

On 19/04/2017 00:17, Kees Cook wrote:
> On Tue, Mar 28, 2017 at 4:46 PM, Mickaël Salaün <mic@digikod.net> wrote:
>> Handle 33 filesystem-related LSM hooks for the Landlock filesystem
>> event: LANDLOCK_SUBTYPE_EVENT_FS.
>>
>> A Landlock event wrap LSM hooks for similar kernel object types (e.g.
>> struct file, struct path...). Multiple LSM hooks can trigger the same
>> Landlock event.
>>
>> Landlock handle nine coarse-grained actions: read, write, execute, new,
>> get, remove, ioctl, lock and fcntl. Each of them abstract LSM hook
>> access control in a way that can be extended in the future.
>>
>> The Landlock LSM hook registration is done after other LSM to only run
>> actions from user-space, via eBPF programs, if the access was granted by
>> major (privileged) LSMs.
>>
>> Changes since v5:
>> * split hooks.[ch] into hooks.[ch] and hooks_fs.[ch]
>> * add more documentation
>> * cosmetic fixes
>>
>> Changes since v4:
>> * add LSM hook abstraction called Landlock event
>>   * use the compiler type checking to verify hooks use by an event
>>   * handle all filesystem related LSM hooks (e.g. file_permission,
>>     mmap_file, sb_mount...)
>> * register BPF programs for Landlock just after LSM hooks registration
>> * move hooks registration after other LSMs
>> * add failsafes to check if a hook is not used by the kernel
>> * allow partial raw value access form the context (needed for programs
>>   generated by LLVM)
>>
>> Changes since v3:
>> * split commit
>> * add hooks dealing with struct inode and struct path pointers:
>>   inode_permission and inode_getattr
>> * add abstraction over eBPF helper arguments thanks to wrapping structs
>>
>> 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>
>> ---
>>  include/linux/lsm_hooks.h    |   5 +
>>  security/landlock/Makefile   |   4 +-
>>  security/landlock/hooks.c    | 115 +++++++++
>>  security/landlock/hooks.h    | 177 ++++++++++++++
>>  security/landlock/hooks_fs.c | 563 +++++++++++++++++++++++++++++++++++++++++++
>>  security/landlock/hooks_fs.h |  19 ++
>>  security/landlock/init.c     |  13 +
>>  security/security.c          |   7 +-
>>  8 files changed, 901 insertions(+), 2 deletions(-)
>>  create mode 100644 security/landlock/hooks.c
>>  create mode 100644 security/landlock/hooks.h
>>  create mode 100644 security/landlock/hooks_fs.c
>>  create mode 100644 security/landlock/hooks_fs.h
>>
>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
>> index e29d4c62a3c8..884289166a0e 100644
>> --- a/include/linux/lsm_hooks.h
>> +++ b/include/linux/lsm_hooks.h
>> @@ -1920,5 +1920,10 @@ void __init loadpin_add_hooks(void);
>>  #else
>>  static inline void loadpin_add_hooks(void) { };
>>  #endif
>> +#ifdef CONFIG_SECURITY_LANDLOCK
>> +extern void __init landlock_add_hooks(void);
>> +#else
>> +static inline void __init landlock_add_hooks(void) { }
>> +#endif /* CONFIG_SECURITY_LANDLOCK */
>>
>>  #endif /* ! __LINUX_LSM_HOOKS_H */
>> diff --git a/security/landlock/Makefile b/security/landlock/Makefile
>> index 7205f9a7a2ee..c0db504a6335 100644
>> --- a/security/landlock/Makefile
>> +++ b/security/landlock/Makefile
>> @@ -1,3 +1,5 @@
>> +ccflags-$(CONFIG_SECURITY_LANDLOCK) += -Werror=unused-function
> 
> Why is this needed? If it can't be avoided, a comment should exist
> here explaining why.

This is useful to catch defined but unused hooks: error out if a
HOOK_NEW_FS(foo) is not used with a HOOK_INIT_FS(foo) in the struct
security_hook_list landlock_hooks.

> 
>> [...]
>> @@ -127,3 +132,11 @@ static struct bpf_prog_type_list bpf_landlock_type __ro_after_init = {
>>         .ops = &bpf_landlock_ops,
>>         .type = BPF_PROG_TYPE_LANDLOCK,
>>  };
>> +
>> +void __init landlock_add_hooks(void)
>> +{
>> +       pr_info("landlock: Version %u", LANDLOCK_VERSION);
>> +       landlock_add_hooks_fs();
>> +       security_add_hooks(NULL, 0, "landlock");
>> +       bpf_register_prog_type(&bpf_landlock_type);
> 
> I'm confused by the separation of hook registration here. The call to
> security_add_hooks is with count=0 is especially weird. Why isn't this
> just a single call with security_add_hooks(landlock_hooks,
> ARRAY_SIZE(landlock_hooks), "landlock")?

Yes, this is ugly with the new security_add_hooks() with three arguments
but I wanted to split the hooks definition in multiple files.

The current security_add_hooks() use lsm_append(lsm, &lsm_names) which
is not exported. Unfortunately, calling multiple security_add_hooks()
with the same LSM name would register multiple names for the same LSM…
Is it OK if I modify this function to not add duplicated entries?


> 
>> +}
>> diff --git a/security/security.c b/security/security.c
>> index d0e07f269b2d..a3e9f4625991 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -64,10 +64,15 @@ int __init security_init(void)
>>         loadpin_add_hooks();
>>
>>         /*
>> -        * Load all the remaining security modules.
>> +        * Load all remaining privileged security modules.
>>          */
>>         do_security_initcalls();
>>
>> +       /*
>> +        * Load potentially-unprivileged security modules at the end.
>> +        */
>> +       landlock_add_hooks();
> 
> Oh, is this to make it last in the list? Is there a reason it has to be last?

Right, this is the intend. I'm not sure it is the only way to register
hooks, though.

For an unprivileged access-control, we don't want to give the ability to
any process to do some checks, through an eBPF program, on kernel
objects (e.g. files) if they should not be accessible (because of a
following LSM hook check).

 Mickaël

[toc] | [prev] | [next] | [standalone]

#1625712 — Re: [PATCH net-next v6 04/11] landlock: Add LSM hooks related to filesystem

On 4/18/2017 3:44 PM, Mickaël Salaün wrote:
> On 19/04/2017 00:17, Kees Cook wrote:
>> On Tue, Mar 28, 2017 at 4:46 PM, Mickaël Salaün <mic@digikod.net> wrote:
>>> Handle 33 filesystem-related LSM hooks for the Landlock filesystem
>>> event: LANDLOCK_SUBTYPE_EVENT_FS.
>>>
>>> A Landlock event wrap LSM hooks for similar kernel object types (e.g.
>>> struct file, struct path...). Multiple LSM hooks can trigger the same
>>> Landlock event.
>>>
>>> Landlock handle nine coarse-grained actions: read, write, execute, new,
>>> get, remove, ioctl, lock and fcntl. Each of them abstract LSM hook
>>> access control in a way that can be extended in the future.
>>>
>>> The Landlock LSM hook registration is done after other LSM to only run
>>> actions from user-space, via eBPF programs, if the access was granted by
>>> major (privileged) LSMs.
>>>
>>> Changes since v5:
>>> * split hooks.[ch] into hooks.[ch] and hooks_fs.[ch]
>>> * add more documentation
>>> * cosmetic fixes
>>>
>>> Changes since v4:
>>> * add LSM hook abstraction called Landlock event
>>>   * use the compiler type checking to verify hooks use by an event
>>>   * handle all filesystem related LSM hooks (e.g. file_permission,
>>>     mmap_file, sb_mount...)
>>> * register BPF programs for Landlock just after LSM hooks registration
>>> * move hooks registration after other LSMs
>>> * add failsafes to check if a hook is not used by the kernel
>>> * allow partial raw value access form the context (needed for programs
>>>   generated by LLVM)
>>>
>>> Changes since v3:
>>> * split commit
>>> * add hooks dealing with struct inode and struct path pointers:
>>>   inode_permission and inode_getattr
>>> * add abstraction over eBPF helper arguments thanks to wrapping structs
>>>
>>> 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>
>>> ---
>>>  include/linux/lsm_hooks.h    |   5 +
>>>  security/landlock/Makefile   |   4 +-
>>>  security/landlock/hooks.c    | 115 +++++++++
>>>  security/landlock/hooks.h    | 177 ++++++++++++++
>>>  security/landlock/hooks_fs.c | 563 +++++++++++++++++++++++++++++++++++++++++++
>>>  security/landlock/hooks_fs.h |  19 ++
>>>  security/landlock/init.c     |  13 +
>>>  security/security.c          |   7 +-
>>>  8 files changed, 901 insertions(+), 2 deletions(-)
>>>  create mode 100644 security/landlock/hooks.c
>>>  create mode 100644 security/landlock/hooks.h
>>>  create mode 100644 security/landlock/hooks_fs.c
>>>  create mode 100644 security/landlock/hooks_fs.h
>>>
>>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
>>> index e29d4c62a3c8..884289166a0e 100644
>>> --- a/include/linux/lsm_hooks.h
>>> +++ b/include/linux/lsm_hooks.h
>>> @@ -1920,5 +1920,10 @@ void __init loadpin_add_hooks(void);
>>>  #else
>>>  static inline void loadpin_add_hooks(void) { };
>>>  #endif
>>> +#ifdef CONFIG_SECURITY_LANDLOCK
>>> +extern void __init landlock_add_hooks(void);
>>> +#else
>>> +static inline void __init landlock_add_hooks(void) { }
>>> +#endif /* CONFIG_SECURITY_LANDLOCK */
>>>
>>>  #endif /* ! __LINUX_LSM_HOOKS_H */
>>> diff --git a/security/landlock/Makefile b/security/landlock/Makefile
>>> index 7205f9a7a2ee..c0db504a6335 100644
>>> --- a/security/landlock/Makefile
>>> +++ b/security/landlock/Makefile
>>> @@ -1,3 +1,5 @@
>>> +ccflags-$(CONFIG_SECURITY_LANDLOCK) += -Werror=unused-function
>> Why is this needed? If it can't be avoided, a comment should exist
>> here explaining why.
> This is useful to catch defined but unused hooks: error out if a
> HOOK_NEW_FS(foo) is not used with a HOOK_INIT_FS(foo) in the struct
> security_hook_list landlock_hooks.
>
>>> [...]
>>> @@ -127,3 +132,11 @@ static struct bpf_prog_type_list bpf_landlock_type __ro_after_init = {
>>>         .ops = &bpf_landlock_ops,
>>>         .type = BPF_PROG_TYPE_LANDLOCK,
>>>  };
>>> +
>>> +void __init landlock_add_hooks(void)
>>> +{
>>> +       pr_info("landlock: Version %u", LANDLOCK_VERSION);
>>> +       landlock_add_hooks_fs();
>>> +       security_add_hooks(NULL, 0, "landlock");
>>> +       bpf_register_prog_type(&bpf_landlock_type);
>> I'm confused by the separation of hook registration here. The call to
>> security_add_hooks is with count=0 is especially weird. Why isn't this
>> just a single call with security_add_hooks(landlock_hooks,
>> ARRAY_SIZE(landlock_hooks), "landlock")?
> Yes, this is ugly with the new security_add_hooks() with three arguments
> but I wanted to split the hooks definition in multiple files.

Why? I'll buy a good argument, but there are dangers in
allowing multiple calls to security_add_hooks(). 

>
> The current security_add_hooks() use lsm_append(lsm, &lsm_names) which
> is not exported. Unfortunately, calling multiple security_add_hooks()
> with the same LSM name would register multiple names for the same LSM…
> Is it OK if I modify this function to not add duplicated entries?

It may seem absurd, but it's conceivable that a module might
have two hooks it wants called. My example is a module that
counts the number of times SELinux denies a process access to
things (which needs to be called before and after SELinux in
order to detect denials) and takes "appropriate action" if
too many denials occur. It would be weird, wonky and hackish,
but that never stopped anybody before.

>
>
>>> +}
>>> diff --git a/security/security.c b/security/security.c
>>> index d0e07f269b2d..a3e9f4625991 100644
>>> --- a/security/security.c
>>> +++ b/security/security.c
>>> @@ -64,10 +64,15 @@ int __init security_init(void)
>>>         loadpin_add_hooks();
>>>
>>>         /*
>>> -        * Load all the remaining security modules.
>>> +        * Load all remaining privileged security modules.
>>>          */
>>>         do_security_initcalls();
>>>
>>> +       /*
>>> +        * Load potentially-unprivileged security modules at the end.
>>> +        */
>>> +       landlock_add_hooks();
>> Oh, is this to make it last in the list? Is there a reason it has to be last?
> Right, this is the intend. I'm not sure it is the only way to register
> hooks, though.
>
> For an unprivileged access-control, we don't want to give the ability to
> any process to do some checks, through an eBPF program, on kernel
> objects (e.g. files) if they should not be accessible (because of a
> following LSM hook check).
>
>  Mickaël
>

[toc] | [prev] | [next] | [standalone]

#1625732

On Tue, Apr 18, 2017 at 4:16 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 4/18/2017 3:44 PM, Mickaël Salaün wrote:
>> On 19/04/2017 00:17, Kees Cook wrote:
>>> On Tue, Mar 28, 2017 at 4:46 PM, Mickaël Salaün <mic@digikod.net> wrote:
>>>> +void __init landlock_add_hooks(void)
>>>> +{
>>>> +       pr_info("landlock: Version %u", LANDLOCK_VERSION);
>>>> +       landlock_add_hooks_fs();
>>>> +       security_add_hooks(NULL, 0, "landlock");
>>>> +       bpf_register_prog_type(&bpf_landlock_type);
>>> I'm confused by the separation of hook registration here. The call to
>>> security_add_hooks is with count=0 is especially weird. Why isn't this
>>> just a single call with security_add_hooks(landlock_hooks,
>>> ARRAY_SIZE(landlock_hooks), "landlock")?
>> Yes, this is ugly with the new security_add_hooks() with three arguments
>> but I wanted to split the hooks definition in multiple files.
>
> Why? I'll buy a good argument, but there are dangers in
> allowing multiple calls to security_add_hooks().
>
>>
>> The current security_add_hooks() use lsm_append(lsm, &lsm_names) which
>> is not exported. Unfortunately, calling multiple security_add_hooks()
>> with the same LSM name would register multiple names for the same LSM…
>> Is it OK if I modify this function to not add duplicated entries?
>
> It may seem absurd, but it's conceivable that a module might
> have two hooks it wants called. My example is a module that
> counts the number of times SELinux denies a process access to
> things (which needs to be called before and after SELinux in
> order to detect denials) and takes "appropriate action" if
> too many denials occur. It would be weird, wonky and hackish,
> but that never stopped anybody before.

If ends up being sane and clear, I'm fine with allowing multiple calls.

-Kees

-- 
Kees Cook
Pixel Security

[toc] | [prev] | [next] | [standalone]

#1626831 — Re: [PATCH net-next v6 04/11] landlock: Add LSM hooks related to filesystem

[Multipart message — attachments visible in raw view] — view raw

On 19/04/2017 01:40, Kees Cook wrote:
> On Tue, Apr 18, 2017 at 4:16 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 4/18/2017 3:44 PM, Mickaël Salaün wrote:
>>> On 19/04/2017 00:17, Kees Cook wrote:
>>>> On Tue, Mar 28, 2017 at 4:46 PM, Mickaël Salaün <mic@digikod.net> wrote:
>>>>> +void __init landlock_add_hooks(void)
>>>>> +{
>>>>> +       pr_info("landlock: Version %u", LANDLOCK_VERSION);
>>>>> +       landlock_add_hooks_fs();
>>>>> +       security_add_hooks(NULL, 0, "landlock");
>>>>> +       bpf_register_prog_type(&bpf_landlock_type);
>>>> I'm confused by the separation of hook registration here. The call to
>>>> security_add_hooks is with count=0 is especially weird. Why isn't this
>>>> just a single call with security_add_hooks(landlock_hooks,
>>>> ARRAY_SIZE(landlock_hooks), "landlock")?
>>> Yes, this is ugly with the new security_add_hooks() with three arguments
>>> but I wanted to split the hooks definition in multiple files.
>>
>> Why? I'll buy a good argument, but there are dangers in
>> allowing multiple calls to security_add_hooks().

I prefer to have one file per hook "family" (e.g. filesystem, network,
ptrace…). This reduce the mess with all the included files (needed for
LSM hook argument types) and make the files easier to read, understand
and maintain.

>>
>>>
>>> The current security_add_hooks() use lsm_append(lsm, &lsm_names) which
>>> is not exported. Unfortunately, calling multiple security_add_hooks()
>>> with the same LSM name would register multiple names for the same LSM…
>>> Is it OK if I modify this function to not add duplicated entries?
>>
>> It may seem absurd, but it's conceivable that a module might
>> have two hooks it wants called. My example is a module that
>> counts the number of times SELinux denies a process access to
>> things (which needs to be called before and after SELinux in
>> order to detect denials) and takes "appropriate action" if
>> too many denials occur. It would be weird, wonky and hackish,
>> but that never stopped anybody before.

Right, but now, with the new lsm_append(), module names are concatenated
("%s,%s") in the lsm_names variable. It would be nice to not pollute
this string with multiple time the same module name.

> 
> If ends up being sane and clear, I'm fine with allowing multiple calls.
> 
> -Kees
> 

[toc] | [prev] | [next] | [standalone]

#1626891 — Re: [kernel-hardening] Re: [PATCH net-next v6 04/11] landlock: Add LSM hooks related to filesystem

On 4/19/2017 3:03 PM, Mickaël Salaün wrote:
> On 19/04/2017 01:40, Kees Cook wrote:
>> On Tue, Apr 18, 2017 at 4:16 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
>>> On 4/18/2017 3:44 PM, Mickaël Salaün wrote:
>>>> On 19/04/2017 00:17, Kees Cook wrote:
>>>>> On Tue, Mar 28, 2017 at 4:46 PM, Mickaël Salaün <mic@digikod.net> wrote:
>>>>>> +void __init landlock_add_hooks(void)
>>>>>> +{
>>>>>> +       pr_info("landlock: Version %u", LANDLOCK_VERSION);
>>>>>> +       landlock_add_hooks_fs();
>>>>>> +       security_add_hooks(NULL, 0, "landlock");
>>>>>> +       bpf_register_prog_type(&bpf_landlock_type);
>>>>> I'm confused by the separation of hook registration here. The call to
>>>>> security_add_hooks is with count=0 is especially weird. Why isn't this
>>>>> just a single call with security_add_hooks(landlock_hooks,
>>>>> ARRAY_SIZE(landlock_hooks), "landlock")?
>>>> Yes, this is ugly with the new security_add_hooks() with three arguments
>>>> but I wanted to split the hooks definition in multiple files.
>>> Why? I'll buy a good argument, but there are dangers in
>>> allowing multiple calls to security_add_hooks().
> I prefer to have one file per hook "family" (e.g. filesystem, network,
> ptrace…). This reduce the mess with all the included files (needed for
> LSM hook argument types) and make the files easier to read, understand
> and maintain.

Yeah, there's that tradeoff and it really is a matter
of taste I suppose.

>>>> The current security_add_hooks() use lsm_append(lsm, &lsm_names) which
>>>> is not exported. Unfortunately, calling multiple security_add_hooks()
>>>> with the same LSM name would register multiple names for the same LSM…
>>>> Is it OK if I modify this function to not add duplicated entries?
>>> It may seem absurd, but it's conceivable that a module might
>>> have two hooks it wants called. My example is a module that
>>> counts the number of times SELinux denies a process access to
>>> things (which needs to be called before and after SELinux in
>>> order to detect denials) and takes "appropriate action" if
>>> too many denials occur. It would be weird, wonky and hackish,
>>> but that never stopped anybody before.
> Right, but now, with the new lsm_append(), module names are concatenated
> ("%s,%s") in the lsm_names variable. It would be nice to not pollute
> this string with multiple time the same module name.

All it would take is a check that the module name
isn't already on the list. It's a trivial change.

>> If ends up being sane and clear, I'm fine with allowing multiple calls.
>>
>> -Kees
>>

[toc] | [prev] | [next] | [standalone]

#1626911

On Wed, Apr 19, 2017 at 3:03 PM, Mickaël Salaün <mic@digikod.net> wrote:
>
> On 19/04/2017 01:40, Kees Cook wrote:
>> On Tue, Apr 18, 2017 at 4:16 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
>>> On 4/18/2017 3:44 PM, Mickaël Salaün wrote:
>>>> On 19/04/2017 00:17, Kees Cook wrote:
>>>>> On Tue, Mar 28, 2017 at 4:46 PM, Mickaël Salaün <mic@digikod.net> wrote:
>>>>>> +void __init landlock_add_hooks(void)
>>>>>> +{
>>>>>> +       pr_info("landlock: Version %u", LANDLOCK_VERSION);
>>>>>> +       landlock_add_hooks_fs();
>>>>>> +       security_add_hooks(NULL, 0, "landlock");
>>>>>> +       bpf_register_prog_type(&bpf_landlock_type);
>>>>> I'm confused by the separation of hook registration here. The call to
>>>>> security_add_hooks is with count=0 is especially weird. Why isn't this
>>>>> just a single call with security_add_hooks(landlock_hooks,
>>>>> ARRAY_SIZE(landlock_hooks), "landlock")?
>>>> Yes, this is ugly with the new security_add_hooks() with three arguments
>>>> but I wanted to split the hooks definition in multiple files.
>>>
>>> Why? I'll buy a good argument, but there are dangers in
>>> allowing multiple calls to security_add_hooks().
>
> I prefer to have one file per hook "family" (e.g. filesystem, network,
> ptrace…). This reduce the mess with all the included files (needed for
> LSM hook argument types) and make the files easier to read, understand
> and maintain.
>
>>>
>>>>
>>>> The current security_add_hooks() use lsm_append(lsm, &lsm_names) which
>>>> is not exported. Unfortunately, calling multiple security_add_hooks()
>>>> with the same LSM name would register multiple names for the same LSM…
>>>> Is it OK if I modify this function to not add duplicated entries?
>>>
>>> It may seem absurd, but it's conceivable that a module might
>>> have two hooks it wants called. My example is a module that
>>> counts the number of times SELinux denies a process access to
>>> things (which needs to be called before and after SELinux in
>>> order to detect denials) and takes "appropriate action" if
>>> too many denials occur. It would be weird, wonky and hackish,
>>> but that never stopped anybody before.
>
> Right, but now, with the new lsm_append(), module names are concatenated
> ("%s,%s") in the lsm_names variable. It would be nice to not pollute
> this string with multiple time the same module name.

Perhaps security_add_hooks could be modified to accept a NULL lsm to
skip the lsm_append() call, so it could do:

security_add_hooks(hooks1, count1, NULL);
security_add_hooks(hooks2, count2, NULL);
security_add_hooks(NULL, 0, "landlock");

Or, as Casey suggests, disregard adding the name when it already exists:

security_add_hooks(hooks1, count1, "landlock");
security_add_hooks(hooks2, count2, "landlock");

Yeah, I think I prefer this...

-Kees

>
>>
>> If ends up being sane and clear, I'm fine with allowing multiple calls.
>>
>> -Kees
>>
>



-- 
Kees Cook
Pixel Security

[toc] | [prev] | [next] | [standalone]

#1625728

On Tue, Apr 18, 2017 at 3:44 PM, Mickaël Salaün <mic@digikod.net> wrote:
>
> On 19/04/2017 00:17, Kees Cook wrote:
>> On Tue, Mar 28, 2017 at 4:46 PM, Mickaël Salaün <mic@digikod.net> wrote:
>>> Handle 33 filesystem-related LSM hooks for the Landlock filesystem
>>> event: LANDLOCK_SUBTYPE_EVENT_FS.
>>>
>>> A Landlock event wrap LSM hooks for similar kernel object types (e.g.
>>> struct file, struct path...). Multiple LSM hooks can trigger the same
>>> Landlock event.
>>>
>>> Landlock handle nine coarse-grained actions: read, write, execute, new,
>>> get, remove, ioctl, lock and fcntl. Each of them abstract LSM hook
>>> access control in a way that can be extended in the future.
>>>
>>> The Landlock LSM hook registration is done after other LSM to only run
>>> actions from user-space, via eBPF programs, if the access was granted by
>>> major (privileged) LSMs.
>>>
>>> Changes since v5:
>>> * split hooks.[ch] into hooks.[ch] and hooks_fs.[ch]
>>> * add more documentation
>>> * cosmetic fixes
>>>
>>> Changes since v4:
>>> * add LSM hook abstraction called Landlock event
>>>   * use the compiler type checking to verify hooks use by an event
>>>   * handle all filesystem related LSM hooks (e.g. file_permission,
>>>     mmap_file, sb_mount...)
>>> * register BPF programs for Landlock just after LSM hooks registration
>>> * move hooks registration after other LSMs
>>> * add failsafes to check if a hook is not used by the kernel
>>> * allow partial raw value access form the context (needed for programs
>>>   generated by LLVM)
>>>
>>> Changes since v3:
>>> * split commit
>>> * add hooks dealing with struct inode and struct path pointers:
>>>   inode_permission and inode_getattr
>>> * add abstraction over eBPF helper arguments thanks to wrapping structs
>>>
>>> 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>
>>> ---
>>>  include/linux/lsm_hooks.h    |   5 +
>>>  security/landlock/Makefile   |   4 +-
>>>  security/landlock/hooks.c    | 115 +++++++++
>>>  security/landlock/hooks.h    | 177 ++++++++++++++
>>>  security/landlock/hooks_fs.c | 563 +++++++++++++++++++++++++++++++++++++++++++
>>>  security/landlock/hooks_fs.h |  19 ++
>>>  security/landlock/init.c     |  13 +
>>>  security/security.c          |   7 +-
>>>  8 files changed, 901 insertions(+), 2 deletions(-)
>>>  create mode 100644 security/landlock/hooks.c
>>>  create mode 100644 security/landlock/hooks.h
>>>  create mode 100644 security/landlock/hooks_fs.c
>>>  create mode 100644 security/landlock/hooks_fs.h
>>>
>>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
>>> index e29d4c62a3c8..884289166a0e 100644
>>> --- a/include/linux/lsm_hooks.h
>>> +++ b/include/linux/lsm_hooks.h
>>> @@ -1920,5 +1920,10 @@ void __init loadpin_add_hooks(void);
>>>  #else
>>>  static inline void loadpin_add_hooks(void) { };
>>>  #endif
>>> +#ifdef CONFIG_SECURITY_LANDLOCK
>>> +extern void __init landlock_add_hooks(void);
>>> +#else
>>> +static inline void __init landlock_add_hooks(void) { }
>>> +#endif /* CONFIG_SECURITY_LANDLOCK */
>>>
>>>  #endif /* ! __LINUX_LSM_HOOKS_H */
>>> diff --git a/security/landlock/Makefile b/security/landlock/Makefile
>>> index 7205f9a7a2ee..c0db504a6335 100644
>>> --- a/security/landlock/Makefile
>>> +++ b/security/landlock/Makefile
>>> @@ -1,3 +1,5 @@
>>> +ccflags-$(CONFIG_SECURITY_LANDLOCK) += -Werror=unused-function
>>
>> Why is this needed? If it can't be avoided, a comment should exist
>> here explaining why.
>
> This is useful to catch defined but unused hooks: error out if a
> HOOK_NEW_FS(foo) is not used with a HOOK_INIT_FS(foo) in the struct
> security_hook_list landlock_hooks.

Gotcha. Please convert into a comment for the next revision. :)

>
>>
>>> [...]
>>> @@ -127,3 +132,11 @@ static struct bpf_prog_type_list bpf_landlock_type __ro_after_init = {
>>>         .ops = &bpf_landlock_ops,
>>>         .type = BPF_PROG_TYPE_LANDLOCK,
>>>  };
>>> +
>>> +void __init landlock_add_hooks(void)
>>> +{
>>> +       pr_info("landlock: Version %u", LANDLOCK_VERSION);
>>> +       landlock_add_hooks_fs();
>>> +       security_add_hooks(NULL, 0, "landlock");
>>> +       bpf_register_prog_type(&bpf_landlock_type);
>>
>> I'm confused by the separation of hook registration here. The call to
>> security_add_hooks is with count=0 is especially weird. Why isn't this
>> just a single call with security_add_hooks(landlock_hooks,
>> ARRAY_SIZE(landlock_hooks), "landlock")?
>
> Yes, this is ugly with the new security_add_hooks() with three arguments
> but I wanted to split the hooks definition in multiple files.
>
> The current security_add_hooks() use lsm_append(lsm, &lsm_names) which
> is not exported. Unfortunately, calling multiple security_add_hooks()
> with the same LSM name would register multiple names for the same LSM…
> Is it OK if I modify this function to not add duplicated entries?

I'm not sure which would be more sane: a single table (constructed
from header-defined function declarations), or changes to this core
area.

>>> diff --git a/security/security.c b/security/security.c
>>> index d0e07f269b2d..a3e9f4625991 100644
>>> --- a/security/security.c
>>> +++ b/security/security.c
>>> @@ -64,10 +64,15 @@ int __init security_init(void)
>>>         loadpin_add_hooks();
>>>
>>>         /*
>>> -        * Load all the remaining security modules.
>>> +        * Load all remaining privileged security modules.
>>>          */
>>>         do_security_initcalls();
>>>
>>> +       /*
>>> +        * Load potentially-unprivileged security modules at the end.
>>> +        */
>>> +       landlock_add_hooks();
>>
>> Oh, is this to make it last in the list? Is there a reason it has to be last?
>
> Right, this is the intend. I'm not sure it is the only way to register
> hooks, though.
>
> For an unprivileged access-control, we don't want to give the ability to
> any process to do some checks, through an eBPF program, on kernel
> objects (e.g. files) if they should not be accessible (because of a
> following LSM hook check).

Ah right, yes. I thought some of that was already going to be handled
by getting handles to things, but yeah, best to run last. (If you can
expand the comment above the call to landlock_add_hooks() that would
be great.)

-Kees

-- 
Kees Cook
Pixel Security

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web