[RFC PATCH v1 0/9] Support TCP listen access-control

[RFC PATCH v1 0/9] Support TCP listen access-control

[Mikhail Ivanov]

Hello! This is v1 RFC patch dedicated to restriction of listening sockets.

It is based on the landlock's mic-next branch on top of v6.10 kernel
version.

Description
===========
LANDLOCK_ACCESS_NET_BIND_TCP is useful to limit the scope of "bindable"
ports to forbid a malicious sandboxed process to impersonate a legitimate
server process. However, bind(2) might be used by (TCP) clients to set the
source port to a (legitimate) value. Controlling the ports that can be
used for listening would allow (TCP) clients to explicitly bind to ports
that are forbidden for listening.

Such control is implemented with a new LANDLOCK_ACCESS_NET_LISTEN_TCP
access right that restricts listening on undesired ports with listen(2).

It's worth noticing that this access right doesn't affect changing 
backlog value using listen(2) on already listening socket. For this case
test ipv4_tcp.double_listen is provided.

Closes: https://github.com/landlock-lsm/linux/issues/15

Code coverage
=============
Code coverage(gcov) report with the launch of all the landlock selftests:
* security/landlock:
lines......: 93.4% (759 of 813 lines)
functions..: 95.3% (101 of 106 functions)

* security/landlock/net.c:
lines......: 100% (77 of 77 lines)
functions..: 100% (9 of 9 functions)

Mikhail Ivanov (9):
  landlock: Refactor current_check_access_socket() access right check
  landlock: Support TCP listen access-control
  selftests/landlock: Support LANDLOCK_ACCESS_NET_LISTEN_TCP
  selftests/landlock: Test listening restriction
  selftests/landlock: Test listen on connected socket
  selftests/landlock: Test listening without explicit bind restriction
  selftests/landlock: Test listen on ULP socket without clone method
  selftests/landlock: Test changing socket backlog with listen(2)
  samples/landlock: Support LANDLOCK_ACCESS_NET_LISTEN

 include/uapi/linux/landlock.h                |  23 +-
 samples/landlock/sandboxer.c                 |  31 +-
 security/landlock/limits.h                   |   2 +-
 security/landlock/net.c                      | 131 +++++-
 security/landlock/syscalls.c                 |   2 +-
 tools/testing/selftests/landlock/base_test.c |   2 +-
 tools/testing/selftests/landlock/config      |   1 +
 tools/testing/selftests/landlock/net_test.c  | 448 +++++++++++++++----
 8 files changed, 519 insertions(+), 121 deletions(-)

-- 
2.34.1

[RFC PATCH v1 1/9] landlock: Refactor current_check_access_socket() access right check

[Mikhail Ivanov]

The current_check_access_socket() function contains a set of address
validation checks for bind(2) and connect(2) hooks. Separate them from
an actual port access right checking. It is required for the (future)
hooks that do not perform address validation.

Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
---
 security/landlock/net.c | 41 ++++++++++++++++++++++++-----------------
 1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/security/landlock/net.c b/security/landlock/net.c
index c8bcd29bde09..669ba260342f 100644
--- a/security/landlock/net.c
+++ b/security/landlock/net.c
@@ -2,7 +2,7 @@
 /*
  * Landlock LSM - Network management and hooks
  *
- * Copyright © 2022-2023 Huawei Tech. Co., Ltd.
+ * Copyright © 2022-2024 Huawei Tech. Co., Ltd.
  * Copyright © 2022-2023 Microsoft Corporation
  */
 
@@ -61,17 +61,34 @@ static const struct landlock_ruleset *get_current_net_domain(void)
 	return dom;
 }
 
-static int current_check_access_socket(struct socket *const sock,
-				       struct sockaddr *const address,
-				       const int addrlen,
-				       access_mask_t access_request)
+static int check_access_socket(const struct landlock_ruleset *const dom,
+			       __be16 port, access_mask_t access_request)
 {
-	__be16 port;
 	layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_NET] = {};
 	const struct landlock_rule *rule;
 	struct landlock_id id = {
 		.type = LANDLOCK_KEY_NET_PORT,
 	};
