netdev - Re: [RFC PATCH v1 01/10] landlock: Support socket access-control

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZhRKOTmoAOuwkujB@google.com>
Date: Mon, 8 Apr 2024 21:49:13 +0200
From: "Günther Noack" <gnoack@...gle.com>
To: Ivanov Mikhail <ivanov.mikhail1@...wei-partners.com>
Cc: mic@...ikod.net, 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, konstantin.meskhidze@...wei.com
Subject: Re: [RFC PATCH v1 01/10] landlock: Support socket access-control

Hello!

Just zooming in on what I think are the most high level questions here,
so that we get the more dramatic changes out of the way early, if needed.

On Mon, Apr 08, 2024 at 05:39:18PM +0800, Ivanov Mikhail wrote:
> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> index 25c8d7677..8551ade38 100644
> --- a/include/uapi/linux/landlock.h
> +++ b/include/uapi/linux/landlock.h
> @@ -37,6 +37,13 @@ struct landlock_ruleset_attr {
>  	 * rule explicitly allow them.
>  	 */
>  	__u64 handled_access_net;
> +
> +	/**
> +	 * @handled_access_net: Bitmask of actions (cf. `Socket flags`_)
                           ^^^
			   Typo

> +	 * that is handled by this ruleset and should then be forbidden if no
> +	 * rule explicitly allow them.
> +	 */
> +	__u64 handled_access_socket;

What is your rationale for introducing and naming this additional field?

I am not convinced that "socket" is the right name to use in this field,
but it is well possible that I'm missing some context.

* If we introduce this additional field in the landlock_ruleset_attr, which
  other socket-related operations will go in the remaining 63 bits?  (I'm having
  a hard time coming up with so many of them.)

* Should this have a more general name than "socket", so that other planned
  features from the bug tracker [1] fit in?

The other alternative is of course to piggy back on the existing
handled_access_net field, whose name already is pretty generic.

For that, I believe we would need to clarify in struct landlock_net_port_attr
which exact values are permitted there.

I imagine you have considered this approach?  Are there more reasons why this
was ruled out, which I am overlooking?

[1] https://github.com/orgs/landlock-lsm/projects/1/views/1

> @@ -244,4 +277,20 @@ struct landlock_net_port_attr {
>  #define LANDLOCK_ACCESS_NET_BIND_TCP			(1ULL << 0)
>  #define LANDLOCK_ACCESS_NET_CONNECT_TCP			(1ULL << 1)
>  /* clang-format on */
> +
> +/**
> + * DOC: socket_acess
> + *
> + * Socket flags
> + * ~~~~~~~~~~~~~~~~

Mega-Nit: This ~~~ underline should only be as long as the text above it ;-)
You might want to fix it for the "Network Flags" headline as well.

> + *
> + * These flags enable to restrict a sandboxed process to a set of
> + * socket-related actions for specific protocols. This is supported
> + * since the Landlock ABI version 5.
> + *
> + * - %LANDLOCK_ACCESS_SOCKET_CREATE: Create a socket
> + */

> diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
> index c7f152678..f4213db09 100644
> --- a/security/landlock/ruleset.h
> +++ b/security/landlock/ruleset.h
> @@ -92,6 +92,12 @@ enum landlock_key_type {
>  	 * node keys.
>  	 */
>  	LANDLOCK_KEY_NET_PORT,
> +
> +	/**
> +	 * @LANDLOCK_KEY_SOCKET: Type of &landlock_ruleset.root_socket's
> +	 * node keys.
> +	 */
> +	LANDLOCK_KEY_SOCKET,
>  };
>  
>  /**
> @@ -177,6 +183,15 @@ struct landlock_ruleset {
>  	struct rb_root root_net_port;
>  #endif /* IS_ENABLED(CONFIG_INET) */
>  
> +	/**
> +	 * @root_socket: Root of a red-black tree containing &struct
> +	 * landlock_rule nodes with socket type, described by (domain, type)
> +	 * pair (see socket(2)). Once a ruleset is tied to a
> +	 * process (i.e. as a domain), this tree is immutable until @usage
> +	 * reaches zero.
> +	 */
> +	struct rb_root root_socket;

The domain is a value between 0 and 45,
and the socket type is one of 1, 2, 3, 4, 5, 6, 10.

The bounds of these are defined with AF_MAX (include/linux/socket.h) and
SOCK_MAX (include/linux/net.h).

Why don't we just combine these two numbers into an index and create a big bit
vector here, like this:

    socket_type_mask_t socket_domains[AF_MAX];

socket_type_mask_t would need to be typedef'd to u16 and ideally have a static
check to test that it has more bits than SOCK_MAX.

Then you can look up whether a socket creation is permitted by checking:

    /* assuming appropriate bounds checks */
    if (dom->socket_domains[domain] & (1 << type)) { /* permitted */ }

and merging the socket_domains of two domains would be a bitwise-AND.

(We can also cram socket_type_mask_t in a u8 but it would require mapping the
existing socket types onto a different number space.)

As I said before, I am very excited to see this patch.

I think this will unlock a tremendous amount of use cases for many programs,
especially for programs that do not use networking at all, which can now lock
themselves down to guarantee that with a sandbox.

Thank you very much for looking into it!
—Günther