[PATCH 0/2] Forbid illegitimate binding via listen(2)

[PATCH 0/2] Forbid illegitimate binding via listen(2)

[Ivanov Mikhail]

listen(2) can be called without explicit bind(2) call. For a TCP socket
it would result in assigning random port(in some range) to this socket
by the kernel. If Landlock sandbox supports LANDLOCK_ACCESS_NET_BIND_TCP,
this may lead to implicit access to a prohibited (by Landlock sandbox)
port. Malicious sandboxed process can accidentally impersonate a
legitimate server process (if listen(2) assigns it a server port number).

Patch adds hook on socket_listen() that prevents such scenario by checking
LANDLOCK_ACCESS_NET_BIND_TCP access for port 0.

Few tests were added to cover this case.

Code coverage(gcov):
* security/landlock:
lines......: 94.5% (745 of 788 lines)
functions..: 97.1% (100 of 103 functions)

Ivanov Mikhail (2):
  landlock: Add hook on socket_listen()
  selftests/landlock: Create 'listen_zero', 'deny_listen_zero' tests

 security/landlock/net.c                     | 104 +++++++++++++++++---
 tools/testing/selftests/landlock/net_test.c |  89 +++++++++++++++++
 2 files changed, 177 insertions(+), 16 deletions(-)

-- 
2.34.1

[PATCH 1/2] landlock: Add hook on socket_listen()

[Ivanov Mikhail]

Make hook for socket_listen(). It will check that the socket protocol is
TCP, and if the socket's local port number is 0 (which means,
that listen(2) was called without any previous bind(2) call),
then listen(2) call will be legitimate only if there is a rule for bind(2)
allowing binding to port 0 (or if LANDLOCK_ACCESS_NET_BIND_TCP is not
supported by the sandbox).

Create a new check_access_socket() function to prevent useless copy paste.
It should be called by hook handlers after they perform special checks and
calculate socket port value.

Signed-off-by: Ivanov Mikhail <ivanov.mikhail1@huawei-partners.com>
Reviewed-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
---
 security/landlock/net.c | 104 +++++++++++++++++++++++++++++++++-------
 1 file changed, 88 insertions(+), 16 deletions(-)

diff --git a/security/landlock/net.c b/security/landlock/net.c
index c8bcd29bde09..c6ae4092cfd6 100644
--- a/security/landlock/net.c
+++ b/security/landlock/net.c
@@ -10,6 +10,7 @@
 #include <linux/net.h>
 #include <linux/socket.h>
 #include <net/ipv6.h>
+#include <net/tcp.h>
 
 #include "common.h"
 #include "cred.h"
@@ -61,17 +62,36 @@ 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 +179,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,
@@ -187,9 +197,71 @@ static int hook_socket_connect(struct socket *const sock,
 					   LANDLOCK_ACCESS_NET_CONNECT_TCP);
 }
 
+/*
+ * Check that socket state and attributes are correct for listen.
+ * It is required to not wrongfully return -EACCES instead of -EINVAL.
+ */
+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 inet_connection_sock *icsk;
+
+	/* Allow only unconnected TCP socket to listen(cf. inet_listen). */
+	if (sock->state != SS_UNCONNECTED)
+		return -EINVAL;
+
+	/* Check sock state consistency. */
+	if (!((1 << cur_sk_state) & (TCPF_CLOSE | TCPF_LISTEN)))
+		return -EINVAL;
+
+	/* Sockets can listen only if ULP control hook has clone method. */
+	icsk = inet_csk(sk);
+	if (icsk->icsk_ulp_ops && !icsk->icsk_ulp_ops->clone)
+		return -EINVAL;
+	return 0;
+}
+
+static int hook_socket_listen(struct socket *const sock,
+			  const int backlog)
+{
+	int err;
+	int family;
+	const struct landlock_ruleset *const dom = get_current_net_domain();
+
+	if (!dom)
+		return 0;
+	if (WARN_ON_ONCE(dom->num_layers < 1))
+		return -EACCES;
+
+	/*
+	 * listen() on a TCP socket without pre-binding is allowed only
+	 * if binding to port 0 is allowed.
+	 */
+	family = sock->sk->__sk_common.skc_family;
+
+	if (family == AF_INET || family == AF_INET6) {
+		/* Checks if it's a (potential) TCP socket. */
+		if (sock->type != SOCK_STREAM)
+			return 0;
+
+		/* Socket is alredy binded to some port. */
+		if (inet_sk(sock->sk)->inet_num != 0)
+			return 0;
+
+		err = check_tcp_socket_can_listen(sock);
+		if (unlikely(err))
+			return err;
+
+		return check_access_socket(dom, 0, LANDLOCK_ACCESS_NET_BIND_TCP);
+	}
+	return 0;
+}
+
 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)
-- 
2.34.1

Re: [PATCH 1/2] landlock: Add hook on socket_listen()

[Mickaël Salaün]

On Mon, Apr 08, 2024 at 05:47:46PM +0800, Ivanov Mikhail wrote:
> Make hook for socket_listen(). It will check that the socket protocol is
> TCP, and if the socket's local port number is 0 (which means,
> that listen(2) was called without any previous bind(2) call),
> then listen(2) call will be legitimate only if there is a rule for bind(2)
> allowing binding to port 0 (or if LANDLOCK_ACCESS_NET_BIND_TCP is not
> supported by the sandbox).

Thanks for this patch and sorry for the late full review.  The code is
good overall.

We should either consider this patch as a fix or add a new flag/access
right to Landlock syscalls for compatibility reason.  I think this
should be a fix.  Calling listen(2) without a previous call to bind(2)
is a corner case that we should properly handle.  The commit message
should make that explicit and highlight the goal of the patch: first
explain why, and then how.

We also need to update the user documentation to explain that
LANDLOCK_ACCESS_NET_BIND_TCP also handles this case.

> 
> Create a new check_access_socket() function to prevent useless copy paste.
> It should be called by hook handlers after they perform special checks and
> calculate socket port value.

You can add this tag:
Fixes: fff69fb03dde ("landlock: Support network rules with TCP bind and connect")

> 
> Signed-off-by: Ivanov Mikhail <ivanov.mikhail1@huawei-partners.com>
> Reviewed-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
> ---
>  security/landlock/net.c | 104 +++++++++++++++++++++++++++++++++-------
>  1 file changed, 88 insertions(+), 16 deletions(-)
> 
> diff --git a/security/landlock/net.c b/security/landlock/net.c
> index c8bcd29bde09..c6ae4092cfd6 100644
> --- a/security/landlock/net.c
> +++ b/security/landlock/net.c
> @@ -10,6 +10,7 @@
>  #include <linux/net.h>
>  #include <linux/socket.h>
>  #include <net/ipv6.h>
> +#include <net/tcp.h>
>  
>  #include "common.h"
>  #include "cred.h"
> @@ -61,17 +62,36 @@ 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)

Please format all patches with clang-format.

>  {
> -	__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;
> +}

This check_access_socket() refactoring should be in a dedicated patch.

> +
> +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 +179,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,
> @@ -187,9 +197,71 @@ static int hook_socket_connect(struct socket *const sock,
>  					   LANDLOCK_ACCESS_NET_CONNECT_TCP);
>  }
>  
> +/*
> + * Check that socket state and attributes are correct for listen.
> + * It is required to not wrongfully return -EACCES instead of -EINVAL.
> + */
> +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 inet_connection_sock *icsk;
> +
> +	/* Allow only unconnected TCP socket to listen(cf. inet_listen). */

nit: Missing space.

The other comments in Landlock are written with the third person
(in theory everywhere): "Allows..."

> +	if (sock->state != SS_UNCONNECTED)
> +		return -EINVAL;
> +
> +	/* Check sock state consistency. */

Can you explain exactly what is going on here (in the comment)? Linking
to a kernel function would help.

> +	if (!((1 << cur_sk_state) & (TCPF_CLOSE | TCPF_LISTEN)))
> +		return -EINVAL;
> +
> +	/* Sockets can listen only if ULP control hook has clone method. */

What is ULP?

> +	icsk = inet_csk(sk);
> +	if (icsk->icsk_ulp_ops && !icsk->icsk_ulp_ops->clone)
> +		return -EINVAL;

Can you please add tests covering all these error cases?

> +	return 0;
> +}
> +
> +static int hook_socket_listen(struct socket *const sock,
> +			  const int backlog)
> +{
> +	int err;
> +	int family;
> +	const struct landlock_ruleset *const dom = get_current_net_domain();
> +
> +	if (!dom)
> +		return 0;
> +	if (WARN_ON_ONCE(dom->num_layers < 1))
> +		return -EACCES;
> +
> +	/*
> +	 * listen() on a TCP socket without pre-binding is allowed only
> +	 * if binding to port 0 is allowed.
> +	 */

This comment should be just before the inet_sk(sock->sk)->inet_num
check.

> +	family = sock->sk->__sk_common.skc_family;
> +
> +	if (family == AF_INET || family == AF_INET6) {

This would make the code simpler:

if (family != AF_INET && family != AF_INET6)
	return 0;


What would be the effect of listen() on an AF_UNSPEC socket?

> +		/* Checks if it's a (potential) TCP socket. */
> +		if (sock->type != SOCK_STREAM)
> +			return 0;

As for current_check_access_socket() this kind of check should be at the
beginning of the function (before the family check) to exit early and
simplify code.

> +
> +		/* Socket is alredy binded to some port. */

This kind of spelling issue can be found by scripts/checkpatch.pl

> +		if (inet_sk(sock->sk)->inet_num != 0)

Why do we want to allow listen() on any socket that is binded?

> +			return 0;
> +
> +		err = check_tcp_socket_can_listen(sock);
> +		if (unlikely(err))
> +			return err;
> +
> +		return check_access_socket(dom, 0, LANDLOCK_ACCESS_NET_BIND_TCP);
> +	}
> +	return 0;
> +}
> +
>  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)
> -- 
> 2.34.1
> 
> 