+
+	id.key.data = (__force uintptr_t)port;
+	BUILD_BUG_ON(sizeof(port) > sizeof(id.key.data));
+
+	rule = landlock_find_rule(dom, id);
+	access_request = landlock_init_layer_masks(
+		dom, access_request, &layer_masks, LANDLOCK_KEY_NET_PORT);
+	if (landlock_unmask_layers(rule, access_request, &layer_masks,
+				   ARRAY_SIZE(layer_masks)))
+		return 0;
+
+	return -EACCES;
+}
+
+static int current_check_access_socket(struct socket *const sock,
+				       struct sockaddr *const address,
+				       const int addrlen,
+				       access_mask_t access_request)
+{
+	__be16 port;
 	const struct landlock_ruleset *const dom = get_current_net_domain();
 
 	if (!dom)
@@ -159,17 +176,7 @@ static int current_check_access_socket(struct socket *const sock,
 			return -EINVAL;
 	}
 
-	id.key.data = (__force uintptr_t)port;
-	BUILD_BUG_ON(sizeof(port) > sizeof(id.key.data));
-
-	rule = landlock_find_rule(dom, id);
-	access_request = landlock_init_layer_masks(
-		dom, access_request, &layer_masks, LANDLOCK_KEY_NET_PORT);
-	if (landlock_unmask_layers(rule, access_request, &layer_masks,
-				   ARRAY_SIZE(layer_masks)))
-		return 0;
-
-	return -EACCES;
+	return check_access_socket(dom, port, access_request);
 }
 
 static int hook_socket_bind(struct socket *const sock,
-- 
2.34.1

[RFC PATCH v1 2/9] landlock: Support TCP listen access-control

[Mikhail Ivanov]

LANDLOCK_ACCESS_NET_BIND_TCP is useful to limit the scope of "bindable"
ports to forbid a malicious sandboxed process to impersonate a legitimate
server process. However, bind(2) might be used by (TCP) clients to set the
source port to a (legitimate) value. Controlling the ports that can be
used for listening would allow (TCP) clients to explicitly bind to ports
that are forbidden for listening.

Such control is implemented with a new LANDLOCK_ACCESS_NET_LISTEN_TCP
access right that restricts listening on undesired ports with listen(2).

It's worth noticing that this access right doesn't affect changing
backlog value using listen(2) on already listening socket.

* Create new LANDLOCK_ACCESS_NET_LISTEN_TCP flag.
* Add hook to socket_listen(), which checks whether the socket is allowed
  to listen on a binded local port.
* Add check_tcp_socket_can_listen() helper, which validates socket
  attributes before the actual access right check.
* Update `struct landlock_net_port_attr` documentation with control of
  binding to ephemeral port with listen(2) description.
* Change ABI version to 6.

Closes: https://github.com/landlock-lsm/linux/issues/15
Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
---
 include/uapi/linux/landlock.h                | 23 +++--
 security/landlock/limits.h                   |  2 +-
 security/landlock/net.c                      | 90 ++++++++++++++++++++
 security/landlock/syscalls.c                 |  2 +-
 tools/testing/selftests/landlock/base_test.c |  2 +-
 5 files changed, 108 insertions(+), 11 deletions(-)

diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
index 68625e728f43..6b8df3293eee 100644
--- a/include/uapi/linux/landlock.h
+++ b/include/uapi/linux/landlock.h
@@ -104,13 +104,16 @@ struct landlock_net_port_attr {
 	/**
 	 * @port: Network port in host endianness.
 	 *
-	 * It should be noted that port 0 passed to :manpage:`bind(2)` will
-	 * bind to an available port from a specific port range. This can be
-	 * configured thanks to the ``/proc/sys/net/ipv4/ip_local_port_range``
-	 * sysctl (also used for IPv6). A Landlock rule with port 0 and the
-	 * ``LANDLOCK_ACCESS_NET_BIND_TCP`` right means that requesting to bind
-	 * on port 0 is allowed and it will automatically translate to binding
-	 * on the related port range.
+	 * It should be noted that some operations cause binding socket to a random
+	 * available port from a specific port range. This can be configured thanks
+	 * to the ``/proc/sys/net/ipv4/ip_local_port_range`` sysctl (also used for
+	 * IPv6). Following operation requests are automatically translate to
+	 * binding on the related port range:
+	 *
+	 * - A Landlock rule with port 0 and the ``LANDLOCK_ACCESS_NET_BIND_TCP``
+	 *   right means that binding on port 0 is allowed.
+	 * - A Landlock rule with port 0 and the ``LANDLOCK_ACCESS_NET_LISTEN_TCP``
+	 *   right means listening without an explicit binding is allowed.
 	 */
 	__u64 port;
 };
@@ -251,7 +254,7 @@ struct landlock_net_port_attr {
  * DOC: net_access
  *
  * Network flags
- * ~~~~~~~~~~~~~~~~
+ * ~~~~~~~~~~~~~
  *
  * These flags enable to restrict a sandboxed process to a set of network
  * actions. This is supported since the Landlock ABI version 4.
@@ -261,9 +264,13 @@ struct landlock_net_port_attr {
  * - %LANDLOCK_ACCESS_NET_BIND_TCP: Bind a TCP socket to a local port.
  * - %LANDLOCK_ACCESS_NET_CONNECT_TCP: Connect an active TCP socket to
  *   a remote port.
+ * - %LANDLOCK_ACCESS_NET_LISTEN_TCP: Listen for TCP socket connections on
+ *   a local port. This access right is available since the sixth version
+ *   of the Landlock ABI.
  */
 /* clang-format off */
 #define LANDLOCK_ACCESS_NET_BIND_TCP			(1ULL << 0)
 #define LANDLOCK_ACCESS_NET_CONNECT_TCP			(1ULL << 1)
+#define LANDLOCK_ACCESS_NET_LISTEN_TCP			(1ULL << 2)
 /* clang-format on */
 #endif /* _UAPI_LINUX_LANDLOCK_H */
diff --git a/security/landlock/limits.h b/security/landlock/limits.h
index 4eb643077a2a..2ef147389474 100644
--- a/security/landlock/limits.h
+++ b/security/landlock/limits.h
@@ -22,7 +22,7 @@
 #define LANDLOCK_MASK_ACCESS_FS		((LANDLOCK_LAST_ACCESS_FS << 1) - 1)
 #define LANDLOCK_NUM_ACCESS_FS		__const_hweight64(LANDLOCK_MASK_ACCESS_FS)
 
-#define LANDLOCK_LAST_ACCESS_NET	LANDLOCK_ACCESS_NET_CONNECT_TCP
+#define LANDLOCK_LAST_ACCESS_NET	LANDLOCK_ACCESS_NET_LISTEN_TCP
 #define LANDLOCK_MASK_ACCESS_NET	((LANDLOCK_LAST_ACCESS_NET << 1) - 1)
 #define LANDLOCK_NUM_ACCESS_NET		__const_hweight64(LANDLOCK_MASK_ACCESS_NET)
 
diff --git a/security/landlock/net.c b/security/landlock/net.c
index 669ba260342f..a29cb27c3f14 100644
--- a/security/landlock/net.c
+++ b/security/landlock/net.c
@@ -6,10 +6,12 @@
  * Copyright © 2022-2023 Microsoft Corporation
  */
 
+#include "net/sock.h"
 #include <linux/in.h>
 #include <linux/net.h>
 #include <linux/socket.h>
 #include <net/ipv6.h>
+#include <net/tcp.h>
 
 #include "common.h"
 #include "cred.h"
@@ -194,9 +196,97 @@ static int hook_socket_connect(struct socket *const sock,
 					   LANDLOCK_ACCESS_NET_CONNECT_TCP);
 }
 
+/*
+ * Checks that socket state and attributes are correct for listen.
+ * It is required to not wrongfully return -EACCES instead of -EINVAL.
+ *
+ * This checker requires sock->sk to be locked.
+ */
+static int check_tcp_socket_can_listen(struct socket *const sock)
+{
+	struct sock *sk = sock->sk;
+	unsigned char cur_sk_state = sk->sk_state;
+	const struct tcp_ulp_ops *icsk_ulp_ops;
+
+	/* Allows only unconnected TCP socket to listen (cf. inet_listen). */
+	if (sock->state != SS_UNCONNECTED)
+		return -EINVAL;
+
+	/*
+	 * Checks sock state. This is needed to ensure consistency with inet stack
+	 * error handling (cf. __inet_listen_sk).
+	 */
+	if (WARN_ON_ONCE(!((1 << cur_sk_state) & (TCPF_CLOSE | TCPF_LISTEN))))
+		return -EINVAL;
+
+	icsk_ulp_ops = inet_csk(sk)->icsk_ulp_ops;
+
+	/*
+	 * ULP (Upper Layer Protocol) stands for protocols which are higher than
+	 * transport protocol in OSI model. Linux has an infrastructure that
+	 * allows TCP sockets to support logic of some ULP (e.g. TLS ULP).
+	 *
+	 * Sockets can listen only if ULP control hook has clone method.
+	 */
+	if (icsk_ulp_ops && !icsk_ulp_ops->clone)
+		return -EINVAL;
+	return 0;
+}
+
+static int hook_socket_listen(struct socket *const sock, const int backlog)
+{
+	int err = 0;
+	int family;
+	__be16 port;
+	struct sock *sk;
+	const struct landlock_ruleset *const dom = get_current_net_domain();
+
+	if (!dom)
+		return 0;
+	if (WARN_ON_ONCE(dom->num_layers < 1))
+		return -EACCES;
+
+	/* Checks if it's a (potential) TCP socket. */
+	if (sock->type != SOCK_STREAM)
+		return 0;
+
+	sk = sock->sk;
+	family = sk->__sk_common.skc_family;
+	/*
+	 * Socket cannot be assigned AF_UNSPEC because this type is used only
+	 * in the context of addresses.
+	 *
+	 * Doesn't restrict listening for non-TCP sockets.
+	 */
+	if (family != AF_INET && family != AF_INET6)
+		return 0;
+
+	lock_sock(sk);
+	/*
+	 * Calling listen(2) for a listening socket does nothing with its state and
+	 * only changes backlog value (cf. __inet_listen_sk). Checking of listen
+	 * access right is not required.
+	 */
+	if (sk->sk_state == TCP_LISTEN)
+		goto release_nocheck;
+
+	err = check_tcp_socket_can_listen(sock);
+	if (unlikely(err))
+		goto release_nocheck;
+
+	port = htons(inet_sk(sk)->inet_num);
+	release_sock(sk);
+	return check_access_socket(dom, port, LANDLOCK_ACCESS_NET_LISTEN_TCP);
+
+release_nocheck:
+	release_sock(sk);
+	return err;
+}
+
 static struct security_hook_list landlock_hooks[] __ro_after_init = {
 	LSM_HOOK_INIT(socket_bind, hook_socket_bind),
 	LSM_HOOK_INIT(socket_connect, hook_socket_connect),
+	LSM_HOOK_INIT(socket_listen, hook_socket_listen),
 };
 
 __init void landlock_add_net_hooks(void)
diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
index 03b470f5a85a..3752bcc033d4 100644
--- a/security/landlock/syscalls.c
+++ b/security/landlock/syscalls.c
@@ -149,7 +149,7 @@ static const struct file_operations ruleset_fops = {
 	.write = fop_dummy_write,
 };
 
-#define LANDLOCK_ABI_VERSION 5
+#define LANDLOCK_ABI_VERSION 6
 
 /**
  * sys_landlock_create_ruleset - Create a new ruleset
diff --git a/tools/testing/selftests/landlock/base_test.c b/tools/testing/selftests/landlock/base_test.c
index 3c1e9f35b531..52b00472a487 100644
--- a/tools/testing/selftests/landlock/base_test.c
+++ b/tools/testing/selftests/landlock/base_test.c
@@ -75,7 +75,7 @@ TEST(abi_version)
 	const struct landlock_ruleset_attr ruleset_attr = {
 		.handled_access_fs = LANDLOCK_ACCESS_FS_READ_FILE,
 	};
-	ASSERT_EQ(5, landlock_create_ruleset(NULL, 0,
+	ASSERT_EQ(6, landlock_create_ruleset(NULL, 0,
 					     LANDLOCK_CREATE_RULESET_VERSION));
 
 	ASSERT_EQ(-1, landlock_create_ruleset(&ruleset_attr, 0,
-- 
2.34.1

Re: [RFC PATCH v1 2/9] landlock: Support TCP listen access-control

[Günther Noack]

Hello!

Thanks for sending these patches!

Most comments are about documentation so far.

In the implementation, I'm mostly unclear about the interaction with the
uncommon Upper Layer Protocols.  I'm also not very familiar with the socket
state machines, maybe someone from the netdev list would have time to double
check that aspect?


On Sun, Jul 28, 2024 at 08:25:55AM +0800, Mikhail Ivanov wrote:
> LANDLOCK_ACCESS_NET_BIND_TCP is useful to limit the scope of "bindable"
> ports to forbid a malicious sandboxed process to impersonate a legitimate
> server process. However, bind(2) might be used by (TCP) clients to set the
> source port to a (legitimate) value. Controlling the ports that can be
> used for listening would allow (TCP) clients to explicitly bind to ports
> that are forbidden for listening.
> 
> Such control is implemented with a new LANDLOCK_ACCESS_NET_LISTEN_TCP
> access right that restricts listening on undesired ports with listen(2).

Nit: I would turn around the first two commit message paragraphs and describe
your changes first, before explaining the problems in the bind(2) support.  I
was initially a bit confused that the description started talking about
LANDLOCK_ACCESS_NET_BIND_TCP.

General recommendations at:
https://www.kernel.org/doc/html/v6.10/process/submitting-patches.html#describe-your-changes


> It's worth noticing that this access right doesn't affect changing
> backlog value using listen(2) on already listening socket.
> 
> * Create new LANDLOCK_ACCESS_NET_LISTEN_TCP flag.
> * Add hook to socket_listen(), which checks whether the socket is allowed
>   to listen on a binded local port.
> * Add check_tcp_socket_can_listen() helper, which validates socket
>   attributes before the actual access right check.
> * Update `struct landlock_net_port_attr` documentation with control of
>   binding to ephemeral port with listen(2) description.
> * Change ABI version to 6.
> 
> Closes: https://github.com/landlock-lsm/linux/issues/15
> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
> ---
>  include/uapi/linux/landlock.h                | 23 +++--
>  security/landlock/limits.h                   |  2 +-
>  security/landlock/net.c                      | 90 ++++++++++++++++++++
>  security/landlock/syscalls.c                 |  2 +-
>  tools/testing/selftests/landlock/base_test.c |  2 +-
>  5 files changed, 108 insertions(+), 11 deletions(-)
> 
> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> index 68625e728f43..6b8df3293eee 100644
> --- a/include/uapi/linux/landlock.h
> +++ b/include/uapi/linux/landlock.h
> @@ -104,13 +104,16 @@ struct landlock_net_port_attr {
>  	/**
>  	 * @port: Network port in host endianness.
>  	 *
> -	 * It should be noted that port 0 passed to :manpage:`bind(2)` will
> -	 * bind to an available port from a specific port range. This can be
> -	 * configured thanks to the ``/proc/sys/net/ipv4/ip_local_port_range``
> -	 * sysctl (also used for IPv6). A Landlock rule with port 0 and the
> -	 * ``LANDLOCK_ACCESS_NET_BIND_TCP`` right means that requesting to bind
> -	 * on port 0 is allowed and it will automatically translate to binding
> -	 * on the related port range.

Please rebase on a more recent revision, we have changed this phrasing in the meantime:

 - s/a specific port range/the ephemeral port range/
 - The paragraph was split in two.

> +	 * It should be noted that some operations cause binding socket to a random
> +	 * available port from a specific port range. This can be configured thanks
> +	 * to the ``/proc/sys/net/ipv4/ip_local_port_range`` sysctl (also used for
> +	 * IPv6). Following operation requests are automatically translate to
> +	 * binding on the related port range:
> +	 *
> +	 * - A Landlock rule with port 0 and the ``LANDLOCK_ACCESS_NET_BIND_TCP``
> +	 *   right means that binding on port 0 is allowed.
> +	 * - A Landlock rule with port 0 and the ``LANDLOCK_ACCESS_NET_LISTEN_TCP``
> +	 *   right means listening without an explicit binding is allowed.

There are some grammatical problems in this documentation section.

Can I suggest an alternative?

  Some socket operations will fall back to using a port from the ephemeral port
  range, if no specific port is requested by the caller.  Among others, this
  happens in the following cases:

  - :manpage:`bind(2)` is invoked with a socket address that uses port 0.
  - :manpage:`listen(2)` is invoked on a socket without previously calling
    :manpage:`bind(2)`.

  These two actions, which implicitly use an ephemeral port, can be allowed with
  a Landlock rule with port 0 and the ``LANDLOCK_ACCESS_NET_BIND_TCP`` /
  ``LANDLOCK_ACCESS_NET_LISTEN_TCP`` right.

  The ephemeral port range is configured in the
  ``/proc/sys/net/ipv4/ip_local_port_range`` sysctl (also used for IPv6).

When we have the documentation wording finalized,
please send an update to the man pages as well,
for this and other documentation updates.

Small remarks on what I've done here:

* I am avoiding the word "binding" when referring to the automatic assignment to
  an ephemeral port - IMHO, this is potentially confusing, since bind(2) is not
  explicitly called.
* I am also dropping the "It should be noted" / "Note that" phrase, which is
  frowned upon in man pages.

>  	 */
>  	__u64 port;
>  };
> @@ -251,7 +254,7 @@ struct landlock_net_port_attr {
>   * DOC: net_access
>   *
>   * Network flags
> - * ~~~~~~~~~~~~~~~~
> + * ~~~~~~~~~~~~~
>   *
>   * These flags enable to restrict a sandboxed process to a set of network
>   * actions. This is supported since the Landlock ABI version 4.
> @@ -261,9 +264,13 @@ struct landlock_net_port_attr {
>   * - %LANDLOCK_ACCESS_NET_BIND_TCP: Bind a TCP socket to a local port.
>   * - %LANDLOCK_ACCESS_NET_CONNECT_TCP: Connect an active TCP socket to
>   *   a remote port.
> + * - %LANDLOCK_ACCESS_NET_LISTEN_TCP: Listen for TCP socket connections on
> + *   a local port. This access right is available since the sixth version
> + *   of the Landlock ABI.
>   */
>  /* clang-format off */
>  #define LANDLOCK_ACCESS_NET_BIND_TCP			(1ULL << 0)
>  #define LANDLOCK_ACCESS_NET_CONNECT_TCP			(1ULL << 1)
> +#define LANDLOCK_ACCESS_NET_LISTEN_TCP			(1ULL << 2)
>  /* clang-format on */
>  #endif /* _UAPI_LINUX_LANDLOCK_H */
> diff --git a/security/landlock/limits.h b/security/landlock/limits.h
> index 4eb643077a2a..2ef147389474 100644
> --- a/security/landlock/limits.h
> +++ b/security/landlock/limits.h
> @@ -22,7 +22,7 @@
>  #define LANDLOCK_MASK_ACCESS_FS		((LANDLOCK_LAST_ACCESS_FS << 1) - 1)
>  #define LANDLOCK_NUM_ACCESS_FS		__const_hweight64(LANDLOCK_MASK_ACCESS_FS)
>  
> -#define LANDLOCK_LAST_ACCESS_NET	LANDLOCK_ACCESS_NET_CONNECT_TCP
> +#define LANDLOCK_LAST_ACCESS_NET	LANDLOCK_ACCESS_NET_LISTEN_TCP
>  #define LANDLOCK_MASK_ACCESS_NET	((LANDLOCK_LAST_ACCESS_NET << 1) - 1)
>  #define LANDLOCK_NUM_ACCESS_NET		__const_hweight64(LANDLOCK_MASK_ACCESS_NET)
>  
> diff --git a/security/landlock/net.c b/security/landlock/net.c
> index 669ba260342f..a29cb27c3f14 100644
> --- a/security/landlock/net.c
> +++ b/security/landlock/net.c
> @@ -6,10 +6,12 @@
>   * Copyright © 2022-2023 Microsoft Corporation
>   */
>  
> +#include "net/sock.h"
>  #include <linux/in.h>
>  #include <linux/net.h>
>  #include <linux/socket.h>
>  #include <net/ipv6.h>
> +#include <net/tcp.h>
>  
>  #include "common.h"
>  #include "cred.h"
> @@ -194,9 +196,97 @@ static int hook_socket_connect(struct socket *const sock,
>  					   LANDLOCK_ACCESS_NET_CONNECT_TCP);
>  }
>  
> +/*
> + * Checks that socket state and attributes are correct for listen.
> + * It is required to not wrongfully return -EACCES instead of -EINVAL.
      ^^^^^^^^^^^^^^

Doc nit: I would just document that this function returns -EINVAL on failure?
In this place, I would expect that the function interface is documented for
callers.  (From that perspective, this is not a requirement, but a guarantee
that the function gives.)

> + *
> + * This checker requires sock->sk to be locked.
> + */
> +static int check_tcp_socket_can_listen(struct socket *const sock)
> +{
> +	struct sock *sk = sock->sk;
> +	unsigned char cur_sk_state = sk->sk_state;
> +	const struct tcp_ulp_ops *icsk_ulp_ops;
> +
> +	/* Allows only unconnected TCP socket to listen (cf. inet_listen). */
> +	if (sock->state != SS_UNCONNECTED)
> +		return -EINVAL;
> +
> +	/*
> +	 * Checks sock state. This is needed to ensure consistency with inet stack
> +	 * error handling (cf. __inet_listen_sk).
> +	 */
> +	if (WARN_ON_ONCE(!((1 << cur_sk_state) & (TCPF_CLOSE | TCPF_LISTEN))))
> +		return -EINVAL;
> +
> +	icsk_ulp_ops = inet_csk(sk)->icsk_ulp_ops;
> +
> +	/*
> +	 * ULP (Upper Layer Protocol) stands for protocols which are higher than
> +	 * transport protocol in OSI model. Linux has an infrastructure that
> +	 * allows TCP sockets to support logic of some ULP (e.g. TLS ULP).
> +	 *
> +	 * Sockets can listen only if ULP control hook has clone method.
> +	 */
> +	if (icsk_ulp_ops && !icsk_ulp_ops->clone)
> +		return -EINVAL;

This seems like an implementation detail in the networking subsystem?

If I understand correctly, these are cases where we use TCP on top of protocols
that are not IP (or have an additional layer in the middle, like TLS?).  This
can not be recognized through the socket family or type?

Do we have cases where we can run TCP on top of something else than plain IPv4
or IPv6, where the clone method exists?

> +	return 0;
> +}
> +
> +static int hook_socket_listen(struct socket *const sock, const int backlog)
> +{
> +	int err = 0;
> +	int family;
> +	__be16 port;
> +	struct sock *sk;
> +	const struct landlock_ruleset *const dom = get_current_net_domain();
> +
> +	if (!dom)
> +		return 0;
> +	if (WARN_ON_ONCE(dom->num_layers < 1))
> +		return -EACCES;
> +
> +	/* Checks if it's a (potential) TCP socket. */
> +	if (sock->type != SOCK_STREAM)
> +		return 0;
> +
> +	sk = sock->sk;
> +	family = sk->__sk_common.skc_family;
> +	/*
> +	 * Socket cannot be assigned AF_UNSPEC because this type is used only
> +	 * in the context of addresses.
> +	 *
> +	 * Doesn't restrict listening for non-TCP sockets.
> +	 */
> +	if (family != AF_INET && family != AF_INET6)
> +		return 0;

Aren't the socket type and family checks duplicated with existing logic that we
have for the connect(2) and bind(2) support?  Should it be deduplicated, or is
that too messy?

> +
> +	lock_sock(sk);
> +	/*
> +	 * Calling listen(2) for a listening socket does nothing with its state and
> +	 * only changes backlog value (cf. __inet_listen_sk). Checking of listen
> +	 * access right is not required.
> +	 */
> +	if (sk->sk_state == TCP_LISTEN)
> +		goto release_nocheck;
> +
> +	err = check_tcp_socket_can_listen(sock);
> +	if (unlikely(err))
> +		goto release_nocheck;
> +
> +	port = htons(inet_sk(sk)->inet_num);
> +	release_sock(sk);
> +	return check_access_socket(dom, port, LANDLOCK_ACCESS_NET_LISTEN_TCP);
> +
> +release_nocheck:
> +	release_sock(sk);
> +	return err;
> +}

Thanks for sending these patches!

—Günther

Re: [RFC PATCH v1 2/9] landlock: Support TCP listen access-control

[Mikhail Ivanov]

7/30/2024 11:24 AM, Günther Noack wrote:
> Hello!
> 
> Thanks for sending these patches!
> 
> Most comments are about documentation so far.
> 
> In the implementation, I'm mostly unclear about the interaction with the
> uncommon Upper Layer Protocols.  I'm also not very familiar with the socket
> state machines, maybe someone from the netdev list would have time to double
> check that aspect?
> 
> 
> On Sun, Jul 28, 2024 at 08:25:55AM +0800, Mikhail Ivanov wrote:
>> LANDLOCK_ACCESS_NET_BIND_TCP is useful to limit the scope of "bindable"
>> ports to forbid a malicious sandboxed process to impersonate a legitimate
>> server process. However, bind(2) might be used by (TCP) clients to set the
>> source port to a (legitimate) value. Controlling the ports that can be
>> used for listening would allow (TCP) clients to explicitly bind to ports
>> that are forbidden for listening.
>>
>> Such control is implemented with a new LANDLOCK_ACCESS_NET_LISTEN_TCP
>> access right that restricts listening on undesired ports with listen(2).
> 
> Nit: I would turn around the first two commit message paragraphs and describe
> your changes first, before explaining the problems in the bind(2) support.  I
> was initially a bit confused that the description started talking about
> LANDLOCK_ACCESS_NET_BIND_TCP.
> 
> General recommendations at:
> https://www.kernel.org/doc/html/v6.10/process/submitting-patches.html#describe-your-changes

I consider the first paragraph as a problem statement for this patch.
According to linux recommendations problem should be established before
the description of changes. Do you think that the changes part should
stand before the problem anyway?

> 
> 
>> It's worth noticing that this access right doesn't affect changing
>> backlog value using listen(2) on already listening socket.
>>
>> * Create new LANDLOCK_ACCESS_NET_LISTEN_TCP flag.
>> * Add hook to socket_listen(), which checks whether the socket is allowed
>>    to listen on a binded local port.
>> * Add check_tcp_socket_can_listen() helper, which validates socket
>>    attributes before the actual access right check.
>> * Update `struct landlock_net_port_attr` documentation with control of
>>    binding to ephemeral port with listen(2) description.
>> * Change ABI version to 6.
>>
>> Closes: https://github.com/landlock-lsm/linux/issues/15
>> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
>> ---
>>   include/uapi/linux/landlock.h                | 23 +++--
>>   security/landlock/limits.h                   |  2 +-
>>   security/landlock/net.c                      | 90 ++++++++++++++++++++
>>   security/landlock/syscalls.c                 |  2 +-
>>   tools/testing/selftests/landlock/base_test.c |  2 +-
>>   5 files changed, 108 insertions(+), 11 deletions(-)
>>
>> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
>> index 68625e728f43..6b8df3293eee 100644
>> --- a/include/uapi/linux/landlock.h
>> +++ b/include/uapi/linux/landlock.h
>> @@ -104,13 +104,16 @@ struct landlock_net_port_attr {
>>   	/**
>>   	 * @port: Network port in host endianness.
>>   	 *
>> -	 * It should be noted that port 0 passed to :manpage:`bind(2)` will
>> -	 * bind to an available port from a specific port range. This can be
>> -	 * configured thanks to the ``/proc/sys/net/ipv4/ip_local_port_range``
>> -	 * sysctl (also used for IPv6). A Landlock rule with port 0 and the
>> -	 * ``LANDLOCK_ACCESS_NET_BIND_TCP`` right means that requesting to bind
>> -	 * on port 0 is allowed and it will automatically translate to binding
>> -	 * on the related port range.
> 
> Please rebase on a more recent revision, we have changed this phrasing in the meantime:
> 
>   - s/a specific port range/the ephemeral port range/
>   - The paragraph was split in two.

ok

> 
>> +	 * It should be noted that some operations cause binding socket to a random
>> +	 * available port from a specific port range. This can be configured thanks
>> +	 * to the ``/proc/sys/net/ipv4/ip_local_port_range`` sysctl (also used for
>> +	 * IPv6). Following operation requests are automatically translate to
>> +	 * binding on the related port range:
>> +	 *
>> +	 * - A Landlock rule with port 0 and the ``LANDLOCK_ACCESS_NET_BIND_TCP``
>> +	 *   right means that binding on port 0 is allowed.
>> +	 * - A Landlock rule with port 0 and the ``LANDLOCK_ACCESS_NET_LISTEN_TCP``
>> +	 *   right means listening without an explicit binding is allowed.
> 
> There are some grammatical problems in this documentation section.
> 
> Can I suggest an alternative?
> 
>    Some socket operations will fall back to using a port from the ephemeral port
>    range, if no specific port is requested by the caller.  Among others, this
>    happens in the following cases:
> 
>    - :manpage:`bind(2)` is invoked with a socket address that uses port 0.
>    - :manpage:`listen(2)` is invoked on a socket without previously calling
>      :manpage:`bind(2)`.
> 
>    These two actions, which implicitly use an ephemeral port, can be allowed with
>    a Landlock rule with port 0 and the ``LANDLOCK_ACCESS_NET_BIND_TCP`` /
>    ``LANDLOCK_ACCESS_NET_LISTEN_TCP`` right.
> 
>    The ephemeral port range is configured in the
>    ``/proc/sys/net/ipv4/ip_local_port_range`` sysctl (also used for IPv6).

Thanks, lets take this one.

> 
> When we have the documentation wording finalized,
> please send an update to the man pages as well,
> for this and other documentation updates.

Should I send it after this patchset would be accepted?

> 
> Small remarks on what I've done here:
> 
> * I am avoiding the word "binding" when referring to the automatic assignment to
>    an ephemeral port - IMHO, this is potentially confusing, since bind(2) is not
>    explicitly called.
> * I am also dropping the "It should be noted" / "Note that" phrase, which is
>    frowned upon in man pages.

Didn't know that, thanks

> 
>>   	 */
>>   	__u64 port;
>>   };
>> @@ -251,7 +254,7 @@ struct landlock_net_port_attr {
>>    * DOC: net_access
>>    *
>>    * Network flags
>> - * ~~~~~~~~~~~~~~~~
>> + * ~~~~~~~~~~~~~
>>    *
>>    * These flags enable to restrict a sandboxed process to a set of network
>>    * actions. This is supported since the Landlock ABI version 4.
>> @@ -261,9 +264,13 @@ struct landlock_net_port_attr {
>>    * - %LANDLOCK_ACCESS_NET_BIND_TCP: Bind a TCP socket to a local port.
>>    * - %LANDLOCK_ACCESS_NET_CONNECT_TCP: Connect an active TCP socket to
>>    *   a remote port.
>> + * - %LANDLOCK_ACCESS_NET_LISTEN_TCP: Listen for TCP socket connections on
>> + *   a local port. This access right is available since the sixth version
>> + *   of the Landlock ABI.
>>    */
>>   /* clang-format off */
>>   #define LANDLOCK_ACCESS_NET_BIND_TCP			(1ULL << 0)
>>   #define LANDLOCK_ACCESS_NET_CONNECT_TCP			(1ULL << 1)
>> +#define LANDLOCK_ACCESS_NET_LISTEN_TCP			(1ULL << 2)
>>   /* clang-format on */
>>   #endif /* _UAPI_LINUX_LANDLOCK_H */
>> diff --git a/security/landlock/limits.h b/security/landlock/limits.h
>> index 4eb643077a2a..2ef147389474 100644
>> --- a/security/landlock/limits.h
>> +++ b/security/landlock/limits.h
>> @@ -22,7 +22,7 @@
>>   #define LANDLOCK_MASK_ACCESS_FS		((LANDLOCK_LAST_ACCESS_FS << 1) - 1)
>>   #define LANDLOCK_NUM_ACCESS_FS		__const_hweight64(LANDLOCK_MASK_ACCESS_FS)
>>   
>> -#define LANDLOCK_LAST_ACCESS_NET	LANDLOCK_ACCESS_NET_CONNECT_TCP
>> +#define LANDLOCK_LAST_ACCESS_NET	LANDLOCK_ACCESS_NET_LISTEN_TCP
>>   #define LANDLOCK_MASK_ACCESS_NET	((LANDLOCK_LAST_ACCESS_NET << 1) - 1)
>>   #define LANDLOCK_NUM_ACCESS_NET		__const_hweight64(LANDLOCK_MASK_ACCESS_NET)
>>   
>> diff --git a/security/landlock/net.c b/security/landlock/net.c
>> index 669ba260342f..a29cb27c3f14 100644
>> --- a/security/landlock/net.c
>> +++ b/security/landlock/net.c
>> @@ -6,10 +6,12 @@
>>    * Copyright © 2022-2023 Microsoft Corporation
>>    */
>>   
>> +#include "net/sock.h"
>>   #include <linux/in.h>
>>   #include <linux/net.h>
>>   #include <linux/socket.h>
>>   #include <net/ipv6.h>
>> +#include <net/tcp.h>
>>   
>>   #include "common.h"
>>   #include "cred.h"
>> @@ -194,9 +196,97 @@ static int hook_socket_connect(struct socket *const sock,
>>   					   LANDLOCK_ACCESS_NET_CONNECT_TCP);
>>   }
>>   
>> +/*
>> + * Checks that socket state and attributes are correct for listen.
>> + * It is required to not wrongfully return -EACCES instead of -EINVAL.
>        ^^^^^^^^^^^^^^
> 
> Doc nit: I would just document that this function returns -EINVAL on failure?
> In this place, I would expect that the function interface is documented for
> callers.  (From that perspective, this is not a requirement, but a guarantee
> that the function gives.)

Agreed, thanks

> 
>> + *
>> + * This checker requires sock->sk to be locked.
>> + */
>> +static int check_tcp_socket_can_listen(struct socket *const sock)
>> +{
>> +	struct sock *sk = sock->sk;
>> +	unsigned char cur_sk_state = sk->sk_state;
>> +	const struct tcp_ulp_ops *icsk_ulp_ops;
>> +
>> +	/* Allows only unconnected TCP socket to listen (cf. inet_listen). */
>> +	if (sock->state != SS_UNCONNECTED)
>> +		return -EINVAL;
>> +
>> +	/*
>> +	 * Checks sock state. This is needed to ensure consistency with inet stack
>> +	 * error handling (cf. __inet_listen_sk).
>> +	 */
>> +	if (WARN_ON_ONCE(!((1 << cur_sk_state) & (TCPF_CLOSE | TCPF_LISTEN))))
>> +		return -EINVAL;
>> +
>> +	icsk_ulp_ops = inet_csk(sk)->icsk_ulp_ops;
>> +
>> +	/*
>> +	 * ULP (Upper Layer Protocol) stands for protocols which are higher than
>> +	 * transport protocol in OSI model. Linux has an infrastructure that
>> +	 * allows TCP sockets to support logic of some ULP (e.g. TLS ULP).
>> +	 *
>> +	 * Sockets can listen only if ULP control hook has clone method.
>> +	 */
>> +	if (icsk_ulp_ops && !icsk_ulp_ops->clone)
>> +		return -EINVAL;
> 
> This seems like an implementation detail in the networking subsystem?

Yeap. There is probably a missing reference to the corresponding network
stack code (inet_csk_listen_start()). I'll add it.

> 
> If I understand correctly, these are cases where we use TCP on top of protocols
> that are not IP (or have an additional layer in the middle, like TLS?).  This
> can not be recognized through the socket family or type?

ULP can be used in the context of TCP protocols as an additional layer
(currently supported only by IP and MPTCP), so it cannot be recognized
with family or type. You can check this test [1] in which TCP IP socket
is created with ULP control hook.

[1] 
https://lore.kernel.org/all/20240728002602.3198398-8-ivanov.mikhail1@huawei-partners.com/

> 
> Do we have cases where we can run TCP on top of something else than plain IPv4
> or IPv6, where the clone method exists?

Yeah, MPTCP protocol for example (see net/mptcp/subflow.c). ULP control
hook is supported only by IP and MPTCP, and in both cases
clone method is checked during listen(2) execution.

> 
>> +	return 0;
>> +}
>> +
>> +static int hook_socket_listen(struct socket *const sock, const int backlog)
>> +{
>> +	int err = 0;
>> +	int family;
>> +	__be16 port;
>> +	struct sock *sk;
>> +	const struct landlock_ruleset *const dom = get_current_net_domain();
>> +
>> +	if (!dom)
>> +		return 0;
>> +	if (WARN_ON_ONCE(dom->num_layers < 1))
>> +		return -EACCES;
>> +
>> +	/* Checks if it's a (potential) TCP socket. */
>> +	if (sock->type != SOCK_STREAM)
>> +		return 0;
>> +
>> +	sk = sock->sk;
>> +	family = sk->__sk_common.skc_family;
>> +	/*
>> +	 * Socket cannot be assigned AF_UNSPEC because this type is used only
>> +	 * in the context of addresses.
>> +	 *
>> +	 * Doesn't restrict listening for non-TCP sockets.
>> +	 */
>> +	if (family != AF_INET && family != AF_INET6)
>> +		return 0;
> 
> Aren't the socket type and family checks duplicated with existing logic that we
> have for the connect(2) and bind(2) support?  Should it be deduplicated, or is
> that too messy?

bind(2) and connect(2) hooks also support AF_UNSPEC family, so I think
such helper is gonna complicate code a little bit. Also it can
complicate switch in current_check_access_socket().

> 
>> +
>> +	lock_sock(sk);
>> +	/*
>> +	 * Calling listen(2) for a listening socket does nothing with its state and
>> +	 * only changes backlog value (cf. __inet_listen_sk). Checking of listen
>> +	 * access right is not required.
>> +	 */
>> +	if (sk->sk_state == TCP_LISTEN)
>> +		goto release_nocheck;
>> +
>> +	err = check_tcp_socket_can_listen(sock);
>> +	if (unlikely(err))
>> +		goto release_nocheck;
>> +
>> +	port = htons(inet_sk(sk)->inet_num);
>> +	release_sock(sk);
>> +	return check_access_socket(dom, port, LANDLOCK_ACCESS_NET_LISTEN_TCP);
>> +
>> +release_nocheck:
>> +	release_sock(sk);
>> +	return err;
>> +}
> 
> Thanks for sending these patches!
> 
> —Günther

Re: [RFC PATCH v1 2/9] landlock: Support TCP listen access-control

[Günther Noack]

On Wed, Jul 31, 2024 at 08:20:41PM +0300, Mikhail Ivanov wrote:
> 7/30/2024 11:24 AM, Günther Noack wrote:
> > On Sun, Jul 28, 2024 at 08:25:55AM +0800, Mikhail Ivanov wrote:
> > > LANDLOCK_ACCESS_NET_BIND_TCP is useful to limit the scope of "bindable"
> > > ports to forbid a malicious sandboxed process to impersonate a legitimate
> > > server process. However, bind(2) might be used by (TCP) clients to set the
> > > source port to a (legitimate) value. Controlling the ports that can be
> > > used for listening would allow (TCP) clients to explicitly bind to ports
> > > that are forbidden for listening.
> > > 
> > > Such control is implemented with a new LANDLOCK_ACCESS_NET_LISTEN_TCP
> > > access right that restricts listening on undesired ports with listen(2).
> > 
> > Nit: I would turn around the first two commit message paragraphs and describe
> > your changes first, before explaining the problems in the bind(2) support.  I
> > was initially a bit confused that the description started talking about
> > LANDLOCK_ACCESS_NET_BIND_TCP.
> > 
> > General recommendations at:
> > https://www.kernel.org/doc/html/v6.10/process/submitting-patches.html#describe-your-changes
> 
> I consider the first paragraph as a problem statement for this patch.
> According to linux recommendations problem should be established before
> the description of changes. Do you think that the changes part should
> stand before the problem anyway?

Up to you. To be fair, I'm sold on the approach in this patchset anyway :)


> > When we have the documentation wording finalized,
> > please send an update to the man pages as well,
> > for this and other documentation updates.
> 
> Should I send it after this patchset would be accepted?

Yes, that would be the normal process which we have been following so far.

(I don't like the process much either, because it decouples feature development
so far from documentation writing, but it's what we have for now.)

An example patch which does that for the network bind(2) and connect(2) features
(and where I would still like a review from Konstantin) is:
https://lore.kernel.org/all/20240723101917.90918-1-gnoack@google.com/


> > Small remarks on what I've done here:
> > 
> > * I am avoiding the word "binding" when referring to the automatic assignment to
> >    an ephemeral port - IMHO, this is potentially confusing, since bind(2) is not
> >    explicitly called.
> > * I am also dropping the "It should be noted" / "Note that" phrase, which is
> >    frowned upon in man pages.
> 
> Didn't know that, thanks

Regarding "note that", see
https://lore.kernel.org/all/0aafcdd6-4ac7-8501-c607-9a24a98597d7@gmail.com/
https://lore.kernel.org/linux-man/20210729223535.qvyomfqvvahzmu5w@localhost.localdomain/
https://lore.kernel.org/linux-man/20230105225235.6cjtz6orjzxzvo6v@illithid/
(The "Kemper notectomy")

This came up in man page reviews, but we'll have an easier time keeping the
kernel and man page documentation in sync if we adhere to man page style
directly.  (The man page style is documented in man-pages(7) and contains some
groff-independent wording advice as well.)


> > If I understand correctly, these are cases where we use TCP on top of protocols
> > that are not IP (or have an additional layer in the middle, like TLS?).  This
> > can not be recognized through the socket family or type?
> 
> ULP can be used in the context of TCP protocols as an additional layer
> (currently supported only by IP and MPTCP), so it cannot be recognized
> with family or type. You can check this test [1] in which TCP IP socket
> is created with ULP control hook.
> 
> [1] https://lore.kernel.org/all/20240728002602.3198398-8-ivanov.mikhail1@huawei-partners.com/

Thanks, this is helpful.

For reference, it seems that ULP were introduced in
https://lore.kernel.org/all/20170614183714.GA80310@davejwatson-mba.dhcp.thefacebook.com/


> > Do we have cases where we can run TCP on top of something else than plain IPv4
> > or IPv6, where the clone method exists?
> 
> Yeah, MPTCP protocol for example (see net/mptcp/subflow.c). ULP control
> hook is supported only by IP and MPTCP, and in both cases
> clone method is checked during listen(2) execution.


> > Aren't the socket type and family checks duplicated with existing logic that we
> > have for the connect(2) and bind(2) support?  Should it be deduplicated, or is
> > that too messy?
> 
> bind(2) and connect(2) hooks also support AF_UNSPEC family, so I think
> such helper is gonna complicate code a little bit. Also it can
> complicate switch in current_check_access_socket().

OK, sounds good. 👍

—Günther

Re: [RFC PATCH v1 2/9] landlock: Support TCP listen access-control

[Mikhail Ivanov]

8/1/2024 1:36 PM, Günther Noack wrote:
> On Wed, Jul 31, 2024 at 08:20:41PM +0300, Mikhail Ivanov wrote:
>> 7/30/2024 11:24 AM, Günther Noack wrote:
>>> On Sun, Jul 28, 2024 at 08:25:55AM +0800, Mikhail Ivanov wrote:
>>>> LANDLOCK_ACCESS_NET_BIND_TCP is useful to limit the scope of "bindable"
>>>> ports to forbid a malicious sandboxed process to impersonate a legitimate
>>>> server process. However, bind(2) might be used by (TCP) clients to set the
>>>> source port to a (legitimate) value. Controlling the ports that can be
>>>> used for listening would allow (TCP) clients to explicitly bind to ports
>>>> that are forbidden for listening.
>>>>
>>>> Such control is implemented with a new LANDLOCK_ACCESS_NET_LISTEN_TCP
>>>> access right that restricts listening on undesired ports with listen(2).
>>>
>>> Nit: I would turn around the first two commit message paragraphs and describe
>>> your changes first, before explaining the problems in the bind(2) support.  I
>>> was initially a bit confused that the description started talking about
>>> LANDLOCK_ACCESS_NET_BIND_TCP.
>>>
>>> General recommendations at:
>>> https://www.kernel.org/doc/html/v6.10/process/submitting-patches.html#describe-your-changes
>>
>> I consider the first paragraph as a problem statement for this patch.
>> According to linux recommendations problem should be established before
>> the description of changes. Do you think that the changes part should
>> stand before the problem anyway?
> 
> Up to you. To be fair, I'm sold on the approach in this patchset anyway :)

Nice :)

> 
> 
>>> When we have the documentation wording finalized,
>>> please send an update to the man pages as well,
>>> for this and other documentation updates.
>>
>> Should I send it after this patchset would be accepted?
> 
> Yes, that would be the normal process which we have been following so far.
> 
> (I don't like the process much either, because it decouples feature development
> so far from documentation writing, but it's what we have for now.)
> 
> An example patch which does that for the network bind(2) and connect(2) features
> (and where I would still like a review from Konstantin) is:
> https://lore.kernel.org/all/20240723101917.90918-1-gnoack@google.com/

got it

> 
> 
>>> Small remarks on what I've done here:
>>>
>>> * I am avoiding the word "binding" when referring to the automatic assignment to
>>>     an ephemeral port - IMHO, this is potentially confusing, since bind(2) is not
>>>     explicitly called.
>>> * I am also dropping the "It should be noted" / "Note that" phrase, which is
>>>     frowned upon in man pages.
>>
>> Didn't know that, thanks
> 
> Regarding "note that", see
> https://lore.kernel.org/all/0aafcdd6-4ac7-8501-c607-9a24a98597d7@gmail.com/
> https://lore.kernel.org/linux-man/20210729223535.qvyomfqvvahzmu5w@localhost.localdomain/
> https://lore.kernel.org/linux-man/20230105225235.6cjtz6orjzxzvo6v@illithid/
> (The "Kemper notectomy")
> 
> This came up in man page reviews, but we'll have an easier time keeping the
> kernel and man page documentation in sync if we adhere to man page style
> directly.  (The man page style is documented in man-pages(7) and contains some
> groff-independent wording advice as well.)

Ok, such phrases should be really avoided in kernel as well.

> 
> 
>>> If I understand correctly, these are cases where we use TCP on top of protocols
>>> that are not IP (or have an additional layer in the middle, like TLS?).  This
>>> can not be recognized through the socket family or type?
>>
>> ULP can be used in the context of TCP protocols as an additional layer
>> (currently supported only by IP and MPTCP), so it cannot be recognized
>> with family or type. You can check this test [1] in which TCP IP socket
>> is created with ULP control hook.
>>
>> [1] https://lore.kernel.org/all/20240728002602.3198398-8-ivanov.mikhail1@huawei-partners.com/
> 
> Thanks, this is helpful.
> 
> For reference, it seems that ULP were introduced in
> https://lore.kernel.org/all/20170614183714.GA80310@davejwatson-mba.dhcp.thefacebook.com/
> 
> 
>>> Do we have cases where we can run TCP on top of something else than plain IPv4
>>> or IPv6, where the clone method exists?
>>
>> Yeah, MPTCP protocol for example (see net/mptcp/subflow.c). ULP control
>> hook is supported only by IP and MPTCP, and in both cases
>> clone method is checked during listen(2) execution.
> 
> 
>>> Aren't the socket type and family checks duplicated with existing logic that we
>>> have for the connect(2) and bind(2) support?  Should it be deduplicated, or is
>>> that too messy?
>>
>> bind(2) and connect(2) hooks also support AF_UNSPEC family, so I think
>> such helper is gonna complicate code a little bit. Also it can
>> complicate switch in current_check_access_socket().
> 
> OK, sounds good. 👍
> 
> —Günther

Re: [RFC PATCH v1 2/9] landlock: Support TCP listen access-control

[Mickaël Salaün]

On Sun, Jul 28, 2024 at 08:25:55AM +0800, Mikhail Ivanov wrote:
> LANDLOCK_ACCESS_NET_BIND_TCP is useful to limit the scope of "bindable"
> ports to forbid a malicious sandboxed process to impersonate a legitimate
> server process. However, bind(2) might be used by (TCP) clients to set the
> source port to a (legitimate) value. Controlling the ports that can be
> used for listening would allow (TCP) clients to explicitly bind to ports
> that are forbidden for listening.
> 
> Such control is implemented with a new LANDLOCK_ACCESS_NET_LISTEN_TCP
> access right that restricts listening on undesired ports with listen(2).
> 
> It's worth noticing that this access right doesn't affect changing
> backlog value using listen(2) on already listening socket.
> 
> * Create new LANDLOCK_ACCESS_NET_LISTEN_TCP flag.
> * Add hook to socket_listen(), which checks whether the socket is allowed
>   to listen on a binded local port.
> * Add check_tcp_socket_can_listen() helper, which validates socket
>   attributes before the actual access right check.
> * Update `struct landlock_net_port_attr` documentation with control of
>   binding to ephemeral port with listen(2) description.
> * Change ABI version to 6.
> 
> Closes: https://github.com/landlock-lsm/linux/issues/15
> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>

Thanks for this series!

I cannot apply this patch series though, could you please provide the
base commit?  BTW, this can be automatically put in the cover letter
with the git format-patch's --base argument.

> ---
>  include/uapi/linux/landlock.h                | 23 +++--
>  security/landlock/limits.h                   |  2 +-
>  security/landlock/net.c                      | 90 ++++++++++++++++++++
>  security/landlock/syscalls.c                 |  2 +-
>  tools/testing/selftests/landlock/base_test.c |  2 +-
>  5 files changed, 108 insertions(+), 11 deletions(-)
> 
> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> index 68625e728f43..6b8df3293eee 100644
> --- a/include/uapi/linux/landlock.h
> +++ b/include/uapi/linux/landlock.h
> @@ -104,13 +104,16 @@ struct landlock_net_port_attr {
>  	/**
>  	 * @port: Network port in host endianness.
>  	 *
> -	 * It should be noted that port 0 passed to :manpage:`bind(2)` will
> -	 * bind to an available port from a specific port range. This can be
> -	 * configured thanks to the ``/proc/sys/net/ipv4/ip_local_port_range``
> -	 * sysctl (also used for IPv6). A Landlock rule with port 0 and the
> -	 * ``LANDLOCK_ACCESS_NET_BIND_TCP`` right means that requesting to bind
> -	 * on port 0 is allowed and it will automatically translate to binding
> -	 * on the related port range.
> +	 * It should be noted that some operations cause binding socket to a random
> +	 * available port from a specific port range. This can be configured thanks
> +	 * to the ``/proc/sys/net/ipv4/ip_local_port_range`` sysctl (also used for
> +	 * IPv6). Following operation requests are automatically translate to
> +	 * binding on the related port range:
> +	 *
> +	 * - A Landlock rule with port 0 and the ``LANDLOCK_ACCESS_NET_BIND_TCP``
> +	 *   right means that binding on port 0 is allowed.
> +	 * - A Landlock rule with port 0 and the ``LANDLOCK_ACCESS_NET_LISTEN_TCP``
> +	 *   right means listening without an explicit binding is allowed.
>  	 */
>  	__u64 port;
>  };
> @@ -251,7 +254,7 @@ struct landlock_net_port_attr {
>   * DOC: net_access
>   *
>   * Network flags
> - * ~~~~~~~~~~~~~~~~
> + * ~~~~~~~~~~~~~
>   *
>   * These flags enable to restrict a sandboxed process to a set of network
>   * actions. This is supported since the Landlock ABI version 4.
> @@ -261,9 +264,13 @@ struct landlock_net_port_attr {
>   * - %LANDLOCK_ACCESS_NET_BIND_TCP: Bind a TCP socket to a local port.
>   * - %LANDLOCK_ACCESS_NET_CONNECT_TCP: Connect an active TCP socket to
>   *   a remote port.
> + * - %LANDLOCK_ACCESS_NET_LISTEN_TCP: Listen for TCP socket connections on
> + *   a local port. This access right is available since the sixth version
> + *   of the Landlock ABI.
>   */
>  /* clang-format off */
>  #define LANDLOCK_ACCESS_NET_BIND_TCP			(1ULL << 0)
>  #define LANDLOCK_ACCESS_NET_CONNECT_TCP			(1ULL << 1)
> +#define LANDLOCK_ACCESS_NET_LISTEN_TCP			(1ULL << 2)
>  /* clang-format on */
>  #endif /* _UAPI_LINUX_LANDLOCK_H */
> diff --git a/security/landlock/limits.h b/security/landlock/limits.h
> index 4eb643077a2a..2ef147389474 100644
> --- a/security/landlock/limits.h
> +++ b/security/landlock/limits.h
> @@ -22,7 +22,7 @@
>  #define LANDLOCK_MASK_ACCESS_FS		((LANDLOCK_LAST_ACCESS_FS << 1) - 1)
>  #define LANDLOCK_NUM_ACCESS_FS		__const_hweight64(LANDLOCK_MASK_ACCESS_FS)
>  
> -#define LANDLOCK_LAST_ACCESS_NET	LANDLOCK_ACCESS_NET_CONNECT_TCP
> +#define LANDLOCK_LAST_ACCESS_NET	LANDLOCK_ACCESS_NET_LISTEN_TCP
>  #define LANDLOCK_MASK_ACCESS_NET	((LANDLOCK_LAST_ACCESS_NET << 1) - 1)
>  #define LANDLOCK_NUM_ACCESS_NET		__const_hweight64(LANDLOCK_MASK_ACCESS_NET)
>  
> diff --git a/security/landlock/net.c b/security/landlock/net.c
> index 669ba260342f..a29cb27c3f14 100644
> --- a/security/landlock/net.c
> +++ b/security/landlock/net.c
> @@ -6,10 +6,12 @@
>   * Copyright © 2022-2023 Microsoft Corporation
>   */
>  
> +#include "net/sock.h"

These should not be quotes.

>  #include <linux/in.h>
>  #include <linux/net.h>
>  #include <linux/socket.h>
>  #include <net/ipv6.h>
> +#include <net/tcp.h>
>  
>  #include "common.h"
>  #include "cred.h"
> @@ -194,9 +196,97 @@ static int hook_socket_connect(struct socket *const sock,
>  					   LANDLOCK_ACCESS_NET_CONNECT_TCP);
>  }
>  
> +/*
> + * Checks that socket state and attributes are correct for listen.
> + * It is required to not wrongfully return -EACCES instead of -EINVAL.
> + *
> + * This checker requires sock->sk to be locked.
> + */
> +static int check_tcp_socket_can_listen(struct socket *const sock)

Is this function still useful with the listen LSM hook?

> +{
> +	struct sock *sk = sock->sk;
> +	unsigned char cur_sk_state = sk->sk_state;
> +	const struct tcp_ulp_ops *icsk_ulp_ops;
> +
> +	/* Allows only unconnected TCP socket to listen (cf. inet_listen). */
> +	if (sock->state != SS_UNCONNECTED)
> +		return -EINVAL;
> +
> +	/*
> +	 * Checks sock state. This is needed to ensure consistency with inet stack
> +	 * error handling (cf. __inet_listen_sk).
> +	 */
> +	if (WARN_ON_ONCE(!((1 << cur_sk_state) & (TCPF_CLOSE | TCPF_LISTEN))))
> +		return -EINVAL;
> +
> +	icsk_ulp_ops = inet_csk(sk)->icsk_ulp_ops;
> +
> +	/*
> +	 * ULP (Upper Layer Protocol) stands for protocols which are higher than
> +	 * transport protocol in OSI model. Linux has an infrastructure that
> +	 * allows TCP sockets to support logic of some ULP (e.g. TLS ULP).
> +	 *
> +	 * Sockets can listen only if ULP control hook has clone method.
> +	 */
> +	if (icsk_ulp_ops && !icsk_ulp_ops->clone)
> +		return -EINVAL;
> +	return 0;
> +}
> +
> +static int hook_socket_listen(struct socket *const sock, const int backlog)
> +{

Why can't we just call current_check_access_socket()?

> +	int err = 0;
> +	int family;
> +	__be16 port;
> +	struct sock *sk;
> +	const struct landlock_ruleset *const dom = get_current_net_domain();
> +
> +	if (!dom)
> +		return 0;
> +	if (WARN_ON_ONCE(dom->num_layers < 1))
> +		return -EACCES;
> +
> +	/* Checks if it's a (potential) TCP socket. */
> +	if (sock->type != SOCK_STREAM)
> +		return 0;
> +
> +	sk = sock->sk;
> +	family = sk->__sk_common.skc_family;
> +	/*
> +	 * Socket cannot be assigned AF_UNSPEC because this type is used only
> +	 * in the context of addresses.
> +	 *
> +	 * Doesn't restrict listening for non-TCP sockets.
> +	 */
> +	if (family != AF_INET && family != AF_INET6)
> +		return 0;
> +
> +	lock_sock(sk);
> +	/*
> +	 * Calling listen(2) for a listening socket does nothing with its state and
> +	 * only changes backlog value (cf. __inet_listen_sk). Checking of listen
> +	 * access right is not required.
> +	 */
> +	if (sk->sk_state == TCP_LISTEN)
> +		goto release_nocheck;
> +
> +	err = check_tcp_socket_can_listen(sock);
> +	if (unlikely(err))
> +		goto release_nocheck;
> +
> +	port = htons(inet_sk(sk)->inet_num);
> +	release_sock(sk);
> +	return check_access_socket(dom, port, LANDLOCK_ACCESS_NET_LISTEN_TCP);
> +
> +release_nocheck:
> +	release_sock(sk);
> +	return err;
> +}
> +
>  static struct security_hook_list landlock_hooks[] __ro_after_init = {
>  	LSM_HOOK_INIT(socket_bind, hook_socket_bind),
>  	LSM_HOOK_INIT(socket_connect, hook_socket_connect),
> +	LSM_HOOK_INIT(socket_listen, hook_socket_listen),
>  };
>  
>  __init void landlock_add_net_hooks(void)
> diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
> index 03b470f5a85a..3752bcc033d4 100644
> --- a/security/landlock/syscalls.c
> +++ b/security/landlock/syscalls.c
> @@ -149,7 +149,7 @@ static const struct file_operations ruleset_fops = {
>  	.write = fop_dummy_write,
>  };
>  
> -#define LANDLOCK_ABI_VERSION 5
> +#define LANDLOCK_ABI_VERSION 6
>  
>  /**
>   * sys_landlock_create_ruleset - Create a new ruleset
> diff --git a/tools/testing/selftests/landlock/base_test.c b/tools/testing/selftests/landlock/base_test.c
> index 3c1e9f35b531..52b00472a487 100644
> --- a/tools/testing/selftests/landlock/base_test.c
> +++ b/tools/testing/selftests/landlock/base_test.c
> @@ -75,7 +75,7 @@ TEST(abi_version)
>  	const struct landlock_ruleset_attr ruleset_attr = {
>  		.handled_access_fs = LANDLOCK_ACCESS_FS_READ_FILE,
>  	};
> -	ASSERT_EQ(5, landlock_create_ruleset(NULL, 0,
> +	ASSERT_EQ(6, landlock_create_ruleset(NULL, 0,
>  					     LANDLOCK_CREATE_RULESET_VERSION));
>  
>  	ASSERT_EQ(-1, landlock_create_ruleset(&ruleset_attr, 0,
> -- 
> 2.34.1
> 
> 

Re: [RFC PATCH v1 2/9] landlock: Support TCP listen access-control

[Mikhail Ivanov]

7/31/2024 9:30 PM, Mickaël Salaün wrote:
> On Sun, Jul 28, 2024 at 08:25:55AM +0800, Mikhail Ivanov wrote:
>> LANDLOCK_ACCESS_NET_BIND_TCP is useful to limit the scope of "bindable"
>> ports to forbid a malicious sandboxed process to impersonate a legitimate
>> server process. However, bind(2) might be used by (TCP) clients to set the
>> source port to a (legitimate) value. Controlling the ports that can be
>> used for listening would allow (TCP) clients to explicitly bind to ports
>> that are forbidden for listening.
>>
>> Such control is implemented with a new LANDLOCK_ACCESS_NET_LISTEN_TCP
>> access right that restricts listening on undesired ports with listen(2).
>>
>> It's worth noticing that this access right doesn't affect changing
>> backlog value using listen(2) on already listening socket.
>>
>> * Create new LANDLOCK_ACCESS_NET_LISTEN_TCP flag.
>> * Add hook to socket_listen(), which checks whether the socket is allowed
>>    to listen on a binded local port.
>> * Add check_tcp_socket_can_listen() helper, which validates socket
>>    attributes before the actual access right check.
>> * Update `struct landlock_net_port_attr` documentation with control of
>>    binding to ephemeral port with listen(2) description.
>> * Change ABI version to 6.
>>
>> Closes: https://github.com/landlock-lsm/linux/issues/15
>> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
> 
> Thanks for this series!
> 
> I cannot apply this patch series though, could you please provide the
> base commit?  BTW, this can be automatically put in the cover letter
> with the git format-patch's --base argument.

base-commit: 591561c2b47b7e7225e229e844f5de75ce0c09ec

Günther said that I should rebase to the latest commits, so I'll do
it in the next version of this patchset.

> 
>> ---
>>   include/uapi/linux/landlock.h                | 23 +++--
>>   security/landlock/limits.h                   |  2 +-
>>   security/landlock/net.c                      | 90 ++++++++++++++++++++
>>   security/landlock/syscalls.c                 |  2 +-
>>   tools/testing/selftests/landlock/base_test.c |  2 +-
>>   5 files changed, 108 insertions(+), 11 deletions(-)
>>
>> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
>> index 68625e728f43..6b8df3293eee 100644
>> --- a/include/uapi/linux/landlock.h
>> +++ b/include/uapi/linux/landlock.h
>> @@ -104,13 +104,16 @@ struct landlock_net_port_attr {
>>   	/**
>>   	 * @port: Network port in host endianness.
>>   	 *
>> -	 * It should be noted that port 0 passed to :manpage:`bind(2)` will
>> -	 * bind to an available port from a specific port range. This can be
>> -	 * configured thanks to the ``/proc/sys/net/ipv4/ip_local_port_range``
>> -	 * sysctl (also used for IPv6). A Landlock rule with port 0 and the
>> -	 * ``LANDLOCK_ACCESS_NET_BIND_TCP`` right means that requesting to bind
>> -	 * on port 0 is allowed and it will automatically translate to binding
>> -	 * on the related port range.
>> +	 * It should be noted that some operations cause binding socket to a random
>> +	 * available port from a specific port range. This can be configured thanks
>> +	 * to the ``/proc/sys/net/ipv4/ip_local_port_range`` sysctl (also used for
>> +	 * IPv6). Following operation requests are automatically translate to
>> +	 * binding on the related port range:
>> +	 *
>> +	 * - A Landlock rule with port 0 and the ``LANDLOCK_ACCESS_NET_BIND_TCP``
>> +	 *   right means that binding on port 0 is allowed.
>> +	 * - A Landlock rule with port 0 and the ``LANDLOCK_ACCESS_NET_LISTEN_TCP``
>> +	 *   right means listening without an explicit binding is allowed.
>>   	 */
>>   	__u64 port;
>>   };
>> @@ -251,7 +254,7 @@ struct landlock_net_port_attr {
>>    * DOC: net_access
>>    *
>>    * Network flags
>> - * ~~~~~~~~~~~~~~~~
>> + * ~~~~~~~~~~~~~
>>    *
>>    * These flags enable to restrict a sandboxed process to a set of network
>>    * actions. This is supported since the Landlock ABI version 4.
>> @@ -261,9 +264,13 @@ struct landlock_net_port_attr {
>>    * - %LANDLOCK_ACCESS_NET_BIND_TCP: Bind a TCP socket to a local port.
>>    * - %LANDLOCK_ACCESS_NET_CONNECT_TCP: Connect an active TCP socket to
>>    *   a remote port.
>> + * - %LANDLOCK_ACCESS_NET_LISTEN_TCP: Listen for TCP socket connections on
>> + *   a local port. This access right is available since the sixth version
>> + *   of the Landlock ABI.
>>    */
>>   /* clang-format off */
>>   #define LANDLOCK_ACCESS_NET_BIND_TCP			(1ULL << 0)
>>   #define LANDLOCK_ACCESS_NET_CONNECT_TCP			(1ULL << 1)
>> +#define LANDLOCK_ACCESS_NET_LISTEN_TCP			(1ULL << 2)
>>   /* clang-format on */
>>   #endif /* _UAPI_LINUX_LANDLOCK_H */
>> diff --git a/security/landlock/limits.h b/security/landlock/limits.h
>> index 4eb643077a2a..2ef147389474 100644
>> --- a/security/landlock/limits.h
>> +++ b/security/landlock/limits.h
>> @@ -22,7 +22,7 @@
>>   #define LANDLOCK_MASK_ACCESS_FS		((LANDLOCK_LAST_ACCESS_FS << 1) - 1)
>>   #define LANDLOCK_NUM_ACCESS_FS		__const_hweight64(LANDLOCK_MASK_ACCESS_FS)
>>   
>> -#define LANDLOCK_LAST_ACCESS_NET	LANDLOCK_ACCESS_NET_CONNECT_TCP
>> +#define LANDLOCK_LAST_ACCESS_NET	LANDLOCK_ACCESS_NET_LISTEN_TCP
>>   #define LANDLOCK_MASK_ACCESS_NET	((LANDLOCK_LAST_ACCESS_NET << 1) - 1)
>>   #define LANDLOCK_NUM_ACCESS_NET		__const_hweight64(LANDLOCK_MASK_ACCESS_NET)
>>   
>> diff --git a/security/landlock/net.c b/security/landlock/net.c
>> index 669ba260342f..a29cb27c3f14 100644
>> --- a/security/landlock/net.c
>> +++ b/security/landlock/net.c
>> @@ -6,10 +6,12 @@
>>    * Copyright © 2022-2023 Microsoft Corporation
>>    */
>>   
>> +#include "net/sock.h"
> 
> These should not be quotes.

will be fixed, thanks

> 
>>   #include <linux/in.h>
>>   #include <linux/net.h>
>>   #include <linux/socket.h>
>>   #include <net/ipv6.h>
>> +#include <net/tcp.h>
>>   
>>   #include "common.h"
>>   #include "cred.h"
>> @@ -194,9 +196,97 @@ static int hook_socket_connect(struct socket *const sock,
>>   					   LANDLOCK_ACCESS_NET_CONNECT_TCP);
>>   }
>>   
>> +/*
>> + * Checks that socket state and attributes are correct for listen.
>> + * It is required to not wrongfully return -EACCES instead of -EINVAL.
>> + *
>> + * This checker requires sock->sk to be locked.
>> + */
>> +static int check_tcp_socket_can_listen(struct socket *const sock)
> 
> Is this function still useful with the listen LSM hook?

Yeap, we need to validate socket structure before checking the access
right. You can see [1] and [2] where the behavior of this function is
tested.

[1] 
https://lore.kernel.org/all/20240728002602.3198398-6-ivanov.mikhail1@huawei-partners.com/
[2] 
https://lore.kernel.org/all/20240728002602.3198398-8-ivanov.mikhail1@huawei-partners.com/

> 
>> +{
>> +	struct sock *sk = sock->sk;
>> +	unsigned char cur_sk_state = sk->sk_state;
>> +	const struct tcp_ulp_ops *icsk_ulp_ops;
>> +
>> +	/* Allows only unconnected TCP socket to listen (cf. inet_listen). */
>> +	if (sock->state != SS_UNCONNECTED)
>> +		return -EINVAL;
>> +
>> +	/*
>> +	 * Checks sock state. This is needed to ensure consistency with inet stack
>> +	 * error handling (cf. __inet_listen_sk).
>> +	 */
>> +	if (WARN_ON_ONCE(!((1 << cur_sk_state) & (TCPF_CLOSE | TCPF_LISTEN))))
>> +		return -EINVAL;
>> +
>> +	icsk_ulp_ops = inet_csk(sk)->icsk_ulp_ops;
>> +
>> +	/*
>> +	 * ULP (Upper Layer Protocol) stands for protocols which are higher than
>> +	 * transport protocol in OSI model. Linux has an infrastructure that
>> +	 * allows TCP sockets to support logic of some ULP (e.g. TLS ULP).
>> +	 *
>> +	 * Sockets can listen only if ULP control hook has clone method.
>> +	 */
>> +	if (icsk_ulp_ops && !icsk_ulp_ops->clone)
>> +		return -EINVAL;
>> +	return 0;
>> +}
>> +
>> +static int hook_socket_listen(struct socket *const sock, const int backlog)
>> +{
> 
> Why can't we just call current_check_access_socket()?

I've mentioned in the message of the previous commit that this method
has address checks for bind(2) and connect(2). In the case of listen(2)
port is extracted from the socket structure, so calling
current_check_access_socket() would be pointless.

> 
>> +	int err = 0;
>> +	int family;
>> +	__be16 port;
>> +	struct sock *sk;
>> +	const struct landlock_ruleset *const dom = get_current_net_domain();
>> +
>> +	if (!dom)
>> +		return 0;
>> +	if (WARN_ON_ONCE(dom->num_layers < 1))
>> +		return -EACCES;
>> +
>> +	/* Checks if it's a (potential) TCP socket. */
>> +	if (sock->type != SOCK_STREAM)
>> +		return 0;
>> +
>> +	sk = sock->sk;
>> +	family = sk->__sk_common.skc_family;
>> +	/*
>> +	 * Socket cannot be assigned AF_UNSPEC because this type is used only
>> +	 * in the context of addresses.
>> +	 *
>> +	 * Doesn't restrict listening for non-TCP sockets.
>> +	 */
>> +	if (family != AF_INET && family != AF_INET6)
>> +		return 0;
>> +
>> +	lock_sock(sk);
>> +	/*
>> +	 * Calling listen(2) for a listening socket does nothing with its state and
>> +	 * only changes backlog value (cf. __inet_listen_sk). Checking of listen
>> +	 * access right is not required.
>> +	 */
>> +	if (sk->sk_state == TCP_LISTEN)
>> +		goto release_nocheck;
>> +
>> +	err = check_tcp_socket_can_listen(sock);
>> +	if (unlikely(err))
>> +		goto release_nocheck;
>> +
>> +	port = htons(inet_sk(sk)->inet_num);
>> +	release_sock(sk);
>> +	return check_access_socket(dom, port, LANDLOCK_ACCESS_NET_LISTEN_TCP);
>> +
>> +release_nocheck:
>> +	release_sock(sk);
>> +	return err;
>> +}
>> +
>>   static struct security_hook_list landlock_hooks[] __ro_after_init = {
>>   	LSM_HOOK_INIT(socket_bind, hook_socket_bind),
>>   	LSM_HOOK_INIT(socket_connect, hook_socket_connect),
>> +	LSM_HOOK_INIT(socket_listen, hook_socket_listen),
>>   };
>>   
>>   __init void landlock_add_net_hooks(void)
>> diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
>> index 03b470f5a85a..3752bcc033d4 100644
>> --- a/security/landlock/syscalls.c
>> +++ b/security/landlock/syscalls.c
>> @@ -149,7 +149,7 @@ static const struct file_operations ruleset_fops = {
>>   	.write = fop_dummy_write,
>>   };
>>   
>> -#define LANDLOCK_ABI_VERSION 5
>> +#define LANDLOCK_ABI_VERSION 6
>>   
>>   /**
>>    * sys_landlock_create_ruleset - Create a new ruleset
>> diff --git a/tools/testing/selftests/landlock/base_test.c b/tools/testing/selftests/landlock/base_test.c
>> index 3c1e9f35b531..52b00472a487 100644
>> --- a/tools/testing/selftests/landlock/base_test.c
>> +++ b/tools/testing/selftests/landlock/base_test.c
>> @@ -75,7 +75,7 @@ TEST(abi_version)
>>   	const struct landlock_ruleset_attr ruleset_attr = {
>>   		.handled_access_fs = LANDLOCK_ACCESS_FS_READ_FILE,
>>   	};
>> -	ASSERT_EQ(5, landlock_create_ruleset(NULL, 0,
>> +	ASSERT_EQ(6, landlock_create_ruleset(NULL, 0,
>>   					     LANDLOCK_CREATE_RULESET_VERSION));
>>   
>>   	ASSERT_EQ(-1, landlock_create_ruleset(&ruleset_attr, 0,
>> -- 
>> 2.34.1
>>
>>

Re: [RFC PATCH v1 2/9] landlock: Support TCP listen access-control

[Mickaël Salaün]

On Thu, Aug 01, 2024 at 10:52:25AM +0300, Mikhail Ivanov wrote:
> 7/31/2024 9:30 PM, Mickaël Salaün wrote:
> > On Sun, Jul 28, 2024 at 08:25:55AM +0800, Mikhail Ivanov wrote:
> > > LANDLOCK_ACCESS_NET_BIND_TCP is useful to limit the scope of "bindable"
> > > ports to forbid a malicious sandboxed process to impersonate a legitimate
> > > server process. However, bind(2) might be used by (TCP) clients to set the
> > > source port to a (legitimate) value. Controlling the ports that can be
> > > used for listening would allow (TCP) clients to explicitly bind to ports
> > > that are forbidden for listening.
> > > 
> > > Such control is implemented with a new LANDLOCK_ACCESS_NET_LISTEN_TCP
> > > access right that restricts listening on undesired ports with listen(2).
> > > 
> > > It's worth noticing that this access right doesn't affect changing
> > > backlog value using listen(2) on already listening socket.
> > > 
> > > * Create new LANDLOCK_ACCESS_NET_LISTEN_TCP flag.
> > > * Add hook to socket_listen(), which checks whether the socket is allowed
> > >    to listen on a binded local port.
> > > * Add check_tcp_socket_can_listen() helper, which validates socket
> > >    attributes before the actual access right check.
> > > * Update `struct landlock_net_port_attr` documentation with control of
> > >    binding to ephemeral port with listen(2) description.
> > > * Change ABI version to 6.
> > > 
> > > Closes: https://github.com/landlock-lsm/linux/issues/15
> > > Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
> > 
> > Thanks for this series!
> > 
> > I cannot apply this patch series though, could you please provide the
> > base commit?  BTW, this can be automatically put in the cover letter
> > with the git format-patch's --base argument.
> 
> base-commit: 591561c2b47b7e7225e229e844f5de75ce0c09ec

Thanks, the following commit makes this series to not apply.

> 
> Günther said that I should rebase to the latest commits, so I'll do
> it in the next version of this patchset.

Yep, currently we're on v6.11-rc1, but please specify the base commit
each time.

> 
> > 
> > > ---
> > >   include/uapi/linux/landlock.h                | 23 +++--
> > >   security/landlock/limits.h                   |  2 +-
> > >   security/landlock/net.c                      | 90 ++++++++++++++++++++
> > >   security/landlock/syscalls.c                 |  2 +-
> > >   tools/testing/selftests/landlock/base_test.c |  2 +-
> > >   5 files changed, 108 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
> > > index 68625e728f43..6b8df3293eee 100644
> > > --- a/include/uapi/linux/landlock.h
> > > +++ b/include/uapi/linux/landlock.h
> > > @@ -104,13 +104,16 @@ struct landlock_net_port_attr {
> > >   	/**
> > >   	 * @port: Network port in host endianness.
> > >   	 *
> > > -	 * It should be noted that port 0 passed to :manpage:`bind(2)` will
> > > -	 * bind to an available port from a specific port range. This can be
> > > -	 * configured thanks to the ``/proc/sys/net/ipv4/ip_local_port_range``
> > > -	 * sysctl (also used for IPv6). A Landlock rule with port 0 and the
> > > -	 * ``LANDLOCK_ACCESS_NET_BIND_TCP`` right means that requesting to bind
> > > -	 * on port 0 is allowed and it will automatically translate to binding
> > > -	 * on the related port range.
> > > +	 * It should be noted that some operations cause binding socket to a random
> > > +	 * available port from a specific port range. This can be configured thanks
> > > +	 * to the ``/proc/sys/net/ipv4/ip_local_port_range`` sysctl (also used for
> > > +	 * IPv6). Following operation requests are automatically translate to
> > > +	 * binding on the related port range:
> > > +	 *
> > > +	 * - A Landlock rule with port 0 and the ``LANDLOCK_ACCESS_NET_BIND_TCP``
> > > +	 *   right means that binding on port 0 is allowed.
> > > +	 * - A Landlock rule with port 0 and the ``LANDLOCK_ACCESS_NET_LISTEN_TCP``
> > > +	 *   right means listening without an explicit binding is allowed.
> > >   	 */
> > >   	__u64 port;
> > >   };
> > > @@ -251,7 +254,7 @@ struct landlock_net_port_attr {
> > >    * DOC: net_access
> > >    *
> > >    * Network flags
> > > - * ~~~~~~~~~~~~~~~~
> > > + * ~~~~~~~~~~~~~
> > >    *
> > >    * These flags enable to restrict a sandboxed process to a set of network
> > >    * actions. This is supported since the Landlock ABI version 4.
> > > @@ -261,9 +264,13 @@ struct landlock_net_port_attr {
> > >    * - %LANDLOCK_ACCESS_NET_BIND_TCP: Bind a TCP socket to a local port.
> > >    * - %LANDLOCK_ACCESS_NET_CONNECT_TCP: Connect an active TCP socket to
> > >    *   a remote port.
> > > + * - %LANDLOCK_ACCESS_NET_LISTEN_TCP: Listen for TCP socket connections on
> > > + *   a local port. This access right is available since the sixth version
> > > + *   of the Landlock ABI.
> > >    */
> > >   /* clang-format off */
> > >   #define LANDLOCK_ACCESS_NET_BIND_TCP			(1ULL << 0)
> > >   #define LANDLOCK_ACCESS_NET_CONNECT_TCP			(1ULL << 1)
> > > +#define LANDLOCK_ACCESS_NET_LISTEN_TCP			(1ULL << 2)
> > >   /* clang-format on */
> > >   #endif /* _UAPI_LINUX_LANDLOCK_H */
> > > diff --git a/security/landlock/limits.h b/security/landlock/limits.h
> > > index 4eb643077a2a..2ef147389474 100644
> > > --- a/security/landlock/limits.h
> > > +++ b/security/landlock/limits.h
> > > @@ -22,7 +22,7 @@
> > >   #define LANDLOCK_MASK_ACCESS_FS		((LANDLOCK_LAST_ACCESS_FS << 1) - 1)
> > >   #define LANDLOCK_NUM_ACCESS_FS		__const_hweight64(LANDLOCK_MASK_ACCESS_FS)
> > > -#define LANDLOCK_LAST_ACCESS_NET	LANDLOCK_ACCESS_NET_CONNECT_TCP
> > > +#define LANDLOCK_LAST_ACCESS_NET	LANDLOCK_ACCESS_NET_LISTEN_TCP
> > >   #define LANDLOCK_MASK_ACCESS_NET	((LANDLOCK_LAST_ACCESS_NET << 1) - 1)
> > >   #define LANDLOCK_NUM_ACCESS_NET		__const_hweight64(LANDLOCK_MASK_ACCESS_NET)
> > > diff --git a/security/landlock/net.c b/security/landlock/net.c
> > > index 669ba260342f..a29cb27c3f14 100644
> > > --- a/security/landlock/net.c
> > > +++ b/security/landlock/net.c
> > > @@ -6,10 +6,12 @@
> > >    * Copyright © 2022-2023 Microsoft Corporation
> > >    */
> > > +#include "net/sock.h"
> > 
> > These should not be quotes.
> 
> will be fixed, thanks
> 
> > 
> > >   #include <linux/in.h>
> > >   #include <linux/net.h>
> > >   #include <linux/socket.h>
> > >   #include <net/ipv6.h>
> > > +#include <net/tcp.h>
> > >   #include "common.h"
> > >   #include "cred.h"
> > > @@ -194,9 +196,97 @@ static int hook_socket_connect(struct socket *const sock,
> > >   					   LANDLOCK_ACCESS_NET_CONNECT_TCP);
> > >   }
> > > +/*
> > > + * Checks that socket state and attributes are correct for listen.
> > > + * It is required to not wrongfully return -EACCES instead of -EINVAL.
> > > + *
> > > + * This checker requires sock->sk to be locked.
> > > + */
> > > +static int check_tcp_socket_can_listen(struct socket *const sock)
> > 
> > Is this function still useful with the listen LSM hook?
> 
> Yeap, we need to validate socket structure before checking the access
> right. You can see [1] and [2] where the behavior of this function is
> tested.
> 
> [1] https://lore.kernel.org/all/20240728002602.3198398-6-ivanov.mikhail1@huawei-partners.com/
> [2] https://lore.kernel.org/all/20240728002602.3198398-8-ivanov.mikhail1@huawei-partners.com/

OK, that's good.

> 
> > 
> > > +{
> > > +	struct sock *sk = sock->sk;
> > > +	unsigned char cur_sk_state = sk->sk_state;
> > > +	const struct tcp_ulp_ops *icsk_ulp_ops;
> > > +
> > > +	/* Allows only unconnected TCP socket to listen (cf. inet_listen). */
> > > +	if (sock->state != SS_UNCONNECTED)
> > > +		return -EINVAL;
> > > +
> > > +	/*
> > > +	 * Checks sock state. This is needed to ensure consistency with inet stack
> > > +	 * error handling (cf. __inet_listen_sk).
> > > +	 */
> > > +	if (WARN_ON_ONCE(!((1 << cur_sk_state) & (TCPF_CLOSE | TCPF_LISTEN))))
> > > +		return -EINVAL;
> > > +
> > > +	icsk_ulp_ops = inet_csk(sk)->icsk_ulp_ops;
> > > +
> > > +	/*
> > > +	 * ULP (Upper Layer Protocol) stands for protocols which are higher than
> > > +	 * transport protocol in OSI model. Linux has an infrastructure that
> > > +	 * allows TCP sockets to support logic of some ULP (e.g. TLS ULP).
> > > +	 *
> > > +	 * Sockets can listen only if ULP control hook has clone method.
> > > +	 */
> > > +	if (icsk_ulp_ops && !icsk_ulp_ops->clone)
> > > +		return -EINVAL;
> > > +	return 0;
> > > +}
> > > +
> > > +static int hook_socket_listen(struct socket *const sock, const int backlog)
> > > +{
> > 
> > Why can't we just call current_check_access_socket()?
> 
> I've mentioned in the message of the previous commit that this method
> has address checks for bind(2) and connect(2). In the case of listen(2)
> port is extracted from the socket structure, so calling
> current_check_access_socket() would be pointless.

Yep, I missed the check_access_socket() refactoring.

> 
> > 
> > > +	int err = 0;
> > > +	int family;
> > > +	__be16 port;
> > > +	struct sock *sk;
> > > +	const struct landlock_ruleset *const dom = get_current_net_domain();
> > > +
> > > +	if (!dom)
> > > +		return 0;
> > > +	if (WARN_ON_ONCE(dom->num_layers < 1))
> > > +		return -EACCES;
> > > +
> > > +	/* Checks if it's a (potential) TCP socket. */
> > > +	if (sock->type != SOCK_STREAM)
> > > +		return 0;
> > > +
> > > +	sk = sock->sk;
> > > +	family = sk->__sk_common.skc_family;
> > > +	/*
> > > +	 * Socket cannot be assigned AF_UNSPEC because this type is used only
> > > +	 * in the context of addresses.
> > > +	 *
> > > +	 * Doesn't restrict listening for non-TCP sockets.
> > > +	 */
> > > +	if (family != AF_INET && family != AF_INET6)
> > > +		return 0;
> > > +
> > > +	lock_sock(sk);
> > > +	/*
> > > +	 * Calling listen(2) for a listening socket does nothing with its state and
> > > +	 * only changes backlog value (cf. __inet_listen_sk). Checking of listen
> > > +	 * access right is not required.
> > > +	 */
> > > +	if (sk->sk_state == TCP_LISTEN)
> > > +		goto release_nocheck;
> > > +
> > > +	err = check_tcp_socket_can_listen(sock);
> > > +	if (unlikely(err))
> > > +		goto release_nocheck;
> > > +
> > > +	port = htons(inet_sk(sk)->inet_num);
> > > +	release_sock(sk);
> > > +	return check_access_socket(dom, port, LANDLOCK_ACCESS_NET_LISTEN_TCP);
> > > +
> > > +release_nocheck:
> > > +	release_sock(sk);
> > > +	return err;
> > > +}
> > > +
> > >   static struct security_hook_list landlock_hooks[] __ro_after_init = {
> > >   	LSM_HOOK_INIT(socket_bind, hook_socket_bind),
> > >   	LSM_HOOK_INIT(socket_connect, hook_socket_connect),
> > > +	LSM_HOOK_INIT(socket_listen, hook_socket_listen),
> > >   };
> > >   __init void landlock_add_net_hooks(void)
> > > diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
> > > index 03b470f5a85a..3752bcc033d4 100644
> > > --- a/security/landlock/syscalls.c
> > > +++ b/security/landlock/syscalls.c
> > > @@ -149,7 +149,7 @@ static const struct file_operations ruleset_fops = {
> > >   	.write = fop_dummy_write,
> > >   };
> > > -#define LANDLOCK_ABI_VERSION 5
> > > +#define LANDLOCK_ABI_VERSION 6
> > >   /**
> > >    * sys_landlock_create_ruleset - Create a new ruleset
> > > diff --git a/tools/testing/selftests/landlock/base_test.c b/tools/testing/selftests/landlock/base_test.c
> > > index 3c1e9f35b531..52b00472a487 100644
> > > --- a/tools/testing/selftests/landlock/base_test.c
> > > +++ b/tools/testing/selftests/landlock/base_test.c
> > > @@ -75,7 +75,7 @@ TEST(abi_version)
> > >   	const struct landlock_ruleset_attr ruleset_attr = {
> > >   		.handled_access_fs = LANDLOCK_ACCESS_FS_READ_FILE,
> > >   	};
> > > -	ASSERT_EQ(5, landlock_create_ruleset(NULL, 0,
> > > +	ASSERT_EQ(6, landlock_create_ruleset(NULL, 0,
> > >   					     LANDLOCK_CREATE_RULESET_VERSION));
> > >   	ASSERT_EQ(-1, landlock_create_ruleset(&ruleset_attr, 0,
> > > -- 
> > > 2.34.1
> > > 
> > > 
> 

Re: [RFC PATCH v1 2/9] landlock: Support TCP listen access-control

[Mikhail Ivanov]

8/1/2024 5:45 PM, Mickaël Salaün wrote:
> On Thu, Aug 01, 2024 at 10:52:25AM +0300, Mikhail Ivanov wrote:
>> 7/31/2024 9:30 PM, Mickaël Salaün wrote:
>>> On Sun, Jul 28, 2024 at 08:25:55AM +0800, Mikhail Ivanov wrote:
>>>> LANDLOCK_ACCESS_NET_BIND_TCP is useful to limit the scope of "bindable"
>>>> ports to forbid a malicious sandboxed process to impersonate a legitimate
>>>> server process. However, bind(2) might be used by (TCP) clients to set the
>>>> source port to a (legitimate) value. Controlling the ports that can be
>>>> used for listening would allow (TCP) clients to explicitly bind to ports
>>>> that are forbidden for listening.
>>>>
>>>> Such control is implemented with a new LANDLOCK_ACCESS_NET_LISTEN_TCP
>>>> access right that restricts listening on undesired ports with listen(2).
>>>>
>>>> It's worth noticing that this access right doesn't affect changing
>>>> backlog value using listen(2) on already listening socket.
>>>>
>>>> * Create new LANDLOCK_ACCESS_NET_LISTEN_TCP flag.
>>>> * Add hook to socket_listen(), which checks whether the socket is allowed
>>>>     to listen on a binded local port.
>>>> * Add check_tcp_socket_can_listen() helper, which validates socket
>>>>     attributes before the actual access right check.
>>>> * Update `struct landlock_net_port_attr` documentation with control of
>>>>     binding to ephemeral port with listen(2) description.
>>>> * Change ABI version to 6.
>>>>
>>>> Closes: https://github.com/landlock-lsm/linux/issues/15
>>>> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
>>>
>>> Thanks for this series!
>>>
>>> I cannot apply this patch series though, could you please provide the
>>> base commit?  BTW, this can be automatically put in the cover letter
>>> with the git format-patch's --base argument.
>>
>> base-commit: 591561c2b47b7e7225e229e844f5de75ce0c09ec
> 
> Thanks, the following commit makes this series to not apply.

Sorry, you mean that the series are succesfully applied, right?

> 
>>
>> Günther said that I should rebase to the latest commits, so I'll do
>> it in the next version of this patchset.
> 
> Yep, currently we're on v6.11-rc1, but please specify the base commit
> each time.

ok

> 
>>
>>>
>>>> ---
>>>>    include/uapi/linux/landlock.h                | 23 +++--
>>>>    security/landlock/limits.h                   |  2 +-
>>>>    security/landlock/net.c                      | 90 ++++++++++++++++++++
>>>>    security/landlock/syscalls.c                 |  2 +-
>>>>    tools/testing/selftests/landlock/base_test.c |  2 +-
>>>>    5 files changed, 108 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/include/uapi/linux/landlock.h b/include/uapi/linux/landlock.h
>>>> index 68625e728f43..6b8df3293eee 100644
>>>> --- a/include/uapi/linux/landlock.h
>>>> +++ b/include/uapi/linux/landlock.h
>>>> @@ -104,13 +104,16 @@ struct landlock_net_port_attr {
>>>>    	/**
>>>>    	 * @port: Network port in host endianness.
>>>>    	 *
>>>> -	 * It should be noted that port 0 passed to :manpage:`bind(2)` will
>>>> -	 * bind to an available port from a specific port range. This can be
>>>> -	 * configured thanks to the ``/proc/sys/net/ipv4/ip_local_port_range``
>>>> -	 * sysctl (also used for IPv6). A Landlock rule with port 0 and the
>>>> -	 * ``LANDLOCK_ACCESS_NET_BIND_TCP`` right means that requesting to bind
>>>> -	 * on port 0 is allowed and it will automatically translate to binding
>>>> -	 * on the related port range.
>>>> +	 * It should be noted that some operations cause binding socket to a random
>>>> +	 * available port from a specific port range. This can be configured thanks
>>>> +	 * to the ``/proc/sys/net/ipv4/ip_local_port_range`` sysctl (also used for
>>>> +	 * IPv6). Following operation requests are automatically translate to
>>>> +	 * binding on the related port range:
>>>> +	 *
>>>> +	 * - A Landlock rule with port 0 and the ``LANDLOCK_ACCESS_NET_BIND_TCP``
>>>> +	 *   right means that binding on port 0 is allowed.
>>>> +	 * - A Landlock rule with port 0 and the ``LANDLOCK_ACCESS_NET_LISTEN_TCP``
>>>> +	 *   right means listening without an explicit binding is allowed.
>>>>    	 */
>>>>    	__u64 port;
>>>>    };
>>>> @@ -251,7 +254,7 @@ struct landlock_net_port_attr {
>>>>     * DOC: net_access
>>>>     *
>>>>     * Network flags
>>>> - * ~~~~~~~~~~~~~~~~
>>>> + * ~~~~~~~~~~~~~
>>>>     *
>>>>     * These flags enable to restrict a sandboxed process to a set of network
>>>>     * actions. This is supported since the Landlock ABI version 4.
>>>> @@ -261,9 +264,13 @@ struct landlock_net_port_attr {
>>>>     * - %LANDLOCK_ACCESS_NET_BIND_TCP: Bind a TCP socket to a local port.
>>>>     * - %LANDLOCK_ACCESS_NET_CONNECT_TCP: Connect an active TCP socket to
>>>>     *   a remote port.
>>>> + * - %LANDLOCK_ACCESS_NET_LISTEN_TCP: Listen for TCP socket connections on
>>>> + *   a local port. This access right is available since the sixth version
>>>> + *   of the Landlock ABI.
>>>>     */
>>>>    /* clang-format off */
>>>>    #define LANDLOCK_ACCESS_NET_BIND_TCP			(1ULL << 0)
>>>>    #define LANDLOCK_ACCESS_NET_CONNECT_TCP			(1ULL << 1)
>>>> +#define LANDLOCK_ACCESS_NET_LISTEN_TCP			(1ULL << 2)
>>>>    /* clang-format on */
>>>>    #endif /* _UAPI_LINUX_LANDLOCK_H */
>>>> diff --git a/security/landlock/limits.h b/security/landlock/limits.h
>>>> index 4eb643077a2a..2ef147389474 100644
>>>> --- a/security/landlock/limits.h
>>>> +++ b/security/landlock/limits.h
>>>> @@ -22,7 +22,7 @@
>>>>    #define LANDLOCK_MASK_ACCESS_FS		((LANDLOCK_LAST_ACCESS_FS << 1) - 1)
>>>>    #define LANDLOCK_NUM_ACCESS_FS		__const_hweight64(LANDLOCK_MASK_ACCESS_FS)
>>>> -#define LANDLOCK_LAST_ACCESS_NET	LANDLOCK_ACCESS_NET_CONNECT_TCP
>>>> +#define LANDLOCK_LAST_ACCESS_NET	LANDLOCK_ACCESS_NET_LISTEN_TCP
>>>>    #define LANDLOCK_MASK_ACCESS_NET	((LANDLOCK_LAST_ACCESS_NET << 1) - 1)
>>>>    #define LANDLOCK_NUM_ACCESS_NET		__const_hweight64(LANDLOCK_MASK_ACCESS_NET)
>>>> diff --git a/security/landlock/net.c b/security/landlock/net.c
>>>> index 669ba260342f..a29cb27c3f14 100644
>>>> --- a/security/landlock/net.c
>>>> +++ b/security/landlock/net.c
>>>> @@ -6,10 +6,12 @@
>>>>     * Copyright © 2022-2023 Microsoft Corporation
>>>>     */
>>>> +#include "net/sock.h"
>>>
>>> These should not be quotes.
>>
>> will be fixed, thanks
>>
>>>
>>>>    #include <linux/in.h>
>>>>    #include <linux/net.h>
>>>>    #include <linux/socket.h>
>>>>    #include <net/ipv6.h>
>>>> +#include <net/tcp.h>
>>>>    #include "common.h"
>>>>    #include "cred.h"
>>>> @@ -194,9 +196,97 @@ static int hook_socket_connect(struct socket *const sock,
>>>>    					   LANDLOCK_ACCESS_NET_CONNECT_TCP);
>>>>    }
>>>> +/*
>>>> + * Checks that socket state and attributes are correct for listen.
>>>> + * It is required to not wrongfully return -EACCES instead of -EINVAL.
>>>> + *
>>>> + * This checker requires sock->sk to be locked.
>>>> + */
>>>> +static int check_tcp_socket_can_listen(struct socket *const sock)
>>>
>>> Is this function still useful with the listen LSM hook?
>>
>> Yeap, we need to validate socket structure before checking the access
>> right. You can see [1] and [2] where the behavior of this function is
>> tested.
>>
>> [1] https://lore.kernel.org/all/20240728002602.3198398-6-ivanov.mikhail1@huawei-partners.com/
>> [2] https://lore.kernel.org/all/20240728002602.3198398-8-ivanov.mikhail1@huawei-partners.com/
> 
> OK, that's good.
> 
>>
>>>
>>>> +{
>>>> +	struct sock *sk = sock->sk;
>>>> +	unsigned char cur_sk_state = sk->sk_state;
>>>> +	const struct tcp_ulp_ops *icsk_ulp_ops;
>>>> +
>>>> +	/* Allows only unconnected TCP socket to listen (cf. inet_listen). */
>>>> +	if (sock->state != SS_UNCONNECTED)
>>>> +		return -EINVAL;
>>>> +
>>>> +	/*
>>>> +	 * Checks sock state. This is needed to ensure consistency with inet stack
>>>> +	 * error handling (cf. __inet_listen_sk).
>>>> +	 */
>>>> +	if (WARN_ON_ONCE(!((1 << cur_sk_state) & (TCPF_CLOSE | TCPF_LISTEN))))
>>>> +		return -EINVAL;
>>>> +
>>>> +	icsk_ulp_ops = inet_csk(sk)->icsk_ulp_ops;
>>>> +
>>>> +	/*
>>>> +	 * ULP (Upper Layer Protocol) stands for protocols which are higher than
>>>> +	 * transport protocol in OSI model. Linux has an infrastructure that
>>>> +	 * allows TCP sockets to support logic of some ULP (e.g. TLS ULP).
>>>> +	 *
>>>> +	 * Sockets can listen only if ULP control hook has clone method.
>>>> +	 */
>>>> +	if (icsk_ulp_ops && !icsk_ulp_ops->clone)
>>>> +		return -EINVAL;
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int hook_socket_listen(struct socket *const sock, const int backlog)
>>>> +{
>>>
>>> Why can't we just call current_check_access_socket()?
>>
>> I've mentioned in the message of the previous commit that this method
>> has address checks for bind(2) and connect(2). In the case of listen(2)
>> port is extracted from the socket structure, so calling
>> current_check_access_socket() would be pointless.
> 
> Yep, I missed the check_access_socket() refactoring.
> 
>>
>>>
>>>> +	int err = 0;
>>>> +	int family;
>>>> +	__be16 port;
>>>> +	struct sock *sk;
>>>> +	const struct landlock_ruleset *const dom = get_current_net_domain();
>>>> +
>>>> +	if (!dom)
>>>> +		return 0;
>>>> +	if (WARN_ON_ONCE(dom->num_layers < 1))
>>>> +		return -EACCES;
>>>> +
>>>> +	/* Checks if it's a (potential) TCP socket. */
>>>> +	if (sock->type != SOCK_STREAM)
>>>> +		return 0;
>>>> +
>>>> +	sk = sock->sk;
>>>> +	family = sk->__sk_common.skc_family;
>>>> +	/*
>>>> +	 * Socket cannot be assigned AF_UNSPEC because this type is used only
>>>> +	 * in the context of addresses.
>>>> +	 *
>>>> +	 * Doesn't restrict listening for non-TCP sockets.
>>>> +	 */
>>>> +	if (family != AF_INET && family != AF_INET6)
>>>> +		return 0;
>>>> +
>>>> +	lock_sock(sk);
>>>> +	/*
>>>> +	 * Calling listen(2) for a listening socket does nothing with its state and
>>>> +	 * only changes backlog value (cf. __inet_listen_sk). Checking of listen
>>>> +	 * access right is not required.
>>>> +	 */
>>>> +	if (sk->sk_state == TCP_LISTEN)
>>>> +		goto release_nocheck;
>>>> +
>>>> +	err = check_tcp_socket_can_listen(sock);
>>>> +	if (unlikely(err))
>>>> +		goto release_nocheck;
>>>> +
>>>> +	port = htons(inet_sk(sk)->inet_num);
>>>> +	release_sock(sk);
>>>> +	return check_access_socket(dom, port, LANDLOCK_ACCESS_NET_LISTEN_TCP);
>>>> +
>>>> +release_nocheck:
>>>> +	release_sock(sk);
>>>> +	return err;
>>>> +}
>>>> +
>>>>    static struct security_hook_list landlock_hooks[] __ro_after_init = {
>>>>    	LSM_HOOK_INIT(socket_bind, hook_socket_bind),
>>>>    	LSM_HOOK_INIT(socket_connect, hook_socket_connect),
>>>> +	LSM_HOOK_INIT(socket_listen, hook_socket_listen),
>>>>    };
>>>>    __init void landlock_add_net_hooks(void)
>>>> diff --git a/security/landlock/syscalls.c b/security/landlock/syscalls.c
>>>> index 03b470f5a85a..3752bcc033d4 100644
>>>> --- a/security/landlock/syscalls.c
>>>> +++ b/security/landlock/syscalls.c
>>>> @@ -149,7 +149,7 @@ static const struct file_operations ruleset_fops = {
>>>>    	.write = fop_dummy_write,
>>>>    };
>>>> -#define LANDLOCK_ABI_VERSION 5
>>>> +#define LANDLOCK_ABI_VERSION 6
>>>>    /**
>>>>     * sys_landlock_create_ruleset - Create a new ruleset
>>>> diff --git a/tools/testing/selftests/landlock/base_test.c b/tools/testing/selftests/landlock/base_test.c
>>>> index 3c1e9f35b531..52b00472a487 100644
>>>> --- a/tools/testing/selftests/landlock/base_test.c
>>>> +++ b/tools/testing/selftests/landlock/base_test.c
>>>> @@ -75,7 +75,7 @@ TEST(abi_version)
>>>>    	const struct landlock_ruleset_attr ruleset_attr = {
>>>>    		.handled_access_fs = LANDLOCK_ACCESS_FS_READ_FILE,
>>>>    	};
>>>> -	ASSERT_EQ(5, landlock_create_ruleset(NULL, 0,
>>>> +	ASSERT_EQ(6, landlock_create_ruleset(NULL, 0,
>>>>    					     LANDLOCK_CREATE_RULESET_VERSION));
>>>>    	ASSERT_EQ(-1, landlock_create_ruleset(&ruleset_attr, 0,
>>>> -- 
>>>> 2.34.1
>>>>
>>>>
>>

Re: [RFC PATCH v1 2/9] landlock: Support TCP listen access-control

[Mickaël Salaün]

On Thu, Aug 01, 2024 at 06:34:41PM +0300, Mikhail Ivanov wrote:
> 8/1/2024 5:45 PM, Mickaël Salaün wrote:
> > On Thu, Aug 01, 2024 at 10:52:25AM +0300, Mikhail Ivanov wrote:
> > > 7/31/2024 9:30 PM, Mickaël Salaün wrote:
> > > > On Sun, Jul 28, 2024 at 08:25:55AM +0800, Mikhail Ivanov wrote:
> > > > > LANDLOCK_ACCESS_NET_BIND_TCP is useful to limit the scope of "bindable"
> > > > > ports to forbid a malicious sandboxed process to impersonate a legitimate
> > > > > server process. However, bind(2) might be used by (TCP) clients to set the
> > > > > source port to a (legitimate) value. Controlling the ports that can be
> > > > > used for listening would allow (TCP) clients to explicitly bind to ports
> > > > > that are forbidden for listening.
> > > > > 
> > > > > Such control is implemented with a new LANDLOCK_ACCESS_NET_LISTEN_TCP
> > > > > access right that restricts listening on undesired ports with listen(2).
> > > > > 
> > > > > It's worth noticing that this access right doesn't affect changing
> > > > > backlog value using listen(2) on already listening socket.
> > > > > 
> > > > > * Create new LANDLOCK_ACCESS_NET_LISTEN_TCP flag.
> > > > > * Add hook to socket_listen(), which checks whether the socket is allowed
> > > > >     to listen on a binded local port.
> > > > > * Add check_tcp_socket_can_listen() helper, which validates socket
> > > > >     attributes before the actual access right check.
> > > > > * Update `struct landlock_net_port_attr` documentation with control of
> > > > >     binding to ephemeral port with listen(2) description.
> > > > > * Change ABI version to 6.
> > > > > 
> > > > > Closes: https://github.com/landlock-lsm/linux/issues/15
> > > > > Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
> > > > 
> > > > Thanks for this series!
> > > > 
> > > > I cannot apply this patch series though, could you please provide the
> > > > base commit?  BTW, this can be automatically put in the cover letter
> > > > with the git format-patch's --base argument.
> > > 
> > > base-commit: 591561c2b47b7e7225e229e844f5de75ce0c09ec
> > 
> > Thanks, the following commit makes this series to not apply.
> 
> Sorry, you mean that the series are succesfully applied, right?

Yes, it works with the commit you provided.  I was talking about a next
(logical) commit f4b89d8ce5a8 ("landlock: Various documentation
improvements") which makes your series not apply, but that's OK now.

Re: [RFC PATCH v1 2/9] landlock: Support TCP listen access-control

[Mikhail Ivanov]

8/1/2024 7:01 PM, Mickaël Salaün wrote:
> On Thu, Aug 01, 2024 at 06:34:41PM +0300, Mikhail Ivanov wrote:
>> 8/1/2024 5:45 PM, Mickaël Salaün wrote:
>>> On Thu, Aug 01, 2024 at 10:52:25AM +0300, Mikhail Ivanov wrote:
>>>> 7/31/2024 9:30 PM, Mickaël Salaün wrote:
>>>>> On Sun, Jul 28, 2024 at 08:25:55AM +0800, Mikhail Ivanov wrote:
>>>>>> LANDLOCK_ACCESS_NET_BIND_TCP is useful to limit the scope of "bindable"
>>>>>> ports to forbid a malicious sandboxed process to impersonate a legitimate
>>>>>> server process. However, bind(2) might be used by (TCP) clients to set the
>>>>>> source port to a (legitimate) value. Controlling the ports that can be
>>>>>> used for listening would allow (TCP) clients to explicitly bind to ports
>>>>>> that are forbidden for listening.
>>>>>>
>>>>>> Such control is implemented with a new LANDLOCK_ACCESS_NET_LISTEN_TCP
>>>>>> access right that restricts listening on undesired ports with listen(2).
>>>>>>
>>>>>> It's worth noticing that this access right doesn't affect changing
>>>>>> backlog value using listen(2) on already listening socket.
>>>>>>
>>>>>> * Create new LANDLOCK_ACCESS_NET_LISTEN_TCP flag.
>>>>>> * Add hook to socket_listen(), which checks whether the socket is allowed
>>>>>>      to listen on a binded local port.
>>>>>> * Add check_tcp_socket_can_listen() helper, which validates socket
>>>>>>      attributes before the actual access right check.
>>>>>> * Update `struct landlock_net_port_attr` documentation with control of
>>>>>>      binding to ephemeral port with listen(2) description.
>>>>>> * Change ABI version to 6.
>>>>>>
>>>>>> Closes: https://github.com/landlock-lsm/linux/issues/15
>>>>>> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
>>>>>
>>>>> Thanks for this series!
>>>>>
>>>>> I cannot apply this patch series though, could you please provide the
>>>>> base commit?  BTW, this can be automatically put in the cover letter
>>>>> with the git format-patch's --base argument.
>>>>
>>>> base-commit: 591561c2b47b7e7225e229e844f5de75ce0c09ec
>>>
>>> Thanks, the following commit makes this series to not apply.
>>
>> Sorry, you mean that the series are succesfully applied, right?
> 
> Yes, it works with the commit you provided.  I was talking about a next
> (logical) commit f4b89d8ce5a8 ("landlock: Various documentation
> improvements") which makes your series not apply, but that's OK now.
Nice

Re: [RFC PATCH v1 2/9] landlock: Support TCP listen access-control

[Mickaël Salaün]

On Sun, Jul 28, 2024 at 08:25:55AM +0800, Mikhail Ivanov wrote:
> LANDLOCK_ACCESS_NET_BIND_TCP is useful to limit the scope of "bindable"
> ports to forbid a malicious sandboxed process to impersonate a legitimate
> server process. However, bind(2) might be used by (TCP) clients to set the
> source port to a (legitimate) value. Controlling the ports that can be
> used for listening would allow (TCP) clients to explicitly bind to ports
> that are forbidden for listening.
> 
> Such control is implemented with a new LANDLOCK_ACCESS_NET_LISTEN_TCP
> access right that restricts listening on undesired ports with listen(2).
> 
> It's worth noticing that this access right doesn't affect changing
> backlog value using listen(2) on already listening socket.
> 
> * Create new LANDLOCK_ACCESS_NET_LISTEN_TCP flag.
> * Add hook to socket_listen(), which checks whether the socket is allowed
>   to listen on a binded local port.
> * Add check_tcp_socket_can_listen() helper, which validates socket
>   attributes before the actual access right check.
> * Update `struct landlock_net_port_attr` documentation with control of
>   binding to ephemeral port with listen(2) description.
> * Change ABI version to 6.
> 
> Closes: https://github.com/landlock-lsm/linux/issues/15
> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
> ---
>  include/uapi/linux/landlock.h                | 23 +++--
>  security/landlock/limits.h                   |  2 +-
>  security/landlock/net.c                      | 90 ++++++++++++++++++++
>  security/landlock/syscalls.c                 |  2 +-
>  tools/testing/selftests/landlock/base_test.c |  2 +-
>  5 files changed, 108 insertions(+), 11 deletions(-)

> diff --git a/security/landlock/net.c b/security/landlock/net.c
> index 669ba260342f..a29cb27c3f14 100644
> --- a/security/landlock/net.c
> +++ b/security/landlock/net.c
> @@ -6,10 +6,12 @@
>   * Copyright © 2022-2023 Microsoft Corporation
>   */
>  
> +#include "net/sock.h"
>  #include <linux/in.h>
>  #include <linux/net.h>
>  #include <linux/socket.h>
>  #include <net/ipv6.h>
> +#include <net/tcp.h>
>  
>  #include "common.h"
>  #include "cred.h"
> @@ -194,9 +196,97 @@ static int hook_socket_connect(struct socket *const sock,
>  					   LANDLOCK_ACCESS_NET_CONNECT_TCP);
>  }
>  
> +/*
> + * Checks that socket state and attributes are correct for listen.
> + * It is required to not wrongfully return -EACCES instead of -EINVAL.
> + *
> + * This checker requires sock->sk to be locked.
> + */
> +static int check_tcp_socket_can_listen(struct socket *const sock)
> +{
> +	struct sock *sk = sock->sk;
> +	unsigned char cur_sk_state = sk->sk_state;
> +	const struct tcp_ulp_ops *icsk_ulp_ops;
> +

I think we can add this assert:
lockdep_assert_held(&sk->sk_lock.slock);

> +	/* Allows only unconnected TCP socket to listen (cf. inet_listen). */
> +	if (sock->state != SS_UNCONNECTED)
> +		return -EINVAL;
> +
> +	/*
> +	 * Checks sock state. This is needed to ensure consistency with inet stack
> +	 * error handling (cf. __inet_listen_sk).
> +	 */
> +	if (WARN_ON_ONCE(!((1 << cur_sk_state) & (TCPF_CLOSE | TCPF_LISTEN))))
> +		return -EINVAL;
> +
> +	icsk_ulp_ops = inet_csk(sk)->icsk_ulp_ops;
> +
> +	/*
> +	 * ULP (Upper Layer Protocol) stands for protocols which are higher than
> +	 * transport protocol in OSI model. Linux has an infrastructure that
> +	 * allows TCP sockets to support logic of some ULP (e.g. TLS ULP).
> +	 *
> +	 * Sockets can listen only if ULP control hook has clone method.
> +	 */
> +	if (icsk_ulp_ops && !icsk_ulp_ops->clone)
> +		return -EINVAL;
> +	return 0;
> +}

Re: [RFC PATCH v1 2/9] landlock: Support TCP listen access-control

[Mikhail Ivanov]

8/1/2024 5:45 PM, Mickaël Salaün wrote:
> On Sun, Jul 28, 2024 at 08:25:55AM +0800, Mikhail Ivanov wrote:
>> LANDLOCK_ACCESS_NET_BIND_TCP is useful to limit the scope of "bindable"
>> ports to forbid a malicious sandboxed process to impersonate a legitimate
>> server process. However, bind(2) might be used by (TCP) clients to set the
>> source port to a (legitimate) value. Controlling the ports that can be
>> used for listening would allow (TCP) clients to explicitly bind to ports
>> that are forbidden for listening.
>>
>> Such control is implemented with a new LANDLOCK_ACCESS_NET_LISTEN_TCP
>> access right that restricts listening on undesired ports with listen(2).
>>
>> It's worth noticing that this access right doesn't affect changing
>> backlog value using listen(2) on already listening socket.
>>
>> * Create new LANDLOCK_ACCESS_NET_LISTEN_TCP flag.
>> * Add hook to socket_listen(), which checks whether the socket is allowed
>>    to listen on a binded local port.
>> * Add check_tcp_socket_can_listen() helper, which validates socket
>>    attributes before the actual access right check.
>> * Update `struct landlock_net_port_attr` documentation with control of
>>    binding to ephemeral port with listen(2) description.
>> * Change ABI version to 6.
>>
>> Closes: https://github.com/landlock-lsm/linux/issues/15
>> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
>> ---
>>   include/uapi/linux/landlock.h                | 23 +++--
>>   security/landlock/limits.h                   |  2 +-
>>   security/landlock/net.c                      | 90 ++++++++++++++++++++
>>   security/landlock/syscalls.c                 |  2 +-
>>   tools/testing/selftests/landlock/base_test.c |  2 +-
>>   5 files changed, 108 insertions(+), 11 deletions(-)
> 
>> diff --git a/security/landlock/net.c b/security/landlock/net.c
>> index 669ba260342f..a29cb27c3f14 100644
>> --- a/security/landlock/net.c
>> +++ b/security/landlock/net.c
>> @@ -6,10 +6,12 @@
>>    * Copyright © 2022-2023 Microsoft Corporation
>>    */
>>   
>> +#include "net/sock.h"
>>   #include <linux/in.h>
>>   #include <linux/net.h>
>>   #include <linux/socket.h>
>>   #include <net/ipv6.h>
>> +#include <net/tcp.h>
>>   
>>   #include "common.h"
>>   #include "cred.h"
>> @@ -194,9 +196,97 @@ static int hook_socket_connect(struct socket *const sock,
>>   					   LANDLOCK_ACCESS_NET_CONNECT_TCP);
>>   }
>>   
>> +/*
>> + * Checks that socket state and attributes are correct for listen.
>> + * It is required to not wrongfully return -EACCES instead of -EINVAL.
>> + *
>> + * This checker requires sock->sk to be locked.
>> + */
>> +static int check_tcp_socket_can_listen(struct socket *const sock)
>> +{
>> +	struct sock *sk = sock->sk;
>> +	unsigned char cur_sk_state = sk->sk_state;
>> +	const struct tcp_ulp_ops *icsk_ulp_ops;
>> +
> 
> I think we can add this assert:
> lockdep_assert_held(&sk->sk_lock.slock);

Ok, let's add it. I just haven't seen this being a common practice in
the network stack.

> 
>> +	/* Allows only unconnected TCP socket to listen (cf. inet_listen). */
>> +	if (sock->state != SS_UNCONNECTED)
>> +		return -EINVAL;
>> +
>> +	/*
>> +	 * Checks sock state. This is needed to ensure consistency with inet stack
>> +	 * error handling (cf. __inet_listen_sk).
>> +	 */
>> +	if (WARN_ON_ONCE(!((1 << cur_sk_state) & (TCPF_CLOSE | TCPF_LISTEN))))
>> +		return -EINVAL;
>> +
>> +	icsk_ulp_ops = inet_csk(sk)->icsk_ulp_ops;
>> +
>> +	/*
>> +	 * ULP (Upper Layer Protocol) stands for protocols which are higher than
>> +	 * transport protocol in OSI model. Linux has an infrastructure that
>> +	 * allows TCP sockets to support logic of some ULP (e.g. TLS ULP).
>> +	 *
>> +	 * Sockets can listen only if ULP control hook has clone method.
>> +	 */
>> +	if (icsk_ulp_ops && !icsk_ulp_ops->clone)
>> +		return -EINVAL;
>> +	return 0;
>> +}

[RFC PATCH v1 3/9] selftests/landlock: Support LANDLOCK_ACCESS_NET_LISTEN_TCP

[Mikhail Ivanov]

* Add listen_variant() to simplify listen(2) return code checking.
* Rename test_bind_and_connect() to test_restricted_net_fixture().
* Extend current net rules with LANDLOCK_ACCESS_NET_LISTEN_TCP access.
* Rename test port_specific.bind_connect_1023 to
  port_specific.port_1023.
* Check little endian port restriction for listen in
  ipv4_tcp.port_endianness.
* Some local renames and comment changes.

Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
---
 tools/testing/selftests/landlock/net_test.c | 198 +++++++++++---------
 1 file changed, 107 insertions(+), 91 deletions(-)

diff --git a/tools/testing/selftests/landlock/net_test.c b/tools/testing/selftests/landlock/net_test.c
index f21cfbbc3638..8126f5c0160f 100644
--- a/tools/testing/selftests/landlock/net_test.c
+++ b/tools/testing/selftests/landlock/net_test.c
@@ -2,7 +2,7 @@
 /*
  * Landlock tests - Network
  *
- * Copyright © 2022-2023 Huawei Tech. Co., Ltd.
+ * Copyright © 2022-2024 Huawei Tech. Co., Ltd.
  * Copyright © 2023 Microsoft Corporation
  */
 
@@ -22,6 +22,17 @@
 
 #include "common.h"
 
+/* clang-format off */
+
+#define ACCESS_LAST LANDLOCK_ACCESS_NET_LISTEN_TCP
+
+#define ACCESS_ALL ( \
+	LANDLOCK_ACCESS_NET_BIND_TCP | \
+	LANDLOCK_ACCESS_NET_CONNECT_TCP | \
+	LANDLOCK_ACCESS_NET_LISTEN_TCP)
+
+/* clang-format on */
+
 const short sock_port_start = (1 << 10);
 
 static const char loopback_ipv4[] = "127.0.0.1";
@@ -282,6 +293,16 @@ static int connect_variant(const int sock_fd,
 	return connect_variant_addrlen(sock_fd, srv, get_addrlen(srv, false));
 }
 
+static int listen_variant(const int sock_fd, const int backlog)
+{
+	int ret;
+
+	ret = listen(sock_fd, backlog);
+	if (ret < 0)
+		return -errno;
+	return ret;
+}
+
 FIXTURE(protocol)
 {
 	struct service_fixture srv0, srv1, srv2, unspec_any0, unspec_srv0;
@@ -438,9 +459,11 @@ FIXTURE_VARIANT_ADD(protocol, tcp_sandbox_with_unix_datagram) {
 	},
 };
 
-static void test_bind_and_connect(struct __test_metadata *const _metadata,
-				  const struct service_fixture *const srv,
-				  const bool deny_bind, const bool deny_connect)
+static void test_restricted_net_fixture(struct __test_metadata *const _metadata,
+					const struct service_fixture *const srv,
+					const bool deny_bind,
+					const bool deny_connect,
+					const bool deny_listen)
 {
 	char buf = '\0';
 	int inval_fd, bind_fd, client_fd, status, ret;
@@ -512,8 +535,14 @@ static void test_bind_and_connect(struct __test_metadata *const _metadata,
 		EXPECT_EQ(0, ret);
 
 		/* Creates a listening socket. */
-		if (srv->protocol.type == SOCK_STREAM)
-			EXPECT_EQ(0, listen(bind_fd, backlog));
+		if (srv->protocol.type == SOCK_STREAM) {
+			ret = listen_variant(bind_fd, backlog);
+			if (deny_listen) {
+				EXPECT_EQ(-EACCES, ret);
+			} else {
+				EXPECT_EQ(0, ret);
+			}
+		}
 	}
 
 	child = fork();
@@ -530,7 +559,7 @@ static void test_bind_and_connect(struct __test_metadata *const _metadata,
 		ret = connect_variant(connect_fd, srv);
 		if (deny_connect) {
 			EXPECT_EQ(-EACCES, ret);
-		} else if (deny_bind) {
+		} else if (deny_bind || deny_listen) {
 			/* No listening server. */
 			EXPECT_EQ(-ECONNREFUSED, ret);
 		} else {
@@ -545,7 +574,7 @@ static void test_bind_and_connect(struct __test_metadata *const _metadata,
 
 	/* Accepts connection from the child. */
 	client_fd = bind_fd;
-	if (!deny_bind && !deny_connect) {
+	if (!deny_bind && !deny_connect && !deny_listen) {
 		if (srv->protocol.type == SOCK_STREAM) {
 			client_fd = accept(bind_fd, NULL, 0);
 			ASSERT_LE(0, client_fd);
@@ -571,16 +600,15 @@ TEST_F(protocol, bind)
 {
 	if (variant->sandbox == TCP_SANDBOX) {
 		const struct landlock_ruleset_attr ruleset_attr = {
-			.handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP |
-					      LANDLOCK_ACCESS_NET_CONNECT_TCP,
+			.handled_access_net = ACCESS_ALL,
 		};
-		const struct landlock_net_port_attr tcp_bind_connect_p0 = {
-			.allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP |
-					  LANDLOCK_ACCESS_NET_CONNECT_TCP,
+		const struct landlock_net_port_attr tcp_not_restricted_p0 = {
+			.allowed_access = ACCESS_ALL,
 			.port = self->srv0.port,
 		};
-		const struct landlock_net_port_attr tcp_connect_p1 = {
-			.allowed_access = LANDLOCK_ACCESS_NET_CONNECT_TCP,
+		const struct landlock_net_port_attr tcp_denied_bind_p1 = {
+			.allowed_access = ACCESS_ALL &
+					  ~LANDLOCK_ACCESS_NET_BIND_TCP,
 			.port = self->srv1.port,
 		};
 		int ruleset_fd;
@@ -589,48 +617,47 @@ TEST_F(protocol, bind)
 						     sizeof(ruleset_attr), 0);
 		ASSERT_LE(0, ruleset_fd);
 
-		/* Allows connect and bind for the first port.  */
+		/* Allows all actions for the first port. */
 		ASSERT_EQ(0,
 			  landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT,
-					    &tcp_bind_connect_p0, 0));
+					    &tcp_not_restricted_p0, 0));
 
-		/* Allows connect and denies bind for the second port. */
+		/* Allows all actions despite bind. */
 		ASSERT_EQ(0,
 			  landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT,
-					    &tcp_connect_p1, 0));
+					    &tcp_denied_bind_p1, 0));
 
 		enforce_ruleset(_metadata, ruleset_fd);
 		EXPECT_EQ(0, close(ruleset_fd));
 	}
+	bool restricted = is_restricted(&variant->prot, variant->sandbox);
 
 	/* Binds a socket to the first port. */
-	test_bind_and_connect(_metadata, &self->srv0, false, false);
+	test_restricted_net_fixture(_metadata, &self->srv0, false, false,
+				    false);
 
 	/* Binds a socket to the second port. */
-	test_bind_and_connect(_metadata, &self->srv1,
-			      is_restricted(&variant->prot, variant->sandbox),
-			      false);
+	test_restricted_net_fixture(_metadata, &self->srv1, restricted, false,
+				    false);
 
 	/* Binds a socket to the third port. */
-	test_bind_and_connect(_metadata, &self->srv2,
-			      is_restricted(&variant->prot, variant->sandbox),
-			      is_restricted(&variant->prot, variant->sandbox));
+	test_restricted_net_fixture(_metadata, &self->srv2, restricted,
+				    restricted, restricted);
 }
 
 TEST_F(protocol, connect)
 {
 	if (variant->sandbox == TCP_SANDBOX) {
 		const struct landlock_ruleset_attr ruleset_attr = {
-			.handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP |
-					      LANDLOCK_ACCESS_NET_CONNECT_TCP,
+			.handled_access_net = ACCESS_ALL,
 		};
-		const struct landlock_net_port_attr tcp_bind_connect_p0 = {
-			.allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP |
-					  LANDLOCK_ACCESS_NET_CONNECT_TCP,
+		const struct landlock_net_port_attr tcp_not_restricted_p0 = {
+			.allowed_access = ACCESS_ALL,
 			.port = self->srv0.port,
 		};
-		const struct landlock_net_port_attr tcp_bind_p1 = {
-			.allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP,
+		const struct landlock_net_port_attr tcp_denied_connect_p1 = {
+			.allowed_access = ACCESS_ALL &
+					  ~LANDLOCK_ACCESS_NET_CONNECT_TCP,
 			.port = self->srv1.port,
 		};
 		int ruleset_fd;
@@ -639,28 +666,27 @@ TEST_F(protocol, connect)
 						     sizeof(ruleset_attr), 0);
 		ASSERT_LE(0, ruleset_fd);
 
-		/* Allows connect and bind for the first port. */
+		/* Allows all actions for the first port. */
 		ASSERT_EQ(0,
 			  landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT,
-					    &tcp_bind_connect_p0, 0));
+					    &tcp_not_restricted_p0, 0));
 
-		/* Allows bind and denies connect for the second port. */
+		/* Allows all actions despite connect. */
 		ASSERT_EQ(0,
 			  landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT,
-					    &tcp_bind_p1, 0));
+					    &tcp_denied_connect_p1, 0));
 
 		enforce_ruleset(_metadata, ruleset_fd);
 		EXPECT_EQ(0, close(ruleset_fd));
 	}
-
-	test_bind_and_connect(_metadata, &self->srv0, false, false);
-
-	test_bind_and_connect(_metadata, &self->srv1, false,
-			      is_restricted(&variant->prot, variant->sandbox));
-
-	test_bind_and_connect(_metadata, &self->srv2,
-			      is_restricted(&variant->prot, variant->sandbox),
-			      is_restricted(&variant->prot, variant->sandbox));
+	bool restricted = is_restricted(&variant->prot, variant->sandbox);
+
+	test_restricted_net_fixture(_metadata, &self->srv0, false, false,
+				    false);
+	test_restricted_net_fixture(_metadata, &self->srv1, false, restricted,
+				    false);
+	test_restricted_net_fixture(_metadata, &self->srv2, restricted,
+				    restricted, restricted);
 }
 
 TEST_F(protocol, bind_unspec)
