| Message ID | 20240904104824.1844082-15-ivanov.mikhail1@huawei-partners.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Support socket access-control | expand |
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@huawei-partners.com> > --- > .../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.c > index 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 other and 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? Mickaël, I believe we have also discussed similar questions for pipe(2) in the past, and you had opinions on that? (On a much more technical note; consider replacing self->allowed with self->socketpair_error to directly indicate the expected error? It feels that this could be more straightforward?) —Günther
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@huawei-partners.com> >> --- >> .../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.c >> index 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 other > and 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@digikod.net/ 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). 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 the > past, and you had opinions on that? > > > (On a much more technical note; consider replacing self->allowed with > self->socketpair_error to directly indicate the expected error? It feels that > this 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
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@huawei-partners.com> >>> --- >>> .../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.c >>> index 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 other >> and 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@digikod.net/ > > 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@linux.alibaba.com/ > > 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 the >> past, and you had opinions on that? >> >> >> (On a much more technical note; consider replacing self->allowed with >> self->socketpair_error to directly indicate the expected error? It >> feels that >> this 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
On Mon, Sep 23, 2024 at 03:57:47PM +0300, 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@huawei-partners.com> > > > --- > > > .../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.c > > > index 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 other > > and 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@digikod.net/ > > 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). > > 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? Thanks for digging up that discussion, that's exactly the one I meant. I have a feeling that this may result in compatibility issues later on? If we leave the current implementation as it is, then we are *blocking* the creation of sockets through socketpair(2). And then we would have users who add it as a restricted ("handled") operation in their ruleset, and who would expect that socketpair(2) can not be used. When that API is already fixed, how do you imagine that people should in the future allow socketpair(2), but disallow the "normal" creation of sockets? In my mind, I would have imagined that the LANDLOCK_ACCESS_SOCKET_CREATE right only restricts socket(2) invocations and leaves socketpair(2) working, and then we could introduce a LANDLOCK_ACCESS_SOCKETPAIR_CREATE right in the future to restrict socketpair(2) as well? If we wanted to permit socketpair(2), but allow socket(2), would we have to change the LSM hook interface? How would that implementation look? > (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.) In my understanding, the disassociation support is closely intertwined with the transport layer - the last paragraph of DESCRIPTION in connect(2) is listing TCP, UDP and Unix Domain sockets in datagram mode. -- The relevant code in in net/ipv4/af_inet.c in inet_dgram_connect() and __inet_stream_connect(), where AF_UNSPEC is handled. I would love to find a way to restrict this independent of the specific transport protocol as well. Remark on the side - in af_inet.c in inet_shutdown(), I also found a worrying scenario where the same sk->sk_prot->disconnect() function is called and sock->state also gets reset to SS_UNCONNECTED. I have done a naive attempt to hit that code path by calling shutdown() on a passive TCP socket, but was not able to reuse the socket for new connections afterwards. (Have not debugged it further though.) I wonder whether this is a scnenario that we also need to cover? > > (On a much more technical note; consider replacing self->allowed with > > self->socketpair_error to directly indicate the expected error? It feels that > > this 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. Fair enough, OK. —Günther
On Fri, Sep 27, 2024 at 11:48:22AM +0200, Günther Noack wrote: > On Mon, Sep 23, 2024 at 03:57:47PM +0300, Mikhail Ivanov wrote: > > (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.) > > In my understanding, the disassociation support is closely intertwined with the > transport layer - the last paragraph of DESCRIPTION in connect(2) is listing > TCP, UDP and Unix Domain sockets in datagram mode. -- The relevant code in in > net/ipv4/af_inet.c in inet_dgram_connect() and __inet_stream_connect(), where > AF_UNSPEC is handled. > > I would love to find a way to restrict this independent of the specific > transport protocol as well. > > Remark on the side - in af_inet.c in inet_shutdown(), I also found a worrying > scenario where the same sk->sk_prot->disconnect() function is called and > sock->state also gets reset to SS_UNCONNECTED. I have done a naive attempt to > hit that code path by calling shutdown() on a passive TCP socket, but was not > able to reuse the socket for new connections afterwards. (Have not debugged it > further though.) I wonder whether this is a scnenario that we also need to > cover? FYI, **this does turn out to work** (I just fumbled in my first experiment). -- It is possible to reset a listening socket with shutdown() into a state where it can be used for at least a new connect(2), and maybe also for new listen(2)s. The same might also be possible if a socket is in the TCP_SYN_SENT state at the time of shutdown() (although that is a bit trickier to try out). So a complete disassociation control for TCP/IP might not only need to have LANDLOCK_ACCESS_SOCKET_CONNECT_UNSPEC (or however we'd call it), but also LANDLOCK_ACCESS_SOCKET_PASSIVE_SHUTDOWN and maybe even another one for the TCP_SYN_SENT case...? * It makes me uneasy to think that I only looked at AF_INET{,6} and TCP so far, and that other protocols would need a similarly close look. It will be difficult to cover all the "disassociation" cases in all the different protocols, and even more difficult to detect new ones when they pop up. If we discover new ones and they'd need new Landlock access rights, it would also potentially mean that existing Landlock users would have to update their rules to spell that out. It might be easier after all to not rely on "disassociation" control too much and instead to design the network-related access rights in a way so that we can provide the desired sandboxing guarantees by restricting the "constructive" operations (the ones that initiate new network connections or that listen on the network). Mikhail, in your email I am quoting above, you are saying that "disassociation control can be really useful"; do you know of any cases where a restriction of connect/listen is *not* enough and where you'd still want the disassociation control? (In my mind, the disassociation control would have mainly been needed if we had gone with Mickaël's "Use socket's Landlock domain" RFC [1]? Mickaël and me have discussed this patch set at LSS and I am also now coming around to the realization that this would have introduced more complication. - It might have been a more "pure" approach, but comes at the expense of complicating Landlock usage.) —Günther [1] https://lore.kernel.org/all/20240719150618.197991-1-mic@digikod.net/ * for later reference, my reasoning in the code is: net/ipv4/af_inet.c implements the entry points for connect() and listen() at the address family layer. Both operations require that the sock->state is SS_UNCONNECTED. So the rest is going through the other occurrences of SS_UNCONNECTED in that same file to see if there are any places where the socket can get back into that state. The places I found where it is set to that state are: 1. inet_create (right after creation, expected) 2. __inet_stream_connect in the AF_UNSPEC case (known issue) 3. __inet_stream_connect in the case of a failed connect (expected) 4. inet_shutdown in the case of TCP_LISTEN or TCP_SYN_SENT (mentioned above)
On Sat, Sep 28, 2024 at 10:06:52PM +0200, Günther Noack wrote: > On Fri, Sep 27, 2024 at 11:48:22AM +0200, Günther Noack wrote: > > On Mon, Sep 23, 2024 at 03:57:47PM +0300, Mikhail Ivanov wrote: > > > (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.) > > > > In my understanding, the disassociation support is closely intertwined with the > > transport layer - the last paragraph of DESCRIPTION in connect(2) is listing > > TCP, UDP and Unix Domain sockets in datagram mode. -- The relevant code in in > > net/ipv4/af_inet.c in inet_dgram_connect() and __inet_stream_connect(), where > > AF_UNSPEC is handled. > > > > I would love to find a way to restrict this independent of the specific > > transport protocol as well. > > > > Remark on the side - in af_inet.c in inet_shutdown(), I also found a worrying > > scenario where the same sk->sk_prot->disconnect() function is called and > > sock->state also gets reset to SS_UNCONNECTED. I have done a naive attempt to > > hit that code path by calling shutdown() on a passive TCP socket, but was not > > able to reuse the socket for new connections afterwards. (Have not debugged it > > further though.) I wonder whether this is a scnenario that we also need to > > cover? > > FYI, **this does turn out to work** (I just fumbled in my first experiment). -- > It is possible to reset a listening socket with shutdown() into a state where it > can be used for at least a new connect(2), and maybe also for new listen(2)s. Interesting syscall... > > The same might also be possible if a socket is in the TCP_SYN_SENT state at the > time of shutdown() (although that is a bit trickier to try out). > > So a complete disassociation control for TCP/IP might not only need to have > LANDLOCK_ACCESS_SOCKET_CONNECT_UNSPEC (or however we'd call it), but also > LANDLOCK_ACCESS_SOCKET_PASSIVE_SHUTDOWN and maybe even another one for the > TCP_SYN_SENT case...? * That would make the Landlock interface too complex, we would need a more generic approach instead e.g. with a single flag. > > It makes me uneasy to think that I only looked at AF_INET{,6} and TCP so far, > and that other protocols would need a similarly close look. It will be > difficult to cover all the "disassociation" cases in all the different > protocols, and even more difficult to detect new ones when they pop up. If we > discover new ones and they'd need new Landlock access rights, it would also > potentially mean that existing Landlock users would have to update their rules > to spell that out. > > It might be easier after all to not rely on "disassociation" control too much > and instead to design the network-related access rights in a way so that we can > provide the desired sandboxing guarantees by restricting the "constructive" > operations (the ones that initiate new network connections or that listen on the > network). I agree. So, with the ability to control socket creation and to also control listen/bind/connect (and sendmsg/recvmsg for datagram protocols) we should be good right? > > Mikhail, in your email I am quoting above, you are saying that "disassociation > control can be really useful"; do you know of any cases where a restriction of > connect/listen is *not* enough and where you'd still want the disassociation > control? > > (In my mind, the disassociation control would have mainly been needed if we had > gone with Mickaël's "Use socket's Landlock domain" RFC [1]? Mickaël and me have > discussed this patch set at LSS and I am also now coming around to the > realization that this would have introduced more complication. - It might have > been a more "pure" approach, but comes at the expense of complicating Landlock > usage.) Indeed, and this RFC will not be continued. We should not think of a socket as a security object (i.e. a real capability), whereas sockets are kernel objects used to configure and exchange data, a bit like a command multiplexer for network actions that can also be used to identify peers. Because Landlock is a sandboxing mechanism, the security policy tied to a task may change during its execution, which is not the case for other access control systems such as SELinux. That's why we should not blindly follow other security models. In the case of socket control, Landlock uses the calling task's credential to check if the call should be allowed. In the case of abstract UNIX socket control (with Linux 5.12), the check is done on the domain that created the peer's socket, not the domain that will received the packet. In this case Landlock can rely on the peer socket's domain because it is a consistent and race-free way to identify a peer, and this peer socket is not the one doing the action. It's a bit different with non-UNIX sockets because peers may not be local to the system. > > —Günther > > [1] https://lore.kernel.org/all/20240719150618.197991-1-mic@digikod.net/ > > * for later reference, my reasoning in the code is: net/ipv4/af_inet.c > implements the entry points for connect() and listen() at the address family > layer. Both operations require that the sock->state is SS_UNCONNECTED. So > the rest is going through the other occurrences of SS_UNCONNECTED in that same > file to see if there are any places where the socket can get back into that > state. The places I found where it is set to that state are: > > 1. inet_create (right after creation, expected) > 2. __inet_stream_connect in the AF_UNSPEC case (known issue) > 3. __inet_stream_connect in the case of a failed connect (expected) > 4. inet_shutdown in the case of TCP_LISTEN or TCP_SYN_SENT (mentioned above) >
On 9/29/2024 8:31 PM, Mickaël Salaün wrote: > On Sat, Sep 28, 2024 at 10:06:52PM +0200, Günther Noack wrote: >> On Fri, Sep 27, 2024 at 11:48:22AM +0200, Günther Noack wrote: >>> On Mon, Sep 23, 2024 at 03:57:47PM +0300, Mikhail Ivanov wrote: >>>> (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.) >>> >>> In my understanding, the disassociation support is closely intertwined with the >>> transport layer - the last paragraph of DESCRIPTION in connect(2) is listing >>> TCP, UDP and Unix Domain sockets in datagram mode. -- The relevant code in in >>> net/ipv4/af_inet.c in inet_dgram_connect() and __inet_stream_connect(), where >>> AF_UNSPEC is handled. >>> >>> I would love to find a way to restrict this independent of the specific >>> transport protocol as well. >>> >>> Remark on the side - in af_inet.c in inet_shutdown(), I also found a worrying >>> scenario where the same sk->sk_prot->disconnect() function is called and >>> sock->state also gets reset to SS_UNCONNECTED. I have done a naive attempt to >>> hit that code path by calling shutdown() on a passive TCP socket, but was not >>> able to reuse the socket for new connections afterwards. (Have not debugged it >>> further though.) I wonder whether this is a scnenario that we also need to >>> cover? >> >> FYI, **this does turn out to work** (I just fumbled in my first experiment). -- >> It is possible to reset a listening socket with shutdown() into a state where it >> can be used for at least a new connect(2), and maybe also for new listen(2)s. > > Interesting syscall... > >> >> The same might also be possible if a socket is in the TCP_SYN_SENT state at the >> time of shutdown() (although that is a bit trickier to try out). >> >> So a complete disassociation control for TCP/IP might not only need to have >> LANDLOCK_ACCESS_SOCKET_CONNECT_UNSPEC (or however we'd call it), but also >> LANDLOCK_ACCESS_SOCKET_PASSIVE_SHUTDOWN and maybe even another one for the >> TCP_SYN_SENT case...? * > > That would make the Landlock interface too complex, we would need a more > generic approach instead e.g. with a single flag. > >> >> It makes me uneasy to think that I only looked at AF_INET{,6} and TCP so far, >> and that other protocols would need a similarly close look. It will be >> difficult to cover all the "disassociation" cases in all the different >> protocols, and even more difficult to detect new ones when they pop up. If we >> discover new ones and they'd need new Landlock access rights, it would also >> potentially mean that existing Landlock users would have to update their rules >> to spell that out. >> >> It might be easier after all to not rely on "disassociation" control too much >> and instead to design the network-related access rights in a way so that we can >> provide the desired sandboxing guarantees by restricting the "constructive" >> operations (the ones that initiate new network connections or that listen on the >> network). > > I agree. So, with the ability to control socket creation and to also > control listen/bind/connect (and sendmsg/recvmsg for datagram protocols) > we should be good right? > >> >> Mikhail, in your email I am quoting above, you are saying that "disassociation >> control can be really useful"; do you know of any cases where a restriction of >> connect/listen is *not* enough and where you'd still want the disassociation >> control? Disassociation is basically about making socket be able to connect or listen (again). If these actions are already controlled, disassociation should always be permitted (as it's currently implemented for TCP connect). I thought that LANDLOCK_ACCESS_SOCKET_CONNECT_UNSPEC would be useful for the protocols that do not have related LANDLOCK_ACCESS_NET_* access rights. It would allow to (for example) create listening socket of non-TCP(UDP) protocol and fully restrict networking (by restricting any disassociation and socket creation). But since disasossication is implemented in the transport layer there is no clear way to control it with socket access. Considering this and that previous scenario can be achieved by implementing networking control (LANDLOCK_ACCESS_NET_* rights) for a needed protocol, potential cost of "disassociation control" implementation is much more than the benefits. >> >> (In my mind, the disassociation control would have mainly been needed if we had >> gone with Mickaël's "Use socket's Landlock domain" RFC [1]? Mickaël and me have >> discussed this patch set at LSS and I am also now coming around to the >> realization that this would have introduced more complication. - It might have >> been a more "pure" approach, but comes at the expense of complicating Landlock >> usage.) > > Indeed, and this RFC will not be continued. We should not think of a > socket as a security object (i.e. a real capability), whereas sockets > are kernel objects used to configure and exchange data, a bit like a > command multiplexer for network actions that can also be used to > identify peers. > > Because Landlock is a sandboxing mechanism, the security policy tied to > a task may change during its execution, which is not the case for other > access control systems such as SELinux. That's why we should not > blindly follow other security models. In the case of socket control, > Landlock uses the calling task's credential to check if the call should > be allowed. In the case of abstract UNIX socket control (with Linux > 5.12), the check is done on the domain that created the peer's socket, > not the domain that will received the packet. In this case Landlock can > rely on the peer socket's domain because it is a consistent and > race-free way to identify a peer, and this peer socket is not the one > doing the action. It's a bit different with non-UNIX sockets because > peers may not be local to the system. > >> >> —Günther >> >> [1] https://lore.kernel.org/all/20240719150618.197991-1-mic@digikod.net/ >> >> * for later reference, my reasoning in the code is: net/ipv4/af_inet.c >> implements the entry points for connect() and listen() at the address family >> layer. Both operations require that the sock->state is SS_UNCONNECTED. So >> the rest is going through the other occurrences of SS_UNCONNECTED in that same >> file to see if there are any places where the socket can get back into that >> state. The places I found where it is set to that state are: >> >> 1. inet_create (right after creation, expected) >> 2. __inet_stream_connect in the AF_UNSPEC case (known issue) >> 3. __inet_stream_connect in the case of a failed connect (expected) >> 4. inet_shutdown in the case of TCP_LISTEN or TCP_SYN_SENT (mentioned above) >>
diff --git a/tools/testing/selftests/landlock/socket_test.c b/tools/testing/selftests/landlock/socket_test.c index 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)); + } +} + TEST_HARNESS_MAIN
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@huawei-partners.com> --- .../testing/selftests/landlock/socket_test.c | 101 ++++++++++++++++++ 1 file changed, 101 insertions(+)