| 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
| ||
|
Message-ID: <20230817130001.1493321-1-mic@digikod.net>
Date: Thu, 17 Aug 2023 15:00:01 +0200
From: Mickaël Salaün <mic@...ikod.net>
To: Konstantin Meskhidze <konstantin.meskhidze@...wei.com>
Cc: Mickaël Salaün <mic@...ikod.net>,
artem.kuzin@...wei.com,
gnoack3000@...il.com,
willemdebruijn.kernel@...il.com,
yusongping@...wei.com,
linux-security-module@...r.kernel.org,
netdev@...r.kernel.org,
netfilter-devel@...r.kernel.org
Subject: [PATCH] landlock: Fix and test network AF inconsistencies
Check af_family consistency while handling AF_UNSPEC specifically.
This patch should be squashed into the "Network support for Landlock"
v11 patch series.
Signed-off-by: Mickaël Salaün <mic@...ikod.net>
---
security/landlock/net.c | 29 ++++-
tools/testing/selftests/landlock/net_test.c | 124 +++++++++++++-------
2 files changed, 108 insertions(+), 45 deletions(-)
diff --git a/security/landlock/net.c b/security/landlock/net.c
index f8d2be53ac0d..ea5373f774f9 100644
--- a/security/landlock/net.c
+++ b/security/landlock/net.c
@@ -80,11 +80,11 @@ static int check_socket_access(struct socket *const sock,
if (WARN_ON_ONCE(domain->num_layers < 1))
return -EACCES;
- /* Checks if it's a TCP socket. */
+ /* Checks if it's a (potential) TCP socket. */
if (sock->type != SOCK_STREAM)
return 0;
- /* Checks for minimal header length. */
+ /* Checks for minimal header length to safely read sa_family. */
if (addrlen < offsetofend(struct sockaddr, sa_family))
return -EINVAL;
@@ -106,7 +106,6 @@ static int check_socket_access(struct socket *const sock,
return 0;
}
- /* Specific AF_UNSPEC handling. */
if (address->sa_family == AF_UNSPEC) {
/*
* Connecting to an address with AF_UNSPEC dissolves the TCP
@@ -114,6 +113,10 @@ static int check_socket_access(struct socket *const sock,
* connection while retaining the socket object (i.e., the file
* descriptor). As for dropping privileges, closing
* connections is always allowed.
+ *
+ * For a TCP access control system, this request is legitimate.
+ * Let the network stack handle potential inconsistencies and
+ * return -EINVAL if needed.
*/
if (access_request == LANDLOCK_ACCESS_NET_CONNECT_TCP)
return 0;
@@ -124,14 +127,34 @@ static int check_socket_access(struct socket *const sock,
* INADDR_ANY (cf. __inet_bind). Checking the address is
* required to not wrongfully return -EACCES instead of
* -EAFNOSUPPORT.
+ *
+ * We could return 0 and let the network stack handle these
+ * checks, but it is safer to return a proper error and test
+ * consistency thanks to kselftest.
*/
if (access_request == LANDLOCK_ACCESS_NET_BIND_TCP) {
+ /* addrlen has already been checked for AF_UNSPEC. */
const struct sockaddr_in *const sockaddr =
(struct sockaddr_in *)address;
+ if (sock->sk->__sk_common.skc_family != AF_INET)
+ return -EINVAL;
+
if (sockaddr->sin_addr.s_addr != htonl(INADDR_ANY))
return -EAFNOSUPPORT;
}
+ } else {
+ /*
+ * 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.
+ *
+ * We could return 0 and let the network stack handle this
+ * check, but it is safer to return a proper error and test
+ * consistency thanks to kselftest.
+ */
+ if (address->sa_family != sock->sk->__sk_common.skc_family)
+ return -EINVAL;
}
id.key.data = (__force uintptr_t)port;
diff --git a/tools/testing/selftests/landlock/net_test.c b/tools/testing/selftests/landlock/net_test.c
index 12dc127ea7d1..504a26c63fd9 100644
--- a/tools/testing/selftests/landlock/net_test.c
+++ b/tools/testing/selftests/landlock/net_test.c
@@ -233,7 +233,7 @@ static int connect_variant(const int sock_fd,
FIXTURE(protocol)
{
- struct service_fixture srv0, srv1, srv2, unspec_any, unspec_srv0;
+ struct service_fixture srv0, srv1, srv2, unspec_any0, unspec_srv0;
};
FIXTURE_VARIANT(protocol)
@@ -257,8 +257,8 @@ FIXTURE_SETUP(protocol)
ASSERT_EQ(0, set_service(&self->unspec_srv0, prot_unspec, 0));
- ASSERT_EQ(0, set_service(&self->unspec_any, prot_unspec, 0));
- self->unspec_any.ipv4_addr.sin_addr.s_addr = htonl(INADDR_ANY);
+ ASSERT_EQ(0, set_service(&self->unspec_any0, prot_unspec, 0));
+ self->unspec_any0.ipv4_addr.sin_addr.s_addr = htonl(INADDR_ANY);
setup_loopback(_metadata);
};
@@ -615,20 +615,18 @@ TEST_F(protocol, connect)
// Kernel FIXME: tcp_sandbox_with_ipv6_tcp and tcp_sandbox_with_unix_stream
TEST_F(protocol, bind_unspec)
{
+ const struct landlock_ruleset_attr ruleset_attr = {
+ .handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP,
+ };
+ const struct landlock_net_service_attr tcp_bind = {
+ .allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP,
+ .port = self->srv0.port,
+ };
int bind_fd, ret;
if (variant->sandbox == TCP_SANDBOX) {
- const struct landlock_ruleset_attr ruleset_attr = {
- .handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP,
- };
- const struct landlock_net_service_attr tcp_bind = {
- .allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP,
- .port = self->srv0.port,
- };
- int ruleset_fd;
-
- ruleset_fd = landlock_create_ruleset(&ruleset_attr,
- sizeof(ruleset_attr), 0);
+ const int ruleset_fd = landlock_create_ruleset(
+ &ruleset_attr, sizeof(ruleset_attr), 0);
ASSERT_LE(0, ruleset_fd);
/* Allows bind. */
@@ -642,8 +640,8 @@ TEST_F(protocol, bind_unspec)
bind_fd = socket_variant(&self->srv0);
ASSERT_LE(0, bind_fd);
- /* Binds on AF_UNSPEC/INADDR_ANY. */
- ret = bind_variant(bind_fd, &self->unspec_any);
+ /* Allowed bind on AF_UNSPEC/INADDR_ANY. */
+ ret = bind_variant(bind_fd, &self->unspec_any0);
if (variant->prot.domain == AF_INET) {
EXPECT_EQ(0, ret)
{
@@ -655,6 +653,33 @@ TEST_F(protocol, bind_unspec)
}
EXPECT_EQ(0, close(bind_fd));
+ if (variant->sandbox == TCP_SANDBOX) {
+ const int ruleset_fd = landlock_create_ruleset(
+ &ruleset_attr, sizeof(ruleset_attr), 0);
+ ASSERT_LE(0, ruleset_fd);
+
+ /* Denies bind. */
+ enforce_ruleset(_metadata, ruleset_fd);
+ EXPECT_EQ(0, close(ruleset_fd));
+ }
+
+ bind_fd = socket_variant(&self->srv0);
+ ASSERT_LE(0, bind_fd);
+
+ /* Denied bind on AF_UNSPEC/INADDR_ANY. */
+ ret = bind_variant(bind_fd, &self->unspec_any0);
+ if (variant->prot.domain == AF_INET) {
+ if (is_restricted(&variant->prot, variant->sandbox)) {
+ EXPECT_EQ(-EACCES, ret);
+ } else {
+ EXPECT_EQ(0, ret);
+ }
+ } else {
+ EXPECT_EQ(-EINVAL, ret);
+ }
+ EXPECT_EQ(0, close(bind_fd));
+
+ /* Checks bind with AF_UNSPEC and the loopback address. */
bind_fd = socket_variant(&self->srv0);
ASSERT_LE(0, bind_fd);
ret = bind_variant(bind_fd, &self->unspec_srv0);
@@ -671,34 +696,16 @@ TEST_F(protocol, bind_unspec)
TEST_F(protocol, connect_unspec)
{
+ const struct landlock_ruleset_attr ruleset_attr = {
+ .handled_access_net = LANDLOCK_ACCESS_NET_CONNECT_TCP,
+ };
+ const struct landlock_net_service_attr tcp_connect = {
+ .allowed_access = LANDLOCK_ACCESS_NET_CONNECT_TCP,
+ .port = self->srv0.port,
+ };
int bind_fd, client_fd, status;
pid_t child;
- if (variant->sandbox == TCP_SANDBOX) {
- const struct landlock_ruleset_attr ruleset_attr = {
- .handled_access_net = LANDLOCK_ACCESS_NET_CONNECT_TCP,
- };
- const struct landlock_net_service_attr tcp_connect = {
- .allowed_access = LANDLOCK_ACCESS_NET_CONNECT_TCP,
- .port = self->srv0.port,
- };
- int ruleset_fd;
-
- ruleset_fd = landlock_create_ruleset(&ruleset_attr,
- sizeof(ruleset_attr), 0);
- ASSERT_LE(0, ruleset_fd);
-
- /* Allows connect. */
- ASSERT_EQ(0, landlock_add_rule(ruleset_fd,
- LANDLOCK_RULE_NET_SERVICE,
- &tcp_connect, 0));
- enforce_ruleset(_metadata, ruleset_fd);
- EXPECT_EQ(0, close(ruleset_fd));
- }
-
- /* Generic connection tests. */
- test_bind_and_connect(_metadata, &self->srv0, false, false);
-
/* Specific connection tests. */
bind_fd = socket_variant(&self->srv0);
ASSERT_LE(0, bind_fd);
@@ -726,8 +733,22 @@ TEST_F(protocol, connect_unspec)
EXPECT_EQ(0, ret);
}
+ if (variant->sandbox == TCP_SANDBOX) {
+ const int ruleset_fd = landlock_create_ruleset(
+ &ruleset_attr, sizeof(ruleset_attr), 0);
+ ASSERT_LE(0, ruleset_fd);
+
+ /* Allows connect. */
+ ASSERT_EQ(0,
+ landlock_add_rule(ruleset_fd,
+ LANDLOCK_RULE_NET_SERVICE,
+ &tcp_connect, 0));
+ enforce_ruleset(_metadata, ruleset_fd);
+ EXPECT_EQ(0, close(ruleset_fd));
+ }
+
/* Disconnects already connected socket, or set peer. */
- ret = connect_variant(connect_fd, &self->unspec_any);
+ ret = connect_variant(connect_fd, &self->unspec_any0);
if (self->srv0.protocol.domain == AF_UNIX &&
self->srv0.protocol.type == SOCK_STREAM) {
EXPECT_EQ(-EINVAL, ret);
@@ -744,6 +765,25 @@ TEST_F(protocol, connect_unspec)
EXPECT_EQ(0, ret);
}
+ if (variant->sandbox == TCP_SANDBOX) {
+ const int ruleset_fd = landlock_create_ruleset(
+ &ruleset_attr, sizeof(ruleset_attr), 0);
+ ASSERT_LE(0, ruleset_fd);
+
+ /* Denies connect. */
+ enforce_ruleset(_metadata, ruleset_fd);
+ EXPECT_EQ(0, close(ruleset_fd));
+ }
+
+ ret = connect_variant(connect_fd, &self->unspec_any0);
+ if (self->srv0.protocol.domain == AF_UNIX &&
+ self->srv0.protocol.type == SOCK_STREAM) {
+ EXPECT_EQ(-EINVAL, ret);
+ } else {
+ /* Always allowed to disconnect. */
+ EXPECT_EQ(0, ret);
+ }
+
EXPECT_EQ(0, close(connect_fd));
_exit(_metadata->passed ? EXIT_SUCCESS : EXIT_FAILURE);
return;
--
2.41.0
Powered by blists - more mailing lists