@@ -761,7 +787,7 @@ TEST_F(protocol, connect_unspec)
 	ASSERT_LE(0, bind_fd);
 	EXPECT_EQ(0, bind_variant(bind_fd, &self->srv0));
 	if (self->srv0.protocol.type == SOCK_STREAM)
-		EXPECT_EQ(0, listen(bind_fd, backlog));
+		EXPECT_EQ(0, listen_variant(bind_fd, backlog));
 
 	child = fork();
 	ASSERT_LE(0, child);
@@ -1127,8 +1153,8 @@ TEST_F(tcp_layers, ruleset_overlap)
 	 * Forbids to connect to the socket because only one ruleset layer
 	 * allows connect.
 	 */
-	test_bind_and_connect(_metadata, &self->srv0, false,
-			      variant->num_layers >= 2);
+	test_restricted_net_fixture(_metadata, &self->srv0, false,
+				    variant->num_layers >= 2, false);
 }
 
 TEST_F(tcp_layers, ruleset_expand)
@@ -1208,11 +1234,12 @@ TEST_F(tcp_layers, ruleset_expand)
 		EXPECT_EQ(0, close(ruleset_fd));
 	}
 
-	test_bind_and_connect(_metadata, &self->srv0, false,
-			      variant->num_layers >= 3);
+	test_restricted_net_fixture(_metadata, &self->srv0, false,
+				    variant->num_layers >= 3, false);
 