Re: [PATCH 1/2] landlock: Add hook on socket_listen()

[Mickaël Salaün]

On Tue, Apr 30, 2024 at 03:36:30PM +0200, Mickaël Salaün wrote:
> On Mon, Apr 08, 2024 at 05:47:46PM +0800, Ivanov Mikhail wrote:
> > Make hook for socket_listen(). It will check that the socket protocol is
> > TCP, and if the socket's local port number is 0 (which means,
> > that listen(2) was called without any previous bind(2) call),
> > then listen(2) call will be legitimate only if there is a rule for bind(2)
> > allowing binding to port 0 (or if LANDLOCK_ACCESS_NET_BIND_TCP is not
> > supported by the sandbox).
> 
> Thanks for this patch and sorry for the late full review.  The code is
> good overall.
> 
> We should either consider this patch as a fix or add a new flag/access
> right to Landlock syscalls for compatibility reason.  I think this
> should be a fix.  Calling listen(2) without a previous call to bind(2)
> is a corner case that we should properly handle.  The commit message
> should make that explicit and highlight the goal of the patch: first
> explain why, and then how.
> 
> We also need to update the user documentation to explain that
> LANDLOCK_ACCESS_NET_BIND_TCP also handles this case.
> 
> > 
> > Create a new check_access_socket() function to prevent useless copy paste.
> > It should be called by hook handlers after they perform special checks and
> > calculate socket port value.
> 
> You can add this tag:
> Fixes: fff69fb03dde ("landlock: Support network rules with TCP bind and connect")
> 
> > 
> > Signed-off-by: Ivanov Mikhail <ivanov.mikhail1@huawei-partners.com>
> > Reviewed-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
> > ---
> >  security/landlock/net.c | 104 +++++++++++++++++++++++++++++++++-------
> >  1 file changed, 88 insertions(+), 16 deletions(-)


> > +		if (inet_sk(sock->sk)->inet_num != 0)
> 
> Why do we want to allow listen() on any socket that is binded?

Please ignore this comment. I was initially thinking about a new access
right, which would be good to have anyway, but with another series.

Re: [PATCH 1/2] landlock: Add hook on socket_listen()

[Ivanov Mikhail]

4/30/2024 4:36 PM, Mickaël Salaün wrote:
> On Mon, Apr 08, 2024 at 05:47:46PM +0800, Ivanov Mikhail wrote:
>> Make hook for socket_listen(). It will check that the socket protocol is
>> TCP, and if the socket's local port number is 0 (which means,
>> that listen(2) was called without any previous bind(2) call),
>> then listen(2) call will be legitimate only if there is a rule for bind(2)
>> allowing binding to port 0 (or if LANDLOCK_ACCESS_NET_BIND_TCP is not
>> supported by the sandbox).
> 
> Thanks for this patch and sorry for the late full review.  The code is
> good overall.
> 
> We should either consider this patch as a fix or add a new flag/access
> right to Landlock syscalls for compatibility reason.  I think this
> should be a fix.  Calling listen(2) without a previous call to bind(2)
> is a corner case that we should properly handle.  The commit message
> should make that explicit and highlight the goal of the patch: first
> explain why, and then how.

Yeap, this is fix-patch. I have covered motivation and proposed solution
in cover letter. Do you have any suggestions on how i can improve this?

> 
> We also need to update the user documentation to explain that
> LANDLOCK_ACCESS_NET_BIND_TCP also handles this case.

ok, i'll update it.

> 
>>
>> Create a new check_access_socket() function to prevent useless copy paste.
>> It should be called by hook handlers after they perform special checks and
>> calculate socket port value.
> 
> You can add this tag:
> Fixes: fff69fb03dde ("landlock: Support network rules with TCP bind and connect")

Yeah, thanks!

> 
>>
>> Signed-off-by: Ivanov Mikhail <ivanov.mikhail1@huawei-partners.com>
>> Reviewed-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
>> ---
>>   security/landlock/net.c | 104 +++++++++++++++++++++++++++++++++-------
>>   1 file changed, 88 insertions(+), 16 deletions(-)
>>
>> diff --git a/security/landlock/net.c b/security/landlock/net.c
>> index c8bcd29bde09..c6ae4092cfd6 100644
>> --- a/security/landlock/net.c
>> +++ b/security/landlock/net.c
>> @@ -10,6 +10,7 @@
>>   #include <linux/net.h>
>>   #include <linux/socket.h>
>>   #include <net/ipv6.h>
>> +#include <net/tcp.h>
>>   
>>   #include "common.h"
>>   #include "cred.h"
>> @@ -61,17 +62,36 @@ 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)
> 
> Please format all patches with clang-format.

will be fixed

> 
>>   {
>> -	__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;
>> +}
> 
> This check_access_socket() refactoring should be in a dedicated patch.

ok, i'll move it.

> 
>> +
>> +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 +179,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,
>> @@ -187,9 +197,71 @@ static int hook_socket_connect(struct socket *const sock,
>>   					   LANDLOCK_ACCESS_NET_CONNECT_TCP);
>>   }
>>   
>> +/*
>> + * Check that socket state and attributes are correct for listen.
>> + * It is required to not wrongfully return -EACCES instead of -EINVAL.
>> + */
>> +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 inet_connection_sock *icsk;
>> +
>> +	/* Allow only unconnected TCP socket to listen(cf. inet_listen). */
> 
> nit: Missing space.

will be fixed

> 
> The other comments in Landlock are written with the third person
> (in theory everywhere): "Allows..."

Indeed, i'll fix comments. Thanks!

> 
>> +	if (sock->state != SS_UNCONNECTED)
>> +		return -EINVAL;
>> +
>> +	/* Check sock state consistency. */
> 
> Can you explain exactly what is going on here (in the comment)? Linking
> to a kernel function would help.

Yeap, i'll fix comment.

> 
>> +	if (!((1 << cur_sk_state) & (TCPF_CLOSE | TCPF_LISTEN)))
>> +		return -EINVAL;
>> +
>> +	/* Sockets can listen only if ULP control hook has clone method. */
> 
> What is ULP?

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). [1]

There is a patch that prevents ULP sockets from listening
if corresponding ULP implementation in linux doesn't have a clone
method. [2]

Landlock shouldn't return EACCES for ULP sockets that cannot listen
due to some ULP restrictions.

[1] 
https://lore.kernel.org/all/20170524162646.GA24128@davejwatson-mba.local/
[2] 
https://lore.kernel.org/all/4b80c3d1dbe3d0ab072f80450c202d9bc88b4b03.1672740602.git.pabeni@redhat.com/

> 
>> +	icsk = inet_csk(sk);
>> +	if (icsk->icsk_ulp_ops && !icsk->icsk_ulp_ops->clone)
>> +		return -EINVAL;
> 
> Can you please add tests covering all these error cases?

Yeap, i'll add a test for first check.

I have not found a way to trigger the second check from userspace.
Since socket wasn't binded to any port, this means that it cannot
be part of a TCP connection in any state, so it has to be in TCPF_CLOSE
state. Nevertheless i think that this check is required:

* for consistency with inet stack (see __inet_listen_sk())

* i have not found any restrictions connected with sock locking
   for TCP-like protocols, so listen(2) can be called after
   sk->sk_prot->connect() method will change sock state in
   __inet_stream_connect() (e.g. to TCP_SYN_SENT). In that case this
   check may be required.

What do you think?
Btw this hook on socket_listen() should be fixed in
order to not check socket that is already in TCP_LISTEN state. Calling
listen(2) only changes backlog value, so landlock shouldn't do anything
in this case.

I'm not sure about ULP checking. I thought of adding test that creates
espintcp ULP (net/xfrm/expintcp.c) socket and tries to listen on it.
Since espintcp doesn't have clone method ULP check will be triggered.
Problem is that kernel doesnt support this ULP module by default and it
should be configured with CONFIG_XFRM_ESPINTCP option enabled. I think
that selftests shouldn't depend on specific kernel configuration to be
fully executed, so probably we should just skip this. What do you think?

> 
>> +	return 0;
>> +}
>> +
>> +static int hook_socket_listen(struct socket *const sock,
>> +			  const int backlog)
>> +{
>> +	int err;
>> +	int family;
>> +	const struct landlock_ruleset *const dom = get_current_net_domain();
>> +
>> +	if (!dom)
>> +		return 0;
>> +	if (WARN_ON_ONCE(dom->num_layers < 1))
>> +		return -EACCES;
>> +
>> +	/*
>> +	 * listen() on a TCP socket without pre-binding is allowed only
>> +	 * if binding to port 0 is allowed.
>> +	 */
> 
> This comment should be just before the inet_sk(sock->sk)->inet_num
> check.

will be fixed

> 
>> +	family = sock->sk->__sk_common.skc_family;
>> +
>> +	if (family == AF_INET || family == AF_INET6) {
> 
> This would make the code simpler:
> 
> if (family != AF_INET && family != AF_INET6)
> 	return 0;

indeed, will be fixed.

> 
> 
> What would be the effect of listen() on an AF_UNSPEC socket?

AF_UNSPEC is a family type that only addresses can use.
Socket itself can only be AF_INET or AF_INET6 in TCP.

> 
>> +		/* Checks if it's a (potential) TCP socket. */
>> +		if (sock->type != SOCK_STREAM)
>> +			return 0;
> 
> As for current_check_access_socket() this kind of check should be at the
> beginning of the function (before the family check) to exit early and
> simplify code.

will be fixed

> 
>> +
>> +		/* Socket is alredy binded to some port. */
> 
> This kind of spelling issue can be found by scripts/checkpatch.pl

will be fixed

> 
>> +		if (inet_sk(sock->sk)->inet_num != 0)
> 
> Why do we want to allow listen() on any socket that is binded?
> 
>> +			return 0;
>> +
>> +		err = check_tcp_socket_can_listen(sock);
>> +		if (unlikely(err))
>> +			return err;
>> +
>> +		return check_access_socket(dom, 0, LANDLOCK_ACCESS_NET_BIND_TCP);
>> +	}
>> +	return 0;
>> +}
>> +
>>   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)
>> -- 
>> 2.34.1
>>
>>

