From patchwork Wed Mar 27 12:00:33 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= X-Patchwork-Id: 13606305 Received: from smtp-8fa8.mail.infomaniak.ch (smtp-8fa8.mail.infomaniak.ch [83.166.143.168]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 5F7991353E1 for ; Wed, 27 Mar 2024 12:08:07 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=83.166.143.168 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1711541295; cv=none; b=b9dy0VU1vXp9g6wSroKcKVJ2AohisgCsk+bzhJd68tQh3r74wotLQHfBnzzsHxqrUhqFnML/7QYwqIwDx7Vy3XOoINF9fp8ZrqXWTm8KjeSsWSsffKXh2Eyxf6mKfgNzp/yR1K31QPKub99D6NYgUKGSWC7hMjHRFSfdtZLnako= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1711541295; c=relaxed/simple; bh=wsJyPEx2+OHIxwu1PDF8iIUG11MmICqv+9NDlJGjClk=; h=From:To:Cc:Subject:Date:Message-ID:MIME-Version:Content-Type; b=JyF1lCTVDS7wleuNp7vZx+fZijya0m26hDyEx48+RnzAch+rT7MKlTgPlqGhtgLa28dGrOVfUJJxAFRZYI6IE5bzu6SyrT84Fg1x1i43AaR0yQX+CnxtNIZ9jKaHSczmOjVqQ4eZw+cN8q1JQaBpocUTAlhWxxHJeqAiLBD5P7A= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=digikod.net; spf=pass smtp.mailfrom=digikod.net; dkim=pass (1024-bit key) header.d=digikod.net header.i=@digikod.net header.b=ckRe9EAU; arc=none smtp.client-ip=83.166.143.168 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=digikod.net Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=digikod.net Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=digikod.net header.i=@digikod.net header.b="ckRe9EAU" Received: from smtp-4-0000.mail.infomaniak.ch (smtp-4-0000.mail.infomaniak.ch [10.7.10.107]) by smtp-3-3000.mail.infomaniak.ch (Postfix) with ESMTPS id 4V4QGy6bSdzCVP; Wed, 27 Mar 2024 13:00:42 +0100 (CET) Received: from unknown by smtp-4-0000.mail.infomaniak.ch (Postfix) with ESMTPA id 4V4QGx73kmzDJl; Wed, 27 Mar 2024 13:00:41 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=digikod.net; s=20191114; t=1711540842; bh=wsJyPEx2+OHIxwu1PDF8iIUG11MmICqv+9NDlJGjClk=; h=From:To:Cc:Subject:Date:From; b=ckRe9EAUoLoeV6FrsJfCbIK0mRmcQrQC/BMWJyAEuPhGLRsiVC5Qlx+Wzl5fzw+XX CUM2NkPPvQDMwJwZtBmdrwDS16tpVRndjPmTeaCvczBe8xqhxPi9SqRyHEwruemKAX VY4+FtC8oIeHXLY9GKGGP9owvJh63Tz9cA7IJkVg= From: =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= To: Paul Moore Cc: =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= , linux-security-module@vger.kernel.org, netdev@vger.kernel.org, Alexey Kodanev , Eric Dumazet , =?utf-8?q?G=C3=BCnther_Noack?= , Ivanov Mikhail , Konstantin Meskhidze , Muhammad Usama Anjum , "Serge E . Hallyn" Subject: [PATCH v1 1/2] lsm: Check and handle error priority for socket_bind and socket_connect Date: Wed, 27 Mar 2024 13:00:33 +0100 Message-ID: <20240327120036.233641-1-mic@digikod.net> Precedence: bulk X-Mailing-List: netdev@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 X-Infomaniak-Routing: alpha Because the security_socket_bind and the security_socket_bind hooks are called before the network stack, it is easy to introduce error code inconsistencies. Instead of adding new checks to current and future LSMs, let's fix the related hook instead. The new checks are already (partially) implemented by SELinux and Landlock, and it should not change user space behavior but improve error code consistency instead. The first check is about the minimal sockaddr length according to the address family. This improves the security of the AF_INET and AF_INET6 sockaddr parsing for current and future LSMs. The second check is about AF_UNSPEC. This fixes error priority for bind on PF_INET6 socket when SELinux (and potentially others) is enabled. Indeed, the IPv6 network stack first checks the sockaddr length (-EINVAL error) before checking the family (-EAFNOSUPPORT error). See commit bbf5a1d0e5d0 ("selinux: Fix error priority for bind with AF_UNSPEC on PF_INET6 socket"). The third check is about consistency between socket family and address family. Only AF_INET and AF_INET6 are tested (by Landlock tests), so no other protocols are checked for now. These new checks should enable to simplify current LSM implementations, but we may want to first land this patch on all stable branches. A following patch adds new tests improving AF_UNSPEC test coverage for Landlock. Cc: Alexey Kodanev Cc: Eric Dumazet Cc: Günther Noack Cc: Ivanov Mikhail Cc: Konstantin Meskhidze Cc: Muhammad Usama Anjum Cc: Paul Moore Cc: Serge E. Hallyn Fixes: 20510f2f4e2d ("security: Convert LSM into a static interface") Signed-off-by: Mickaël Salaün --- security/security.c | 96 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 96 insertions(+) diff --git a/security/security.c b/security/security.c index 7e118858b545..64fe07a73b14 100644 --- a/security/security.c +++ b/security/security.c @@ -28,7 +28,9 @@ #include #include #include +#include #include +#include /* How many LSMs were built into the kernel? */ #define LSM_COUNT (__end_lsm_info - __start_lsm_info) @@ -4415,6 +4417,82 @@ int security_socket_socketpair(struct socket *socka, struct socket *sockb) } EXPORT_SYMBOL(security_socket_socketpair); +static int validate_inet_addr(struct socket *sock, struct sockaddr *address, + int addrlen, bool bind) +{ + const int sock_family = sock->sk->sk_family; + + /* Checks for minimal header length to safely read sa_family. */ + if (addrlen < offsetofend(typeof(*address), sa_family)) + return -EINVAL; + + /* Only handle inet sockets for now. */ + switch (sock_family) { + case PF_INET: + case PF_INET6: + break; + default: + return 0; + } + + /* Checks minimal address length for inet sockets. */ + switch (address->sa_family) { + case AF_UNSPEC: { + const struct sockaddr_in *sa_in; + + /* Cf. inet_dgram_connect(), __inet_stream_connect() */ + if (!bind) + return 0; + + if (sock_family == PF_INET6) { + /* Length check from inet6_bind_sk() */ + if (addrlen < SIN6_LEN_RFC2133) + return -EINVAL; + + /* Family check from __inet6_bind() */ + goto err_af; + } + + /* Length check from inet_bind_sk() */ + if (addrlen < sizeof(struct sockaddr_in)) + return -EINVAL; + + sa_in = (struct sockaddr_in *)address; + if (sa_in->sin_addr.s_addr != htonl(INADDR_ANY)) + goto err_af; + + return 0; + } + case AF_INET: + /* Length check from inet_bind_sk() */ + if (addrlen < sizeof(struct sockaddr_in)) + return -EINVAL; + break; + case AF_INET6: + /* Length check from inet6_bind_sk() */ + if (addrlen < SIN6_LEN_RFC2133) + return -EINVAL; + break; + } + + /* + * Checks sa_family consistency to not wrongfully return -EACCES + * instead of -EINVAL. Valid sa_family changes are only (from AF_INET + * or AF_INET6) to AF_UNSPEC. + */ + if (address->sa_family != sock_family) + return -EINVAL; + + return 0; + +err_af: + /* SCTP services expect -EINVAL, others -EAFNOSUPPORT. */ + if (sock->sk->sk_protocol == IPPROTO_SCTP) + return -EINVAL; + + return -EAFNOSUPPORT; +} + /** * security_socket_bind() - Check if a socket bind operation is allowed * @sock: socket @@ -4425,11 +4503,23 @@ EXPORT_SYMBOL(security_socket_socketpair); * and the socket @sock is bound to the address specified in the @address * parameter. * + * For security reasons and to get consistent error code whatever LSM are + * enabled, we first do the same sanity checks against sockaddr as the ones + * done by the network stack (executed after hook). Currently only AF_UNSPEC, + * AF_INET, and AF_INET6 are handled. Please add support for other family + * specificities when handled by an LSM. + * * Return: Returns 0 if permission is granted. */ int security_socket_bind(struct socket *sock, struct sockaddr *address, int addrlen) { + int err; + + err = validate_inet_addr(sock, address, addrlen, true); + if (err) + return err; + return call_int_hook(socket_bind, sock, address, addrlen); } @@ -4447,6 +4537,12 @@ int security_socket_bind(struct socket *sock, int security_socket_connect(struct socket *sock, struct sockaddr *address, int addrlen) { + int err; + + err = validate_inet_addr(sock, address, addrlen, false); + if (err) + return err; + return call_int_hook(socket_connect, sock, address, addrlen); } From patchwork Wed Mar 27 12:00:34 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= X-Patchwork-Id: 13606709 Received: from smtp-8fae.mail.infomaniak.ch (smtp-8fae.mail.infomaniak.ch [83.166.143.174]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 6828D13F007 for ; Wed, 27 Mar 2024 13:48:07 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=83.166.143.174 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1711547296; cv=none; b=RTfG3YMOECMhqFs8x2whP6yv7nOSNPL+AOmuGXi/h9EiXbk3iNoV2u/pDg4JDSdgX8gS9KXwCf2kVYVleQ7jbeyTzKoycOeOWLLOeSx9R4bOYuIpYqpPbbz1uoob+ho+TlO/cl0odHi+ybUtQZh8ddxVu3NMFop7VNm1fJSJ5T0= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1711547296; c=relaxed/simple; bh=amdXCZoqj2z7s46MEj+Fcv8Tjjs58JdD4tUlu13JbKk=; h=From:To:Cc:Subject:Date:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=h0Zgb7sP2BF6oriocDUp5hBO+eKLKgNZWeY7vtfIHPMVRPkbS4Q/Se9dLqUHASOsdH9HjgirpKBABW+NVhixIf/vE84inn2YiBH5vfsI9eIwxork4xG2SwCMJJCS8ORQ0nJh2AHdXJ3gNsO8HwigCIdaNcJlFk/EPYmvcTlRQfs= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=digikod.net; spf=pass smtp.mailfrom=digikod.net; dkim=pass (1024-bit key) header.d=digikod.net header.i=@digikod.net header.b=bYVWGxp/; arc=none smtp.client-ip=83.166.143.174 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=digikod.net Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=digikod.net Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=digikod.net header.i=@digikod.net header.b="bYVWGxp/" Received: from smtp-3-0000.mail.infomaniak.ch (smtp-3-0000.mail.infomaniak.ch [10.4.36.107]) by smtp-3-3000.mail.infomaniak.ch (Postfix) with ESMTPS id 4V4QGz5HCCzC90; Wed, 27 Mar 2024 13:00:43 +0100 (CET) Received: from unknown by smtp-3-0000.mail.infomaniak.ch (Postfix) with ESMTPA id 4V4QGz2KgCz3d; Wed, 27 Mar 2024 13:00:43 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=digikod.net; s=20191114; t=1711540843; bh=amdXCZoqj2z7s46MEj+Fcv8Tjjs58JdD4tUlu13JbKk=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=bYVWGxp/o3FfD53NBkQhGzibdQJ9oi+Y+2oU1Lnl5r3xA1bR8f9ECKufh3+EEOAKw BO3ef5tTZj8ED8woDIKpmODifwsNG1dJJ4Mfk98gywE6+NkrutfBTWtgrpRbYlWsD/ AAuy9OBo87QDRD66LWhAVc6lnAmxBcjs3nsmt+6w= From: =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= To: Paul Moore Cc: =?utf-8?q?Micka=C3=ABl_Sala=C3=BCn?= , linux-security-module@vger.kernel.org, netdev@vger.kernel.org, Eric Dumazet , =?utf-8?q?G=C3=BCnther_Noack?= , Ivanov Mikhail , Konstantin Meskhidze , "Serge E . Hallyn" Subject: [PATCH v1 2/2] selftests/landlock: Improve AF_UNSPEC tests Date: Wed, 27 Mar 2024 13:00:34 +0100 Message-ID: <20240327120036.233641-2-mic@digikod.net> In-Reply-To: <20240327120036.233641-1-mic@digikod.net> References: <20240327120036.233641-1-mic@digikod.net> Precedence: bulk X-Mailing-List: netdev@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 X-Infomaniak-Routing: alpha Test that an IPv6 socket requested to be binded with AF_UNSPEC returns -EAFNOSUPPORT if the sockaddr length is valid. Cc: Eric Dumazet Cc: Günther Noack Cc: Ivanov Mikhail Cc: Konstantin Meskhidze Cc: Paul Moore Cc: Serge E. Hallyn Signed-off-by: Mickaël Salaün --- tools/testing/selftests/landlock/net_test.c | 37 +++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/tools/testing/selftests/landlock/net_test.c b/tools/testing/selftests/landlock/net_test.c index f21cfbbc3638..f15cf565f856 100644 --- a/tools/testing/selftests/landlock/net_test.c +++ b/tools/testing/selftests/landlock/net_test.c @@ -673,6 +673,7 @@ TEST_F(protocol, bind_unspec) .port = self->srv0.port, }; int bind_fd, ret; + socklen_t inet_len, inet6_len, unix_len; if (variant->sandbox == TCP_SANDBOX) { const int ruleset_fd = landlock_create_ruleset( @@ -736,12 +737,48 @@ TEST_F(protocol, bind_unspec) if (variant->prot.domain == AF_INET) { EXPECT_EQ(-EAFNOSUPPORT, ret); } else { + /* The sockaddr length is less than SIN6_LEN_RFC2133. */ EXPECT_EQ(-EINVAL, ret) { TH_LOG("Wrong bind error: %s", strerror(errno)); } } EXPECT_EQ(0, close(bind_fd)); + + /* Stores the minimal sockaddr lengths per family. */ + self->unspec_srv0.protocol.domain = AF_INET; + inet_len = get_addrlen(&self->unspec_srv0, true); + self->unspec_srv0.protocol.domain = AF_INET6; + inet6_len = get_addrlen(&self->unspec_srv0, true); + self->unspec_srv0.protocol.domain = AF_UNIX; + unix_len = get_addrlen(&self->unspec_srv0, true); + self->unspec_srv0.protocol.domain = AF_UNSPEC; + + /* Checks bind with AF_UNSPEC and less than IPv4 sockaddr length. */ + bind_fd = socket_variant(&self->srv0); + ASSERT_LE(0, bind_fd); + ret = bind_variant_addrlen(bind_fd, &self->unspec_srv0, inet_len - 1); + EXPECT_EQ(-EINVAL, ret); + EXPECT_EQ(0, close(bind_fd)); + + /* Checks bind with AF_UNSPEC and IPv6 sockaddr length. */ + bind_fd = socket_variant(&self->srv0); + ASSERT_LE(0, bind_fd); + ret = bind_variant_addrlen(bind_fd, &self->unspec_srv0, inet6_len); + if (variant->prot.domain == AF_INET || + variant->prot.domain == AF_INET6) { + EXPECT_EQ(-EAFNOSUPPORT, ret); + } else { + EXPECT_EQ(-EINVAL, ret); + } + EXPECT_EQ(0, close(bind_fd)); + + /* Checks bind with AF_UNSPEC and unix sockaddr length. */ + bind_fd = socket_variant(&self->srv0); + ASSERT_LE(0, bind_fd); + ret = bind_variant_addrlen(bind_fd, &self->unspec_srv0, unix_len); + EXPECT_EQ(-EINVAL, ret); + EXPECT_EQ(0, close(bind_fd)); } TEST_F(protocol, connect_unspec)