On 9/23/2024 3:57 PM, Mikhail Ivanov wrote:
On 9/18/2024 4:47 PM, Günther Noack wrote:On Wed, Sep 04, 2024 at 06:48:19PM +0800, Mikhail Ivanov wrote:Add test that checks the restriction on socket creation using socketpair(2). Add `socket_creation` fixture to configure sandboxing in tests in which different socket creation actions are tested. Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@xxxxxxxxxxxxxxxxxxx> --- .../testing/selftests/landlock/socket_test.c | 101 ++++++++++++++++++ 1 file changed, 101 insertions(+)diff --git a/tools/testing/selftests/landlock/socket_test.c b/tools/testing/selftests/landlock/socket_test.cindex 8fc507bf902a..67db0e1c1121 100644 --- a/tools/testing/selftests/landlock/socket_test.c +++ b/tools/testing/selftests/landlock/socket_test.c @@ -738,4 +738,105 @@ TEST_F(packet_protocol, alias_restriction) EXPECT_EQ(0, test_socket_variant(&self->prot_tested)); } +static int test_socketpair(int family, int type, int protocol) +{ + int fds[2]; + int err; + + err = socketpair(family, type | SOCK_CLOEXEC, protocol, fds); + if (err) + return errno; + /*+ * Mixing error codes from close(2) and socketpair(2) should not lead to+ * any (access type) confusion for this test. + */ + if (close(fds[0]) != 0) + return errno; + if (close(fds[1]) != 0) + return errno; + return 0; +} + +FIXTURE(socket_creation) +{ + bool sandboxed; + bool allowed; +}; + +FIXTURE_VARIANT(socket_creation) +{ + bool sandboxed; + bool allowed; +}; + +FIXTURE_SETUP(socket_creation) +{ + self->sandboxed = variant->sandboxed; + self->allowed = variant->allowed; + + setup_loopback(_metadata); +}; + +FIXTURE_TEARDOWN(socket_creation) +{ +} + +/* clang-format off */ +FIXTURE_VARIANT_ADD(socket_creation, no_sandbox) { + /* clang-format on */ + .sandboxed = false, +}; + +/* clang-format off */ +FIXTURE_VARIANT_ADD(socket_creation, sandbox_allow) { + /* clang-format on */ + .sandboxed = true, + .allowed = true, +}; + +/* clang-format off */ +FIXTURE_VARIANT_ADD(socket_creation, sandbox_deny) { + /* clang-format on */ + .sandboxed = true, + .allowed = false, +}; + +TEST_F(socket_creation, socketpair) +{ + const struct landlock_ruleset_attr ruleset_attr = { + .handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE, + }; + struct landlock_socket_attr unix_socket_create = { + .allowed_access = LANDLOCK_ACCESS_SOCKET_CREATE, + .family = AF_UNIX, + .type = SOCK_STREAM, + }; + int ruleset_fd; + + if (self->sandboxed) { + ruleset_fd = landlock_create_ruleset(&ruleset_attr, + sizeof(ruleset_attr), 0); + ASSERT_LE(0, ruleset_fd); + + if (self->allowed) { + ASSERT_EQ(0, landlock_add_rule(ruleset_fd, + LANDLOCK_RULE_SOCKET, + &unix_socket_create, 0)); + } + enforce_ruleset(_metadata, ruleset_fd); + ASSERT_EQ(0, close(ruleset_fd)); + } + + if (!self->sandboxed || self->allowed) { + /* + * Tries to create sockets when ruleset is not established + * or protocol is allowed. + */ + EXPECT_EQ(0, test_socketpair(AF_UNIX, SOCK_STREAM, 0)); + } else { + /* Tries to create sockets when protocol is restricted. */ + EXPECT_EQ(EACCES, test_socketpair(AF_UNIX, SOCK_STREAM, 0)); + }I am torn on whether socketpair() should be denied at all -- * on one hand, the created sockets are connected to each otherand the creating process can only talk to itself (or pass one of them on),which seems legitimate and harmless. * on the other hand, it *does* create two sockets, and if they are datagram sockets, it it probably currently possible to disassociate them with connect(AF_UNSPEC). > What are your thoughts on that?Good catch! According to the discussion that you've mentioned [1] (I believe I found correct one), you've already discussed socketpair(2) control with Mickaël and came to the conclusion that socketpair(2) and unnamed pipes do not give access to new resources to the process, therefore should not be restricted.[1] https://lore.kernel.org/all/e7e24682-5da7-3b09-323e-a4f784f10158@xxxxxxxxxxx/Therefore, this is more like connect(AF_UNSPEC)-related issue. On security summit you've mentioned that it will be useful to implement restriction of connection dissociation for sockets. This feature will solve the problem of reusage of UNIX sockets that were created with socketpair(2).
Btw, I can suggest one more scenario, where restriction of disassociation can be useful. SMC sockets (AF_SMC+SOCK_STREAM) can fall back to TCP during the connection (cf. smc_connect_decline_fallback). Then user can call connect(AF_UNSPEC) to eventually get a TCP socket in the initial (TCP_CLOSE) state which can be used to establish another connection. This can be considered as an issue for the current patchset, because there is a way to "create" a TCP socket while TCP is restricted by Landlock (if ruleset allows SMC). Besides it, there is another issue with SMC restriction that I'm going to fix in the next RFC: recently has been applied patchset that allows to create SMC sockets via AF_INET [1]. Such creation should be denied if Landlock restricts SMC.[1] https://lore.kernel.org/all/1718301630-63692-1-git-send-email-alibuda@xxxxxxxxxxxxxxxxx/
If we want such feature to be implemented, I suggest leaving current implementation as it is (to prevent vulnerable creation of UNIX dgram sockets) and enable socketpair(2) in the patchset dedicated to connect(AF_UNSPEC) restriction. Also it will be useful to create a dedicated issue on github. WDYT? (Btw I think that disassociation control can be really useful. If it were possible to restrict this action for each protocol, we would have stricter control over the protocols used.)Mickaël, I believe we have also discussed similar questions for pipe(2) in thepast, and you had opinions on that? (On a much more technical note; consider replacing self->allowed withself->socketpair_error to directly indicate the expected error? It feels thatthis could be more straightforward?)I've considered this approach and decided that this would * negatively affect the readability of conditional for adding Landlock rule, * make checking the test_socketpair() error code less explicit.—Günther
