Re: [PATCH v8 08/12] landlock: Implement TCP network hooks

["Konstantin Meskhidze (A)" <konstantin.meskhidze@huawei.com>]

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@huawei.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/


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.

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