Message-ID: <9a10a7f0-e7ec-a33d-f3ae-978814a1627a@digikod.net>
Date:   Mon, 5 Dec 2022 14:18:01 +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>,
        Paul Moore <paul@...l-moore.com>
Subject: Re: [PATCH v8 08/12] landlock: Implement TCP network hooks


On 05/12/2022 03:55, Konstantin Meskhidze (A) wrote:
> 
> 
> 12/2/2022 4:01 PM, Mickaël Salaün пишет:
>>
>> 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.
>>
> 
>     You are right. This makes sense. Thanks for the comment.
>>>
>>>
>>> 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.
>>
> 
>    It should be allowed with checking:
> "return check_socket_access(dom, get_port(address),
>                                    LANDLOCK_ACCESS_NET_CONNECT_TCP);
>>
>> 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).
> 
>      I got your point. Thanks.
>>
>>>
>>>>
>>>> We could then reduce the hook codes to just:
>>>> return current_check_access_socket(sock, address, LANDLOCK_ACCESS_NET_*);

This current_check_access_socket() helper should contain all the access 
control code.

>>>> .
>>
>> As for SELinux, the connect hook should first do this check (with an
>> appropriate comment):
>> if (address->sa_family == AF_UNSPEC)
>> 	return 0;
> 
>     In case of Landlock it looks like a landlocked process could connnect
> to the ports it's not allowed to connect to.
> So we need just to return check_socket_access(dom, get_port(address),
> 				   LANDLOCK_ACCESS_NET_CONNECT_TCP);
> I'm I correct? Did I miss something?

Using AF_UNSPEC with connect(2) doesn't connect the socket to a port, 
and in fact completely ignore the port. We can move the AF_UNSPEC check 
to the current_check_access_socket() helper:

  	switch (address->sa_family) {
  	case AF_UNSPEC:
+		/*
+		 * Connecting to an address with AF_UNSPEC dissolves the TCP
+		 * association, which have the same effect as closing the
+		 * connection while retaining the socket object (i.e., the file
+		 * descriptor).  As for dropping privileges, closing
+		 * connections is always allowed.
+		 */
+		if (access_request == LANDLOCK_ACCESS_NET_CONNECT_TCP)
+			return 0;
+
+		/*
+		 * For compatibility reason, accept AF_UNSPEC for bind
+		 * accesses (mapped to AF_INET) only if the address is
+		 * INADDR_ANY (cf. __inet_bind).  Checking the address is
+		 * required to not wrongfully return -EACCES instead of
+		 * -EAFNOSUPPORT.
+		 */
+		if (access_request == LANDLOCK_ACCESS_NET_BIND_TCP) {
+			const struct sockaddr_in *const sockaddr =
+				(struct sockaddr_in *)address;
+
+			if (sockaddr->sin_addr.s_addr != htonl(INADDR_ANY))
+				return -EAFNOSUPPORT;
+		}
+
+		fallthrough;
  	case AF_INET:
  #if IS_ENABLED(CONFIG_IPV6)
  	case AF_INET6:


I also added another check (copied from SELinux) with the appropriate 
explanation. All this needs dedicated tests to make sure everything is 
covered.

We also need to add extra checks (and related tests) for addrlen as do 
other LSMs.

Powered by blists - more mailing lists