| Message ID | 20220516152038.39594-10-konstantin.meskhidze@huawei.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Network support for Landlock | expand |
On 16/05/2022 17:20, Konstantin Meskhidze wrote: > Adds selftests for bind socket action. > The first is with no landlock restrictions: > - bind_no_restrictions_ip4; > - bind_no_restrictions_ip6; > The second ones is with mixed landlock rules: > - bind_with_restrictions_ip4; > - bind_with_restrictions_ip6; > > Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com> > --- > > Changes since v3: > * Split commit. > * Add helper create_socket. > * Add FIXTURE_SETUP. > > Changes since v4: > * Adds port[MAX_SOCKET_NUM], struct sockaddr_in addr4 > and struct sockaddr_in addr6 in FIXTURE. > * Refactoring FIXTURE_SETUP: > - initializing self->port, self->addr4 and self->addr6. > - adding network namespace. > * Refactoring code with self->port, self->addr4 and > self->addr6 variables. > * Adds selftests for IP6 family: > - bind_no_restrictions_ip6. > - bind_with_restrictions_ip6. > * Refactoring selftests/landlock/config > * Moves enforce_ruleset() into common.h > > --- > tools/testing/selftests/landlock/common.h | 9 + > tools/testing/selftests/landlock/config | 5 +- > tools/testing/selftests/landlock/fs_test.c | 10 - > tools/testing/selftests/landlock/net_test.c | 237 ++++++++++++++++++++ > 4 files changed, 250 insertions(+), 11 deletions(-) > create mode 100644 tools/testing/selftests/landlock/net_test.c > > diff --git a/tools/testing/selftests/landlock/common.h b/tools/testing/selftests/landlock/common.h > index 7ba18eb23783..c5381e641dfd 100644 > --- a/tools/testing/selftests/landlock/common.h > +++ b/tools/testing/selftests/landlock/common.h > @@ -102,6 +102,15 @@ static inline int landlock_restrict_self(const int ruleset_fd, > } > #endif > > +static void enforce_ruleset(struct __test_metadata *const _metadata, > + const int ruleset_fd) > +{ > + ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)); > + ASSERT_EQ(0, landlock_restrict_self(ruleset_fd, 0)) { > + TH_LOG("Failed to enforce ruleset: %s", strerror(errno)); > + } > +} > + Please create a commit which moves all the needed code for all network tests. I think there is only this helper though. > static void _init_caps(struct __test_metadata *const _metadata, bool drop_all) > { > cap_t cap_p; > diff --git a/tools/testing/selftests/landlock/config b/tools/testing/selftests/landlock/config > index 0f0a65287bac..b56f3274d3f5 100644 > --- a/tools/testing/selftests/landlock/config > +++ b/tools/testing/selftests/landlock/config > @@ -1,7 +1,10 @@ > +CONFIG_INET=y > +CONFIG_IPV6=y > +CONFIG_NET=y > CONFIG_OVERLAY_FS=y > CONFIG_SECURITY_LANDLOCK=y > CONFIG_SECURITY_PATH=y > CONFIG_SECURITY=y > CONFIG_SHMEM=y > CONFIG_TMPFS_XATTR=y > -CONFIG_TMPFS=y > +CONFIG_TMPFS=y > \ No newline at end of file You add whitespace changes. > diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c > index 21a2ce8fa739..036dd6f8f9ea 100644 > --- a/tools/testing/selftests/landlock/fs_test.c > +++ b/tools/testing/selftests/landlock/fs_test.c > @@ -551,16 +551,6 @@ static int create_ruleset(struct __test_metadata *const _metadata, > return ruleset_fd; > } > > -static void enforce_ruleset(struct __test_metadata *const _metadata, > - const int ruleset_fd) > -{ > - ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)); > - ASSERT_EQ(0, landlock_restrict_self(ruleset_fd, 0)) > - { > - TH_LOG("Failed to enforce ruleset: %s", strerror(errno)); > - } > -} > - > TEST_F_FORK(layout1, proc_nsfs) > { > const struct rule rules[] = { > diff --git a/tools/testing/selftests/landlock/net_test.c b/tools/testing/selftests/landlock/net_test.c > new file mode 100644 > index 000000000000..478ef2eff559 > --- /dev/null > +++ b/tools/testing/selftests/landlock/net_test.c > @@ -0,0 +1,237 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Landlock tests - Network > + * > + * Copyright (C) 2022 Huawei Tech. Co., Ltd. > + */ > + > +#define _GNU_SOURCE > +#include <arpa/inet.h> > +#include <errno.h> > +#include <fcntl.h> > +#include <linux/landlock.h> > +#include <netinet/in.h> > +#include <sched.h> > +#include <string.h> > +#include <sys/prctl.h> > +#include <sys/socket.h> > +#include <sys/types.h> > + > +#include "common.h" > + > +#define MAX_SOCKET_NUM 10 > + > +#define SOCK_PORT_START 3470 > +#define SOCK_PORT_ADD 10 > + > +#define IP_ADDRESS "127.0.0.1" > + > +/* Number pending connections queue to be hold */ > +#define BACKLOG 10 "Number of pending connection queues to be hold." maybe? This is not use in this patch so it shouldn't be added by this patch. > + > +static int create_socket(struct __test_metadata *const _metadata, > + bool ip6, bool reuse_addr) This helper is good and I think you can improve it by leveraging test variants. You could even factor out all the ipv4/ipv6 tests thanks to new helpers such as bind_variant() and connect_variant(). No need to add _metadata to those though. This would avoid duplicating all ipv4/ipv6 tests and even simplifying bind() and connect() calls. Something like this: // rename "socket_test" to "socket" (no need to duplicate "test") FIXTURE_VARIANT(socket) { const bool is_ipv4; }; /* clang-format off */ FIXTURE_VARIANT_ADD(socket, ipv4) { /* clang-format on */ .is_ipv4 = true, }; /* clang-format off */ FIXTURE_VARIANT_ADD(socket, ipv6) { /* clang-format on */ .is_ipv4 = false, }; static int socket_variant(const FIXTURE_VARIANT(socket) *const variant, const int type) { if (variant->is_ipv4) return socket(AF_INET, type | SOCK_CLOEXEC, 0); else return socket(AF_INET6, type | SOCK_CLOEXEC, 0); } socket_variant(variant, SOCK_STREAM); // this could be used to create UDP sockets too static int bind_variant(const FIXTURE_VARIANT(socket) *const variant, const int sockfd, const FIXTURE_DATA(socket) *const self, const size_t index) { if (variant->is_ipv4) return bind(sockfd, &self->addr4[index], sizeof(self->addr4[index])); else return bind(sockfd, &self->addr6[index], sizeof(self->addr6[index])); } bind_variant(variant, sockfd, self, 0); > +{ > + int sockfd; > + int one = 1; > + > + if (ip6) > + sockfd = socket(AF_INET6, SOCK_STREAM | SOCK_CLOEXEC, 0); > + else > + sockfd = socket(AF_INET, SOCK_STREAM | SOCK_CLOEXEC, 0); > + > + ASSERT_LE(0, sockfd); > + /* Allows to reuse of local address */ > + if (reuse_addr) > + ASSERT_EQ(0, setsockopt(sockfd, SOL_SOCKET, > + SO_REUSEADDR, &one, sizeof(one))); This reuse_addr part is not used in this patch and I think it would simplify this helper to not add reuse_addr but to explicitely call setsockopt() when required. This also enables to get rid of _metadata in this helper. > + return sockfd; > +} > + > +FIXTURE(socket_test) { > + uint port[MAX_SOCKET_NUM]; > + struct sockaddr_in addr4[MAX_SOCKET_NUM]; > + struct sockaddr_in6 addr6[MAX_SOCKET_NUM]; > +}; > + > +FIXTURE_SETUP(socket_test) > +{ > + int i; > + /* Creates IP4 socket addresses */ > + for (i = 0; i < MAX_SOCKET_NUM; i++) { Nice! > + self->port[i] = SOCK_PORT_START + SOCK_PORT_ADD*i; > + self->addr4[i].sin_family = AF_INET; > + self->addr4[i].sin_port = htons(self->port[i]); > + self->addr4[i].sin_addr.s_addr = htonl(INADDR_ANY); Could you use the local addr (127.0.0.1) instead? > + memset(&(self->addr4[i].sin_zero), '\0', 8); > + } > + > + /* Creates IP6 socket addresses */ > + for (i = 0; i < MAX_SOCKET_NUM; i++) { > + self->port[i] = SOCK_PORT_START + SOCK_PORT_ADD*i; > + self->addr6[i].sin6_family = AF_INET6; > + self->addr6[i].sin6_port = htons(self->port[i]); > + self->addr6[i].sin6_addr = in6addr_any; ditto > + } > + > + set_cap(_metadata, CAP_SYS_ADMIN); > + ASSERT_EQ(0, unshare(CLONE_NEWNET)); > + ASSERT_EQ(0, system("ip link set dev lo up")); If this is really required, could you avoid calling system() but set up the network in C? You can strace it to see what is going on underneath. > + clear_cap(_metadata, CAP_SYS_ADMIN); > +} > + > +FIXTURE_TEARDOWN(socket_test) > +{ } > + > +TEST_F_FORK(socket_test, bind_no_restrictions_ip4) { > + > + int sockfd; > + > + sockfd = create_socket(_metadata, false, false); > + ASSERT_LE(0, sockfd); > + > + /* Binds a socket to port[0] */ This comment is not very useful in this context considering the below line. It will be even more clear with the bind_variant() call. > + ASSERT_EQ(0, bind(sockfd, (struct sockaddr *)&self->addr4[0], sizeof(self->addr4[0]))); > + > + ASSERT_EQ(0, close(sockfd)); > +} > + > +TEST_F_FORK(socket_test, bind_no_restrictions_ip6) { > + > + int sockfd; > + > + sockfd = create_socket(_metadata, true, false); > + ASSERT_LE(0, sockfd); > + > + /* Binds a socket to port[0] */ > + ASSERT_EQ(0, bind(sockfd, (struct sockaddr *)&self->addr6[0], sizeof(self->addr6[0]))); > + > + ASSERT_EQ(0, close(sockfd)); > +} > + > +TEST_F_FORK(socket_test, bind_with_restrictions_ip4) { > + > + int sockfd; > + > + 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 = self->port[0], > + }; > + struct landlock_net_service_attr net_service_2 = { > + .allowed_access = LANDLOCK_ACCESS_NET_CONNECT_TCP, > + .port = self->port[1], > + }; > + struct landlock_net_service_attr net_service_3 = { > + .allowed_access = 0, > + .port = self->port[2], > + }; > + > + const int ruleset_fd = landlock_create_ruleset(&ruleset_attr, > + sizeof(ruleset_attr), 0); > + ASSERT_LE(0, ruleset_fd); > + > + /* Allows connect and bind operations to the port[0] socket. */ This comment is useful though because the below call is more complex. > + ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_SERVICE, > + &net_service_1, 0)); > + /* Allows connect and deny bind operations to the port[1] socket. */ > + 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 port[2] socket. > + */ > + ASSERT_EQ(-1, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_SERVICE, > + &net_service_3, 0)); > + ASSERT_EQ(ENOMSG, errno); > + > + /* Enforces the ruleset. */ > + enforce_ruleset(_metadata, ruleset_fd); > + > + sockfd = create_socket(_metadata, false, false); > + ASSERT_LE(0, sockfd); > + /* Binds a socket to port[0] */ > + ASSERT_EQ(0, bind(sockfd, (struct sockaddr *)&self->addr4[0], sizeof(self->addr4[0]))); > + > + /* Close bounded socket*/ > + ASSERT_EQ(0, close(sockfd)); > + > + sockfd = create_socket(_metadata, false, false); > + ASSERT_LE(0, sockfd); > + /* Binds a socket to port[1] */ > + ASSERT_EQ(-1, bind(sockfd, (struct sockaddr *)&self->addr4[1], sizeof(self->addr4[1]))); > + ASSERT_EQ(EACCES, errno); > + > + sockfd = create_socket(_metadata, false, false); > + ASSERT_LE(0, sockfd); > + /* Binds a socket to port[2] */ > + ASSERT_EQ(-1, bind(sockfd, (struct sockaddr *)&self->addr4[2], sizeof(self->addr4[2]))); > + ASSERT_EQ(EACCES, errno); > +} > + > +TEST_F_FORK(socket_test, bind_with_restrictions_ip6) { > + > + int sockfd; > + > + 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 = self->port[0], > + }; > + struct landlock_net_service_attr net_service_2 = { > + .allowed_access = LANDLOCK_ACCESS_NET_CONNECT_TCP, > + .port = self->port[1], > + }; > + struct landlock_net_service_attr net_service_3 = { > + .allowed_access = 0, > + .port = self->port[2], > + }; > + > + const int ruleset_fd = landlock_create_ruleset(&ruleset_attr, > + sizeof(ruleset_attr), 0); > + ASSERT_LE(0, ruleset_fd); > + > + /* Allows connect and bind operations to the port[0] socket. */ > + ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_SERVICE, > + &net_service_1, 0)); > + /* Allows connect and deny bind operations to the port[1] socket. */ > + 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 port[2] socket. > + */ > + ASSERT_EQ(-1, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_SERVICE, > + &net_service_3, 0)); > + ASSERT_EQ(ENOMSG, errno); > + > + /* Enforces the ruleset. */ > + enforce_ruleset(_metadata, ruleset_fd); > + > + sockfd = create_socket(_metadata, true, false); > + ASSERT_LE(0, sockfd); > + /* Binds a socket to port[0] */ > + ASSERT_EQ(0, bind(sockfd, (struct sockaddr *)&self->addr6[0], sizeof(self->addr6[0]))); > + > + /* Close bounded socket*/ > + ASSERT_EQ(0, close(sockfd)); > + > + sockfd = create_socket(_metadata, false, false); > + ASSERT_LE(0, sockfd); > + /* Binds a socket to port[1] */ > + ASSERT_EQ(-1, bind(sockfd, (struct sockaddr *)&self->addr6[1], sizeof(self->addr6[1]))); > + ASSERT_EQ(EACCES, errno); > + > + sockfd = create_socket(_metadata, false, false); > + ASSERT_LE(0, sockfd); > + /* Binds a socket to port[2] */ > + ASSERT_EQ(-1, bind(sockfd, (struct sockaddr *)&self->addr6[2], sizeof(self->addr6[2]))); > + ASSERT_EQ(EACCES, errno); > +} > +TEST_HARNESS_MAIN > -- > 2.25.1 >
5/17/2022 12:11 AM, Mickaël Salaün пишет: > > On 16/05/2022 17:20, Konstantin Meskhidze wrote: >> Adds selftests for bind socket action. >> The first is with no landlock restrictions: >> - bind_no_restrictions_ip4; >> - bind_no_restrictions_ip6; >> The second ones is with mixed landlock rules: >> - bind_with_restrictions_ip4; >> - bind_with_restrictions_ip6; >> >> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com> >> --- >> >> Changes since v3: >> * Split commit. >> * Add helper create_socket. >> * Add FIXTURE_SETUP. >> >> Changes since v4: >> * Adds port[MAX_SOCKET_NUM], struct sockaddr_in addr4 >> and struct sockaddr_in addr6 in FIXTURE. >> * Refactoring FIXTURE_SETUP: >> - initializing self->port, self->addr4 and self->addr6. >> - adding network namespace. >> * Refactoring code with self->port, self->addr4 and >> self->addr6 variables. >> * Adds selftests for IP6 family: >> - bind_no_restrictions_ip6. >> - bind_with_restrictions_ip6. >> * Refactoring selftests/landlock/config >> * Moves enforce_ruleset() into common.h >> >> --- >> tools/testing/selftests/landlock/common.h | 9 + >> tools/testing/selftests/landlock/config | 5 +- >> tools/testing/selftests/landlock/fs_test.c | 10 - >> tools/testing/selftests/landlock/net_test.c | 237 ++++++++++++++++++++ >> 4 files changed, 250 insertions(+), 11 deletions(-) >> create mode 100644 tools/testing/selftests/landlock/net_test.c >> >> diff --git a/tools/testing/selftests/landlock/common.h >> b/tools/testing/selftests/landlock/common.h >> index 7ba18eb23783..c5381e641dfd 100644 >> --- a/tools/testing/selftests/landlock/common.h >> +++ b/tools/testing/selftests/landlock/common.h >> @@ -102,6 +102,15 @@ static inline int landlock_restrict_self(const >> int ruleset_fd, >> } >> #endif >> >> +static void enforce_ruleset(struct __test_metadata *const _metadata, >> + const int ruleset_fd) >> +{ >> + ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)); >> + ASSERT_EQ(0, landlock_restrict_self(ruleset_fd, 0)) { >> + TH_LOG("Failed to enforce ruleset: %s", strerror(errno)); >> + } >> +} >> + > > Please create a commit which moves all the needed code for all network > tests. I think there is only this helper though. Ok. I will create one additional commit for moving this helper. But after I have moved the helper to common.h, I got warnings while compiling seltests where I don't use the one (base_test and ptrace_test) > > >> static void _init_caps(struct __test_metadata *const _metadata, bool >> drop_all) >> { >> cap_t cap_p; >> diff --git a/tools/testing/selftests/landlock/config >> b/tools/testing/selftests/landlock/config >> index 0f0a65287bac..b56f3274d3f5 100644 >> --- a/tools/testing/selftests/landlock/config >> +++ b/tools/testing/selftests/landlock/config >> @@ -1,7 +1,10 @@ >> +CONFIG_INET=y >> +CONFIG_IPV6=y >> +CONFIG_NET=y >> CONFIG_OVERLAY_FS=y >> CONFIG_SECURITY_LANDLOCK=y >> CONFIG_SECURITY_PATH=y >> CONFIG_SECURITY=y >> CONFIG_SHMEM=y >> CONFIG_TMPFS_XATTR=y >> -CONFIG_TMPFS=y >> +CONFIG_TMPFS=y >> \ No newline at end of file > > You add whitespace changes. > OK. I will fix it. Thank you. > >> diff --git a/tools/testing/selftests/landlock/fs_test.c >> b/tools/testing/selftests/landlock/fs_test.c >> index 21a2ce8fa739..036dd6f8f9ea 100644 >> --- a/tools/testing/selftests/landlock/fs_test.c >> +++ b/tools/testing/selftests/landlock/fs_test.c >> @@ -551,16 +551,6 @@ static int create_ruleset(struct __test_metadata >> *const _metadata, >> return ruleset_fd; >> } >> >> -static void enforce_ruleset(struct __test_metadata *const _metadata, >> - const int ruleset_fd) >> -{ >> - ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)); >> - ASSERT_EQ(0, landlock_restrict_self(ruleset_fd, 0)) >> - { >> - TH_LOG("Failed to enforce ruleset: %s", strerror(errno)); >> - } >> -} >> - >> TEST_F_FORK(layout1, proc_nsfs) >> { >> const struct rule rules[] = { >> diff --git a/tools/testing/selftests/landlock/net_test.c >> b/tools/testing/selftests/landlock/net_test.c >> new file mode 100644 >> index 000000000000..478ef2eff559 >> --- /dev/null >> +++ b/tools/testing/selftests/landlock/net_test.c >> @@ -0,0 +1,237 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * Landlock tests - Network >> + * >> + * Copyright (C) 2022 Huawei Tech. Co., Ltd. >> + */ >> + >> +#define _GNU_SOURCE >> +#include <arpa/inet.h> >> +#include <errno.h> >> +#include <fcntl.h> >> +#include <linux/landlock.h> >> +#include <netinet/in.h> >> +#include <sched.h> >> +#include <string.h> >> +#include <sys/prctl.h> >> +#include <sys/socket.h> >> +#include <sys/types.h> >> + >> +#include "common.h" >> + >> +#define MAX_SOCKET_NUM 10 >> + >> +#define SOCK_PORT_START 3470 >> +#define SOCK_PORT_ADD 10 >> + >> +#define IP_ADDRESS "127.0.0.1" >> + >> +/* Number pending connections queue to be hold */ >> +#define BACKLOG 10 > > "Number of pending connection queues to be hold." maybe? This is not use > in this patch so it shouldn't be added by this patch. > You are right. I will move it in the patch where listen() function appear. Thank you for noticing. > >> + >> +static int create_socket(struct __test_metadata *const _metadata, >> + bool ip6, bool reuse_addr) > > This helper is good and I think you can improve it by leveraging test > variants. You could even factor out all the ipv4/ipv6 tests thanks to > new helpers such as bind_variant() and connect_variant(). No need to add > _metadata to those though. This would avoid duplicating all ipv4/ipv6 > tests and even simplifying bind() and connect() calls. Something like this: > > // rename "socket_test" to "socket" (no need to duplicate "test") > FIXTURE_VARIANT(socket) > { > const bool is_ipv4; > }; > > /* clang-format off */ > FIXTURE_VARIANT_ADD(socket, ipv4) { > /* clang-format on */ > .is_ipv4 = true, > }; > > /* clang-format off */ > FIXTURE_VARIANT_ADD(socket, ipv6) { > /* clang-format on */ > .is_ipv4 = false, > }; > > static int socket_variant(const FIXTURE_VARIANT(socket) *const variant, > const int type) > { > if (variant->is_ipv4) > return socket(AF_INET, type | SOCK_CLOEXEC, 0); > else > return socket(AF_INET6, type | SOCK_CLOEXEC, 0); > } > > socket_variant(variant, SOCK_STREAM); > // this could be used to create UDP sockets too > > > static int bind_variant(const FIXTURE_VARIANT(socket) *const variant, > const int sockfd, const FIXTURE_DATA(socket) *const self, const size_t > index) > { > if (variant->is_ipv4) > return bind(sockfd, &self->addr4[index], > sizeof(self->addr4[index])); > else > return bind(sockfd, &self->addr6[index], > sizeof(self->addr6[index])); > } > > bind_variant(variant, sockfd, self, 0); > Ok. Thank you for this suggestion. > >> +{ >> + int sockfd; >> + int one = 1; >> + >> + if (ip6) >> + sockfd = socket(AF_INET6, SOCK_STREAM | SOCK_CLOEXEC, 0); >> + else >> + sockfd = socket(AF_INET, SOCK_STREAM | SOCK_CLOEXEC, 0); >> + >> + ASSERT_LE(0, sockfd); >> + /* Allows to reuse of local address */ >> + if (reuse_addr) >> + ASSERT_EQ(0, setsockopt(sockfd, SOL_SOCKET, >> + SO_REUSEADDR, &one, sizeof(one))); > > This reuse_addr part is not used in this patch and I think it would > simplify this helper to not add reuse_addr but to explicitely call > setsockopt() when required. This also enables to get rid of _metadata in > this helper. > Yep. You are right. I will fix it. > >> + return sockfd; >> +} >> + >> +FIXTURE(socket_test) { >> + uint port[MAX_SOCKET_NUM]; >> + struct sockaddr_in addr4[MAX_SOCKET_NUM]; >> + struct sockaddr_in6 addr6[MAX_SOCKET_NUM]; >> +}; >> + >> +FIXTURE_SETUP(socket_test) >> +{ >> + int i; >> + /* Creates IP4 socket addresses */ >> + for (i = 0; i < MAX_SOCKET_NUM; i++) { > > Nice! > >> + self->port[i] = SOCK_PORT_START + SOCK_PORT_ADD*i; >> + self->addr4[i].sin_family = AF_INET; >> + self->addr4[i].sin_port = htons(self->port[i]); >> + self->addr4[i].sin_addr.s_addr = htonl(INADDR_ANY); > > Could you use the local addr (127.0.0.1) instead? Why cant I use INADDR_ANY here? > >> + memset(&(self->addr4[i].sin_zero), '\0', 8); >> + } >> + >> + /* Creates IP6 socket addresses */ >> + for (i = 0; i < MAX_SOCKET_NUM; i++) { >> + self->port[i] = SOCK_PORT_START + SOCK_PORT_ADD*i; >> + self->addr6[i].sin6_family = AF_INET6; >> + self->addr6[i].sin6_port = htons(self->port[i]); >> + self->addr6[i].sin6_addr = in6addr_any; > > ditto Why cant I use in6addr_any here? > >> + } >> + >> + set_cap(_metadata, CAP_SYS_ADMIN); >> + ASSERT_EQ(0, unshare(CLONE_NEWNET)); >> + ASSERT_EQ(0, system("ip link set dev lo up")); > > If this is really required, could you avoid calling system() but set up > the network in C? You can strace it to see what is going on underneath. > I did check. It's a lot of code to be run under the hood (more than one line) and it will just will complicate the test so I suggest to leave just ONE line of code here. > >> + clear_cap(_metadata, CAP_SYS_ADMIN); >> +} >> + >> +FIXTURE_TEARDOWN(socket_test) >> +{ } >> + >> +TEST_F_FORK(socket_test, bind_no_restrictions_ip4) { >> + >> + int sockfd; >> + >> + sockfd = create_socket(_metadata, false, false); >> + ASSERT_LE(0, sockfd); >> + >> + /* Binds a socket to port[0] */ > > This comment is not very useful in this context considering the below > line. It will be even more clear with the bind_variant() call. > Ok. I will fix it. > >> + ASSERT_EQ(0, bind(sockfd, (struct sockaddr *)&self->addr4[0], >> sizeof(self->addr4[0]))); >> + >> + ASSERT_EQ(0, close(sockfd)); >> +} >> + >> +TEST_F_FORK(socket_test, bind_no_restrictions_ip6) { >> + >> + int sockfd; >> + >> + sockfd = create_socket(_metadata, true, false); >> + ASSERT_LE(0, sockfd); >> + >> + /* Binds a socket to port[0] */ >> + ASSERT_EQ(0, bind(sockfd, (struct sockaddr *)&self->addr6[0], >> sizeof(self->addr6[0]))); >> + >> + ASSERT_EQ(0, close(sockfd)); >> +} >> + >> +TEST_F_FORK(socket_test, bind_with_restrictions_ip4) { >> + >> + int sockfd; >> + >> + 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 = self->port[0], >> + }; >> + struct landlock_net_service_attr net_service_2 = { >> + .allowed_access = LANDLOCK_ACCESS_NET_CONNECT_TCP, >> + .port = self->port[1], >> + }; >> + struct landlock_net_service_attr net_service_3 = { >> + .allowed_access = 0, >> + .port = self->port[2], >> + }; >> + >> + const int ruleset_fd = landlock_create_ruleset(&ruleset_attr, >> + sizeof(ruleset_attr), 0); >> + ASSERT_LE(0, ruleset_fd); >> + >> + /* Allows connect and bind operations to the port[0] socket. */ > > This comment is useful though because the below call is more complex. > So I can leave it as it's, cant I? > >> + ASSERT_EQ(0, landlock_add_rule(ruleset_fd, >> LANDLOCK_RULE_NET_SERVICE, >> + &net_service_1, 0)); >> + /* Allows connect and deny bind operations to the port[1] socket. */ >> + 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 port[2] socket. >> + */ >> + ASSERT_EQ(-1, landlock_add_rule(ruleset_fd, >> LANDLOCK_RULE_NET_SERVICE, >> + &net_service_3, 0)); >> + ASSERT_EQ(ENOMSG, errno); >> + >> + /* Enforces the ruleset. */ >> + enforce_ruleset(_metadata, ruleset_fd); >> + >> + sockfd = create_socket(_metadata, false, false); >> + ASSERT_LE(0, sockfd); >> + /* Binds a socket to port[0] */ >> + ASSERT_EQ(0, bind(sockfd, (struct sockaddr *)&self->addr4[0], >> sizeof(self->addr4[0]))); >> + >> + /* Close bounded socket*/ >> + ASSERT_EQ(0, close(sockfd)); >> + >> + sockfd = create_socket(_metadata, false, false); >> + ASSERT_LE(0, sockfd); >> + /* Binds a socket to port[1] */ >> + ASSERT_EQ(-1, bind(sockfd, (struct sockaddr *)&self->addr4[1], >> sizeof(self->addr4[1]))); >> + ASSERT_EQ(EACCES, errno); >> + >> + sockfd = create_socket(_metadata, false, false); >> + ASSERT_LE(0, sockfd); >> + /* Binds a socket to port[2] */ >> + ASSERT_EQ(-1, bind(sockfd, (struct sockaddr *)&self->addr4[2], >> sizeof(self->addr4[2]))); >> + ASSERT_EQ(EACCES, errno); >> +} >> + >> +TEST_F_FORK(socket_test, bind_with_restrictions_ip6) { >> + >> + int sockfd; >> + >> + 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 = self->port[0], >> + }; >> + struct landlock_net_service_attr net_service_2 = { >> + .allowed_access = LANDLOCK_ACCESS_NET_CONNECT_TCP, >> + .port = self->port[1], >> + }; >> + struct landlock_net_service_attr net_service_3 = { >> + .allowed_access = 0, >> + .port = self->port[2], >> + }; >> + >> + const int ruleset_fd = landlock_create_ruleset(&ruleset_attr, >> + sizeof(ruleset_attr), 0); >> + ASSERT_LE(0, ruleset_fd); >> + >> + /* Allows connect and bind operations to the port[0] socket. */ >> + ASSERT_EQ(0, landlock_add_rule(ruleset_fd, >> LANDLOCK_RULE_NET_SERVICE, >> + &net_service_1, 0)); >> + /* Allows connect and deny bind operations to the port[1] socket. */ >> + 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 port[2] socket. >> + */ >> + ASSERT_EQ(-1, landlock_add_rule(ruleset_fd, >> LANDLOCK_RULE_NET_SERVICE, >> + &net_service_3, 0)); >> + ASSERT_EQ(ENOMSG, errno); >> + >> + /* Enforces the ruleset. */ >> + enforce_ruleset(_metadata, ruleset_fd); >> + >> + sockfd = create_socket(_metadata, true, false); >> + ASSERT_LE(0, sockfd); >> + /* Binds a socket to port[0] */ >> + ASSERT_EQ(0, bind(sockfd, (struct sockaddr *)&self->addr6[0], >> sizeof(self->addr6[0]))); >> + >> + /* Close bounded socket*/ >> + ASSERT_EQ(0, close(sockfd)); >> + >> + sockfd = create_socket(_metadata, false, false); >> + ASSERT_LE(0, sockfd); >> + /* Binds a socket to port[1] */ >> + ASSERT_EQ(-1, bind(sockfd, (struct sockaddr *)&self->addr6[1], >> sizeof(self->addr6[1]))); >> + ASSERT_EQ(EACCES, errno); >> + >> + sockfd = create_socket(_metadata, false, false); >> + ASSERT_LE(0, sockfd); >> + /* Binds a socket to port[2] */ >> + ASSERT_EQ(-1, bind(sockfd, (struct sockaddr *)&self->addr6[2], >> sizeof(self->addr6[2]))); >> + ASSERT_EQ(EACCES, errno); >> +} >> +TEST_HARNESS_MAIN >> -- >> 2.25.1 >> > .
On 19/05/2022 14:10, Konstantin Meskhidze wrote: > > > 5/17/2022 12:11 AM, Mickaël Salaün пишет: >> >> On 16/05/2022 17:20, Konstantin Meskhidze wrote: >>> Adds selftests for bind socket action. >>> The first is with no landlock restrictions: >>> - bind_no_restrictions_ip4; >>> - bind_no_restrictions_ip6; >>> The second ones is with mixed landlock rules: >>> - bind_with_restrictions_ip4; >>> - bind_with_restrictions_ip6; >>> >>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com> >>> --- >>> >>> Changes since v3: >>> * Split commit. >>> * Add helper create_socket. >>> * Add FIXTURE_SETUP. >>> >>> Changes since v4: >>> * Adds port[MAX_SOCKET_NUM], struct sockaddr_in addr4 >>> and struct sockaddr_in addr6 in FIXTURE. >>> * Refactoring FIXTURE_SETUP: >>> - initializing self->port, self->addr4 and self->addr6. >>> - adding network namespace. >>> * Refactoring code with self->port, self->addr4 and >>> self->addr6 variables. >>> * Adds selftests for IP6 family: >>> - bind_no_restrictions_ip6. >>> - bind_with_restrictions_ip6. >>> * Refactoring selftests/landlock/config >>> * Moves enforce_ruleset() into common.h >>> >>> --- >>> tools/testing/selftests/landlock/common.h | 9 + >>> tools/testing/selftests/landlock/config | 5 +- >>> tools/testing/selftests/landlock/fs_test.c | 10 - >>> tools/testing/selftests/landlock/net_test.c | 237 ++++++++++++++++++++ >>> 4 files changed, 250 insertions(+), 11 deletions(-) >>> create mode 100644 tools/testing/selftests/landlock/net_test.c >>> >>> diff --git a/tools/testing/selftests/landlock/common.h >>> b/tools/testing/selftests/landlock/common.h >>> index 7ba18eb23783..c5381e641dfd 100644 >>> --- a/tools/testing/selftests/landlock/common.h >>> +++ b/tools/testing/selftests/landlock/common.h >>> @@ -102,6 +102,15 @@ static inline int landlock_restrict_self(const >>> int ruleset_fd, >>> } >>> #endif >>> >>> +static void enforce_ruleset(struct __test_metadata *const _metadata, >>> + const int ruleset_fd) >>> +{ >>> + ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)); >>> + ASSERT_EQ(0, landlock_restrict_self(ruleset_fd, 0)) { >>> + TH_LOG("Failed to enforce ruleset: %s", strerror(errno)); >>> + } >>> +} >>> + >> >> Please create a commit which moves all the needed code for all network >> tests. I think there is only this helper though. > > Ok. I will create one additional commit for moving this helper. > But after I have moved the helper to common.h, I got warnings while > compiling seltests where I don't use the one (base_test and ptrace_test) Move it after clear_cap() and use the same attributes. [...] >>> diff --git a/tools/testing/selftests/landlock/config >>> b/tools/testing/selftests/landlock/config >>> index 0f0a65287bac..b56f3274d3f5 100644 >>> --- a/tools/testing/selftests/landlock/config >>> +++ b/tools/testing/selftests/landlock/config >>> @@ -1,7 +1,10 @@ >>> +CONFIG_INET=y >>> +CONFIG_IPV6=y >>> +CONFIG_NET=y >>> CONFIG_OVERLAY_FS=y >>> CONFIG_SECURITY_LANDLOCK=y >>> CONFIG_SECURITY_PATH=y >>> CONFIG_SECURITY=y >>> CONFIG_SHMEM=y >>> CONFIG_TMPFS_XATTR=y >>> -CONFIG_TMPFS=y >>> +CONFIG_TMPFS=y >>> \ No newline at end of file You also need to add CONFIG_NET_NS. [...] >> >>> + self->port[i] = SOCK_PORT_START + SOCK_PORT_ADD*i; >>> + self->addr4[i].sin_family = AF_INET; >>> + self->addr4[i].sin_port = htons(self->port[i]); >>> + self->addr4[i].sin_addr.s_addr = htonl(INADDR_ANY); >> >> Could you use the local addr (127.0.0.1) instead? > > Why cant I use INADDR_ANY here? You can, but it is cleaner to bind to a specified address (i.e. you control where a connection come from), and I guess this variable/address could be used to establish connections as well. >> >>> + memset(&(self->addr4[i].sin_zero), '\0', 8); >>> + } >>> + >>> + /* Creates IP6 socket addresses */ >>> + for (i = 0; i < MAX_SOCKET_NUM; i++) { >>> + self->port[i] = SOCK_PORT_START + SOCK_PORT_ADD*i; >>> + self->addr6[i].sin6_family = AF_INET6; >>> + self->addr6[i].sin6_port = htons(self->port[i]); >>> + self->addr6[i].sin6_addr = in6addr_any; >> >> ditto > > Why cant I use in6addr_any here? Same as for IPV4. > >> >>> + } >>> + >>> + set_cap(_metadata, CAP_SYS_ADMIN); >>> + ASSERT_EQ(0, unshare(CLONE_NEWNET)); >>> + ASSERT_EQ(0, system("ip link set dev lo up")); >> >> If this is really required, could you avoid calling system() but set >> up the network in C? You can strace it to see what is going on >> underneath. >> > I did check. It's a lot of code to be run under the hood (more than > one line) and it will just will complicate the test so I suggest to > leave just ONE line of code here. OK >> >>> + clear_cap(_metadata, CAP_SYS_ADMIN); >>> +} >>> + >>> +FIXTURE_TEARDOWN(socket_test) >>> +{ } >>> + >>> +TEST_F_FORK(socket_test, bind_no_restrictions_ip4) { >>> + >>> + int sockfd; >>> + >>> + sockfd = create_socket(_metadata, false, false); >>> + ASSERT_LE(0, sockfd); >>> + >>> + /* Binds a socket to port[0] */ >> >> This comment is not very useful in this context considering the below >> line. It will be even more clear with the bind_variant() call. >> > Ok. I will fix it. >> >>> + ASSERT_EQ(0, bind(sockfd, (struct sockaddr *)&self->addr4[0], >>> sizeof(self->addr4[0]))); >>> + >>> + ASSERT_EQ(0, close(sockfd)); >>> +} >>> + >>> +TEST_F_FORK(socket_test, bind_no_restrictions_ip6) { >>> + >>> + int sockfd; >>> + >>> + sockfd = create_socket(_metadata, true, false); >>> + ASSERT_LE(0, sockfd); >>> + >>> + /* Binds a socket to port[0] */ >>> + ASSERT_EQ(0, bind(sockfd, (struct sockaddr *)&self->addr6[0], >>> sizeof(self->addr6[0]))); >>> + >>> + ASSERT_EQ(0, close(sockfd)); >>> +} >>> + >>> +TEST_F_FORK(socket_test, bind_with_restrictions_ip4) { >>> + >>> + int sockfd; >>> + >>> + 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 = self->port[0], >>> + }; >>> + struct landlock_net_service_attr net_service_2 = { >>> + .allowed_access = LANDLOCK_ACCESS_NET_CONNECT_TCP, >>> + .port = self->port[1], >>> + }; >>> + struct landlock_net_service_attr net_service_3 = { >>> + .allowed_access = 0, >>> + .port = self->port[2], >>> + }; >>> + >>> + const int ruleset_fd = landlock_create_ruleset(&ruleset_attr, >>> + sizeof(ruleset_attr), 0); >>> + ASSERT_LE(0, ruleset_fd); >>> + >>> + /* Allows connect and bind operations to the port[0] socket. */ >> >> This comment is useful though because the below call is more complex. >> > So I can leave it as it's, cant I? Yes, keep it, I'd just like a fair amount of useful comments. ;)
5/19/2022 5:29 PM, Mickaël Salaün пишет: > > On 19/05/2022 14:10, Konstantin Meskhidze wrote: >> >> >> 5/17/2022 12:11 AM, Mickaël Salaün пишет: >>> >>> On 16/05/2022 17:20, Konstantin Meskhidze wrote: >>>> Adds selftests for bind socket action. >>>> The first is with no landlock restrictions: >>>> - bind_no_restrictions_ip4; >>>> - bind_no_restrictions_ip6; >>>> The second ones is with mixed landlock rules: >>>> - bind_with_restrictions_ip4; >>>> - bind_with_restrictions_ip6; >>>> >>>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com> >>>> --- >>>> >>>> Changes since v3: >>>> * Split commit. >>>> * Add helper create_socket. >>>> * Add FIXTURE_SETUP. >>>> >>>> Changes since v4: >>>> * Adds port[MAX_SOCKET_NUM], struct sockaddr_in addr4 >>>> and struct sockaddr_in addr6 in FIXTURE. >>>> * Refactoring FIXTURE_SETUP: >>>> - initializing self->port, self->addr4 and self->addr6. >>>> - adding network namespace. >>>> * Refactoring code with self->port, self->addr4 and >>>> self->addr6 variables. >>>> * Adds selftests for IP6 family: >>>> - bind_no_restrictions_ip6. >>>> - bind_with_restrictions_ip6. >>>> * Refactoring selftests/landlock/config >>>> * Moves enforce_ruleset() into common.h >>>> >>>> --- >>>> tools/testing/selftests/landlock/common.h | 9 + >>>> tools/testing/selftests/landlock/config | 5 +- >>>> tools/testing/selftests/landlock/fs_test.c | 10 - >>>> tools/testing/selftests/landlock/net_test.c | 237 >>>> ++++++++++++++++++++ >>>> 4 files changed, 250 insertions(+), 11 deletions(-) >>>> create mode 100644 tools/testing/selftests/landlock/net_test.c >>>> >>>> diff --git a/tools/testing/selftests/landlock/common.h >>>> b/tools/testing/selftests/landlock/common.h >>>> index 7ba18eb23783..c5381e641dfd 100644 >>>> --- a/tools/testing/selftests/landlock/common.h >>>> +++ b/tools/testing/selftests/landlock/common.h >>>> @@ -102,6 +102,15 @@ static inline int landlock_restrict_self(const >>>> int ruleset_fd, >>>> } >>>> #endif >>>> >>>> +static void enforce_ruleset(struct __test_metadata *const _metadata, >>>> + const int ruleset_fd) >>>> +{ >>>> + ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)); >>>> + ASSERT_EQ(0, landlock_restrict_self(ruleset_fd, 0)) { >>>> + TH_LOG("Failed to enforce ruleset: %s", strerror(errno)); >>>> + } >>>> +} >>>> + >>> >>> Please create a commit which moves all the needed code for all >>> network tests. I think there is only this helper though. >> >> Ok. I will create one additional commit for moving this helper. >> But after I have moved the helper to common.h, I got warnings while >> compiling seltests where I don't use the one (base_test and ptrace_test) > > Move it after clear_cap() and use the same attributes. > Ok. Thank you. > [...] > > >>> diff --git a/tools/testing/selftests/landlock/config > >>> b/tools/testing/selftests/landlock/config > >>> index 0f0a65287bac..b56f3274d3f5 100644 > >>> --- a/tools/testing/selftests/landlock/config > >>> +++ b/tools/testing/selftests/landlock/config > >>> @@ -1,7 +1,10 @@ > >>> +CONFIG_INET=y > >>> +CONFIG_IPV6=y > >>> +CONFIG_NET=y > >>> CONFIG_OVERLAY_FS=y > >>> CONFIG_SECURITY_LANDLOCK=y > >>> CONFIG_SECURITY_PATH=y > >>> CONFIG_SECURITY=y > >>> CONFIG_SHMEM=y > >>> CONFIG_TMPFS_XATTR=y > >>> -CONFIG_TMPFS=y > >>> +CONFIG_TMPFS=y > >>> \ No newline at end of file > > You also need to add CONFIG_NET_NS. Yep. I have forgotten about it. Thanks. > > [...] > >>> >>>> + self->port[i] = SOCK_PORT_START + SOCK_PORT_ADD*i; >>>> + self->addr4[i].sin_family = AF_INET; >>>> + self->addr4[i].sin_port = htons(self->port[i]); >>>> + self->addr4[i].sin_addr.s_addr = htonl(INADDR_ANY); >>> >>> Could you use the local addr (127.0.0.1) instead? >> >> Why cant I use INADDR_ANY here? > > You can, but it is cleaner to bind to a specified address (i.e. you > control where a connection come from), and I guess this variable/address > could be used to establish connections as well. > Ok. I got it. >>> >>>> + memset(&(self->addr4[i].sin_zero), '\0', 8); >>>> + } >>>> + >>>> + /* Creates IP6 socket addresses */ >>>> + for (i = 0; i < MAX_SOCKET_NUM; i++) { >>>> + self->port[i] = SOCK_PORT_START + SOCK_PORT_ADD*i; >>>> + self->addr6[i].sin6_family = AF_INET6; >>>> + self->addr6[i].sin6_port = htons(self->port[i]); >>>> + self->addr6[i].sin6_addr = in6addr_any; >>> >>> ditto >> >> Why cant I use in6addr_any here? > > Same as for IPV4. Ok. > >> >>> >>>> + } >>>> + >>>> + set_cap(_metadata, CAP_SYS_ADMIN); >>>> + ASSERT_EQ(0, unshare(CLONE_NEWNET)); >>>> + ASSERT_EQ(0, system("ip link set dev lo up")); >>> >>> If this is really required, could you avoid calling system() but set >>> up the network in C? You can strace it to see what is going on >>> underneath. >>> >> I did check. It's a lot of code to be run under the hood (more than >> one line) and it will just will complicate the test so I suggest to >> leave just ONE line of code here. > > OK > > >>> >>>> + clear_cap(_metadata, CAP_SYS_ADMIN); >>>> +} >>>> + >>>> +FIXTURE_TEARDOWN(socket_test) >>>> +{ } >>>> + >>>> +TEST_F_FORK(socket_test, bind_no_restrictions_ip4) { >>>> + >>>> + int sockfd; >>>> + >>>> + sockfd = create_socket(_metadata, false, false); >>>> + ASSERT_LE(0, sockfd); >>>> + >>>> + /* Binds a socket to port[0] */ >>> >>> This comment is not very useful in this context considering the below >>> line. It will be even more clear with the bind_variant() call. >>> >> Ok. I will fix it. >>> >>>> + ASSERT_EQ(0, bind(sockfd, (struct sockaddr *)&self->addr4[0], >>>> sizeof(self->addr4[0]))); >>>> + >>>> + ASSERT_EQ(0, close(sockfd)); >>>> +} >>>> + >>>> +TEST_F_FORK(socket_test, bind_no_restrictions_ip6) { >>>> + >>>> + int sockfd; >>>> + >>>> + sockfd = create_socket(_metadata, true, false); >>>> + ASSERT_LE(0, sockfd); >>>> + >>>> + /* Binds a socket to port[0] */ >>>> + ASSERT_EQ(0, bind(sockfd, (struct sockaddr *)&self->addr6[0], >>>> sizeof(self->addr6[0]))); >>>> + >>>> + ASSERT_EQ(0, close(sockfd)); >>>> +} >>>> + >>>> +TEST_F_FORK(socket_test, bind_with_restrictions_ip4) { >>>> + >>>> + int sockfd; >>>> + >>>> + 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 = self->port[0], >>>> + }; >>>> + struct landlock_net_service_attr net_service_2 = { >>>> + .allowed_access = LANDLOCK_ACCESS_NET_CONNECT_TCP, >>>> + .port = self->port[1], >>>> + }; >>>> + struct landlock_net_service_attr net_service_3 = { >>>> + .allowed_access = 0, >>>> + .port = self->port[2], >>>> + }; >>>> + >>>> + const int ruleset_fd = landlock_create_ruleset(&ruleset_attr, >>>> + sizeof(ruleset_attr), 0); >>>> + ASSERT_LE(0, ruleset_fd); >>>> + >>>> + /* Allows connect and bind operations to the port[0] socket. */ >>> >>> This comment is useful though because the below call is more complex. >>> >> So I can leave it as it's, cant I? > > Yes, keep it, I'd just like a fair amount of useful comments. ;) Ok. Thank you! > .
diff --git a/tools/testing/selftests/landlock/common.h b/tools/testing/selftests/landlock/common.h index 7ba18eb23783..c5381e641dfd 100644 --- a/tools/testing/selftests/landlock/common.h +++ b/tools/testing/selftests/landlock/common.h @@ -102,6 +102,15 @@ static inline int landlock_restrict_self(const int ruleset_fd, } #endif +static void enforce_ruleset(struct __test_metadata *const _metadata, + const int ruleset_fd) +{ + ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)); + ASSERT_EQ(0, landlock_restrict_self(ruleset_fd, 0)) { + TH_LOG("Failed to enforce ruleset: %s", strerror(errno)); + } +} + static void _init_caps(struct __test_metadata *const _metadata, bool drop_all) { cap_t cap_p; diff --git a/tools/testing/selftests/landlock/config b/tools/testing/selftests/landlock/config index 0f0a65287bac..b56f3274d3f5 100644 --- a/tools/testing/selftests/landlock/config +++ b/tools/testing/selftests/landlock/config @@ -1,7 +1,10 @@ +CONFIG_INET=y +CONFIG_IPV6=y +CONFIG_NET=y CONFIG_OVERLAY_FS=y CONFIG_SECURITY_LANDLOCK=y CONFIG_SECURITY_PATH=y CONFIG_SECURITY=y CONFIG_SHMEM=y CONFIG_TMPFS_XATTR=y -CONFIG_TMPFS=y +CONFIG_TMPFS=y \ No newline at end of file diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c index 21a2ce8fa739..036dd6f8f9ea 100644 --- a/tools/testing/selftests/landlock/fs_test.c +++ b/tools/testing/selftests/landlock/fs_test.c @@ -551,16 +551,6 @@ static int create_ruleset(struct __test_metadata *const _metadata, return ruleset_fd; } -static void enforce_ruleset(struct __test_metadata *const _metadata, - const int ruleset_fd) -{ - ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)); - ASSERT_EQ(0, landlock_restrict_self(ruleset_fd, 0)) - { - TH_LOG("Failed to enforce ruleset: %s", strerror(errno)); - } -} - TEST_F_FORK(layout1, proc_nsfs) { const struct rule rules[] = { diff --git a/tools/testing/selftests/landlock/net_test.c b/tools/testing/selftests/landlock/net_test.c new file mode 100644 index 000000000000..478ef2eff559 --- /dev/null +++ b/tools/testing/selftests/landlock/net_test.c @@ -0,0 +1,237 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Landlock tests - Network + * + * Copyright (C) 2022 Huawei Tech. Co., Ltd. + */ + +#define _GNU_SOURCE +#include <arpa/inet.h> +#include <errno.h> +#include <fcntl.h> +#include <linux/landlock.h> +#include <netinet/in.h> +#include <sched.h> +#include <string.h> +#include <sys/prctl.h> +#include <sys/socket.h> +#include <sys/types.h> + +#include "common.h" + +#define MAX_SOCKET_NUM 10 + +#define SOCK_PORT_START 3470 +#define SOCK_PORT_ADD 10 + +#define IP_ADDRESS "127.0.0.1" + +/* Number pending connections queue to be hold */ +#define BACKLOG 10 + +static int create_socket(struct __test_metadata *const _metadata, + bool ip6, bool reuse_addr) +{ + int sockfd; + int one = 1; + + if (ip6) + sockfd = socket(AF_INET6, SOCK_STREAM | SOCK_CLOEXEC, 0); + else + sockfd = socket(AF_INET, SOCK_STREAM | SOCK_CLOEXEC, 0); + + ASSERT_LE(0, sockfd); + /* Allows to reuse of local address */ + if (reuse_addr) + ASSERT_EQ(0, setsockopt(sockfd, SOL_SOCKET, + SO_REUSEADDR, &one, sizeof(one))); + return sockfd; +} + +FIXTURE(socket_test) { + uint port[MAX_SOCKET_NUM]; + struct sockaddr_in addr4[MAX_SOCKET_NUM]; + struct sockaddr_in6 addr6[MAX_SOCKET_NUM]; +}; + +FIXTURE_SETUP(socket_test) +{ + int i; + /* Creates IP4 socket addresses */ + for (i = 0; i < MAX_SOCKET_NUM; i++) { + self->port[i] = SOCK_PORT_START + SOCK_PORT_ADD*i; + self->addr4[i].sin_family = AF_INET; + self->addr4[i].sin_port = htons(self->port[i]); + self->addr4[i].sin_addr.s_addr = htonl(INADDR_ANY); + memset(&(self->addr4[i].sin_zero), '\0', 8); + } + + /* Creates IP6 socket addresses */ + for (i = 0; i < MAX_SOCKET_NUM; i++) { + self->port[i] = SOCK_PORT_START + SOCK_PORT_ADD*i; + self->addr6[i].sin6_family = AF_INET6; + self->addr6[i].sin6_port = htons(self->port[i]); + self->addr6[i].sin6_addr = in6addr_any; + } + + set_cap(_metadata, CAP_SYS_ADMIN); + ASSERT_EQ(0, unshare(CLONE_NEWNET)); + ASSERT_EQ(0, system("ip link set dev lo up")); + clear_cap(_metadata, CAP_SYS_ADMIN); +} + +FIXTURE_TEARDOWN(socket_test) +{ } + +TEST_F_FORK(socket_test, bind_no_restrictions_ip4) { + + int sockfd; + + sockfd = create_socket(_metadata, false, false); + ASSERT_LE(0, sockfd); + + /* Binds a socket to port[0] */ + ASSERT_EQ(0, bind(sockfd, (struct sockaddr *)&self->addr4[0], sizeof(self->addr4[0]))); + + ASSERT_EQ(0, close(sockfd)); +} + +TEST_F_FORK(socket_test, bind_no_restrictions_ip6) { + + int sockfd; + + sockfd = create_socket(_metadata, true, false); + ASSERT_LE(0, sockfd); + + /* Binds a socket to port[0] */ + ASSERT_EQ(0, bind(sockfd, (struct sockaddr *)&self->addr6[0], sizeof(self->addr6[0]))); + + ASSERT_EQ(0, close(sockfd)); +} + +TEST_F_FORK(socket_test, bind_with_restrictions_ip4) { + + int sockfd; + + 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 = self->port[0], + }; + struct landlock_net_service_attr net_service_2 = { + .allowed_access = LANDLOCK_ACCESS_NET_CONNECT_TCP, + .port = self->port[1], + }; + struct landlock_net_service_attr net_service_3 = { + .allowed_access = 0, + .port = self->port[2], + }; + + const int ruleset_fd = landlock_create_ruleset(&ruleset_attr, + sizeof(ruleset_attr), 0); + ASSERT_LE(0, ruleset_fd); + + /* Allows connect and bind operations to the port[0] socket. */ + ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_SERVICE, + &net_service_1, 0)); + /* Allows connect and deny bind operations to the port[1] socket. */ + 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 port[2] socket. + */ + ASSERT_EQ(-1, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_SERVICE, + &net_service_3, 0)); + ASSERT_EQ(ENOMSG, errno); + + /* Enforces the ruleset. */ + enforce_ruleset(_metadata, ruleset_fd); + + sockfd = create_socket(_metadata, false, false); + ASSERT_LE(0, sockfd); + /* Binds a socket to port[0] */ + ASSERT_EQ(0, bind(sockfd, (struct sockaddr *)&self->addr4[0], sizeof(self->addr4[0]))); + + /* Close bounded socket*/ + ASSERT_EQ(0, close(sockfd)); + + sockfd = create_socket(_metadata, false, false); + ASSERT_LE(0, sockfd); + /* Binds a socket to port[1] */ + ASSERT_EQ(-1, bind(sockfd, (struct sockaddr *)&self->addr4[1], sizeof(self->addr4[1]))); + ASSERT_EQ(EACCES, errno); + + sockfd = create_socket(_metadata, false, false); + ASSERT_LE(0, sockfd); + /* Binds a socket to port[2] */ + ASSERT_EQ(-1, bind(sockfd, (struct sockaddr *)&self->addr4[2], sizeof(self->addr4[2]))); + ASSERT_EQ(EACCES, errno); +} + +TEST_F_FORK(socket_test, bind_with_restrictions_ip6) { + + int sockfd; + + 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 = self->port[0], + }; + struct landlock_net_service_attr net_service_2 = { + .allowed_access = LANDLOCK_ACCESS_NET_CONNECT_TCP, + .port = self->port[1], + }; + struct landlock_net_service_attr net_service_3 = { + .allowed_access = 0, + .port = self->port[2], + }; + + const int ruleset_fd = landlock_create_ruleset(&ruleset_attr, + sizeof(ruleset_attr), 0); + ASSERT_LE(0, ruleset_fd); + + /* Allows connect and bind operations to the port[0] socket. */ + ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_SERVICE, + &net_service_1, 0)); + /* Allows connect and deny bind operations to the port[1] socket. */ + 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 port[2] socket. + */ + ASSERT_EQ(-1, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_SERVICE, + &net_service_3, 0)); + ASSERT_EQ(ENOMSG, errno); + + /* Enforces the ruleset. */ + enforce_ruleset(_metadata, ruleset_fd); + + sockfd = create_socket(_metadata, true, false); + ASSERT_LE(0, sockfd); + /* Binds a socket to port[0] */ + ASSERT_EQ(0, bind(sockfd, (struct sockaddr *)&self->addr6[0], sizeof(self->addr6[0]))); + + /* Close bounded socket*/ + ASSERT_EQ(0, close(sockfd)); + + sockfd = create_socket(_metadata, false, false); + ASSERT_LE(0, sockfd); + /* Binds a socket to port[1] */ + ASSERT_EQ(-1, bind(sockfd, (struct sockaddr *)&self->addr6[1], sizeof(self->addr6[1]))); + ASSERT_EQ(EACCES, errno); + + sockfd = create_socket(_metadata, false, false); + ASSERT_LE(0, sockfd); + /* Binds a socket to port[2] */ + ASSERT_EQ(-1, bind(sockfd, (struct sockaddr *)&self->addr6[2], sizeof(self->addr6[2]))); + ASSERT_EQ(EACCES, errno); +} +TEST_HARNESS_MAIN
Adds selftests for bind socket action. The first is with no landlock restrictions: - bind_no_restrictions_ip4; - bind_no_restrictions_ip6; The second ones is with mixed landlock rules: - bind_with_restrictions_ip4; - bind_with_restrictions_ip6; Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com> --- Changes since v3: * Split commit. * Add helper create_socket. * Add FIXTURE_SETUP. Changes since v4: * Adds port[MAX_SOCKET_NUM], struct sockaddr_in addr4 and struct sockaddr_in addr6 in FIXTURE. * Refactoring FIXTURE_SETUP: - initializing self->port, self->addr4 and self->addr6. - adding network namespace. * Refactoring code with self->port, self->addr4 and self->addr6 variables. * Adds selftests for IP6 family: - bind_no_restrictions_ip6. - bind_with_restrictions_ip6. * Refactoring selftests/landlock/config * Moves enforce_ruleset() into common.h --- tools/testing/selftests/landlock/common.h | 9 + tools/testing/selftests/landlock/config | 5 +- tools/testing/selftests/landlock/fs_test.c | 10 - tools/testing/selftests/landlock/net_test.c | 237 ++++++++++++++++++++ 4 files changed, 250 insertions(+), 11 deletions(-) create mode 100644 tools/testing/selftests/landlock/net_test.c -- 2.25.1