-	test_bind_and_connect(_metadata, &self->srv1, variant->num_layers >= 1,
-			      variant->num_layers >= 2);
+	test_restricted_net_fixture(_metadata, &self->srv1,
+				    variant->num_layers >= 1,
+				    variant->num_layers >= 2, false);
 }
 
 /* clang-format off */
@@ -1230,16 +1257,6 @@ FIXTURE_TEARDOWN(mini)
 {
 }
 
-/* clang-format off */
-
-#define ACCESS_LAST LANDLOCK_ACCESS_NET_CONNECT_TCP
-
-#define ACCESS_ALL ( \
-	LANDLOCK_ACCESS_NET_BIND_TCP | \
-	LANDLOCK_ACCESS_NET_CONNECT_TCP)
-
-/* clang-format on */
-
 TEST_F(mini, network_access_rights)
 {
 	const struct landlock_ruleset_attr ruleset_attr = {
@@ -1454,8 +1471,9 @@ TEST_F(mini, tcp_port_overflow)
 
 	enforce_ruleset(_metadata, ruleset_fd);
 
-	test_bind_and_connect(_metadata, &srv_denied, true, true);
-	test_bind_and_connect(_metadata, &srv_max_allowed, false, false);
+	test_restricted_net_fixture(_metadata, &srv_denied, true, true, false);
+	test_restricted_net_fixture(_metadata, &srv_max_allowed, false, false,
+				    false);
 }
 
 FIXTURE(ipv4_tcp)
@@ -1485,22 +1503,21 @@ FIXTURE_TEARDOWN(ipv4_tcp)
 TEST_F(ipv4_tcp, port_endianness)
 {
 	const struct landlock_ruleset_attr ruleset_attr = {
-		.handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP |
-				      LANDLOCK_ACCESS_NET_CONNECT_TCP,
+		.handled_access_net = ACCESS_ALL,
 	};
 	const struct landlock_net_port_attr bind_host_endian_p0 = {
 		.allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP,
 		/* Host port format. */
 		.port = self->srv0.port,
 	};
-	const struct landlock_net_port_attr connect_big_endian_p0 = {
-		.allowed_access = LANDLOCK_ACCESS_NET_CONNECT_TCP,
+	const struct landlock_net_port_attr connect_listen_big_endian_p0 = {
+		.allowed_access = LANDLOCK_ACCESS_NET_CONNECT_TCP |
+				  LANDLOCK_ACCESS_NET_LISTEN_TCP,
 		/* Big endian port format. */
 		.port = htons(self->srv0.port),
 	};
-	const struct landlock_net_port_attr bind_connect_host_endian_p1 = {
-		.allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP |
-				  LANDLOCK_ACCESS_NET_CONNECT_TCP,
+	const struct landlock_net_port_attr not_restricted_host_endian_p1 = {
+		.allowed_access = ACCESS_ALL,
 		/* Host port format. */
 		.port = self->srv1.port,
 	};
@@ -1514,16 +1531,18 @@ TEST_F(ipv4_tcp, port_endianness)
 	ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT,
 				       &bind_host_endian_p0, 0));
 	ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT,
-				       &connect_big_endian_p0, 0));
+				       &connect_listen_big_endian_p0, 0));
 	ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT,
