Message-ID: <7c8ed332-c4ec-81e7-a94a-e1b62d820dd3@huawei-partners.com>
Date: Thu, 1 Aug 2024 18:34:41 +0300
From: Mikhail Ivanov <ivanov.mikhail1@...wei-partners.com>
To: Mickaël Salaün <mic@...ikod.net>
CC: <willemdebruijn.kernel@...il.com>, <gnoack3000@...il.com>,
	<linux-security-module@...r.kernel.org>, <netdev@...r.kernel.org>,
	<netfilter-devel@...r.kernel.org>, <yusongping@...wei.com>,
	<artem.kuzin@...wei.com>, <konstantin.meskhidze@...wei.com>
Subject: Re: [RFC PATCH v1 2/9] landlock: Support TCP listen access-control

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@...wei-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
>>>>
>>>>
>>

Powered by blists - more mailing lists