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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Mon, 7 Feb 2022 10:11:08 +0300 From: Konstantin Meskhidze <konstantin.meskhidze@...wei.com> To: Mickaël Salaün <mic@...ikod.net> CC: <linux-security-module@...r.kernel.org>, <netdev@...r.kernel.org>, <netfilter@...r.kernel.org>, <yusongping@...wei.com>, <artem.kuzin@...wei.com> Subject: Re: [RFC PATCH 2/2] landlock: selftests for bind and connect hooks 2/1/2022 9:31 PM, Mickaël Salaün пишет: > > On 24/01/2022 09:02, Konstantin Meskhidze wrote: >> Support 4 tests for bind and connect networks actions: > > Good to see such tests! > > >> 1. bind() a socket with no landlock restrictions. >> 2. bind() sockets with landllock restrictions. > > You can leverage the FIXTURE_VARIANT helpers to factor out this kind of > tests (see ptrace_test.c). Thanks. I will check this out. > > >> 3. connect() a socket to listening one with no landlock restricitons. >> 4. connect() sockets with landlock restrictions. > > Same here, you can factor out code. I guess you could create helpers for > client and server parts. Ok. I got it. > > We also need to test with IPv4, IPv6 and the AF_UNSPEC tricks. Yep. I will add AF_UNSPEC case. > > Please provide the kernel test coverage and explain why the uncovered > code cannot be covered: > https://www.kernel.org/doc/html/latest/dev-tools/gcov.html Thanks for the link. I will check it out. > > You'll probably see that there are a multiple parts of the kernel that > are not covered. For instance, it is important to test different > combinations of layered network rules (see layout1/ruleset_overlap, > layer_rule_unions, non_overlapping_accesses, > interleaved_masked_accesses… in fs_test.c). Tests in fs_test.c are more > complex because handling file system rules is more complex, but you can > get some inspiration in it, especially the edge cases. I got your point. I agree that we need to cover as many network rules combinations as possible. > > We also need to test invalid user space supplied data (see layout1/inval > test in fs_test.c). Ok. I will add this kind of test. Thanks. > > >> >> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@...wei.com> >> --- >> .../testing/selftests/landlock/network_test.c | 346 ++++++++++++++++++ >> 1 file changed, 346 insertions(+) >> create mode 100644 tools/testing/selftests/landlock/network_test.c >> >> diff --git a/tools/testing/selftests/landlock/network_test.c >> b/tools/testing/selftests/landlock/network_test.c >> new file mode 100644 >> index 000000000000..9dfe37a2fb20 >> --- /dev/null >> +++ b/tools/testing/selftests/landlock/network_test.c >> @@ -0,0 +1,346 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Landlock tests - Common user space base >> + * >> + * Copyright © 2017-2020 Mickaël Salaün <mic@...ikod.net> >> + * Copyright © 2019-2020 ANSSI > > You need to update this header with an appropriate description and the > copyright holder (your employer). > Ok. I got it. > >> + */ >> + >> +#define _GNU_SOURCE >> +#include <errno.h> >> +#include <fcntl.h> >> +#include <linux/landlock.h> >> +#include <string.h> >> +#include <sys/prctl.h> >> +#include <sys/socket.h> >> +#include <sys/types.h> >> +#include <netinet/in.h> >> +#include <arpa/inet.h> > > To make it determinisitic (and ease patching/diff/merging), you should > sort all the included files (in tests and in the kernel code). Sorry. Did not get your point here. Could you explain in a bit more details please. > > >> + >> +#include "common.h" >> + >> +#define SOCK_PORT_1 3470 >> +#define SOCK_PORT_2 3480 >> +#define SOCK_PORT_3 3490 > > To avoid port collision and create a clean and stable test environement > (to avoid flaky tests), you should create a network namespace with > FIXTURE_SETUP, test with TEST_F_FORK (to not polute the parent process, > and which works with test variants), and use the set_cap and clear_cap > helpers (see fs_test.c). Ok. I got it. > > >> + >> +#define IP_ADDRESS "127.0.0.1" >> + >> +/* Number pending connections queue tobe hold */ >> +#define BACKLOG 10 >> + >> +TEST(socket_bind_no_restrictions) { >> + >> + int sockfd; >> + struct sockaddr_in addr; >> + const int one = 1; >> + >> + /* Create a socket */ >> + sockfd = socket(AF_INET, SOCK_STREAM, 0); >> + ASSERT_LE(0, sockfd); >> + /* Allow reuse of local addresses */ >> + ASSERT_EQ(0, setsockopt(sockfd, SOL_SOCKET, SO_REUSEADDR, &one, >> sizeof(one))); > > With a dedicated namespace, SO_REUSEADDR should not be required. Yes. I agree. Will be refactored. > > >> + >> + /* Set socket address parameters */ >> + addr.sin_family = AF_INET; >> + addr.sin_port = htons(SOCK_PORT_1); >> + addr.sin_addr.s_addr = inet_addr(IP_ADDRESS); >> + memset(&(addr.sin_zero), '\0', 8); >> + >> + /* Bind the socket to IP address */ >> + ASSERT_EQ(0, bind(sockfd, (struct sockaddr *)&addr, sizeof(addr))); >> +} >> + >> +TEST(sockets_bind_with_restrictions) { >> + >> + int sockfd_1, sockfd_2, sockfd_3; >> + struct sockaddr_in addr_1, addr_2, addr_3; >> + const int one = 1; >> + >> + struct landlock_ruleset_attr ruleset_attr = { >> + .handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP | >> + LANDLOCK_ACCESS_NET_CONNECT_TCP, >> + }; >> + struct landlock_net_service_attr net_service_1 = { >> + .allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP | >> + LANDLOCK_ACCESS_NET_CONNECT_TCP, >> + .port = SOCK_PORT_1, >> + }; >> + struct landlock_net_service_attr net_service_2 = { >> + .allowed_access = LANDLOCK_ACCESS_NET_CONNECT_TCP, >> + .port = SOCK_PORT_2, >> + }; >> + struct landlock_net_service_attr net_service_3 = { >> + .allowed_access = 0, >> + .port = SOCK_PORT_3, >> + }; > > Good to have these three different rules! > > >> + >> + const int ruleset_fd = landlock_create_ruleset(&ruleset_attr, >> + sizeof(ruleset_attr), 0); >> + ASSERT_LE(0, ruleset_fd); >> + >> + /* Allow connect and bind operations to the SOCK_PORT_1 socket >> "object" */ > > You can omit "object" but use full sentences at the third person > (because it explains what do the next lines). Ok. I got it. > > >> + ASSERT_EQ(0, landlock_add_rule(ruleset_fd, >> LANDLOCK_RULE_NET_SERVICE, >> + &net_service_1, 0)); >> + /* Allow connect and deny bind operations to the SOCK_PORT_2 >> socket "object" */ >> + ASSERT_EQ(0, landlock_add_rule(ruleset_fd, >> LANDLOCK_RULE_NET_SERVICE, >> + &net_service_2, 0)); >> + /* Empty allowed_access (i.e. deny rules) are ignored in network >> actions >> + * for SOCK_PORT_3 socket "object" >> + */ >> + ASSERT_EQ(-1, landlock_add_rule(ruleset_fd, >> LANDLOCK_RULE_NET_SERVICE, >> + &net_service_3, 0)); >> + ASSERT_EQ(ENOMSG, errno); >> + >> + /* Enforces the ruleset. */ >> + ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)); >> + ASSERT_EQ(0, landlock_restrict_self(ruleset_fd, 0)); >> + ASSERT_EQ(0, close(ruleset_fd)); >> + >> + /* Create a socket 1 */ >> + sockfd_1 = socket(AF_INET, SOCK_STREAM, 0); > > Please create all FD with SOCK_CLOEXEC and also close them when not > needed. This could also reduce the number of FD. Ok. Thanks for noticing. It makes sense. > > >> + ASSERT_LE(0, sockfd_1); >> + /* Allow reuse of local addresses */ >> + ASSERT_EQ(0, setsockopt(sockfd_1, SOL_SOCKET, SO_REUSEADDR, &one, >> sizeof(one))); >> + >> + /* Set socket 1 address parameters */ >> + addr_1.sin_family = AF_INET; >> + addr_1.sin_port = htons(SOCK_PORT_1); >> + addr_1.sin_addr.s_addr = inet_addr(IP_ADDRESS); >> + memset(&(addr_1.sin_zero), '\0', 8); >> + /* Bind the socket 1 to IP address */ >> + ASSERT_EQ(0, bind(sockfd_1, (struct sockaddr *)&addr_1, >> sizeof(addr_1))); >> + >> + /* Create a socket 2 */ >> + sockfd_2 = socket(AF_INET, SOCK_STREAM, 0); >> + ASSERT_LE(0, sockfd_2); >> + /* Allow reuse of local addresses */ >> + ASSERT_EQ(0, setsockopt(sockfd_2, SOL_SOCKET, SO_REUSEADDR, &one, >> sizeof(one))); >> + >> + /* Set socket 2 address parameters */ >> + addr_2.sin_family = AF_INET; >> + addr_2.sin_port = htons(SOCK_PORT_2); >> + addr_2.sin_addr.s_addr = inet_addr(IP_ADDRESS); >> + memset(&(addr_2.sin_zero), '\0', 8); > > These part could be factored out with helpers or/and test variants. Thanks. Will be factored out. > > >> + /* Bind the socket 2 to IP address */ >> + ASSERT_EQ(-1, bind(sockfd_2, (struct sockaddr *)&addr_2, >> sizeof(addr_2))); >> + ASSERT_EQ(EACCES, errno); >> + >> + /* Create a socket 3 */ >> + sockfd_3 = socket(AF_INET, SOCK_STREAM, 0); >> + ASSERT_LE(0, sockfd_3); >> + /* Allow reuse of local addresses */ >> + ASSERT_EQ(0, setsockopt(sockfd_3, SOL_SOCKET, SO_REUSEADDR, &one, >> sizeof(one))); >> + >> + /* Set socket 3 address parameters */ >> + addr_3.sin_family = AF_INET; >> + addr_3.sin_port = htons(SOCK_PORT_3); >> + addr_3.sin_addr.s_addr = inet_addr(IP_ADDRESS); >> + memset(&(addr_3.sin_zero), '\0', 8); >> + /* Bind the socket 3 to IP address */ >> + ASSERT_EQ(0, bind(sockfd_3, (struct sockaddr *)&addr_3, >> sizeof(addr_3))); > > Why is it allowed to bind to SOCK_PORT_3 whereas net_service_3 forbids it? It's allowed cause net_service_3 has empty access field. /* Empty allowed_access (i.e. deny rules) are ignored in network * actions for SOCK_PORT_3 socket "object" */ ASSERT_EQ(-1, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_SERVICE, &net_service_3, 0)); ASSERT_EQ(ENOMSG, errno); Applying this rule returns ENOMSG errno: /* Informs about useless rule: empty allowed_access (i.e. deny rules) * are ignored in network actions */ if (!net_service_attr.allowed_access) { err = -ENOMSG; goto out_put_ruleset; } This means binding socket 3 is not restricted. For path_beneath_attr.allowed_access = 0 there is the same logic. > > >> +} >> + >> +TEST(socket_connect_no_restrictions) { >> + >> + int sockfd, new_fd; >> + struct sockaddr_in addr; >> + pid_t child; >> + int status; >> + const int one = 1; >> + >> + /* Create a server socket */ >> + sockfd = socket(AF_INET, SOCK_STREAM, 0); >> + ASSERT_LE(0, sockfd); >> + /* Allow reuse of local addresses */ >> + ASSERT_EQ(0, setsockopt(sockfd, SOL_SOCKET, SO_REUSEADDR, &one, >> sizeof(one))); >> + >> + /* Set socket address parameters */ >> + addr.sin_family = AF_INET; >> + addr.sin_port = htons(SOCK_PORT_1); >> + addr.sin_addr.s_addr = inet_addr(IP_ADDRESS); >> + memset(&(addr.sin_zero), '\0', 8); >> + >> + /* Bind the socket to IP address */ >> + ASSERT_EQ(0, bind(sockfd, (struct sockaddr *)&addr, sizeof(addr))); >> + >> + /* Make listening socket */ >> + ASSERT_EQ(0, listen(sockfd, BACKLOG)); >> + >> + child = fork(); >> + ASSERT_LE(0, child); >> + if (child == 0) { >> + int child_sockfd; >> + struct sockaddr_in connect_addr; >> + >> + /* Close listening socket for the child */ >> + ASSERT_EQ(0, close(sockfd)); >> + /* Create a stream client socket */ >> + child_sockfd = socket(AF_INET, SOCK_STREAM, 0); >> + ASSERT_LE(0, child_sockfd); >> + >> + /* Set server's socket address parameters*/ >> + connect_addr.sin_family = AF_INET; >> + connect_addr.sin_port = htons(SOCK_PORT_1); >> + connect_addr.sin_addr.s_addr = htonl(INADDR_ANY); >> + memset(&(connect_addr.sin_zero), '\0', 8); >> + >> + /* Make connection to the listening socket */ >> + ASSERT_EQ(0, connect(child_sockfd, (struct sockaddr >> *)&connect_addr, >> + sizeof(struct sockaddr))); >> + _exit(_metadata->passed ? EXIT_SUCCESS : EXIT_FAILURE); >> + return; >> + } >> + /* Accept connection from the child */ >> + new_fd = accept(sockfd, NULL, 0); >> + ASSERT_LE(0, new_fd); >> + >> + /* Close connection */ >> + ASSERT_EQ(0, close(new_fd)); >> + >> + ASSERT_EQ(child, waitpid(child, &status, 0)); >> + ASSERT_EQ(1, WIFEXITED(status)); >> + ASSERT_EQ(EXIT_SUCCESS, WEXITSTATUS(status)); >> +} >> + >> +TEST(sockets_connect_with_restrictions) { >> + >> + int new_fd; >> + int sockfd_1, sockfd_2; >> + struct sockaddr_in addr_1, addr_2; >> + pid_t child_1, child_2; >> + int status; >> + const int one = 1; >> + >> + struct landlock_ruleset_attr ruleset_attr = { >> + .handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP | >> + LANDLOCK_ACCESS_NET_CONNECT_TCP, >> + }; >> + struct landlock_net_service_attr net_service_1 = { >> + .allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP | >> + LANDLOCK_ACCESS_NET_CONNECT_TCP, >> + .port = SOCK_PORT_1, >> + }; >> + struct landlock_net_service_attr net_service_2 = { >> + .allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP, >> + .port = SOCK_PORT_2, >> + }; >> + >> + const int ruleset_fd = landlock_create_ruleset(&ruleset_attr, >> + sizeof(ruleset_attr), 0); >> + ASSERT_LE(0, ruleset_fd); >> + >> + /* Allow connect and bind operations to the SOCK_PORT_1 socket >> "object" */ >> + ASSERT_EQ(0, landlock_add_rule(ruleset_fd, >> LANDLOCK_RULE_NET_SERVICE, >> + &net_service_1, 0)); >> + /* Allow connect and deny bind operations to the SOCK_PORT_2 >> socket "object" */ >> + ASSERT_EQ(0, landlock_add_rule(ruleset_fd, >> LANDLOCK_RULE_NET_SERVICE, >> + &net_service_2, 0)); >> + >> + /* Enforces the ruleset. */ >> + ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)); >> + ASSERT_EQ(0, landlock_restrict_self(ruleset_fd, 0)); >> + ASSERT_EQ(0, close(ruleset_fd)); >> + >> + /* Create a server socket 1 */ >> + sockfd_1 = socket(AF_INET, SOCK_STREAM, 0); >> + ASSERT_LE(0, sockfd_1); >> + /* Allow reuse of local addresses */ >> + ASSERT_EQ(0, setsockopt(sockfd_1, SOL_SOCKET, SO_REUSEADDR, &one, >> sizeof(one))); >> + >> + /* Set socket 1 address parameters */ >> + addr_1.sin_family = AF_INET; >> + addr_1.sin_port = htons(SOCK_PORT_1); >> + addr_1.sin_addr.s_addr = inet_addr(IP_ADDRESS); >> + memset(&(addr_1.sin_zero), '\0', 8); >> + >> + /* Bind the socket 1 to IP address */ >> + ASSERT_EQ(0, bind(sockfd_1, (struct sockaddr *)&addr_1, >> sizeof(addr_1))); >> + >> + /* Make listening socket 1 */ >> + ASSERT_EQ(0, listen(sockfd_1, BACKLOG)); >> + >> + child_1 = fork(); >> + ASSERT_LE(0, child_1); >> + if (child_1 == 0) { >> + int child_sockfd; >> + struct sockaddr_in connect_addr; >> + >> + /* Close listening socket for the child */ >> + ASSERT_EQ(0, close(sockfd_1)); >> + /* Create a stream client socket */ >> + child_sockfd = socket(AF_INET, SOCK_STREAM, 0); >> + ASSERT_LE(0, child_sockfd); >> + >> + /* Set server's socket 1 address parameters*/ >> + connect_addr.sin_family = AF_INET; >> + connect_addr.sin_port = htons(SOCK_PORT_1); >> + connect_addr.sin_addr.s_addr = htonl(INADDR_ANY); >> + memset(&(connect_addr.sin_zero), '\0', 8); >> + >> + /* Make connection to the listening socket 1 */ >> + ASSERT_EQ(0, connect(child_sockfd, (struct sockaddr >> *)&connect_addr, >> + sizeof(struct sockaddr))); >> + _exit(_metadata->passed ? EXIT_SUCCESS : EXIT_FAILURE); >> + return; >> + } >> + /* Accept connection from the child 1 */ >> + new_fd = accept(sockfd_1, NULL, 0); >> + ASSERT_LE(0, new_fd); >> + >> + /* Close connection */ >> + ASSERT_EQ(0, close(new_fd)); >> + >> + ASSERT_EQ(child_1, waitpid(child_1, &status, 0)); >> + ASSERT_EQ(1, WIFEXITED(status)); >> + ASSERT_EQ(EXIT_SUCCESS, WEXITSTATUS(status)); >> + >> + /* Create a server socket 2 */ >> + sockfd_2 = socket(AF_INET, SOCK_STREAM, 0); >> + ASSERT_LE(0, sockfd_2); >> + /* Allow reuse of local addresses */ >> + ASSERT_EQ(0, setsockopt(sockfd_2, SOL_SOCKET, SO_REUSEADDR, &one, >> sizeof(one))); >> + >> + /* Set socket 2 address parameters */ >> + addr_2.sin_family = AF_INET; >> + addr_2.sin_port = htons(SOCK_PORT_2); >> + addr_2.sin_addr.s_addr = inet_addr(IP_ADDRESS); >> + memset(&(addr_2.sin_zero), '\0', 8); >> + >> + /* Bind the socket 2 to IP address */ >> + ASSERT_EQ(0, bind(sockfd_2, (struct sockaddr *)&addr_2, >> sizeof(addr_2))); >> + >> + /* Make listening socket 2 */ >> + ASSERT_EQ(0, listen(sockfd_2, BACKLOG)); >> + >> + child_2 = fork(); >> + ASSERT_LE(0, child_2); >> + if (child_2 == 0) { >> + int child_sockfd; >> + struct sockaddr_in connect_addr; >> + >> + /* Close listening socket for the child */ >> + ASSERT_EQ(0, close(sockfd_2)); >> + /* Create a stream client socket */ >> + child_sockfd = socket(AF_INET, SOCK_STREAM, 0); >> + ASSERT_LE(0, child_sockfd); >> + >> + /* Set server's socket address parameters*/ >> + connect_addr.sin_family = AF_INET; >> + connect_addr.sin_port = htons(SOCK_PORT_2); >> + connect_addr.sin_addr.s_addr = htonl(INADDR_ANY); >> + memset(&(connect_addr.sin_zero), '\0', 8); >> + >> + /* Make connection to the listening socket */ >> + ASSERT_EQ(-1, connect(child_sockfd, (struct sockaddr >> *)&connect_addr, >> + sizeof(struct sockaddr))); >> + ASSERT_EQ(EACCES, errno); >> + _exit(_metadata->passed ? EXIT_SUCCESS : EXIT_FAILURE); >> + return; >> + } >> + >> + ASSERT_EQ(child_2, waitpid(child_2, &status, 0)); >> + ASSERT_EQ(1, WIFEXITED(status)); >> + ASSERT_EQ(EXIT_SUCCESS, WEXITSTATUS(status)); >> +} >> + >> +TEST_HARNESS_MAIN > .
Powered by blists - more mailing lists