Re: [PATCH 1/2] landlock: Add hook on socket_listen()

[Mickaël Salaün]

On Mon, May 13, 2024 at 03:15:50PM +0300, Ivanov Mikhail wrote:
> 
> 
> 4/30/2024 4:36 PM, Mickaël Salaün wrote:
> > On Mon, Apr 08, 2024 at 05:47:46PM +0800, Ivanov Mikhail wrote:
> > > Make hook for socket_listen(). It will check that the socket protocol is
> > > TCP, and if the socket's local port number is 0 (which means,
> > > that listen(2) was called without any previous bind(2) call),
> > > then listen(2) call will be legitimate only if there is a rule for bind(2)
> > > allowing binding to port 0 (or if LANDLOCK_ACCESS_NET_BIND_TCP is not
> > > supported by the sandbox).
> > 
> > Thanks for this patch and sorry for the late full review.  The code is
> > good overall.
> > 
> > We should either consider this patch as a fix or add a new flag/access
> > right to Landlock syscalls for compatibility reason.  I think this
> > should be a fix.  Calling listen(2) without a previous call to bind(2)
> > is a corner case that we should properly handle.  The commit message
> > should make that explicit and highlight the goal of the patch: first
> > explain why, and then how.
> 
> Yeap, this is fix-patch. I have covered motivation and proposed solution
> in cover letter. Do you have any suggestions on how i can improve this?

You can start this commit message with the same description as in the
cover letter.

[...]

> > 
> > > +	if (!((1 << cur_sk_state) & (TCPF_CLOSE | TCPF_LISTEN)))
> > > +		return -EINVAL;
> > > +
> > > +	/* Sockets can listen only if ULP control hook has clone method. */
> > 
> > What is ULP?
> 
> 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). [1]

OK, you can extend the comment with this information, but no need for
the links.

> 
> There is a patch that prevents ULP sockets from listening
> if corresponding ULP implementation in linux doesn't have a clone
> method. [2]
> 
> Landlock shouldn't return EACCES for ULP sockets that cannot listen
> due to some ULP restrictions.

Looks good.

> 
> [1]
> https://lore.kernel.org/all/20170524162646.GA24128@davejwatson-mba.local/
> [2] https://lore.kernel.org/all/4b80c3d1dbe3d0ab072f80450c202d9bc88b4b03.1672740602.git.pabeni@redhat.com/
> 
> > 
> > > +	icsk = inet_csk(sk);
> > > +	if (icsk->icsk_ulp_ops && !icsk->icsk_ulp_ops->clone)
> > > +		return -EINVAL;
> > 
> > Can you please add tests covering all these error cases?
> 
> Yeap, i'll add a test for first check.
> 
> I have not found a way to trigger the second check from userspace.
> Since socket wasn't binded to any port, this means that it cannot
> be part of a TCP connection in any state, so it has to be in TCPF_CLOSE

If you're sure this cannot be triggered from user space, you can wrap
the test with WARN_ON_ONCE(), but we need to be careful.  I'd like to
get the point of view of kernel network expert though.

Eric, is this assumption correct?

> state. Nevertheless i think that this check is required:
> 
> * for consistency with inet stack (see __inet_listen_sk())
> 
> * i have not found any restrictions connected with sock locking
>   for TCP-like protocols, so listen(2) can be called after
>   sk->sk_prot->connect() method will change sock state in
>   __inet_stream_connect() (e.g. to TCP_SYN_SENT). In that case this
>   check may be required.
> 
> What do you think?

This looks good, but we need to explain this rationale in comments, with
explicit mention of network stack functions.

> Btw this hook on socket_listen() should be fixed in
> order to not check socket that is already in TCP_LISTEN state. Calling
> listen(2) only changes backlog value, so landlock shouldn't do anything
> in this case.
> 
> I'm not sure about ULP checking. I thought of adding test that creates
> espintcp ULP (net/xfrm/expintcp.c) socket and tries to listen on it.
> Since espintcp doesn't have clone method ULP check will be triggered.
> Problem is that kernel doesnt support this ULP module by default and it
> should be configured with CONFIG_XFRM_ESPINTCP option enabled. I think
> that selftests shouldn't depend on specific kernel configuration to be
> fully executed, so probably we should just skip this. What do you think?

Testing with espintcp makes sense for this clone case.  I hope it would
not require too much boilerplate code though.  We can and should add all
the required kernel option in tools/testing/selftests/landlock/config,
and we should not restrict tests to default kernel options, quite the
contrary if it makes sense.

[...]

> > 
> > > +	family = sock->sk->__sk_common.skc_family;
> > > +
> > > +	if (family == AF_INET || family == AF_INET6) {
> > 
> > This would make the code simpler:
> > 
> > if (family != AF_INET && family != AF_INET6)
> > 	return 0;
> 
> indeed, will be fixed.
> 
> > 
> > 
> > What would be the effect of listen() on an AF_UNSPEC socket?
> 
> AF_UNSPEC is a family type that only addresses can use.
> Socket itself can only be AF_INET or AF_INET6 in TCP.

Indeed, it is worth mentioning in a comment.

Re: [PATCH 1/2] landlock: Add hook on socket_listen()

[Günther Noack]

I agree with Mickaël's comment: this seems like an important fix.

Mostly for completeness: I played with the "socket type" patch set in a "TCP
server" example, where *all* possible operations are restricted with Landlock,
including the ones from the "socket type" patch set V2 with the little fix we
discussed.

 - socket()
 - bind()
 - enforce a landlock ruleset restricting:
   - file system access
   - all TCP bind and connect
   - socket creation
 - listen()
 - accept()

From the connection handler (which would be the place where an attacker can
usually provide input), it is now still possible to bind a socket due to this
problem.  The steps are:

  1) connect() on client_fd with AF_UNSPEC to disassociate the client FD
  2) listen() on the client_fd

This succeeds and it listens on an ephemeral port.

The code is at [1], if you are interested.

[1] https://github.com/gnoack/landlock-examples/blob/main/tcpserver.c


On Mon, May 13, 2024 at 03:15:50PM +0300, Ivanov Mikhail wrote:
> 4/30/2024 4:36 PM, Mickaël Salaün wrote:
> > On Mon, Apr 08, 2024 at 05:47:46PM +0800, Ivanov Mikhail wrote:
> > > Make hook for socket_listen(). It will check that the socket protocol is
> > > TCP, and if the socket's local port number is 0 (which means,
> > > that listen(2) was called without any previous bind(2) call),
> > > then listen(2) call will be legitimate only if there is a rule for bind(2)
> > > allowing binding to port 0 (or if LANDLOCK_ACCESS_NET_BIND_TCP is not
> > > supported by the sandbox).
> > 
> > Thanks for this patch and sorry for the late full review.  The code is
> > good overall.
> > 
> > We should either consider this patch as a fix or add a new flag/access
> > right to Landlock syscalls for compatibility reason.  I think this
> > should be a fix.  Calling listen(2) without a previous call to bind(2)
> > is a corner case that we should properly handle.  The commit message
> > should make that explicit and highlight the goal of the patch: first
> > explain why, and then how.
> 
> Yeap, this is fix-patch. I have covered motivation and proposed solution
> in cover letter. Do you have any suggestions on how i can improve this?

Without wanting to turn around the direction of this code review now, I am still
slightly concerned about the assymetry of this special case being implemented
for listen() but not for connect().

The reason is this: My colleague Mr. B. recently pointed out to me that you can
also do a bind() on a socket before a connect(!). The steps are:

* create socket with socket()
* bind() to a local port 9090
* connect() to a remote port 8080

This gives you a connection between ports 9090 and 8080.

A regular connect() without an explicit bind() is of course the more usual
scenario.  In that case, we are also using up ("implicitly binding") one of the
ephemeral ports.

It seems that, with respect to the port binding, listen() and connect() work
quite similarly then?  This being considered, maybe it *is* the listen()
operation on a port which we should be restricting, and not bind()?

With some luck, that would then also free us from having to implement the
check_tcp_socket_can_listen() logic, which is seemingly emulating logic from
elsewhere in the kernel?

(I am by far not an expert in Linux networking, so I'll put this out for
consideration and will happily stand corrected if I am misunderstanding
something.  Maybe someone with more networking background can chime in?)


> > > +		/* Socket is alredy binded to some port. */
> > 
> > This kind of spelling issue can be found by scripts/checkpatch.pl
> 
> will be fixed

P.S. there are two typos here, the obvious one in "alredy",
but also the passive of "to bind" is "bound", not "binded".
(That is also mis-spelled in a few more places I think.)

—Günther

Re: [PATCH 1/2] landlock: Add hook on socket_listen()

[Mickaël Salaün]