-				       &bind_connect_host_endian_p1, 0));
+				       &not_restricted_host_endian_p1, 0));
 	enforce_ruleset(_metadata, ruleset_fd);
 
 	/* No restriction for big endinan CPU. */
-	test_bind_and_connect(_metadata, &self->srv0, false, little_endian);
+	test_restricted_net_fixture(_metadata, &self->srv0, false,
+				    little_endian, little_endian);
 
 	/* No restriction for any CPU. */
-	test_bind_and_connect(_metadata, &self->srv1, false, false);
+	test_restricted_net_fixture(_metadata, &self->srv1, false, false,
+				    false);
 }
 
 TEST_F(ipv4_tcp, with_fs)
@@ -1691,7 +1710,7 @@ TEST_F(port_specific, bind_connect_zero)
 	ret = bind_variant(bind_fd, &self->srv0);
 	EXPECT_EQ(0, ret);
 
-	EXPECT_EQ(0, listen(bind_fd, backlog));
+	EXPECT_EQ(0, listen_variant(bind_fd, backlog));
 
 	/* Connects on port 0. */
 	ret = connect_variant(connect_fd, &self->srv0);
@@ -1714,26 +1733,23 @@ TEST_F(port_specific, bind_connect_zero)
 	EXPECT_EQ(0, close(bind_fd));
 }
 
