netdev - Re: [RFC PATCH v1 1/2] landlock: Fix non-TCP sockets restriction

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <06f1d60a-91b6-7fa4-8839-e1752dbc2ec8@huawei-partners.com>
Date: Mon, 7 Oct 2024 14:58:43 +0300
From: Mikhail Ivanov <ivanov.mikhail1@...wei-partners.com>
To: Mickaël Salaün <mic@...ikod.net>, Paul Moore
	<paul@...l-moore.com>
CC: <gnoack@...gle.com>, <willemdebruijn.kernel@...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>, Matthieu Buffet
	<matthieu@...fet.re>
Subject: Re: [RFC PATCH v1 1/2] landlock: Fix non-TCP sockets restriction

On 10/7/2024 2:06 PM, Mikhail Ivanov wrote:
> On 10/5/2024 6:49 PM, Mickaël Salaün wrote:
>> On Fri, Oct 04, 2024 at 09:16:56PM +0300, Mikhail Ivanov wrote:
>>> On 10/4/2024 1:13 PM, Mickaël Salaün wrote:
>>>> On Fri, Oct 04, 2024 at 12:30:02AM +0300, Mikhail Ivanov wrote:
>>>>> On 10/3/2024 8:45 PM, Mickaël Salaün wrote:
>>>>>> Please also add Matthieu in Cc for the network patch series.
>>>>>>
>>>>>> On Thu, Oct 03, 2024 at 10:39:31PM +0800, Mikhail Ivanov wrote:
>>>>>>> Do not check TCP access right if socket protocol is not IPPROTO_TCP.
>>>>>>> LANDLOCK_ACCESS_NET_BIND_TCP and LANDLOCK_ACCESS_NET_CONNECT_TCP
>>>>>>> should not restrict bind(2) and connect(2) for non-TCP protocols
>>>>>>> (SCTP, MPTCP, SMC).
>>>>>>>
>>>>>>> Closes: https://github.com/landlock-lsm/linux/issues/40
>>>>>>> Fixes: fff69fb03dde ("landlock: Support network rules with TCP 
>>>>>>> bind and connect")
>>>>>>> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@...wei-partners.com>
>>>>>>> ---
>>>>>>>     security/landlock/net.c | 2 +-
>>>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/security/landlock/net.c b/security/landlock/net.c
>>>>>>> index bc3d943a7118..6f59dd98bb13 100644
>>>>>>> --- a/security/landlock/net.c
>>>>>>> +++ b/security/landlock/net.c
>>>>>>> @@ -68,7 +68,7 @@ static int current_check_access_socket(struct 
>>>>>>> socket *const sock,
>>>>>>>             return -EACCES;
>>>>>>>         /* Checks if it's a (potential) TCP socket. */
>>>>>>
>>>>>> We can extend this comment to explain that we don't use sk_is_tcp()
>>>>>> because we need to handle the AF_UNSPEC case.
>>>>>
>>>>> Indeed, I'll do this.
>>>
>>> I've noticed that we still should check sk->sk_family = AF_INET{,6}
>>> here (so sk_is_tcp() is suitable). AF_UNSPEC can be only related to
>>> addresses and we should not provide any checks (for address) if socket
>>> is unrestrictable (i.e. it's not TCP). It's not useful and might lead to
>>> error incosistency for non-TCP sockets.
>>
>> Good catch, let's use sk_is_tcp().
>>
>>>
>>> Btw, I suppose we can improve error consistency by bringing more checks
>>> from INET/TCP stack. For example it may be useful to return EISCONN
>>> instead of EACCES while connect(2) is called on a connected socket.
>>
>> Yes, that would be nice (with the related tests).
>>
>>>
>>> This should be done really carefully and only for some useful cases.
>>> Anyway it's not related to the current patch (since it's not a bug).
>>
>> Sure.
> 
> I have a little question to clarify before sending a next version. Are
> we condisering order of network checks for error consistency?
> 
> For example, in the current_check_access_socket() we have following
> order of checks for ipv4 connect(2) action:
> (1) addrlen < sizeof(struct sockaddr_in) -> return -EINVAL
> (2) sa_family != sk_family -> return -EINVAL
> 
> The ipv4 stack has a check for sock->state before (1) and (2), which can
> return -EISCONN if the socket is already connected.
> 
> This results in the possiblity of two following scenarios:
> 
> Landlock enabled:
> 1. socket(ipv4) -> OK
> 2. connect(ipv4 address) -> OK
> 3. connect(ipv6 address) -> -EINVAL (sa_family != sk_family)
> 
> Landlock disabled:
> 1. socket(ipv4) -> OK
> 2. connect(ipv4 address) -> OK
> 3. connect(ipv6 address) -> -EISCONN (socket is already connected)
> 
> I have always considered the order of network checks as part of error
> consistency, and I'd like to make sure that we're on the same page
> before extending current patch with error inconsistency fixes.

BTW, a similar inconsistency in the error order was also found in
selinux hooks. Accounting [1], I wonder if validating socket state
in security hooks for bind/connect actions has been considered before.

[1] https://lore.kernel.org/all/20231228113917.62089-1-mic@digikod.net/