On Wed, Jun 19, 2024 at 09:05:03PM +0200, Günther Noack wrote:
> I agree with Mickaël's comment: this seems like an important fix.
> 
> Mostly for completeness: I played with the "socket type" patch set in a "TCP
> server" example, where *all* possible operations are restricted with Landlock,
> including the ones from the "socket type" patch set V2 with the little fix we
> discussed.
> 
>  - socket()
>  - bind()
>  - enforce a landlock ruleset restricting:
>    - file system access
>    - all TCP bind and connect
>    - socket creation
>  - listen()
>  - accept()
> 
> From the connection handler (which would be the place where an attacker can
> usually provide input), it is now still possible to bind a socket due to this
> problem.  The steps are:
> 
>   1) connect() on client_fd with AF_UNSPEC to disassociate the client FD
>   2) listen() on the client_fd
> 
> This succeeds and it listens on an ephemeral port.
> 
> The code is at [1], if you are interested.
> 
> [1] https://github.com/gnoack/landlock-examples/blob/main/tcpserver.c
> 
> 
> On Mon, May 13, 2024 at 03:15:50PM +0300, Ivanov Mikhail wrote:
> > 4/30/2024 4:36 PM, Mickaël Salaün wrote:
> > > On Mon, Apr 08, 2024 at 05:47:46PM +0800, Ivanov Mikhail wrote:
> > > > Make hook for socket_listen(). It will check that the socket protocol is
> > > > TCP, and if the socket's local port number is 0 (which means,
> > > > that listen(2) was called without any previous bind(2) call),
> > > > then listen(2) call will be legitimate only if there is a rule for bind(2)
> > > > allowing binding to port 0 (or if LANDLOCK_ACCESS_NET_BIND_TCP is not
> > > > supported by the sandbox).
> > > 
> > > Thanks for this patch and sorry for the late full review.  The code is
> > > good overall.
> > > 
> > > We should either consider this patch as a fix or add a new flag/access
> > > right to Landlock syscalls for compatibility reason.  I think this
> > > should be a fix.  Calling listen(2) without a previous call to bind(2)
> > > is a corner case that we should properly handle.  The commit message
> > > should make that explicit and highlight the goal of the patch: first
> > > explain why, and then how.
> > 
> > Yeap, this is fix-patch. I have covered motivation and proposed solution
> > in cover letter. Do you have any suggestions on how i can improve this?
> 
> Without wanting to turn around the direction of this code review now, I am still
> slightly concerned about the assymetry of this special case being implemented
> for listen() but not for connect().
> 
> The reason is this: My colleague Mr. B. recently pointed out to me that you can
> also do a bind() on a socket before a connect(!). The steps are:
> 
> * create socket with socket()
> * bind() to a local port 9090
> * connect() to a remote port 8080
> 
> This gives you a connection between ports 9090 and 8080.

Yes, this should not be an issue, but something to keep in mind.

> 
> A regular connect() without an explicit bind() is of course the more usual
> scenario.  In that case, we are also using up ("implicitly binding") one of the
> ephemeral ports.
> 
> It seems that, with respect to the port binding, listen() and connect() work
> quite similarly then?  This being considered, maybe it *is* the listen()
> operation on a port which we should be restricting, and not bind()?

I agree that we should be able to control listen according to the binded
port, see https://github.com/landlock-lsm/linux/issues/15
In a nutshell, the LANDLOCK_ACCESS_NET_LISTEN_TCP should make more sense
for most use cases, but I think LANDLOCK_ACCESS_NET_BIND_TCP is also
useful to limit opened (well-known) ports and port spoofing.

> 
> With some luck, that would then also free us from having to implement the
> check_tcp_socket_can_listen() logic, which is seemingly emulating logic from
> elsewhere in the kernel?

An alternative could be to only use LANDLOCK_ACCESS_NET_BIND_TCP for
explicit binding (i.e. current state, but with appropriate
documentation), and delegate to LANDLOCK_ACCESS_NET_LISTEN_TCP the
control of binding with listen(2).  That should free us from
implementing check_tcp_socket_can_listen().  The rationale would be that
a malicious sandboxed process could not explicitly bind to a
well-specified port, but only to a range of dedicated random ports (the
same range use for auto-binding with connect).  That could also help
developers by staying close to the kernel syscall ABI (principle of
least astonishment).

> 
> (I am by far not an expert in Linux networking, so I'll put this out for
> consideration and will happily stand corrected if I am misunderstanding
> something.  Maybe someone with more networking background can chime in?)

That would be good indeed.  Netfilter or network folks? Eric?

> 
> 
> > > > +		/* Socket is alredy binded to some port. */
> > > 
> > > This kind of spelling issue can be found by scripts/checkpatch.pl
> > 
> > will be fixed
> 
> P.S. there are two typos here, the obvious one in "alredy",
> but also the passive of "to bind" is "bound", not "binded".
> (That is also mis-spelled in a few more places I think.)
> 
> —Günther
> 

Re: [PATCH 1/2] landlock: Add hook on socket_listen()

[Ivanov Mikhail]

6/19/2024 10:05 PM, Günther Noack wrote:
> I agree with Mickaël's comment: this seems like an important fix.
> 
> Mostly for completeness: I played with the "socket type" patch set in a "TCP
> server" example, where *all* possible operations are restricted with Landlock,
> including the ones from the "socket type" patch set V2 with the little fix we
> discussed.
> 
>   - socket()
>   - bind()
>   - enforce a landlock ruleset restricting:
>     - file system access
>     - all TCP bind and connect
>     - socket creation
>   - listen()
>   - accept()
> 
>>From the connection handler (which would be the place where an attacker can
> usually provide input), it is now still possible to bind a socket due to this
> problem.  The steps are:
> 
>    1) connect() on client_fd with AF_UNSPEC to disassociate the client FD
>    2) listen() on the client_fd
> 
> This succeeds and it listens on an ephemeral port.
> 
> The code is at [1], if you are interested.
> 
> [1] https://github.com/gnoack/landlock-examples/blob/main/tcpserver.c

Do you mean that this scenario works with patch-fix currently being
discussed?

> 
> 
> On Mon, May 13, 2024 at 03:15:50PM +0300, Ivanov Mikhail wrote:
>> 4/30/2024 4:36 PM, Mickaël Salaün wrote:
>>> On Mon, Apr 08, 2024 at 05:47:46PM +0800, Ivanov Mikhail wrote:
>>>> Make hook for socket_listen(). It will check that the socket protocol is
>>>> TCP, and if the socket's local port number is 0 (which means,
>>>> that listen(2) was called without any previous bind(2) call),
>>>> then listen(2) call will be legitimate only if there is a rule for bind(2)
>>>> allowing binding to port 0 (or if LANDLOCK_ACCESS_NET_BIND_TCP is not
>>>> supported by the sandbox).
>>>
>>> Thanks for this patch and sorry for the late full review.  The code is
>>> good overall.
>>>
>>> We should either consider this patch as a fix or add a new flag/access
>>> right to Landlock syscalls for compatibility reason.  I think this
>>> should be a fix.  Calling listen(2) without a previous call to bind(2)
>>> is a corner case that we should properly handle.  The commit message
>>> should make that explicit and highlight the goal of the patch: first
>>> explain why, and then how.
>>
>> Yeap, this is fix-patch. I have covered motivation and proposed solution
>> in cover letter. Do you have any suggestions on how i can improve this?
> 
> Without wanting to turn around the direction of this code review now, I am still
> slightly concerned about the assymetry of this special case being implemented
> for listen() but not for connect().
> 
> The reason is this: My colleague Mr. B. recently pointed out to me that you can
> also do a bind() on a socket before a connect(!). The steps are:
> 
> * create socket with socket()
> * bind() to a local port 9090
> * connect() to a remote port 8080
> 
> This gives you a connection between ports 9090 and 8080.
> 
> A regular connect() without an explicit bind() is of course the more usual
> scenario.  In that case, we are also using up ("implicitly binding") one of the
> ephemeral ports.
> 
> It seems that, with respect to the port binding, listen() and connect() work
> quite similarly then?  This being considered, maybe it *is* the listen()
> operation on a port which we should be restricting, and not bind()?

Do you mean that ability to restrict auto-binding for connect() should
also be implemented? This looks like good idea if we want to provide
full control over port binding. But it's hard for me to come up with an
idea how it can be implemented: current Landlock API allows to restrict
only the destination port for connect().

I think an independent restriction of auto-binding for bind() and
listen() is a good approach: API is more clear and Landlock rules do
not affect each other's behavior. Did I understood your suggestion
correctly?

> 
> With some luck, that would then also free us from having to implement the
> check_tcp_socket_can_listen() logic, which is seemingly emulating logic from
> elsewhere in the kernel?

But check_tcp_socket_can_listen() will be required for
LANDLOCK_ACCESS_NET_LISTEN_TCP hook anyway. Did I miss smth?

> 
> (I am by far not an expert in Linux networking, so I'll put this out for
> consideration and will happily stand corrected if I am misunderstanding
> something.  Maybe someone with more networking background can chime in?)
> 
> 
>>>> +		/* Socket is alredy binded to some port. */
>>>
>>> This kind of spelling issue can be found by scripts/checkpatch.pl
>>
>> will be fixed
> 
> P.S. there are two typos here, the obvious one in "alredy",
> but also the passive of "to bind" is "bound", not "binded".
> (That is also mis-spelled in a few more places I think.)

Thanks, I'll fix them.

> 
> —Günther

Re: [PATCH 1/2] landlock: Add hook on socket_listen()

[Günther Noack]

Hello!

