| Message ID | 20241017110454.265818-2-ivanov.mikhail1@huawei-partners.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Fix non-TCP restriction and inconsistency of TCP errors | expand |
Hi Mikhail and Landlock maintainers, +cc MPTCP list. On 17/10/2024 13:04, 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). Thank you for the patch! I'm part of the MPTCP team, and I'm wondering if MPTCP should not be treated like TCP here. MPTCP is an extension to TCP: on the wire, we can see TCP packets with extra TCP options. On Linux, there is indeed a dedicated MPTCP socket (IPPROTO_MPTCP), but that's just internal, because we needed such dedicated socket to talk to the userspace. I don't know Landlock well, but I think it is important to know that an MPTCP socket can be used to discuss with "plain" TCP packets: the kernel will do a fallback to "plain" TCP if MPTCP is not supported by the other peer or by a middlebox. It means that with this patch, if TCP is blocked by Landlock, someone can simply force an application to create an MPTCP socket -- e.g. via LD_PRELOAD -- and bypass the restrictions. It will certainly work, even when connecting to a peer not supporting MPTCP. Please note that I'm not against this modification -- especially here when we remove restrictions around MPTCP sockets :) -- I'm just saying it might be less confusing for users if MPTCP is considered as being part of TCP. A bit similar to what someone would do with a firewall: if TCP is blocked, MPTCP is blocked as well. I understand that a future goal might probably be to have dedicated restrictions for MPTCP and the other stream protocols (and/or for all stream protocols like it was before this patch), but in the meantime, it might be less confusing considering MPTCP as being part of TCP (I'm not sure about the other stream protocols). > sk_is_tcp() is used for this to check address family of the socket > before doing INET-specific address length validation. This is required > for error consistency. > > Closes: https://github.com/landlock-lsm/linux/issues/40 > Fixes: fff69fb03dde ("landlock: Support network rules with TCP bind and connect") I don't know how fixes are considered in Landlock, but should this patch be considered as a fix? It might be surprising for someone who thought all "stream" connections were blocked to have them unblocked when updating to a minor kernel version, no? (Personally, I would understand such behaviour change when upgrading to a major version, and still, maybe only if there were alternatives to continue having the same behaviour, e.g. a way to restrict all stream sockets the same way, or something per stream socket. But that's just me :) ) Cheers, Matt
diff --git a/security/landlock/net.c b/security/landlock/net.c index fdc1bb0a9c5d..1e80782ba239 100644 --- a/security/landlock/net.c +++ b/security/landlock/net.c @@ -66,8 +66,8 @@ static int current_check_access_socket(struct socket *const sock, if (WARN_ON_ONCE(dom->num_layers < 1)) return -EACCES; - /* Checks if it's a (potential) TCP socket. */ - if (sock->type != SOCK_STREAM) + /* Do not restrict non-TCP sockets. */ + if (!sk_is_tcp(sock->sk)) return 0; /* Checks for minimal header length to safely read sa_family. */
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). sk_is_tcp() is used for this to check address family of the socket before doing INET-specific address length validation. This is required for error consistency. 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@huawei-partners.com> --- Changes since v1: * Validate socket family (=INET{,6}) before any other checks with sk_is_tcp(). --- security/landlock/net.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)