[PATCH v13 10/12] selftests/landlock: Add 7 new test variants dedicated to network
Konstantin Meskhidze (A)
konstantin.meskhidze at huawei.com
Fri Oct 20 11:41:42 UTC 2023
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 at huawei.com>
>> Link: https://lore.kernel.org/r/20230920092641.832134-11-konstantin.meskhidze@huawei.com
>> Co-developed-by:: Mickaël Salaün <mic at digikod.net>
>> Signed-off-by: Mickaël Salaün <mic at digikod.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?
>
>> + 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.
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???
>
>> +
>> + /* 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.
>
>> + 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.
> Why tcp_bind_zero?
Beacause it's a bind action with port zero rule.
>
> 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
>>
> .
More information about the Linux-security-module-archive
mailing list