On Fri, Jun 28, 2024 at 07:51:00PM +0300, Ivanov Mikhail wrote:
> 6/19/2024 10:05 PM, Günther Noack wrote:
> > I agree with Mickaël's comment: this seems like an important fix.
> > 
> > Mostly for completeness: I played with the "socket type" patch set in a "TCP
> > server" example, where *all* possible operations are restricted with Landlock,
> > including the ones from the "socket type" patch set V2 with the little fix we
> > discussed.
> > 
> >   - socket()
> >   - bind()
> >   - enforce a landlock ruleset restricting:
> >     - file system access
> >     - all TCP bind and connect
> >     - socket creation
> >   - listen()
> >   - accept()
> > 
> > > From the connection handler (which would be the place where an attacker can
> > usually provide input), it is now still possible to bind a socket due to this
> > problem.  The steps are:
> > 
> >    1) connect() on client_fd with AF_UNSPEC to disassociate the client FD
> >    2) listen() on the client_fd
> > 
> > This succeeds and it listens on an ephemeral port.
> > 
> > The code is at [1], if you are interested.
> > 
> > [1] https://github.com/gnoack/landlock-examples/blob/main/tcpserver.c
> 
> Do you mean that this scenario works with patch-fix currently being
> discussed?

I did not mean to say that, no, I mostly wanted to spell out the scenario to
make sure we are on the same page about the goal.

I have tried it out with a kernel that had V2 of the "socket type" patch set
patched in, with the minor fix that we discussed on the "socket type" patch
thread after the initial submission.  On that kernel, I did not have the
patch-fix applied.

The patch-fix should keep the listen() from working, yes, but I have not tried
it out yet.


> > On Mon, May 13, 2024 at 03:15:50PM +0300, Ivanov Mikhail wrote:
> > > 4/30/2024 4:36 PM, Mickaël Salaün wrote:
> > > > On Mon, Apr 08, 2024 at 05:47:46PM +0800, Ivanov Mikhail wrote:
> > > > > Make hook for socket_listen(). It will check that the socket protocol is
> > > > > TCP, and if the socket's local port number is 0 (which means,
> > > > > that listen(2) was called without any previous bind(2) call),
> > > > > then listen(2) call will be legitimate only if there is a rule for bind(2)
> > > > > allowing binding to port 0 (or if LANDLOCK_ACCESS_NET_BIND_TCP is not
> > > > > supported by the sandbox).
> > > > 
> > > > Thanks for this patch and sorry for the late full review.  The code is
> > > > good overall.
> > > > 
> > > > We should either consider this patch as a fix or add a new flag/access
> > > > right to Landlock syscalls for compatibility reason.  I think this
> > > > should be a fix.  Calling listen(2) without a previous call to bind(2)
> > > > is a corner case that we should properly handle.  The commit message
> > > > should make that explicit and highlight the goal of the patch: first
> > > > explain why, and then how.
> > > 
> > > Yeap, this is fix-patch. I have covered motivation and proposed solution
> > > in cover letter. Do you have any suggestions on how i can improve this?
> > 
> > Without wanting to turn around the direction of this code review now, I am still
> > slightly concerned about the assymetry of this special case being implemented
> > for listen() but not for connect().
> > 
> > The reason is this: My colleague Mr. B. recently pointed out to me that you can
> > also do a bind() on a socket before a connect(!). The steps are:
> > 
> > * create socket with socket()
> > * bind() to a local port 9090
> > * connect() to a remote port 8080
> > 
> > This gives you a connection between ports 9090 and 8080.
> > 
> > A regular connect() without an explicit bind() is of course the more usual
> > scenario.  In that case, we are also using up ("implicitly binding") one of the
> > ephemeral ports.
> > 
> > It seems that, with respect to the port binding, listen() and connect() work
> > quite similarly then?  This being considered, maybe it *is* the listen()
> > operation on a port which we should be restricting, and not bind()?
> 
> Do you mean that ability to restrict auto-binding for connect() should
> also be implemented? This looks like good idea if we want to provide
> full control over port binding. But it's hard for me to come up with an
> idea how it can be implemented: current Landlock API allows to restrict
> only the destination port for connect().

I do not think that restricting auto-binding for connect as part of
LANDLOCK_ACCESS_NET_BIND_TCP would be the correct way.


> I think an independent restriction of auto-binding for bind() and
> listen() is a good approach: API is more clear and Landlock rules do
> not affect each other's behavior. Did I understood your suggestion
> correctly?

I believe you did; After reading a lot of documentation on that subject
recently, let me try to phrase it in yet another way, so that we are on the same
page:

The socket operations do the following things:

 - listen() and connect() make the local port available from the outside.

 - bind(): Userspace processes call bind() to express that they want to use a
   specific local address (IP+port) with the given socket.  With TCP, userspace
   may always omit the call to bind().  If omitted, the kernel picks an
   ephemeral port.

So, bind() behaves the same way, whether is is being used with listen() or
connect().  The common way is to use listen() with bind() and connect() without
bind(), but the opposite can also be done: listen() without bind() will listen
on an ephemeral port, and connect() with bind() will use the desired port.

(The Unix Network Programming book remarks that listen() without bind() is done
for SunRPC servers, where the separately running portmapper daemon provides a
lookup facility for the running services, and services can therefore be offered
on any port.)

A good description I found in the man pages is this:

From ip(7):

  An ephemeral port is allocated to a socket in the following circumstances:

  •  the port number in a socket address is specified as 0 when calling bind(2);
  •  listen(2) is called on a stream socket that was not previously bound;
  •  connect(2) was called on a socket that was not previously bound;
  •  sendto(2) is called on a datagram socket that was not previously bound.

(This section of the ip(7) man page is referenced from connect(2) and listen(2),
in their ERRORS sections.)

So, due to the symmetry of how bind() behaves for both connect() and listen(),
my suggestion would be:

 * Keep the LANDLOCK_ACCESS_NET_BIND_TCP implementation as it is.

 * Clarify in LANDLOCK_ACCESS_NET_BIND_TCP that this only makes calls to bind()
   return errors, but that this does not keep a socket from listening on
   ephemeral ports.

 * Create a new LANDLOCK_ACCESS_NET_LISTEN_TCP access right and restrict
   listen() with that.  Looking at your patch set again, the code in
   hook_socket_listen() should be very similar, but we might want to call
   check_access_socket() with the port number that was previously bound (if
   bind() was called).

Does that sound reasonable?


With the current patch-fix as you sent it on the top of this thread, there are
otherwise some confusing aspects to it, such as:

 * connect() is also implicitly using a local ephemeral port, just like
   listen().  But while calls to listen() are checked against
   LANDLOCK_ACCESS_NET_BIND_TCP, calls to connect() are not.

 * listen() can return an error due to LANDLOCK_ACCESS_NET_BIND_TCP,
   even when the userspace program never called bind().

Both of these are potentially puzzling and might be more in-line with BSD socket
concepts if we did it differently.


> > With some luck, that would then also free us from having to implement the
> > check_tcp_socket_can_listen() logic, which is seemingly emulating logic from
> > elsewhere in the kernel?
> 
> But check_tcp_socket_can_listen() will be required for
> LANDLOCK_ACCESS_NET_LISTEN_TCP hook anyway. Did I miss smth?

You are right -- my fault, I misread that.

—Günther

Re: [PATCH 1/2] landlock: Add hook on socket_listen()

[Ivanov Mikhail]

7/1/2024 1:16 PM, Günther Noack wrote:
> Hello!
> 
> On Fri, Jun 28, 2024 at 07:51:00PM +0300, Ivanov Mikhail wrote:
>> 6/19/2024 10:05 PM, Günther Noack wrote:
>>> I agree with Mickaël's comment: this seems like an important fix.
>>>
>>> Mostly for completeness: I played with the "socket type" patch set in a "TCP
>>> server" example, where *all* possible operations are restricted with Landlock,
>>> including the ones from the "socket type" patch set V2 with the little fix we
>>> discussed.
>>>
>>>    - socket()
>>>    - bind()
>>>    - enforce a landlock ruleset restricting:
>>>      - file system access
>>>      - all TCP bind and connect
>>>      - socket creation
>>>    - listen()
>>>    - accept()
>>>
>>>>  From the connection handler (which would be the place where an attacker can
>>> usually provide input), it is now still possible to bind a socket due to this
>>> problem.  The steps are:
>>>
>>>     1) connect() on client_fd with AF_UNSPEC to disassociate the client FD
>>>     2) listen() on the client_fd
>>>
>>> This succeeds and it listens on an ephemeral port.
>>>
>>> The code is at [1], if you are interested.
>>>
>>> [1] https://github.com/gnoack/landlock-examples/blob/main/tcpserver.c
>>
>> Do you mean that this scenario works with patch-fix currently being
>> discussed?
> 
> I did not mean to say that, no, I mostly wanted to spell out the scenario to
> make sure we are on the same page about the goal.
> 
> I have tried it out with a kernel that had V2 of the "socket type" patch set
> patched in, with the minor fix that we discussed on the "socket type" patch
> thread after the initial submission.  On that kernel, I did not have the
> patch-fix applied.
> 
> The patch-fix should keep the listen() from working, yes, but I have not tried
> it out yet.

I got it, thanks for the clarification! Indeed, goal of this patch-fix
is to restrict such scenarios.

