Date: Tue, 27 Jun 2023 18:14:04 +0200
From: Mickaël Salaün <mic@...ikod.net>
To: Konstantin Meskhidze <konstantin.meskhidze@...wei.com>
Cc: willemdebruijn.kernel@...il.com, gnoack3000@...il.com,
 linux-security-module@...r.kernel.org, netdev@...r.kernel.org,
 netfilter-devel@...r.kernel.org, yusongping@...wei.com,
 artem.kuzin@...wei.com
Subject: Re: [PATCH v11 08/12] landlock: Add network rules and TCP hooks
 support


On 15/05/2023 18:13, Konstantin Meskhidze wrote:
> This commit adds network rules support in the ruleset management
> helpers and the landlock_create_ruleset syscall.
> Refactor user space API to support network actions. Add new network
> access flags, network rule and network attributes. Increment Landlock
> ABI version. Expand access_masks_t to u32 to be sure network access
> rights can be stored. Implement socket_bind() and socket_connect()
> LSM hooks, which enables to restrict TCP socket binding and connection
> to specific ports.
> 
> Co-developed-by: Mickaël Salaün <mic@...ikod.net>
> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@...wei.com>
> ---
> 
> Changes since v10:
> * Removes "packed" attribute.
> * Applies Mickaёl's patch with some refactoring.
> * Deletes get_port() and check_addrlen() helpers.
> * Refactors check_socket_access() by squashing get_port() and
> check_addrlen() helpers into it.
> * Fixes commit message.
> 
> Changes since v9:
> * Changes UAPI port field to __u64.
> * Moves shared code into check_socket_access().
> * Adds get_raw_handled_net_accesses() and
> get_current_net_domain() helpers.
> * Minor fixes.
> 
> Changes since v8:
> * Squashes commits.
> * Refactors commit message.
> * Changes UAPI port field to __be16.
> * Changes logic of bind/connect hooks with AF_UNSPEC families.
> * Adds address length checking.
> * Minor fixes.
> 
> Changes since v7:
> * Squashes commits.
> * Increments ABI version to 4.
> * Refactors commit message.
> * Minor fixes.
> 
> Changes since v6:
> * Renames landlock_set_net_access_mask() to landlock_add_net_access_mask()
>    because it OR values.
> * Makes landlock_add_net_access_mask() more resilient incorrect values.
> * Refactors landlock_get_net_access_mask().
> * Renames LANDLOCK_MASK_SHIFT_NET to LANDLOCK_SHIFT_ACCESS_NET and use
>    LANDLOCK_NUM_ACCESS_FS as value.
> * Updates access_masks_t to u32 to support network access actions.
> * Refactors landlock internal functions to support network actions with
>    landlock_key/key_type/id types.
> 
> Changes since v5:
> * Gets rid of partial revert from landlock_add_rule
> syscall.
> * Formats code with clang-format-14.
> 
> Changes since v4:
> * Refactors landlock_create_ruleset() - splits ruleset and
> masks checks.
> * Refactors landlock_create_ruleset() and landlock mask
> setters/getters to support two rule types.
> * Refactors landlock_add_rule syscall add_rule_path_beneath
> function by factoring out get_ruleset_from_fd() and
> landlock_put_ruleset().
> 
> Changes since v3:
> * Splits commit.
> * Adds network rule support for internal landlock functions.
> * Adds set_mask and get_mask for network.
> * Adds rb_root root_net_port.
> 
> ---
>   include/uapi/linux/landlock.h                |  48 +++++
>   security/landlock/Kconfig                    |   1 +
>   security/landlock/Makefile                   |   2 +
>   security/landlock/limits.h                   |   6 +-
>   security/landlock/net.c                      | 174 +++++++++++++++++++
>   security/landlock/net.h                      |  26 +++
>   security/landlock/ruleset.c                  |  52 +++++-
>   security/landlock/ruleset.h                  |  63 +++++--
>   security/landlock/setup.c                    |   2 +
>   security/landlock/syscalls.c                 |  72 +++++++-
>   tools/testing/selftests/landlock/base_test.c |   2 +-
>   11 files changed, 425 insertions(+), 23 deletions(-)
>   create mode 100644 security/landlock/net.c
>   create mode 100644 security/landlock/net.h
> 
> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> index 81d09ef9aa50..93794759dad4 100644
> --- a/include/uapi/linux/landlock.h
> +++ b/include/uapi/linux/landlock.h
> @@ -31,6 +31,13 @@ struct landlock_ruleset_attr {
>   	 * this access right.
>   	 */
>   	__u64 handled_access_fs;
> +
> +	/**
> +	 * @handled_access_net: Bitmask of actions (cf. `Network flags`_)
> +	 * that is handled by this ruleset and should then be forbidden if no
> +	 * rule explicitly allow them.
> +	 */
> +	__u64 handled_access_net;
>   };
> 
>   /*
> @@ -54,6 +61,11 @@ enum landlock_rule_type {
>   	 * landlock_path_beneath_attr .
>   	 */
>   	LANDLOCK_RULE_PATH_BENEATH = 1,
> +	/**
> +	 * @LANDLOCK_RULE_NET_SERVICE: Type of a &struct
> +	 * landlock_net_service_attr .
> +	 */
> +	LANDLOCK_RULE_NET_SERVICE = 2,
>   };
> 
>   /**
> @@ -79,6 +91,23 @@ struct landlock_path_beneath_attr {
>   	 */
>   } __attribute__((packed));
> 
> +/**
> + * struct landlock_net_service_attr - TCP subnet definition
> + *
> + * Argument of sys_landlock_add_rule().
> + */
> +struct landlock_net_service_attr {
> +	/**
> +	 * @allowed_access: Bitmask of allowed access network for services
> +	 * (cf. `Network flags`_).
> +	 */
> +	__u64 allowed_access;
> +	/**
> +	 * @port: Network port.
> +	 */
> +	__u64 port;
> +};

The "net service" name reflects the semantic but it doesn't fit well 
with the data type. It works with TCP, UDP and other protocols such as 
VSOCK, but not unix sockets. I think it makes more sense to rename 
LANDLOCK_RULE_NET_SERVICE to LANDLOCK_RULE_NET_PORT and 
landlock_net_service_attr to landlock_net_port_attr. Please be careful 
with the documentation, non-kernel code, and comments as well.

In the future, we could add a landlock_net_path_attr to identify a unix 
abstract path, which would also be a network service, but it would not 
accept the TCP access rights.

The access right names (LANDLOCK_ACCESS_NET_{BIND,CONNECT}_TCP) are 
still good.


I'm still improving tests, so you might wait a bit before renaming the 
related files.

Powered by blists - more mailing lists