-TEST_F(port_specific, bind_connect_1023)
+TEST_F(port_specific, port_1023)
 {
 	int bind_fd, connect_fd, ret;
 
-	/* Adds a rule layer with bind and connect actions. */
+	/* Adds a rule layer with all actions. */
 	if (variant->sandbox == TCP_SANDBOX) {
 		const struct landlock_ruleset_attr ruleset_attr = {
-			.handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP |
-					      LANDLOCK_ACCESS_NET_CONNECT_TCP
+			.handled_access_net = ACCESS_ALL
 		};
 		/* A rule with port value less than 1024. */
-		const struct landlock_net_port_attr tcp_bind_connect_low_range = {
-			.allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP |
-					  LANDLOCK_ACCESS_NET_CONNECT_TCP,
+		const struct landlock_net_port_attr tcp_low_range_port = {
+			.allowed_access = ACCESS_ALL,
 			.port = 1023,
 		};
 		/* A rule with 1024 port. */
-		const struct landlock_net_port_attr tcp_bind_connect = {
-			.allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP |
-					  LANDLOCK_ACCESS_NET_CONNECT_TCP,
+		const struct landlock_net_port_attr tcp_port_1024 = {
+			.allowed_access = ACCESS_ALL,
 			.port = 1024,
 		};
 		int ruleset_fd;
@@ -1744,10 +1760,10 @@ TEST_F(port_specific, bind_connect_1023)
 
 		ASSERT_EQ(0,
 			  landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT,
-					    &tcp_bind_connect_low_range, 0));
+					    &tcp_low_range_port, 0));
 		ASSERT_EQ(0,
 			  landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT,
-					    &tcp_bind_connect, 0));
+					    &tcp_port_1024, 0));
 
 		enforce_ruleset(_metadata, ruleset_fd);
 		EXPECT_EQ(0, close(ruleset_fd));