> 
> 
>>> On Mon, May 13, 2024 at 03:15:50PM +0300, Ivanov Mikhail wrote:
>>>> 4/30/2024 4:36 PM, Mickaël Salaün wrote:
>>>>> On Mon, Apr 08, 2024 at 05:47:46PM +0800, Ivanov Mikhail wrote:
>>>>>> Make hook for socket_listen(). It will check that the socket protocol is
>>>>>> TCP, and if the socket's local port number is 0 (which means,
>>>>>> that listen(2) was called without any previous bind(2) call),
>>>>>> then listen(2) call will be legitimate only if there is a rule for bind(2)
>>>>>> allowing binding to port 0 (or if LANDLOCK_ACCESS_NET_BIND_TCP is not
>>>>>> supported by the sandbox).
>>>>>
>>>>> Thanks for this patch and sorry for the late full review.  The code is
>>>>> good overall.
>>>>>
>>>>> We should either consider this patch as a fix or add a new flag/access
>>>>> right to Landlock syscalls for compatibility reason.  I think this
>>>>> should be a fix.  Calling listen(2) without a previous call to bind(2)
>>>>> is a corner case that we should properly handle.  The commit message
>>>>> should make that explicit and highlight the goal of the patch: first
>>>>> explain why, and then how.
>>>>
>>>> Yeap, this is fix-patch. I have covered motivation and proposed solution
>>>> in cover letter. Do you have any suggestions on how i can improve this?
>>>
>>> Without wanting to turn around the direction of this code review now, I am still
>>> slightly concerned about the assymetry of this special case being implemented
>>> for listen() but not for connect().
>>>
>>> The reason is this: My colleague Mr. B. recently pointed out to me that you can
>>> also do a bind() on a socket before a connect(!). The steps are:
>>>
>>> * create socket with socket()
>>> * bind() to a local port 9090
>>> * connect() to a remote port 8080
>>>
>>> This gives you a connection between ports 9090 and 8080.
>>>
>>> A regular connect() without an explicit bind() is of course the more usual
>>> scenario.  In that case, we are also using up ("implicitly binding") one of the
>>> ephemeral ports.
>>>
>>> It seems that, with respect to the port binding, listen() and connect() work
>>> quite similarly then?  This being considered, maybe it *is* the listen()
>>> operation on a port which we should be restricting, and not bind()?
>>
>> Do you mean that ability to restrict auto-binding for connect() should
>> also be implemented? This looks like good idea if we want to provide
>> full control over port binding. But it's hard for me to come up with an
>> idea how it can be implemented: current Landlock API allows to restrict
>> only the destination port for connect().
> 
> I do not think that restricting auto-binding for connect as part of
> LANDLOCK_ACCESS_NET_BIND_TCP would be the correct way.
> 
> 
>> I think an independent restriction of auto-binding for bind() and
>> listen() is a good approach: API is more clear and Landlock rules do
>> not affect each other's behavior. Did I understood your suggestion
>> correctly?
> 
> I believe you did; After reading a lot of documentation on that subject
> recently, let me try to phrase it in yet another way, so that we are on the same
> page:
> 
> The socket operations do the following things:
> 
>   - listen() and connect() make the local port available from the outside.
> 
>   - bind(): Userspace processes call bind() to express that they want to use a
>     specific local address (IP+port) with the given socket.  With TCP, userspace
>     may always omit the call to bind().  If omitted, the kernel picks an
>     ephemeral port.
> 
> So, bind() behaves the same way, whether is is being used with listen() or
> connect().  The common way is to use listen() with bind() and connect() without
> bind(), but the opposite can also be done: listen() without bind() will listen
> on an ephemeral port, and connect() with bind() will use the desired port.
> 
> (The Unix Network Programming book remarks that listen() without bind() is done
> for SunRPC servers, where the separately running portmapper daemon provides a
> lookup facility for the running services, and services can therefore be offered
> on any port.)
> 
> A good description I found in the man pages is this:
> 
>>From ip(7):
> 
>    An ephemeral port is allocated to a socket in the following circumstances:
> 
>    •  the port number in a socket address is specified as 0 when calling bind(2);
>    •  listen(2) is called on a stream socket that was not previously bound;
>    •  connect(2) was called on a socket that was not previously bound;
>    •  sendto(2) is called on a datagram socket that was not previously bound.
> 
> (This section of the ip(7) man page is referenced from connect(2) and listen(2),
> in their ERRORS sections.)
> 
> So, due to the symmetry of how bind() behaves for both connect() and listen(),
> my suggestion would be:
> 
>   * Keep the LANDLOCK_ACCESS_NET_BIND_TCP implementation as it is.
> 
>   * Clarify in LANDLOCK_ACCESS_NET_BIND_TCP that this only makes calls to bind()
>     return errors, but that this does not keep a socket from listening on
>     ephemeral ports.
> 
>   * Create a new LANDLOCK_ACCESS_NET_LISTEN_TCP access right and restrict
>     listen() with that.  Looking at your patch set again, the code in
>     hook_socket_listen() should be very similar, but we might want to call
>     check_access_socket() with the port number that was previously bound (if
>     bind() was called).
> 
> Does that sound reasonable?
> 
> 
> With the current patch-fix as you sent it on the top of this thread, there are
> otherwise some confusing aspects to it, such as:
> 
>   * connect() is also implicitly using a local ephemeral port, just like
>     listen().  But while calls to listen() are checked against
>     LANDLOCK_ACCESS_NET_BIND_TCP, calls to connect() are not.
> 
>   * listen() can return an error due to LANDLOCK_ACCESS_NET_BIND_TCP,
>     even when the userspace program never called bind().
> 
> Both of these are potentially puzzling and might be more in-line with BSD socket
> concepts if we did it differently.

Thanks for the great explanation! We're on the same page.

Considering that binding to ephemeral ports can be done not only with
bind() or listen(), I think your approach is more correct.
Restricting any possible binding to ephemeral ports just using
LANDLOCK_ACCESS_NET_BIND_TCP wouldn't allow sandboxed processes
to deny listen() without pre-binding (which is quite unsafe) and
use connect() in the usuall way (without pre-binding).

Controlling ephemeral ports allocation for listen() can be done in the
same way as for LANDLOCK_ACCESS_NET_BIND_TCP in the patch with
LANDLOCK_ACCESS_NET_LISTEN_TCP access right implementation.

I'm only concerned about controlling the auto-binding for other
operations (such as connect() and sendto() for UDP). As I said, I think
this can also be useful: users will be able to control which processes
are allowed to use ephemeral ports from ip_local_port_range and which
are not, and they must assign ports for each operation explicitly. If
you agree that such control is reasonable, we'll probably  have to
consider some API changes, since such control is currently not possible.

We should clarify this before I send a patch with the
LANDLOCK_ACCESS_NET_LISTEN_TCP implementation. WDYT?

> 
> 
>>> With some luck, that would then also free us from having to implement the
>>> check_tcp_socket_can_listen() logic, which is seemingly emulating logic from
>>> elsewhere in the kernel?
>>
>> But check_tcp_socket_can_listen() will be required for
>> LANDLOCK_ACCESS_NET_LISTEN_TCP hook anyway. Did I miss smth?
> 
> You are right -- my fault, I misread that.
> 
> —Günther

Re: [PATCH 1/2] landlock: Add hook on socket_listen()

[Günther Noack]

On Mon, Jul 01, 2024 at 04:10:27PM +0300, Ivanov Mikhail wrote:
> Thanks for the great explanation! We're on the same page.
> 
> Considering that binding to ephemeral ports can be done not only with
> bind() or listen(), I think your approach is more correct.
> Restricting any possible binding to ephemeral ports just using
> LANDLOCK_ACCESS_NET_BIND_TCP wouldn't allow sandboxed processes
> to deny listen() without pre-binding (which is quite unsafe) and
> use connect() in the usuall way (without pre-binding).
> 
> Controlling ephemeral ports allocation for listen() can be done in the
> same way as for LANDLOCK_ACCESS_NET_BIND_TCP in the patch with
> LANDLOCK_ACCESS_NET_LISTEN_TCP access right implementation.

That sounds good, yes! 👍


> I'm only concerned about controlling the auto-binding for other
> operations (such as connect() and sendto() for UDP). As I said, I think
> this can also be useful: users will be able to control which processes
> are allowed to use ephemeral ports from ip_local_port_range and which
> are not, and they must assign ports for each operation explicitly. If
> you agree that such control is reasonable, we'll probably  have to
> consider some API changes, since such control is currently not possible.
> 
> We should clarify this before I send a patch with the
> LANDLOCK_ACCESS_NET_LISTEN_TCP implementation. WDYT?

LANDLOCK_ACCESS_NET_LISTEN_TCP seems like the most important to me.

For connect() and sendto(), I think the access rights are less urgent:

connect(): We already have LANDLOCK_ACCESS_NET_CONNECT_TCP, but that one is
getting restricted for the *remote* port number.

 (a) I think it would be possible to do the same for the *local* port number, by
     introducing a separate LANDLOCK_ACCESS_NET_CONNECT_TCP_LOCALPORT right.
     (Yes, the name is absolutely horrible, this is just for the example :))
     hook_socket_connect() would then need to do both a check for the remote
     port using LANDLOCK_ACCESS_NET_CONNECT_TCP, as it already does today, as
     well as a check for the (previously bound?) local port using
     LANDLOCK_ACCESS_NET_CONNECT_TCP_LOCALPORT.
     
     So I think it is extensible in that direction, in principle, even though I
     don't currently have a good name for that access right. :)
     
 (b) Compared to what LANDLOCK_ACCESS_NET_BIND_TCP already restricts, a
     hypothetical LANDLOCK_ACCESS_NET_CONNECT_TCP_LOCALPORT right would only
     additionally restrict the use of ephemeral ports.  I'm currently having a
     hard time seeing what an attacker could do with that (use up all ephemeral
     ports?).

sendto(): I think this is not relevant yet, because as the documentation said,
ephemeral ports are only handed out when sendto() is used with datagram (UDP)
sockets.

Once Landlock starts having UDP support, this would become relevant, but for
this patch set, I think that the TCP server use case as discussed further above
in this thread is very compelling.

Thanks,
—Günther

Re: [PATCH 1/2] landlock: Add hook on socket_listen()

[Ivanov Mikhail]

