Message-ID: <200bd6ce-de44-7335-63d9-04c17b1b1cf9@digikod.net>
Date:   Fri, 2 Dec 2022 14:01:13 +0100
From:   Mickaël Salaün <mic@...ikod.net>
To:     "Konstantin Meskhidze (A)" <konstantin.meskhidze@...wei.com>,
        willemdebruijn.kernel@...il.com
Cc:     gnoack3000@...il.com, linux-security-module@...r.kernel.org,
        netdev@...r.kernel.org, netfilter-devel@...r.kernel.org,
        artem.kuzin@...wei.com, linux-api@...r.kernel.org,
        "Alejandro Colomar (man-pages)" <alx.manpages@...il.com>
Subject: Re: [PATCH v8 08/12] landlock: Implement TCP network hooks


On 02/12/2022 04:13, Konstantin Meskhidze (A) wrote:
> 
> 
> 11/29/2022 12:00 AM, Mickaël Salaün пишет:
>> The previous commit provides an interface to theoretically restrict
>> network access (i.e. ruleset handled network accesses), but in fact this
>> is not enforced until this commit. I like this split but to avoid any
>> inconsistency, please squash this commit into the previous one: "7/12
>> landlock: Add network rules support"
>> You should keep all the commit messages but maybe tweak them a bit.
>>
>     Ok. Will be squashed.
>>
>> On 28/11/2022 09:21, Konstantin Meskhidze (A) wrote:
>>>
>>>
>>> 11/17/2022 9:43 PM, Mickaël Salaün пишет:
>>>>
>>>> On 21/10/2022 17:26, Konstantin Meskhidze wrote:
>>>>> This patch adds support of socket_bind() and socket_connect() hooks.
>>>>> It's possible to restrict binding and connecting of TCP sockets to
>>>>> particular ports.
>>>>
>>>> Implement socket_bind() and socket_connect LSM hooks, which enable to
>>>> restrict TCP socket binding and connection to specific ports.
>>>>
>>>      Ok. Thanks.
>>>>
>>>>>
>>>>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@...wei.com>
>>>>> ---
>>
>> [...]
>>
>>>>> +static int hook_socket_connect(struct socket *sock, struct sockaddr *address,
>>>>> +			       int addrlen)
>>>>> +{
>>>>> +	const struct landlock_ruleset *const dom =
>>>>> +		landlock_get_current_domain();
>>>>> +
>>>>> +	if (!dom)
>>>>> +		return 0;
>>>>> +
>>>>> +	/* Check if it's a TCP socket. */
>>>>> +	if (sock->type != SOCK_STREAM)
>>>>> +		return 0;
>>>>> +
>>>>> +	/* Check if the hook is AF_INET* socket's action. */
>>>>> +	switch (address->sa_family) {
>>>>> +	case AF_INET:
>>>>> +#if IS_ENABLED(CONFIG_IPV6)
>>>>> +	case AF_INET6:
>>>>> +#endif
>>>>> +		return check_socket_access(dom, get_port(address),
>>>>> +					   LANDLOCK_ACCESS_NET_CONNECT_TCP);
>>>>> +	case AF_UNSPEC: {
>>>>> +		u16 i;
>>>>
>>>> You can move "i" after the "dom" declaration to remove the extra braces.
>>>>
>>>      Ok. Thanks.
>>>>
>>>>> +
>>>>> +		/*
>>>>> +		 * If just in a layer a mask supports connect access,
>>>>> +		 * the socket_connect() hook with AF_UNSPEC family flag
>>>>> +		 * must be banned. This prevents from disconnecting already
>>>>> +		 * connected sockets.
>>>>> +		 */
>>>>> +		for (i = 0; i < dom->num_layers; i++) {
>>>>> +			if (landlock_get_net_access_mask(dom, i) &
>>>>> +			    LANDLOCK_ACCESS_NET_CONNECT_TCP)
>>>>> +				return -EACCES;
>>>>
>>>> I'm wondering if this is the right error code for this case. EPERM may
>>>> be more appropriate.
>>>
>>>      Ok. Will be refactored.
>>>>
>>>> Thinking more about this case, I don't understand what is the rationale
>>>> to deny such action. What would be the consequence to always allow
>>>> connection with AF_UNSPEC (i.e. to disconnect a socket)?
>>>>
>>>      I thought we have come to a conclusion about connect(...AF_UNSPEC..)
>>>     behaviour in the patchset V3:
>>> https://lore.kernel.org/linux-security-module/19ad3a01-d76e-0e73-7833-99acd4afd97e@huawei.com/
>>
>> The conclusion was that AF_UNSPEC disconnects a socket, but I'm asking
>> if this is a security issue. I don't think it is more dangerous than a
>> new (unconnected) socket. Am I missing something? Which kind of rule
>> could be bypassed? What are we protecting against by restricting AF_UNSPEC?
> 
> I just follow Willem de Bruijn concerns about this issue:
> 
> quote: "It is valid to pass an address with AF_UNSPEC to a PF_INET(6)
> socket. And there are legitimate reasons to want to deny this. Such as
> passing a connection to a unprivileged process and disallow it from
> disconnect and opening a different new connection."
> 
> https://lore.kernel.org/linux-security-module/CA+FuTSf4EjgjBCCOiu-PHJcTMia41UkTh8QJ0+qdxL_J8445EA@mail.gmail.com/

I agree with the fact that we want to deny this, but in this example the 
new connection should still be restricted by the Landlock domain. Using 
AF_UNSPEC on a connected socket should not make this socket allowed to 
create any connection if the process is restricted with TCP_CONNECT. 
Being allowed to close a connection should not be an issue, and any new 
connection must be vetted by Landlock.

> 
> 
> quote: "The intended use-case is for a privileged process to open a
> connection (i.e., bound and connected socket) and pass that to a
> restricted process. The intent is for that process to only be allowed to
> communicate over this pre-established channel.
> 
> In practice, it is able to disconnect (while staying bound) and
> elevate its privileges to that of a listening server: ..."
> 
> https://lore.kernel.org/linux-security-module/CA+FuTScaoby-=xRKf_Dz3koSYHqrMN0cauCg4jMmy_nDxwPADA@mail.gmail.com/
> 
> Looks like it's a security issue here.

It the provided example, if child_process() is restricted with 
TCP_CONNECT and TCP_BIND, any call to connect() or bind() will return an 
access error. listen() and accept() would work if the socket is bound, 
which is the case here, and then implicitly allowed by the parent 
process. I don' see any security issue. Am I missing something?

In fact, connect with AF_UNSPEC should always be allowed to be 
consistent with close(2), which is a way to drop privileges.


What Willem said:
> It would be good to also
> ensure that a now-bound socket cannot call listen.

This is not relevant for Landlock because the security model is to check 
process's requests to get new accesses (e.g. create a new file 
descriptor), but not to check passed accesses (e.g. inherited from a 
parent process, or pass through a unix socket) which are delegated to 
the sender/parent. The goal of a sandbox is to limit the set of new 
access requested (to the kernel) from within this sandbox. All already 
opened file descriptors were previously vetted by Landlock (and other 
access control systems).

> 
>>
>> We could then reduce the hook codes to just:
>> return current_check_access_socket(sock, address, LANDLOCK_ACCESS_NET_*);
>> .

As for SELinux, the connect hook should first do this check (with an 
appropriate comment):
if (address->sa_family == AF_UNSPEC)
	return 0;

Powered by blists - more mailing lists