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@xxxxxxxxxxxxxxxxxxx>
---
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@xxxxxxxxxxx/