7/1/2024 6:47 PM, Günther Noack wrote:
> On Mon, Jul 01, 2024 at 04:10:27PM +0300, Ivanov Mikhail wrote:
>> Thanks for the great explanation! We're on the same page.
>>
>> Considering that binding to ephemeral ports can be done not only with
>> bind() or listen(), I think your approach is more correct.
>> Restricting any possible binding to ephemeral ports just using
>> LANDLOCK_ACCESS_NET_BIND_TCP wouldn't allow sandboxed processes
>> to deny listen() without pre-binding (which is quite unsafe) and
>> use connect() in the usuall way (without pre-binding).
>>
>> Controlling ephemeral ports allocation for listen() can be done in the
>> same way as for LANDLOCK_ACCESS_NET_BIND_TCP in the patch with
>> LANDLOCK_ACCESS_NET_LISTEN_TCP access right implementation.
> 
> That sounds good, yes! 👍
> 
> 
>> I'm only concerned about controlling the auto-binding for other
>> operations (such as connect() and sendto() for UDP). As I said, I think
>> this can also be useful: users will be able to control which processes
>> are allowed to use ephemeral ports from ip_local_port_range and which
>> are not, and they must assign ports for each operation explicitly. If
>> you agree that such control is reasonable, we'll probably  have to
>> consider some API changes, since such control is currently not possible.
>>
>> We should clarify this before I send a patch with the
>> LANDLOCK_ACCESS_NET_LISTEN_TCP implementation. WDYT?
> 
> LANDLOCK_ACCESS_NET_LISTEN_TCP seems like the most important to me.
> 
> For connect() and sendto(), I think the access rights are less urgent:
> 
> connect(): We already have LANDLOCK_ACCESS_NET_CONNECT_TCP, but that one is
> getting restricted for the *remote* port number.
> 
>   (a) I think it would be possible to do the same for the *local* port number, by
>       introducing a separate LANDLOCK_ACCESS_NET_CONNECT_TCP_LOCALPORT right.
>       (Yes, the name is absolutely horrible, this is just for the example :))
>       hook_socket_connect() would then need to do both a check for the remote
>       port using LANDLOCK_ACCESS_NET_CONNECT_TCP, as it already does today, as
>       well as a check for the (previously bound?) local port using
>       LANDLOCK_ACCESS_NET_CONNECT_TCP_LOCALPORT.
>       
>       So I think it is extensible in that direction, in principle, even though I
>       don't currently have a good name for that access right. :)

Indeed, implementing a new type of rule seems to be an optimal approach
for this.

>       
>   (b) Compared to what LANDLOCK_ACCESS_NET_BIND_TCP already restricts, a
>       hypothetical LANDLOCK_ACCESS_NET_CONNECT_TCP_LOCALPORT right would only
>       additionally restrict the use of ephemeral ports.  I'm currently having a
>       hard time seeing what an attacker could do with that (use up all ephemeral
>       ports?).

I thought about something like that, yeah) Also, I tried to find out
if there are cases where port remains bound to the client socket after
the connection is completed. In this case, listen() can be called to a
socket with a port bound via connect() call. In the case of TCP, such
scenario is not possible, port is always deallocated in tcp_set_state()
method.

So, I don't see any realworld vulnerabilities, we can leave this case
until someone comes up with a real issue.

> 
> sendto(): I think this is not relevant yet, because as the documentation said,
> ephemeral ports are only handed out when sendto() is used with datagram (UDP)
> sockets.
> 
> Once Landlock starts having UDP support, this would become relevant, but for
> this patch set, I think that the TCP server use case as discussed further above
> in this thread is very compelling.

Agreed. Anyway, if someone finds any interesting cases with
auto-binding via sendto(), we can implement a new rule, as you suggested
for connect().

Thanks you for your research and ideas, Günther!
I'll prepare the LANDLOCK_ACCESS_NET_LISTEN_TCP patchset for the review.

> 
> Thanks,
> —Günther

[PATCH 2/2] selftests/landlock: Create 'listen_zero', 'deny_listen_zero' tests

[Ivanov Mikhail]

Suggested code test scenarios where listen(2) call without explicit
bind(2) is allowed and forbidden.

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

diff --git a/tools/testing/selftests/landlock/net_test.c b/tools/testing/selftests/landlock/net_test.c
index 936cfc879f1d..6d6b5aef387f 100644
--- a/tools/testing/selftests/landlock/net_test.c
+++ b/tools/testing/selftests/landlock/net_test.c
@@ -1714,6 +1714,95 @@ TEST_F(port_specific, bind_connect_zero)
 	EXPECT_EQ(0, close(bind_fd));
 }
 
+TEST_F(port_specific, listen_zero)
+{
+	int listen_fd, connect_fd;
+	uint16_t port;
+
+	/* Adds a rule layer with bind actions. */
+	if (variant->sandbox == TCP_SANDBOX) {
+		const struct landlock_ruleset_attr ruleset_attr = {
+			.handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP,
+		};
+		const struct landlock_net_port_attr tcp_bind_zero = {
+			.allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP,
+			.port = 0,
+		};
+		int ruleset_fd;
+
+		ruleset_fd = landlock_create_ruleset(&ruleset_attr,
+						     sizeof(ruleset_attr), 0);
+		ASSERT_LE(0, ruleset_fd);
+
+		/* Checks zero port value on bind action. */
+		EXPECT_EQ(0,
+			  landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT,
+					    &tcp_bind_zero, 0));
+
+		enforce_ruleset(_metadata, ruleset_fd);
+		EXPECT_EQ(0, close(ruleset_fd));
+	}
+
+	listen_fd = socket_variant(&self->srv0);
+	ASSERT_LE(0, listen_fd);
+
+	connect_fd = socket_variant(&self->srv0);
+	ASSERT_LE(0, listen_fd);
+	/*
+	 * Allow listen(2) to select a random port for the socket,
+	 * since bind(2) wasn't called.
+	 */
+	EXPECT_EQ(0, listen(listen_fd, backlog));
+
+	/* Sets binded (by listen(2)) port for both protocol families. */
+	port = get_binded_port(listen_fd, &variant->prot);
+	EXPECT_NE(0, port);
+	set_port(&self->srv0, port);
+
+	/* Connects on the binded port. */
+	EXPECT_EQ(0, connect_variant(connect_fd, &self->srv0));
+
+	EXPECT_EQ(0, close(listen_fd));
+	EXPECT_EQ(0, close(connect_fd));
+}
+
+TEST_F(port_specific, deny_listen_zero)
+{
+	int listen_fd, ret;
+
+	/* Adds a rule layer with bind actions. */
+	if (variant->sandbox == TCP_SANDBOX) {
+		const struct landlock_ruleset_attr ruleset_attr = {
+			.handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP,
+		};
+		int ruleset_fd;
+
+		ruleset_fd = landlock_create_ruleset(&ruleset_attr,
+						     sizeof(ruleset_attr), 0);
+		ASSERT_LE(0, ruleset_fd);
+
+		/* Forbid binding to any port. */
+		enforce_ruleset(_metadata, ruleset_fd);
+		EXPECT_EQ(0, close(ruleset_fd));
+	}
+
+	listen_fd = socket_variant(&self->srv0);
+	ASSERT_LE(0, listen_fd);
+	/* 
+	 * Check that listen(2) call is prohibited without first calling bind(2).
+	 */
+	ret = listen(listen_fd, backlog);
+	if (is_restricted(&variant->prot, variant->sandbox)) {
+		/* Denied by Landlock. */
+		EXPECT_NE(0, ret);
+		EXPECT_EQ(EACCES, errno);
+	} else {
+		EXPECT_EQ(0, ret);
+	}
+
+	EXPECT_EQ(0, close(listen_fd));
+}
+
 TEST_F(port_specific, bind_connect_1023)
 {
 	int bind_fd, connect_fd, ret;
-- 
2.34.1

Re: [PATCH 2/2] selftests/landlock: Create 'listen_zero', 'deny_listen_zero' tests

[Mickaël Salaün]

The subject should be something like:
"selftests/landlock: Test listening on socket without binding"

On Mon, Apr 08, 2024 at 05:47:47PM +0800, Ivanov Mikhail wrote:
> Suggested code test scenarios where listen(2) call without explicit
> bind(2) is allowed and forbidden.
> 
> Signed-off-by: Ivanov Mikhail <ivanov.mikhail1@huawei-partners.com>
> Reviewed-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
> ---
>  tools/testing/selftests/landlock/net_test.c | 89 +++++++++++++++++++++
>  1 file changed, 89 insertions(+)
> 
> diff --git a/tools/testing/selftests/landlock/net_test.c b/tools/testing/selftests/landlock/net_test.c
> index 936cfc879f1d..6d6b5aef387f 100644
> --- a/tools/testing/selftests/landlock/net_test.c
> +++ b/tools/testing/selftests/landlock/net_test.c
> @@ -1714,6 +1714,95 @@ TEST_F(port_specific, bind_connect_zero)
>  	EXPECT_EQ(0, close(bind_fd));
>  }
>  
> +TEST_F(port_specific, listen_zero)
> +{
> +	int listen_fd, connect_fd;
> +	uint16_t port;
> +
> +	/* Adds a rule layer with bind actions. */
> +	if (variant->sandbox == TCP_SANDBOX) {
> +		const struct landlock_ruleset_attr ruleset_attr = {
> +			.handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP,
> +		};
> +		const struct landlock_net_port_attr tcp_bind_zero = {
> +			.allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP,
> +			.port = 0,
> +		};
> +		int ruleset_fd;
> +
> +		ruleset_fd = landlock_create_ruleset(&ruleset_attr,
> +						     sizeof(ruleset_attr), 0);
> +		ASSERT_LE(0, ruleset_fd);
> +
> +		/* Checks zero port value on bind action. */
> +		EXPECT_EQ(0,
> +			  landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT,
> +					    &tcp_bind_zero, 0));
> +
> +		enforce_ruleset(_metadata, ruleset_fd);
> +		EXPECT_EQ(0, close(ruleset_fd));
> +	}
> +
> +	listen_fd = socket_variant(&self->srv0);
> +	ASSERT_LE(0, listen_fd);
> +
> +	connect_fd = socket_variant(&self->srv0);
> +	ASSERT_LE(0, listen_fd);
> +	/*
> +	 * Allow listen(2) to select a random port for the socket,
> +	 * since bind(2) wasn't called.
> +	 */
> +	EXPECT_EQ(0, listen(listen_fd, backlog));
> +
> +	/* Sets binded (by listen(2)) port for both protocol families. */
> +	port = get_binded_port(listen_fd, &variant->prot);
> +	EXPECT_NE(0, port);
> +	set_port(&self->srv0, port);
> +
> +	/* Connects on the binded port. */
> +	EXPECT_EQ(0, connect_variant(connect_fd, &self->srv0));
> +
> +	EXPECT_EQ(0, close(listen_fd));
> +	EXPECT_EQ(0, close(connect_fd));
> +}
> +
> +TEST_F(port_specific, deny_listen_zero)
> +{
> +	int listen_fd, ret;
> +
> +	/* Adds a rule layer with bind actions. */
> +	if (variant->sandbox == TCP_SANDBOX) {
> +		const struct landlock_ruleset_attr ruleset_attr = {
> +			.handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP,
> +		};
> +		int ruleset_fd;
> +
> +		ruleset_fd = landlock_create_ruleset(&ruleset_attr,
> +						     sizeof(ruleset_attr), 0);
> +		ASSERT_LE(0, ruleset_fd);
> +
> +		/* Forbid binding to any port. */
> +		enforce_ruleset(_metadata, ruleset_fd);
> +		EXPECT_EQ(0, close(ruleset_fd));
> +	}
> +
> +	listen_fd = socket_variant(&self->srv0);
> +	ASSERT_LE(0, listen_fd);
> +	/* 

nit: Extra space

> +	 * Check that listen(2) call is prohibited without first calling bind(2).

This should fit in 80 columns.

> +	 */
> +	ret = listen(listen_fd, backlog);
> +	if (is_restricted(&variant->prot, variant->sandbox)) {
> +		/* Denied by Landlock. */
> +		EXPECT_NE(0, ret);
> +		EXPECT_EQ(EACCES, errno);
> +	} else {
> +		EXPECT_EQ(0, ret);
> +	}
> +
> +	EXPECT_EQ(0, close(listen_fd));
> +}