@@ -1771,7 +1787,7 @@ TEST_F(port_specific, bind_connect_1023)
 	ret = bind_variant(bind_fd, &self->srv0);
 	clear_cap(_metadata, CAP_NET_BIND_SERVICE);
 	EXPECT_EQ(0, ret);
-	EXPECT_EQ(0, listen(bind_fd, backlog));
+	EXPECT_EQ(0, listen_variant(bind_fd, backlog));
 
 	/* Connects on the binded port 1023. */
 	ret = connect_variant(connect_fd, &self->srv0);
@@ -1791,7 +1807,7 @@ TEST_F(port_specific, bind_connect_1023)
 	/* Binds on port 1024. */
 	ret = bind_variant(bind_fd, &self->srv0);
 	EXPECT_EQ(0, ret);
-	EXPECT_EQ(0, listen(bind_fd, backlog));
+	EXPECT_EQ(0, listen_variant(bind_fd, backlog));
 
 	/* Connects on the binded port 1024. */
 	ret = connect_variant(connect_fd, &self->srv0);
-- 
2.34.1

[RFC PATCH v1 4/9] selftests/landlock: Test listening restriction

[Mikhail Ivanov]

Add a test for listening restriction. It's similar to protocol.bind and
protocol.connect tests.

Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
---
 tools/testing/selftests/landlock/net_test.c | 44 +++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/tools/testing/selftests/landlock/net_test.c b/tools/testing/selftests/landlock/net_test.c
index 8126f5c0160f..b6fe9bde205f 100644
--- a/tools/testing/selftests/landlock/net_test.c
+++ b/tools/testing/selftests/landlock/net_test.c
@@ -689,6 +689,50 @@ TEST_F(protocol, connect)
 				    restricted, restricted);
 }
 
+TEST_F(protocol, listen)
+{
+	if (variant->sandbox == TCP_SANDBOX) {
+		const struct landlock_ruleset_attr ruleset_attr = {
+			.handled_access_net = ACCESS_ALL,
+		};
+		const struct landlock_net_port_attr tcp_not_restricted_p0 = {
+			.allowed_access = ACCESS_ALL,
+			.port = self->srv0.port,
+		};
+		const struct landlock_net_port_attr tcp_denied_listen_p1 = {
+			.allowed_access = ACCESS_ALL &
+					  ~LANDLOCK_ACCESS_NET_LISTEN_TCP,
+			.port = self->srv1.port,
+		};
+		int ruleset_fd;
+
+		ruleset_fd = landlock_create_ruleset(&ruleset_attr,
+						     sizeof(ruleset_attr), 0);
+		ASSERT_LE(0, ruleset_fd);
+
+		/* Allows all actions for the first port. */
+		ASSERT_EQ(0,
+			  landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT,
+					    &tcp_not_restricted_p0, 0));
+
+		/* Allows all actions despite listen. */
+		ASSERT_EQ(0,
+			  landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT,
+					    &tcp_denied_listen_p1, 0));
+
+		enforce_ruleset(_metadata, ruleset_fd);
+		EXPECT_EQ(0, close(ruleset_fd));
+	}
+	bool restricted = is_restricted(&variant->prot, variant->sandbox);
+
+	test_restricted_net_fixture(_metadata, &self->srv0, false, false,
+				    false);
+	test_restricted_net_fixture(_metadata, &self->srv1, false, false,
+				    restricted);
+	test_restricted_net_fixture(_metadata, &self->srv2, restricted,
+				    restricted, restricted);
+}
+
 TEST_F(protocol, bind_unspec)
 {
 	const struct landlock_ruleset_attr ruleset_attr = {
-- 
2.34.1

[RFC PATCH v1 5/9] selftests/landlock: Test listen on connected socket

[Mikhail Ivanov]

Test checks that listen(2) doesn't wrongfully return -EACCES instead
of -EINVAL when trying to listen for an incorrect socket state.

Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
---
 tools/testing/selftests/landlock/net_test.c | 65 +++++++++++++++++++++
 1 file changed, 65 insertions(+)

diff --git a/tools/testing/selftests/landlock/net_test.c b/tools/testing/selftests/landlock/net_test.c
index b6fe9bde205f..a8385f1373f6 100644
--- a/tools/testing/selftests/landlock/net_test.c
+++ b/tools/testing/selftests/landlock/net_test.c
@@ -1644,6 +1644,71 @@ TEST_F(ipv4_tcp, with_fs)
 	EXPECT_EQ(-EACCES, bind_variant(bind_fd, &self->srv1));
 }
 
+TEST_F(ipv4_tcp, listen_on_connected)
+{
+	const struct landlock_ruleset_attr ruleset_attr = {
+		.handled_access_net = ACCESS_ALL,
+	};
+	const struct landlock_net_port_attr tcp_not_restricted_p0 = {
+		.allowed_access = ACCESS_ALL,
+		.port = self->srv0.port,
+	};
+	const struct landlock_net_port_attr tcp_denied_listen_p1 = {
+		.allowed_access = ACCESS_ALL & ~LANDLOCK_ACCESS_NET_LISTEN_TCP,
+		.port = self->srv1.port,
+	};
+	int ruleset_fd;
+	int bind_fd, status;
+	pid_t child;
+
+	ruleset_fd =
+		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
+	ASSERT_LE(0, ruleset_fd);
+
+	/* Allows all actions for the first port. */
+	ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT,
+				       &tcp_not_restricted_p0, 0));
+
+	/* Deny listen for the second port. */
+	ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT,
+				       &tcp_denied_listen_p1, 0));
+
+	enforce_ruleset(_metadata, ruleset_fd);
+	EXPECT_EQ(0, close(ruleset_fd));
+
+	/* Init listening socket. */
+	bind_fd = socket_variant(&self->srv0);
+	ASSERT_LE(0, bind_fd);
+	EXPECT_EQ(0, bind_variant(bind_fd, &self->srv0));
+	EXPECT_EQ(0, listen_variant(bind_fd, backlog));
+
+	child = fork();
+	ASSERT_LE(0, child);
+	if (child == 0) {
+		int connect_fd;
+
+		/* Closes listening socket for the child. */
+		EXPECT_EQ(0, close(bind_fd));
+
+		connect_fd = socket_variant(&self->srv1);
+		ASSERT_LE(0, connect_fd);
+		EXPECT_EQ(0, connect_variant(connect_fd, &self->srv0));
+
+		/* Tries to listen on connected socket. */
+		EXPECT_EQ(-EINVAL, listen_variant(connect_fd, backlog));
+
+		EXPECT_EQ(0, close(connect_fd));
+		_exit(_metadata->exit_code);
+		return;
+	}
+
+	EXPECT_EQ(child, waitpid(child, &status, 0));
+	EXPECT_EQ(1, WIFEXITED(status));
+	EXPECT_EQ(EXIT_SUCCESS, WEXITSTATUS(status));
+
+	EXPECT_EQ(0, close(bind_fd));
+}
+
 FIXTURE(port_specific)
 {
 	struct service_fixture srv0;
-- 
2.34.1

Re: [RFC PATCH v1 5/9] selftests/landlock: Test listen on connected socket

[Mickaël Salaün]

On Sun, Jul 28, 2024 at 08:25:58AM +0800, Mikhail Ivanov wrote:
> Test checks that listen(2) doesn't wrongfully return -EACCES instead
> of -EINVAL when trying to listen for an incorrect socket state.
> 
> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>

Good to have this test!

> ---
>  tools/testing/selftests/landlock/net_test.c | 65 +++++++++++++++++++++
>  1 file changed, 65 insertions(+)
> 
> diff --git a/tools/testing/selftests/landlock/net_test.c b/tools/testing/selftests/landlock/net_test.c
> index b6fe9bde205f..a8385f1373f6 100644
> --- a/tools/testing/selftests/landlock/net_test.c
> +++ b/tools/testing/selftests/landlock/net_test.c
> @@ -1644,6 +1644,71 @@ TEST_F(ipv4_tcp, with_fs)
>  	EXPECT_EQ(-EACCES, bind_variant(bind_fd, &self->srv1));
>  }
>  
> +TEST_F(ipv4_tcp, listen_on_connected)

We should use the "protocol" fixture and its variants instead to test
with different protocols and also without sandboxing (which is crutial).

I guess espintcp_listen should use "protocol" too.

ipv4_tcp is to run tests that only make sense on an IPv4 socket, but
when we test EINVAL, we should make sure Landlock doesn't introduce
inconsistencies for other/unsupported protocols.

> +{
> +	const struct landlock_ruleset_attr ruleset_attr = {
> +		.handled_access_net = ACCESS_ALL,
> +	};
> +	const struct landlock_net_port_attr tcp_not_restricted_p0 = {
> +		.allowed_access = ACCESS_ALL,
> +		.port = self->srv0.port,
> +	};
> +	const struct landlock_net_port_attr tcp_denied_listen_p1 = {
> +		.allowed_access = ACCESS_ALL & ~LANDLOCK_ACCESS_NET_LISTEN_TCP,
> +		.port = self->srv1.port,
> +	};
> +	int ruleset_fd;
> +	int bind_fd, status;
> +	pid_t child;
> +
> +	ruleset_fd =
> +		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
> +	ASSERT_LE(0, ruleset_fd);
> +
> +	/* Allows all actions for the first port. */
> +	ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT,
> +				       &tcp_not_restricted_p0, 0));
> +
> +	/* Deny listen for the second port. */

nit: Denies listening

> +	ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT,
> +				       &tcp_denied_listen_p1, 0));
> +
> +	enforce_ruleset(_metadata, ruleset_fd);
> +	EXPECT_EQ(0, close(ruleset_fd));
> +
> +	/* Init listening socket. */

nit: Initializes

> +	bind_fd = socket_variant(&self->srv0);
> +	ASSERT_LE(0, bind_fd);
> +	EXPECT_EQ(0, bind_variant(bind_fd, &self->srv0));
> +	EXPECT_EQ(0, listen_variant(bind_fd, backlog));
> +
> +	child = fork();
> +	ASSERT_LE(0, child);
> +	if (child == 0) {
> +		int connect_fd;
> +
> +		/* Closes listening socket for the child. */
> +		EXPECT_EQ(0, close(bind_fd));
> +
> +		connect_fd = socket_variant(&self->srv1);
> +		ASSERT_LE(0, connect_fd);
> +		EXPECT_EQ(0, connect_variant(connect_fd, &self->srv0));
> +
> +		/* Tries to listen on connected socket. */
> +		EXPECT_EQ(-EINVAL, listen_variant(connect_fd, backlog));
> +
> +		EXPECT_EQ(0, close(connect_fd));
> +		_exit(_metadata->exit_code);
> +		return;
> +	}
> +
> +	EXPECT_EQ(child, waitpid(child, &status, 0));
> +	EXPECT_EQ(1, WIFEXITED(status));
> +	EXPECT_EQ(EXIT_SUCCESS, WEXITSTATUS(status));
> +
> +	EXPECT_EQ(0, close(bind_fd));
> +}
> +
>  FIXTURE(port_specific)
>  {
>  	struct service_fixture srv0;
> -- 
> 2.34.1
> 
> 

Re: [RFC PATCH v1 5/9] selftests/landlock: Test listen on connected socket

[Mikhail Ivanov]

8/1/2024 5:46 PM, Mickaël Salaün wrote:
> On Sun, Jul 28, 2024 at 08:25:58AM +0800, Mikhail Ivanov wrote:
>> Test checks that listen(2) doesn't wrongfully return -EACCES instead
>> of -EINVAL when trying to listen for an incorrect socket state.
>>
>> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
> 
> Good to have this test!
> 
>> ---
>>   tools/testing/selftests/landlock/net_test.c | 65 +++++++++++++++++++++
>>   1 file changed, 65 insertions(+)
>>
>> diff --git a/tools/testing/selftests/landlock/net_test.c b/tools/testing/selftests/landlock/net_test.c
>> index b6fe9bde205f..a8385f1373f6 100644
>> --- a/tools/testing/selftests/landlock/net_test.c
>> +++ b/tools/testing/selftests/landlock/net_test.c
>> @@ -1644,6 +1644,71 @@ TEST_F(ipv4_tcp, with_fs)
>>   	EXPECT_EQ(-EACCES, bind_variant(bind_fd, &self->srv1));
>>   }
>>   
>> +TEST_F(ipv4_tcp, listen_on_connected)
> 
> We should use the "protocol" fixture and its variants instead to test
> with different protocols and also without sandboxing (which is crutial).
> 
> I guess espintcp_listen should use "protocol" too.
> 
> ipv4_tcp is to run tests that only make sense on an IPv4 socket, but
> when we test EINVAL, we should make sure Landlock doesn't introduce
> inconsistencies for other/unsupported protocols.

Makes sense, let's use "protocol".

