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

["Mickaël Salaün" <mic@digikod.net>]

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.


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?

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