These tests look good!

> +
>  TEST_F(port_specific, bind_connect_1023)
>  {
>  	int bind_fd, connect_fd, ret;
> -- 
> 2.34.1
> 
> 

Re: [PATCH 2/2] selftests/landlock: Create 'listen_zero', 'deny_listen_zero' tests

[Ivanov Mikhail]

4/30/2024 4:36 PM, Mickaël Salaün wrote:
> The subject should be something like:
> "selftests/landlock: Test listening on socket without binding"

thanks, will be fixed.

> 
> On Mon, Apr 08, 2024 at 05:47:47PM +0800, Ivanov Mikhail wrote:
>> Suggested code test scenarios where listen(2) call without explicit
>> bind(2) is allowed and forbidden.
>>
>> Signed-off-by: Ivanov Mikhail <ivanov.mikhail1@huawei-partners.com>
>> Reviewed-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
>> ---
>>   tools/testing/selftests/landlock/net_test.c | 89 +++++++++++++++++++++
>>   1 file changed, 89 insertions(+)
>>
>> diff --git a/tools/testing/selftests/landlock/net_test.c b/tools/testing/selftests/landlock/net_test.c
>> index 936cfc879f1d..6d6b5aef387f 100644
>> --- a/tools/testing/selftests/landlock/net_test.c
>> +++ b/tools/testing/selftests/landlock/net_test.c
>> @@ -1714,6 +1714,95 @@ TEST_F(port_specific, bind_connect_zero)
>>   	EXPECT_EQ(0, close(bind_fd));
>>   }
>>   
>> +TEST_F(port_specific, listen_zero)
>> +{
>> +	int listen_fd, connect_fd;
>> +	uint16_t port;
>> +
>> +	/* Adds a rule layer with bind actions. */
>> +	if (variant->sandbox == TCP_SANDBOX) {
>> +		const struct landlock_ruleset_attr ruleset_attr = {
>> +			.handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP,
>> +		};
>> +		const struct landlock_net_port_attr tcp_bind_zero = {
>> +			.allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP,
>> +			.port = 0,
>> +		};
>> +		int ruleset_fd;
>> +
>> +		ruleset_fd = landlock_create_ruleset(&ruleset_attr,
>> +						     sizeof(ruleset_attr), 0);
>> +		ASSERT_LE(0, ruleset_fd);
>> +
>> +		/* Checks zero port value on bind action. */
>> +		EXPECT_EQ(0,
>> +			  landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT,
>> +					    &tcp_bind_zero, 0));
>> +
>> +		enforce_ruleset(_metadata, ruleset_fd);
>> +		EXPECT_EQ(0, close(ruleset_fd));
>> +	}
>> +
>> +	listen_fd = socket_variant(&self->srv0);
>> +	ASSERT_LE(0, listen_fd);
>> +
>> +	connect_fd = socket_variant(&self->srv0);
>> +	ASSERT_LE(0, listen_fd);
>> +	/*
>> +	 * Allow listen(2) to select a random port for the socket,
>> +	 * since bind(2) wasn't called.
>> +	 */
>> +	EXPECT_EQ(0, listen(listen_fd, backlog));
>> +
>> +	/* Sets binded (by listen(2)) port for both protocol families. */
>> +	port = get_binded_port(listen_fd, &variant->prot);
>> +	EXPECT_NE(0, port);
>> +	set_port(&self->srv0, port);
>> +
>> +	/* Connects on the binded port. */
>> +	EXPECT_EQ(0, connect_variant(connect_fd, &self->srv0));
>> +
>> +	EXPECT_EQ(0, close(listen_fd));
>> +	EXPECT_EQ(0, close(connect_fd));
>> +}
>> +
>> +TEST_F(port_specific, deny_listen_zero)
>> +{
>> +	int listen_fd, ret;
>> +
>> +	/* Adds a rule layer with bind actions. */
>> +	if (variant->sandbox == TCP_SANDBOX) {
>> +		const struct landlock_ruleset_attr ruleset_attr = {
>> +			.handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP,
>> +		};
>> +		int ruleset_fd;
>> +
>> +		ruleset_fd = landlock_create_ruleset(&ruleset_attr,
>> +						     sizeof(ruleset_attr), 0);
>> +		ASSERT_LE(0, ruleset_fd);
>> +
>> +		/* Forbid binding to any port. */
>> +		enforce_ruleset(_metadata, ruleset_fd);
>> +		EXPECT_EQ(0, close(ruleset_fd));
>> +	}
>> +
>> +	listen_fd = socket_variant(&self->srv0);
>> +	ASSERT_LE(0, listen_fd);
>> +	/*
> 
> nit: Extra space

will be fixed

> 
>> +	 * Check that listen(2) call is prohibited without first calling bind(2).
> 
> This should fit in 80 columns.

will be fixed

> 
>> +	 */
>> +	ret = listen(listen_fd, backlog);
>> +	if (is_restricted(&variant->prot, variant->sandbox)) {
>> +		/* Denied by Landlock. */
>> +		EXPECT_NE(0, ret);
>> +		EXPECT_EQ(EACCES, errno);
>> +	} else {
>> +		EXPECT_EQ(0, ret);
>> +	}
>> +
>> +	EXPECT_EQ(0, close(listen_fd));
>> +}
> 
> These tests look good!
> 
>> +
>>   TEST_F(port_specific, bind_connect_1023)
>>   {
>>   	int bind_fd, connect_fd, ret;
>> -- 
>> 2.34.1
>>
>>

Re: [PATCH 0/2] Forbid illegitimate binding via listen(2)

[Mickaël Salaün]

Could you please send a v2 for this patch? I'd like this issue to be
fixed, especially before any other Landlock feature get merged.

On Mon, Apr 08, 2024 at 05:47:45PM +0800, Ivanov Mikhail wrote:
> listen(2) can be called without explicit bind(2) call. For a TCP socket
> it would result in assigning random port(in some range) to this socket
> by the kernel. If Landlock sandbox supports LANDLOCK_ACCESS_NET_BIND_TCP,
> this may lead to implicit access to a prohibited (by Landlock sandbox)
> port. Malicious sandboxed process can accidentally impersonate a
> legitimate server process (if listen(2) assigns it a server port number).
> 
> Patch adds hook on socket_listen() that prevents such scenario by checking
> LANDLOCK_ACCESS_NET_BIND_TCP access for port 0.
> 
> Few tests were added to cover this case.
> 
> Code coverage(gcov):
> * security/landlock:
> lines......: 94.5% (745 of 788 lines)
> functions..: 97.1% (100 of 103 functions)
> 
> Ivanov Mikhail (2):
>   landlock: Add hook on socket_listen()
>   selftests/landlock: Create 'listen_zero', 'deny_listen_zero' tests
> 
>  security/landlock/net.c                     | 104 +++++++++++++++++---
>  tools/testing/selftests/landlock/net_test.c |  89 +++++++++++++++++
>  2 files changed, 177 insertions(+), 16 deletions(-)
> 
> -- 
> 2.34.1
> 
>