> 
>> +{
>> +	const struct landlock_ruleset_attr ruleset_attr = {
>> +		.handled_access_net = ACCESS_ALL,
>> +	};
>> +	const struct landlock_net_port_attr tcp_not_restricted_p0 = {
>> +		.allowed_access = ACCESS_ALL,
>> +		.port = self->srv0.port,
>> +	};
>> +	const struct landlock_net_port_attr tcp_denied_listen_p1 = {
>> +		.allowed_access = ACCESS_ALL & ~LANDLOCK_ACCESS_NET_LISTEN_TCP,
>> +		.port = self->srv1.port,
>> +	};
>> +	int ruleset_fd;
>> +	int bind_fd, status;
>> +	pid_t child;
>> +
>> +	ruleset_fd =
>> +		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
>> +	ASSERT_LE(0, ruleset_fd);
>> +
>> +	/* Allows all actions for the first port. */
>> +	ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT,
>> +				       &tcp_not_restricted_p0, 0));
>> +
>> +	/* Deny listen for the second port. */
> 
> nit: Denies listening

will be fixed

> 
>> +	ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT,
>> +				       &tcp_denied_listen_p1, 0));
>> +
>> +	enforce_ruleset(_metadata, ruleset_fd);
>> +	EXPECT_EQ(0, close(ruleset_fd));
>> +
>> +	/* Init listening socket. */
> 
> nit: Initializes

will be fixed

> 
>> +	bind_fd = socket_variant(&self->srv0);
>> +	ASSERT_LE(0, bind_fd);
>> +	EXPECT_EQ(0, bind_variant(bind_fd, &self->srv0));
>> +	EXPECT_EQ(0, listen_variant(bind_fd, backlog));
>> +
>> +	child = fork();
>> +	ASSERT_LE(0, child);
>> +	if (child == 0) {
>> +		int connect_fd;
>> +
>> +		/* Closes listening socket for the child. */
>> +		EXPECT_EQ(0, close(bind_fd));
>> +
>> +		connect_fd = socket_variant(&self->srv1);
>> +		ASSERT_LE(0, connect_fd);
>> +		EXPECT_EQ(0, connect_variant(connect_fd, &self->srv0));
>> +
>> +		/* Tries to listen on connected socket. */
>> +		EXPECT_EQ(-EINVAL, listen_variant(connect_fd, backlog));
>> +
>> +		EXPECT_EQ(0, close(connect_fd));
>> +		_exit(_metadata->exit_code);
>> +		return;
>> +	}
>> +
>> +	EXPECT_EQ(child, waitpid(child, &status, 0));
>> +	EXPECT_EQ(1, WIFEXITED(status));
>> +	EXPECT_EQ(EXIT_SUCCESS, WEXITSTATUS(status));
>> +
>> +	EXPECT_EQ(0, close(bind_fd));
>> +}
>> +
>>   FIXTURE(port_specific)
>>   {
>>   	struct service_fixture srv0;
>> -- 
>> 2.34.1
>>
>>

[RFC PATCH v1 6/9] selftests/landlock: Test listening without explicit bind restriction

[Mikhail Ivanov]

Test scenarios where listen(2) call without explicit bind(2) is allowed
and forbidden.

Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
---
 tools/testing/selftests/landlock/net_test.c | 83 +++++++++++++++++++++
 1 file changed, 83 insertions(+)

diff --git a/tools/testing/selftests/landlock/net_test.c b/tools/testing/selftests/landlock/net_test.c
index a8385f1373f6..1a4c4d1cabc2 100644
--- a/tools/testing/selftests/landlock/net_test.c
+++ b/tools/testing/selftests/landlock/net_test.c
@@ -1842,6 +1842,89 @@ TEST_F(port_specific, bind_connect_zero)
 	EXPECT_EQ(0, close(bind_fd));
 }
 
+TEST_F(port_specific, listen_without_bind_allowed)
+{
+	if (variant->sandbox == TCP_SANDBOX) {
+		const struct landlock_ruleset_attr ruleset_attr = {
+			.handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP |
+					      LANDLOCK_ACCESS_NET_LISTEN_TCP
+		};
+		const struct landlock_net_port_attr tcp_listen_zero = {
+			.allowed_access = LANDLOCK_ACCESS_NET_LISTEN_TCP,
+			.port = 0,
+		};
+		int ruleset_fd;
+
+		ruleset_fd = landlock_create_ruleset(&ruleset_attr,
+						     sizeof(ruleset_attr), 0);
+		ASSERT_LE(0, ruleset_fd);
+
+		/*
+		 * Allow listening without explicit bind
+		 * (cf. landlock_net_port_attr).
+		 */
+		EXPECT_EQ(0,
+			  landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT,
+					    &tcp_listen_zero, 0));
+
+		enforce_ruleset(_metadata, ruleset_fd);
+		EXPECT_EQ(0, close(ruleset_fd));
+	}
+	int listen_fd, connect_fd;
+	__u64 port;
+
+	listen_fd = socket_variant(&self->srv0);
+	ASSERT_LE(0, listen_fd);
+
+	connect_fd = socket_variant(&self->srv0);
+	ASSERT_LE(0, connect_fd);
+	/*
+	 * Allow listen(2) to select a random port for the socket,
+	 * since bind(2) wasn't called.
+	 */
+	EXPECT_EQ(0, listen_variant(listen_fd, backlog));
+
+	/* Connects on the binded port. */
+	port = get_binded_port(listen_fd, &variant->prot);
+	EXPECT_NE(0, port);
+	set_port(&self->srv0, port);
+	EXPECT_EQ(0, connect_variant(connect_fd, &self->srv0));
+
+	EXPECT_EQ(0, close(connect_fd));
+	EXPECT_EQ(0, close(listen_fd));
+}
+
+TEST_F(port_specific, listen_without_bind_denied)
+{
+	if (variant->sandbox == TCP_SANDBOX) {
+		const struct landlock_ruleset_attr ruleset_attr = {
+			.handled_access_net = LANDLOCK_ACCESS_NET_LISTEN_TCP
+		};
+		int ruleset_fd;
+
+		ruleset_fd = landlock_create_ruleset(&ruleset_attr,
+						     sizeof(ruleset_attr), 0);
+		ASSERT_LE(0, ruleset_fd);
+
+		/* Deny listening. */
+		enforce_ruleset(_metadata, ruleset_fd);
+		EXPECT_EQ(0, close(ruleset_fd));
+	}
+	int listen_fd, ret;
+
+	listen_fd = socket_variant(&self->srv0);
+	ASSERT_LE(0, listen_fd);
+
+	/* Checks that listening without explicit binding is prohibited. */
+	ret = listen_variant(listen_fd, backlog);
+	if (is_restricted(&variant->prot, variant->sandbox)) {
+		/* Denied by Landlock. */
+		EXPECT_EQ(-EACCES, ret);
+	} else {
+		EXPECT_EQ(0, ret);
+	}
+}
+
 TEST_F(port_specific, port_1023)
 {
 	int bind_fd, connect_fd, ret;
-- 
2.34.1

[RFC PATCH v1 7/9] selftests/landlock: Test listen on ULP socket without clone method

[Mikhail Ivanov]

Test checks that listen(2) doesn't wrongfully return -EACCES instead of
-EINVAL when trying to listen on a socket which is set to ULP that doesn't
have clone method in inet_csk(sk)->icsk_ulp_ops (espintcp).

Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
---
 tools/testing/selftests/landlock/config     |  1 +
 tools/testing/selftests/landlock/net_test.c | 38 +++++++++++++++++++++
 2 files changed, 39 insertions(+)

diff --git a/tools/testing/selftests/landlock/config b/tools/testing/selftests/landlock/config
index 0086efaa7b68..014401fe6114 100644
--- a/tools/testing/selftests/landlock/config
+++ b/tools/testing/selftests/landlock/config
@@ -12,3 +12,4 @@ CONFIG_SHMEM=y
 CONFIG_SYSFS=y
 CONFIG_TMPFS=y
 CONFIG_TMPFS_XATTR=y
+CONFIG_INET_ESPINTCP=y
\ No newline at end of file
diff --git a/tools/testing/selftests/landlock/net_test.c b/tools/testing/selftests/landlock/net_test.c
index 1a4c4d1cabc2..caf5f38996ed 100644
--- a/tools/testing/selftests/landlock/net_test.c
+++ b/tools/testing/selftests/landlock/net_test.c
@@ -12,6 +12,7 @@
 #include <fcntl.h>
 #include <linux/landlock.h>
 #include <linux/in.h>
+#include <linux/tcp.h>
 #include <sched.h>
 #include <stdint.h>
 #include <string.h>
@@ -1709,6 +1710,43 @@ TEST_F(ipv4_tcp, listen_on_connected)
 	EXPECT_EQ(0, close(bind_fd));
 }
 
+TEST_F(ipv4_tcp, espintcp_listen)
+{
+	const struct landlock_ruleset_attr ruleset_attr = {
+		.handled_access_net = ACCESS_ALL,
+	};
+	const struct landlock_net_port_attr tcp_denied_listen_p0 = {
+		.allowed_access = ACCESS_ALL & ~LANDLOCK_ACCESS_NET_LISTEN_TCP,
+		.port = self->srv0.port,
+	};
+	int ruleset_fd;
+	int listen_fd;
+
+	ruleset_fd =
+		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
+	ASSERT_LE(0, ruleset_fd);
+
+	/* Deny listen. */
+	ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT,
+				       &tcp_denied_listen_p0, 0));
+
+	enforce_ruleset(_metadata, ruleset_fd);
+	EXPECT_EQ(0, close(ruleset_fd));
+
+	listen_fd = socket_variant(&self->srv0);
+	ASSERT_LE(0, listen_fd);
+
+	/* Set espintcp ULP. */
+	EXPECT_EQ(0, setsockopt(listen_fd, IPPROTO_TCP, TCP_ULP, "espintcp",
+				sizeof("espintcp")));
+
+	EXPECT_EQ(0, bind_variant(listen_fd, &self->srv0));
+
+	/* Espintcp ULP doesn't have clone method, so listen is denied. */
+	EXPECT_EQ(-EINVAL, listen_variant(listen_fd, backlog));
+	EXPECT_EQ(0, close(listen_fd));
+}
+
 FIXTURE(port_specific)
 {
 	struct service_fixture srv0;
-- 
2.34.1

Re: [RFC PATCH v1 7/9] selftests/landlock: Test listen on ULP socket without clone method

[Mickaël Salaün]

On Sun, Jul 28, 2024 at 08:26:00AM +0800, Mikhail Ivanov wrote:
> Test checks that listen(2) doesn't wrongfully return -EACCES instead of
> -EINVAL when trying to listen on a socket which is set to ULP that doesn't
> have clone method in inet_csk(sk)->icsk_ulp_ops (espintcp).
> 
> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
> ---
>  tools/testing/selftests/landlock/config     |  1 +
>  tools/testing/selftests/landlock/net_test.c | 38 +++++++++++++++++++++
>  2 files changed, 39 insertions(+)
> 
> diff --git a/tools/testing/selftests/landlock/config b/tools/testing/selftests/landlock/config
> index 0086efaa7b68..014401fe6114 100644
> --- a/tools/testing/selftests/landlock/config
> +++ b/tools/testing/selftests/landlock/config
> @@ -12,3 +12,4 @@ CONFIG_SHMEM=y
>  CONFIG_SYSFS=y
>  CONFIG_TMPFS=y
>  CONFIG_TMPFS_XATTR=y
> +CONFIG_INET_ESPINTCP=y
> \ No newline at end of file

There are missing dependencies, and also please sort entries. I think it should
be:

 CONFIG_CGROUPS=y
 CONFIG_CGROUP_SCHED=y
 CONFIG_INET=y
+CONFIG_INET_ESPINTCP=y
+CONFIG_INET_ESP=y
 CONFIG_IPV6=y
+CONFIG_IPV6_ESP=y
+CONFIG_INET6_ESPINTCP=y
 CONFIG_NET=y
 CONFIG_NET_NS=y
 CONFIG_OVERLAY_FS=y

This works with check-linux.sh from
https://github.com/landlock-lsm/landlock-test-tools

IPv6 is currently not tested, which should be the case (with the "protocol"
variants).

Re: [RFC PATCH v1 7/9] selftests/landlock: Test listen on ULP socket without clone method

[Mikhail Ivanov]

8/1/2024 6:08 PM, Mickaël Salaün wrote:
> On Sun, Jul 28, 2024 at 08:26:00AM +0800, Mikhail Ivanov wrote:
>> Test checks that listen(2) doesn't wrongfully return -EACCES instead of
>> -EINVAL when trying to listen on a socket which is set to ULP that doesn't
>> have clone method in inet_csk(sk)->icsk_ulp_ops (espintcp).
>>
>> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
>> ---
>>   tools/testing/selftests/landlock/config     |  1 +
>>   tools/testing/selftests/landlock/net_test.c | 38 +++++++++++++++++++++
>>   2 files changed, 39 insertions(+)
>>
>> diff --git a/tools/testing/selftests/landlock/config b/tools/testing/selftests/landlock/config
>> index 0086efaa7b68..014401fe6114 100644
>> --- a/tools/testing/selftests/landlock/config
>> +++ b/tools/testing/selftests/landlock/config
>> @@ -12,3 +12,4 @@ CONFIG_SHMEM=y
>>   CONFIG_SYSFS=y
>>   CONFIG_TMPFS=y
>>   CONFIG_TMPFS_XATTR=y
>> +CONFIG_INET_ESPINTCP=y
>> \ No newline at end of file
> 
> There are missing dependencies, and also please sort entries. I think it should
> be:
> 
>   CONFIG_CGROUPS=y
>   CONFIG_CGROUP_SCHED=y
>   CONFIG_INET=y
> +CONFIG_INET_ESPINTCP=y
> +CONFIG_INET_ESP=y
>   CONFIG_IPV6=y
> +CONFIG_IPV6_ESP=y
> +CONFIG_INET6_ESPINTCP=y
>   CONFIG_NET=y
>   CONFIG_NET_NS=y
>   CONFIG_OVERLAY_FS=y
> 
> This works with check-linux.sh from
> https://github.com/landlock-lsm/landlock-test-tools

Thanks, I'll fix this.

> 
> IPv6 is currently not tested, which should be the case (with the "protocol"
> variants).

[RFC PATCH v1 8/9] selftests/landlock: Test changing socket backlog with listen(2)

[Mikhail Ivanov]

listen(2) can be used to change length of the pending connections queue
of the listening socket. Such scenario shouldn't be restricted by Landlock
since socket doesn't change its state.

* Implement test that validates this case.

Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
---
 tools/testing/selftests/landlock/net_test.c | 26 +++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/tools/testing/selftests/landlock/net_test.c b/tools/testing/selftests/landlock/net_test.c
index caf5f38996ed..31ab7e7442e4 100644
--- a/tools/testing/selftests/landlock/net_test.c
+++ b/tools/testing/selftests/landlock/net_test.c
@@ -1747,6 +1747,32 @@ TEST_F(ipv4_tcp, espintcp_listen)
 	EXPECT_EQ(0, close(listen_fd));
 }
 
+TEST_F(ipv4_tcp, double_listen)
+{
+	const struct landlock_ruleset_attr ruleset_attr = {
+		.handled_access_net = LANDLOCK_ACCESS_NET_LISTEN_TCP,
+	};
+	int ruleset_fd;
+	int listen_fd;
+
+	listen_fd = socket_variant(&self->srv0);
+	ASSERT_LE(0, listen_fd);
+
+	EXPECT_EQ(0, bind_variant(listen_fd, &self->srv0));
+	EXPECT_EQ(0, listen_variant(listen_fd, backlog));
+
+	ruleset_fd =
+		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
+	ASSERT_LE(0, ruleset_fd);
+
+	/* Denies listen. */
+	enforce_ruleset(_metadata, ruleset_fd);
+	EXPECT_EQ(0, close(ruleset_fd));
+
+	/* Tries to change backlog value of listening socket. */
+	EXPECT_EQ(0, listen_variant(listen_fd, backlog + 1));
+}
+
 FIXTURE(port_specific)
 {
 	struct service_fixture srv0;
-- 
2.34.1

[RFC PATCH v1 9/9] samples/landlock: Support LANDLOCK_ACCESS_NET_LISTEN

[Mikhail Ivanov]

Extend sample with TCP listen control logic.

Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com>
---
 samples/landlock/sandboxer.c | 31 ++++++++++++++++++++++++++-----
 1 file changed, 26 insertions(+), 5 deletions(-)

diff --git a/samples/landlock/sandboxer.c b/samples/landlock/sandboxer.c
index e8223c3e781a..3f50cb3f8039 100644
--- a/samples/landlock/sandboxer.c
+++ b/samples/landlock/sandboxer.c
@@ -55,6 +55,7 @@ static inline int landlock_restrict_self(const int ruleset_fd,
 #define ENV_FS_RW_NAME "LL_FS_RW"
 #define ENV_TCP_BIND_NAME "LL_TCP_BIND"
 #define ENV_TCP_CONNECT_NAME "LL_TCP_CONNECT"
+#define ENV_TCP_LISTEN_NAME "LL_TCP_LISTEN"
 #define ENV_DELIMITER ":"
 
 static int parse_path(char *env_path, const char ***const path_list)
@@ -208,7 +209,7 @@ static int populate_ruleset_net(const char *const env_var, const int ruleset_fd,
 
 /* clang-format on */
 
-#define LANDLOCK_ABI_LAST 5
+#define LANDLOCK_ABI_LAST 6
 
 int main(const int argc, char *const argv[], char *const *const envp)
 {
@@ -222,15 +223,16 @@ int main(const int argc, char *const argv[], char *const *const envp)
 	struct landlock_ruleset_attr ruleset_attr = {
 		.handled_access_fs = access_fs_rw,
 		.handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP |
-				      LANDLOCK_ACCESS_NET_CONNECT_TCP,
+				      LANDLOCK_ACCESS_NET_CONNECT_TCP |
+				      LANDLOCK_ACCESS_NET_LISTEN_TCP,
 	};
 
 	if (argc < 2) {
 		fprintf(stderr,
-			"usage: %s=\"...\" %s=\"...\" %s=\"...\" %s=\"...\"%s "
+			"usage: %s=\"...\" %s=\"...\" %s=\"...\" %s=\"...\" %s=\"...\"%s "
 			"<cmd> [args]...\n\n",
 			ENV_FS_RO_NAME, ENV_FS_RW_NAME, ENV_TCP_BIND_NAME,
-			ENV_TCP_CONNECT_NAME, argv[0]);
+			ENV_TCP_CONNECT_NAME, ENV_TCP_LISTEN_NAME, argv[0]);
 		fprintf(stderr,
 			"Execute a command in a restricted environment.\n\n");
 		fprintf(stderr,
@@ -251,15 +253,19 @@ int main(const int argc, char *const argv[], char *const *const envp)
 		fprintf(stderr,
 			"* %s: list of ports allowed to connect (client).\n",
 			ENV_TCP_CONNECT_NAME);
+		fprintf(stderr,
+			"* %s: list of ports allowed to listen (server).\n",
+			ENV_TCP_LISTEN_NAME);
 		fprintf(stderr,
 			"\nexample:\n"
 			"%s=\"${PATH}:/lib:/usr:/proc:/etc:/dev/urandom\" "
 			"%s=\"/dev/null:/dev/full:/dev/zero:/dev/pts:/tmp\" "
 			"%s=\"9418\" "
 			"%s=\"80:443\" "
+			"%s=\"9418\" "
 			"%s bash -i\n\n",
 			ENV_FS_RO_NAME, ENV_FS_RW_NAME, ENV_TCP_BIND_NAME,
-			ENV_TCP_CONNECT_NAME, argv[0]);
+			ENV_TCP_CONNECT_NAME, ENV_TCP_LISTEN_NAME, argv[0]);
 		fprintf(stderr,
 			"This sandboxer can use Landlock features "
 			"up to ABI version %d.\n",
@@ -326,6 +332,11 @@ int main(const int argc, char *const argv[], char *const *const envp)
 	case 4:
 		/* Removes LANDLOCK_ACCESS_FS_IOCTL_DEV for ABI < 5 */
 		ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_IOCTL_DEV;
+		__attribute__((fallthrough));
+	case 5:
+		/* Removes LANDLOCK_ACCESS_NET_LISTEN support for ABI < 6 */
+		ruleset_attr.handled_access_net &=
+			~(LANDLOCK_ACCESS_NET_LISTEN_TCP);
 
 		fprintf(stderr,
 			"Hint: You should update the running kernel "
@@ -357,6 +368,12 @@ int main(const int argc, char *const argv[], char *const *const envp)
 		ruleset_attr.handled_access_net &=
 			~LANDLOCK_ACCESS_NET_CONNECT_TCP;
 	}
+	/* Removes listen access attribute if not supported by a user. */
+	env_port_name = getenv(ENV_TCP_LISTEN_NAME);
+	if (!env_port_name) {
+		ruleset_attr.handled_access_net &=
+			~LANDLOCK_ACCESS_NET_LISTEN_TCP;
+	}
 
 	ruleset_fd =
 		landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
@@ -380,6 +397,10 @@ int main(const int argc, char *const argv[], char *const *const envp)
 				 LANDLOCK_ACCESS_NET_CONNECT_TCP)) {
 		goto err_close_ruleset;
 	}
+	if (populate_ruleset_net(ENV_TCP_LISTEN_NAME, ruleset_fd,
+				 LANDLOCK_ACCESS_NET_LISTEN_TCP)) {
+		goto err_close_ruleset;
+	}
 
 	if (prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)) {
 		perror("Failed to restrict privileges");
-- 
2.34.1