lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <3daaab37-4e04-0237-cc24-e4dc526a0abe@huawei.com> Date: Mon, 23 Oct 2023 12:15:02 +0300 From: "Konstantin Meskhidze (A)" <konstantin.meskhidze@...wei.com> To: Mickaël Salaün <mic@...ikod.net> CC: <willemdebruijn.kernel@...il.com>, <gnoack3000@...il.com>, <linux-security-module@...r.kernel.org>, <netdev@...r.kernel.org>, <netfilter-devel@...r.kernel.org>, <yusongping@...wei.com>, <artem.kuzin@...wei.com> Subject: Re: [PATCH v13 10/12] selftests/landlock: Add 7 new test variants dedicated to network 10/23/2023 11:44 AM, Mickaël Salaün пишет: > On Mon, Oct 23, 2023 at 10:09:54AM +0300, Konstantin Meskhidze (A) wrote: >> >> >> 10/20/2023 6:40 PM, Mickaël Salaün пишет: >> > On Fri, Oct 20, 2023 at 02:41:42PM +0300, Konstantin Meskhidze (A) wrote: >> > > >> > > >> > > 10/18/2023 3:32 PM, Mickaël Salaün пишет: >> > > > You can update the subject with: >> > > > "selftests/landlock: Add network tests" >> > > >> > > Ok. >> > > > > On Mon, Oct 16, 2023 at 09:50:28AM +0800, Konstantin Meskhidze >> > > wrote: >> > > > > These test suites try to check edge cases for TCP sockets >> > > > > bind() and connect() actions. >> > > > > You can replace with that: >> > > > Add 77 test suites to check edge cases related to bind() and connect() >> > > > actions. They are defined with 6 fixtures and their variants: >> > > > > > > > protocol: >> > > > > * bind: Tests with non-landlocked/landlocked ipv4, ipv6 and unix sockets. >> > > > > As you already did, you can write one paragraph per fixture, but >> > > > starting by explaining the fixture and its related variants, and then >> > > > listing the tests and explaining their specificities. For instance: >> > > > > The "protocol" fixture is extended with 12 variants defined as a >> > > matrix >> > > > of: sandboxed/not-sandboxed, IPv4/IPv6/unix network domain, and >> > > > stream/datagram socket. 4 related tests suites are defined: >> > > > * bind: Test bind combinations with increasingly more >> > > > restricting domains. >> > > > * connect: Test connect combinations with increasingly more >> > > > restricting domains. >> > > > ... >> > > >> > > Ok. Will be updated. >> > > > > s/ipv/IPv/g >> > > >> > > Got it. Thanks. >> > > > > > * connect: Tests with non-landlocked/landlocked ipv4, ipv6 and >> > > unix >> > > > > sockets. >> > > > > * bind_unspec: Tests with non-landlocked/landlocked restrictions >> > > > > for bind action with AF_UNSPEC socket family. >> > > > > * connect_unspec: Tests with non-landlocked/landlocked restrictions >> > > > > for connect action with AF_UNSPEC socket family. >> > > > > > > ipv4: >> > > > > * from_unix_to_inet: Tests to make sure unix sockets' actions are not >> > > > > restricted by Landlock rules applied to TCP ones. >> > > > > > > tcp_layers: >> > > > > * ruleset_overlap. >> > > > > * ruleset_expand. >> > > > > > > mini: >> > > > > * network_access_rights: Tests with legitimate access values. >> > > > > * unknown_access_rights: Tests with invalid attributes, out of access range. >> > > > > * inval: >> > > > > - unhandled allowed access. >> > > > > - zero access value. >> > > > > * tcp_port_overflow: Tests with wrong port values more than U16_MAX. >> > > > > > > ipv4_tcp: >> > > > > * port_endianness: Tests with big/little endian port formats. >> > > > > > > port_specific: >> > > > > * bind_connect: Tests with specific port values. >> > > > > > > layout1: >> > > > > * with_net: Tests with network bind() socket action within >> > > > > filesystem directory access test. >> > > > > > > Test coverage for security/landlock is 94.5% of 932 lines >> > > according >> > > > > to gcc/gcov-11. >> > > > > > > Signed-off-by: Konstantin Meskhidze >> > > <konstantin.meskhidze@...wei.com> >> > > > > Link: https://lore.kernel.org/r/20230920092641.832134-11-konstantin.meskhidze@huawei.com >> > > > > Co-developed-by:: Mickaël Salaün <mic@...ikod.net> >> > > > > Signed-off-by: Mickaël Salaün <mic@...ikod.net> >> > > > > --- >> > > > > > > Changes since v12: >> > > > > * Renames port_zero to port_specific fixture. >> > > > > * Refactors port_specific test: >> > > > > - Adds set_port() and get_binded_port() helpers. >> > > > > - Adds checks for port 0, allowed by Landlock in this version. >> > > > > - Adds checks for port 1023. >> > > > > * Refactors commit message. >> > > > > > > > +static void set_port(struct service_fixture *const srv, >> > > in_port_t port) >> > > > > +{ >> > > > > + switch (srv->protocol.domain) { >> > > > > + case AF_UNSPEC: >> > > > > + case AF_INET: >> > > > > + srv->ipv4_addr.sin_port = port; >> > > > > We should call htons() here, and make port a uint16_t. >> > > >> > > Done. >> > > > > > + return; >> > > > > + >> > > > > + case AF_INET6: >> > > > > + srv->ipv6_addr.sin6_port = port; >> > > > > + return; >> > > > > + >> > > > > + default: >> > > > > + return; >> > > > > + } >> > > > > +} >> > > > > + >> > > > > +static in_port_t get_binded_port(int socket_fd, >> > > > > The returned type should be uint16_t (i.e. host endianess). >> > > >> > > Done. >> > > > > > + const struct protocol_variant *const prot) >> > > > > +{ >> > > > > + struct sockaddr_in ipv4_addr; >> > > > > + struct sockaddr_in6 ipv6_addr; >> > > > > + socklen_t ipv4_addr_len, ipv6_addr_len; >> > > > > + >> > > > > + /* Gets binded port. */ >> > > > > + switch (prot->domain) { >> > > > > + case AF_UNSPEC: >> > > > > + case AF_INET: >> > > > > + ipv4_addr_len = sizeof(ipv4_addr); >> > > > > + getsockname(socket_fd, &ipv4_addr, &ipv4_addr_len); >> > > > > + return ntohs(ipv4_addr.sin_port); >> > > > > + >> > > > > + case AF_INET6: >> > > > > + ipv6_addr_len = sizeof(ipv6_addr); >> > > > > + getsockname(socket_fd, &ipv6_addr, &ipv6_addr_len); >> > > > > + return ntohs(ipv6_addr.sin6_port); >> > > > > + >> > > > > + default: >> > > > > + return 0; >> > > > > + } >> > > > > +} >> > > > > These are good helpers! >> > > > > > > +FIXTURE_TEARDOWN(ipv4) >> > > > > +{ >> > > > > +} >> > > > > + >> > > > > +// Kernel FIXME: tcp_sandbox_with_tcp and tcp_sandbox_with_udp >> > > > > No FIXME should remain. >> > > >> > > Ok. Deleted. >> > > > > > +TEST_F(ipv4, from_unix_to_inet) >> > > > > > +TEST_F(mini, network_access_rights) >> > > > > +{ >> > > > > + const struct landlock_ruleset_attr ruleset_attr = { >> > > > > + .handled_access_net = ACCESS_ALL, >> > > > > + }; >> > > > > + struct landlock_net_port_attr net_service = { >> > > > > Please rename to "net_port" everywhere. >> > > >> > > Done. >> > > > > > +TEST_F(port_specific, bind_connect) >> > > > > +{ >> > > > > + int socket_fd, ret; >> > > > > + >> > > > > + /* Adds the first rule layer with bind and connect actions. */ >> > > > > + if (variant->sandbox == TCP_SANDBOX) { >> > > > > + const struct landlock_ruleset_attr ruleset_attr = { >> > > > > + .handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP | >> > > > > + LANDLOCK_ACCESS_NET_CONNECT_TCP >> > > > > + }; >> > > > > + const struct landlock_net_port_attr tcp_bind_connect_zero = { >> > > > > + .allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP | >> > > > > + LANDLOCK_ACCESS_NET_CONNECT_TCP, >> > > > > + .port = htons(0), >> > > > > We don't need any htons() calls anymore. It doesn't change the 0 >> > > value >> > > > in this case but this is not correct. >> > > >> > > Yep. We call htons(port) in landlock_append_net_rule(). >> > > Thanks. >> > > > > > + }; >> > > > > + >> > > > > Useless new line. >> > > >> > > Ok. Thanks. >> > > > > > + int ruleset_fd; >> > > > > + >> > > > > + ruleset_fd = landlock_create_ruleset(&ruleset_attr, >> > > > > + sizeof(ruleset_attr), 0); >> > > > > + ASSERT_LE(0, ruleset_fd); >> > > > > + >> > > > > + /* Checks zero port value on bind and connect actions. */ >> > > > > + EXPECT_EQ(0, >> > > > > + landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT, >> > > > > + &tcp_bind_connect_zero, 0)); >> > > > > + >> > > > > + enforce_ruleset(_metadata, ruleset_fd); >> > > > > + EXPECT_EQ(0, close(ruleset_fd)); >> > > > > + } >> > > > > + >> > > > > + socket_fd = socket_variant(&self->srv0); >> > > > > + ASSERT_LE(0, socket_fd); >> > > > > + >> > > > > + /* Sets address port to 0 for both protocol families. */ >> > > > > + set_port(&self->srv0, htons(0)); >> > > > > ditto >> > > > > > + >> > > > > + /* Binds on port 0. */ >> > > > > + ret = bind_variant(socket_fd, &self->srv0); >> > > > > + if (is_restricted(&variant->prot, variant->sandbox)) { >> > > > > + /* Binds to a random port within ip_local_port_range. */ >> > > > > + EXPECT_EQ(0, ret); >> > > > > + } else { >> > > > > + /* Binds to a random port within ip_local_port_range. */ >> > > > > + EXPECT_EQ(0, ret); >> > > > > If the results are the same, no need to add an if block. >> > > >> > > Right. Updated. >> > > > > > + } >> > > > > + >> > > > > + /* Connects on port 0. */ >> > > > > + ret = connect_variant(socket_fd, &self->srv0); >> > > > > + if (is_restricted(&variant->prot, variant->sandbox)) { >> > > > > + EXPECT_EQ(-ECONNREFUSED, ret); >> > > > > + } else { >> > > > > + EXPECT_EQ(-ECONNREFUSED, ret); >> > > > > + } >> > > > > ditto >> > > > Updated. >> > > > > + >> > > > > + /* Binds on port 0. */ >> > > > > Please close sockets once they are used, and recreate one for >> > > another >> > > > bind/connect to avoid wrong checks. >> > > >> > > Ok. But I can reuse socket_fd after closeing a socket. Correct? >> > >> > It would be clearer to have one variable for the client socket >> > (connect_fd) and another variable for the server socket (bind_fd). >> > But once the socket is closed, you can reuse the same variable by >> > storing a new socket in it. You then only need two variables for sockets >> > in this test. >> >> Ok. Thanks. >> > >> > > > > > + ret = bind_variant(socket_fd, &self->srv0); >> > > > > + if (is_restricted(&variant->prot, variant->sandbox)) { >> > > > > + /* Binds to a random port within ip_local_port_range. */ >> > > > > + EXPECT_EQ(0, ret); >> > > > > + } else { >> > > > > + /* Binds to a random port within ip_local_port_range. */ >> > > > > + EXPECT_EQ(0, ret); >> > > > > + } >> > > > > Why this second bind() block? Furthermore, it is using the same >> > > > socket_fd. >> > >> > Is this block useful? >> >> For a self-connected socket after connection try we need to rebind it >> again. I checked this logic in a small standalone test (with gdb on). So for >> 2 sockets (differnt fds) there is no need to do that. >> > >> > > >> > > I will refactor the code this way - sockets will be recreated for each >> > > bind/connect, and I prefer to use self-connected sockets (use one socket >> > > descriptor) in these tests to make code simpler; testing logic remains the >> > > same way as if we have 2 sockets. >> > > >> > > What do you think??? >> > >> > I find it confusing to use self-connected sockets, it's not clear at all >> > what is going on, and AFAIK it doesn't reflect real use cases. Please >> > don't do that. >> > >> > Using the same variable for both bind and connect socket will lead to >> > issues difficult to debug, and leaked FDs. For instance with the bind + >> > get_binded_port + connect test you should use one variable per socket. >> > To make it easier to read, please follow this rule everywhere (the only >> > case when this is done seems to be with the port_specific.bind_connect >> > test). >> >> OK. I will use 2 fds for bind and connect sockets. >> > >> > > >> > > > > > + >> > > > > + /* Sets binded port for both protocol families. */ >> > > > > + set_port(&self->srv0, >> > > > > + htons(get_binded_port(socket_fd, &variant->prot))); >> > > > > Ditto, these two endianess translations are useless. >> > > >> > > Updated. Thanks. >> > > > > You can also add this to make sure the returned port is not 0: >> > > > port = get_binded_port(socket_fd, &variant->prot); >> > > > EXPECT_NE(0, port); >> > > > set_port(&self->srv0, port); >> > > >> > > Ok. Thanks for the tip. >> > > > > > + >> > > > > + /* Connects on the binded port. */ >> > > > > + ret = connect_variant(socket_fd, &self->srv0); >> > > > > + if (is_restricted(&variant->prot, variant->sandbox)) { >> > > > > + /* Denied by Landlock. */ >> > > > > + EXPECT_EQ(-EACCES, ret); >> > > > > + } else { >> > > > > + EXPECT_EQ(0, ret); >> > > > > + } >> > > > > + >> > > > > + EXPECT_EQ(0, close(socket_fd)); >> > > > > + >> > > > > > > > + /* Adds the second rule layer with just bind action. */ >> > > > > There is not only bind actions here. >> > > >> > > Right. >> > > > > This second part of the tests should be in a dedicated >> > > > TEST_F(port_specific, bind_1023). >> > > >> > > Got it. >> > > > > > + if (variant->sandbox == TCP_SANDBOX) { >> > > > > + const struct landlock_ruleset_attr ruleset_attr = { >> > > > > + .handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP | >> > > > > + LANDLOCK_ACCESS_NET_CONNECT_TCP >> > > > > + }; >> > > > > + >> > > > > + const struct landlock_net_port_attr tcp_bind_zero = { >> > > > > + .allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP, >> > > > > + .port = htons(0), >> > > > > + }; >> > > > > + >> > > > > Useless new lines. >> > > >> > > Got it. >> > > > > > + /* A rule with port value less than 1024. */ >> > > > > + const struct landlock_net_port_attr tcp_bind_lower_range = { >> > > > > + .allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP, >> > > > > + .port = htons(1023), >> > > > > + }; >> > > > > + >> > > > > Useless new line. >> > > >> > > Got it. >> > > > > > + int ruleset_fd; >> > > > > + >> > > > > + ruleset_fd = landlock_create_ruleset(&ruleset_attr, >> > > > > + sizeof(ruleset_attr), 0); >> > > > > + ASSERT_LE(0, ruleset_fd); >> > > > > + >> > > > > + ASSERT_EQ(0, >> > > > > + landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT, >> > > > > + &tcp_bind_lower_range, 0)); >> > > > > + ASSERT_EQ(0, >> > > > > + landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT, >> > > > > + &tcp_bind_zero, 0)); >> > > > > + >> > > > > + enforce_ruleset(_metadata, ruleset_fd); >> > > > > + EXPECT_EQ(0, close(ruleset_fd)); >> > > > > + } >> > > > > + >> > > > > + socket_fd = socket_variant(&self->srv0); >> > > > > We must have one socket FD dedicated to bind an another >> > > dedicated to >> > > > connect, e.g. bind_fd and connect_fd, an close them after each use, >> > > > otherwise tests might be inconsistent. >> > > >> > > Why can't we use self-connected sockets here? Why tests might be >> > > inconsistent? Tests will be working the same way as if we have 2 sockets, >> > > plus the code is simpler. >> > >> > AFAIK it doesn't reflect real use cases of sockets, and I find it really >> > confusing. Where did you see this kind of usage? >> > >> > Test might be inconsistent for instance if you change the port from 1023 >> > to 1024 and you adjust the (denied by system) EXPECT_EQ(-EACCES, ret), >> > you'll get a new error in the following block, which doesn't make sense >> > at first, but then you realize it is because the socket is already >> > binded. To avoid this kind of issues, and avoid leaking FDs, please use >> > a socket per usage and close them before testing something else. >> >> Ok. Got it. >> > >> > > > > > + ASSERT_LE(0, socket_fd); >> > > > > + >> > > > > + /* Sets address port to 1023 for both protocol families. */ >> > > > > + set_port(&self->srv0, htons(1023)); >> > > > > + >> > > > > + /* Binds on port 1023. */ >> > > > > + ret = bind_variant(socket_fd, &self->srv0); >> > > > > + if (is_restricted(&variant->prot, variant->sandbox)) { >> > > > > No need to add this check if the result is the same for >> > > sandboxed and >> > > > not sandboxed tests. >> > > >> > > Ok. Thanks. >> > > > > Instead, use set_cap(_metadata, CAP_NET_BIND_SERVICE) and >> > > clear_cap() >> > > > around this bind_variant() to make this test useful. >> > > > > You will also need to patch common.h like this: >> > > > @@ -112,10 +112,13 @@ static void _init_caps(struct __test_metadata *const _metadata, bool drop_all) >> > > > cap_t cap_p; >> > > > /* Only these three capabilities are useful for the tests. */ >> > > > const cap_value_t caps[] = { >> > > > + /* clang-format off */ >> > > > CAP_DAC_OVERRIDE, >> > > > CAP_MKNOD, >> > > > CAP_SYS_ADMIN, >> > > > CAP_SYS_CHROOT, >> > > > + CAP_NET_BIND_SERVICE, >> > > > + /* clang-format on */ >> > > > }; >> > > >> > > OK. Thanks. >> > > > > > + /* Denied by the system. */ >> > > > > + EXPECT_EQ(-EACCES, ret); >> > > > > + } else { >> > > > > + /* Denied by the system. */ >> > > > > + EXPECT_EQ(-EACCES, ret); >> > > > > + } >> > > > > + >> > > > > I don't see why the following part is useful. Why did you add >> > > it? >> > > Binding to ports < 1024 are forbidden by the system, not by Landlock. >> > > I added a rule with port 1023 to make sure it works as expected. >> > >> > Landlock, as any LSM, can only add more restrictions. That's why it >> > would make more sense to test with CAP_NET_BIND_SERVICE, to make sure >> > Landlock rules work the same with this kind of privileged ports, but you >> > can test both cases (all within the same TEST_F though, and without >> > other tests). >> >> Do you mean during the test to set CAP_NET_BIND_SERVICE, check it with >> landlock (it will success), then switch CAP_NET_BIND_SERVICE cap off and >> bind it again ( will be refused by the system)? >> Am I correct? > > Yes, you can use something like this: > > set_cap(_metadata, CAP_NET_BIND_SERVICE); > ret = bind_variant(socket_fd, &self->srv0); > clear_cap(_metadata, CAP_NET_BIND_SERVICE); > yep. I think the same way. Thanks. > >> > >> > > >> > > > Why tcp_bind_zero? >> > > Beacause it's a bind action with port zero rule. >> > >> > Yes but I don't see why it's relevant to test that here, because it was >> > tested just before. >> > >> OK. I just leave binding to 1023 port here. >> I'm thinking to add binding to 1024 port then to show that this port is >> allowed by the system but denied by landlock ( we have just rule with 1023 >> port). >> What do you think? > > Yes, you can do that. I guess you could use test_bind_and_connect() for > this test. You can group these checks (ports 1023 and 1024) in the same > dedicated TEST_F. Don't think that test_bind_and_connect() has all functionality for this test - it will fail without set_cap(_metadata, CAP_NET_BIND_SERVICE) on. I prefer to use bind_variant/connect_variant functions here. > >> > > >> > > > > The other parts are good though! >> > > > > > + /* Sets address port to 0 for both protocol families. */ >> > > > > + set_port(&self->srv0, htons(0)); >> > > > > + >> > > > > + /* Binds on port 0. */ >> > > > > + ret = bind_variant(socket_fd, &self->srv0); >> > > > > + if (is_restricted(&variant->prot, variant->sandbox)) { >> > > > > + /* Binds to a random port within ip_local_port_range. */ >> > > > > + EXPECT_EQ(0, ret); >> > > > > + } else { >> > > > > + /* Binds to a random port within ip_local_port_range. */ >> > > > > + EXPECT_EQ(0, ret); >> > > > > + } >> > > > > + >> > > > > + /* Sets binded port for both protocol families. */ >> > > > > + set_port(&self->srv0, >> > > > > + htons(get_binded_port(socket_fd, &variant->prot))); >> > > > > + >> > > > > + /* Connects on the binded port. */ >> > > > > + ret = connect_variant(socket_fd, &self->srv0); >> > > > > + if (is_restricted(&variant->prot, variant->sandbox)) { >> > > > > + /* Denied by Landlock. */ >> > > > > + EXPECT_EQ(-EACCES, ret); >> > > > > + } else { >> > > > > + EXPECT_EQ(0, ret); >> > > > > + } >> > > > > + >> > > > > + EXPECT_EQ(0, close(socket_fd)); >> > > > > +} >> > > > > + >> > > > > +TEST_HARNESS_MAIN >> > > > > -- >> > > > > 2.25.1 >> > > > > > . >> > . > .
Powered by blists - more mailing lists