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

[Casey Schaufler <casey@schaufler-ca.com>]

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
>