On Sat, Aug 12, 2023 at 12:03:02AM +0300, Konstantin Meskhidze (A) wrote: > > > 7/6/2023 5:55 PM, Mickaël Salaün пишет: > > From: Konstantin Meskhidze <konstantin.meskhidze@xxxxxxxxxx> > > > > This patch is a revamp of the v11 tests [1] with new tests (see the > > "Changes since v11" description). I (Mickaël) only added the following > > todo list and the "Changes since v11" sections in this commit message. > > I think this patch is good but it would appreciate reviews. > > You can find the diff of my changes here but it is not really readable: > > https://git.kernel.org/mic/c/78edf722fba5 (landlock-net-v11 branch) > > [1] https://lore.kernel.org/all/20230515161339.631577-11-konstantin.meskhidze@xxxxxxxxxx/ > > TODO: > > - Rename all "net_service" to "net_port". > > - Fix the two kernel bugs found with the new tests. > > - Update this commit message with a small description of all tests. > > > > These test suites try to check edge cases for TCP sockets > > bind() and connect() actions. > > > > inet: > > * bind: Tests with non-landlocked/landlocked ipv4 and ipv6 sockets. > > * connect: Tests with non-landlocked/landlocked ipv4 and ipv6 sockets. > > * bind_afunspec: Tests with non-landlocked/landlocked restrictions > > for bind action with AF_UNSPEC socket family. > > * connect_afunspec: Tests with non-landlocked/landlocked restrictions > > for connect action with AF_UNSPEC socket family. > > * ruleset_overlap: Tests with overlapping rules for one port. > > * ruleset_expanding: Tests with expanding rulesets in which rules are > > gradually added one by one, restricting sockets' connections. > > * inval_port_format: Tests with wrong port format for ipv4/ipv6 sockets > > and with port values more than U16_MAX. > > > > port: > > * inval: Tests with invalid user space supplied data: > > - out of range ruleset attribute; > > - unhandled allowed access; > > - zero port value; > > - zero access value; > > - legitimate access values; > > * bind_connect_inval_addrlen: Tests with invalid address length. > > * bind_connect_unix_*_socket: Tests to make sure unix sockets' actions > > are not restricted by Landlock rules applied to TCP ones. > > > > layout1: > > * with_net: Tests with network bind() socket action within > > filesystem directory access test. > > > > Test coverage for security/landlock is 94.8% of 934 lines according > > to gcc/gcov-11. > > > > Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@xxxxxxxxxx> > > Co-developed-by: Mickaël Salaün <mic@xxxxxxxxxxx> > > Signed-off-by: Mickaël Salaün <mic@xxxxxxxxxxx> [...] > > +// Kernel FIXME: tcp_sandbox_with_tcp and tcp_sandbox_with_udp > > I debugged the code in qemu and came to a conclusion that we don't > check if socket's family equals to address's one in check_socket_access(...) > function in net.c > So I added the next lines (marked with !!!): > > static int check_socket_access(struct socket *const sock, > struct sockaddr *const address, > const int addrlen, > const access_mask_t access_request) > { > __be16 port; > layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_NET] = {}; > const struct landlock_rule *rule; > access_mask_t handled_access; > struct landlock_id id = { > .type = LANDLOCK_KEY_NET_PORT, > }; > const struct landlock_ruleset *const domain = > get_current_net_domain(); > > if (!domain) > return 0; > if (WARN_ON_ONCE(domain->num_layers < 1)) > return -EACCES; > > /* FIXES network tests */ !!! > if (sock->sk->__sk_common.skc_family != address->sa_family) !!! > return 0; !!! > /* Checks if it's a TCP socket. */ > if (sock->type != SOCK_STREAM) > return 0; > ...... > > So now all network tests pass. > What do you think? Good catch, we should indeed check this inconsistency, but this fix also adds two issues: - sa_family is read before checking if it is out of bound (see offsetofend() check bellow). A simple fix is to move down the new check. - access request with AF_UNSPEC on a bind operation doesn't go through the specific AF_UNSPEC handling branch. There are two things to fix here: handle AF_UNSPEC specifically in this new af_family check, and add a new test to make sure bind with AF_UNSPEC and INADDR_ANY is properly restricted. I'll reply to this message with a patch extending your fix. > > > +TEST_F(ipv4, from_unix_to_inet) > > +{ > > + int unix_stream_fd, unix_dgram_fd; > > + > > + 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_service_attr tcp_bind_connect_p0 = { > > + .allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP | > > + LANDLOCK_ACCESS_NET_CONNECT_TCP, > > + .port = self->srv0.port, > > + }; > > + int ruleset_fd; > > + > > + /* Denies connect and bind to check errno value. */ > > + ruleset_fd = landlock_create_ruleset(&ruleset_attr, > > + sizeof(ruleset_attr), 0); > > + ASSERT_LE(0, ruleset_fd); > > + > > + /* Allows connect and bind for srv0. */ > > + ASSERT_EQ(0, landlock_add_rule(ruleset_fd, > > + LANDLOCK_RULE_NET_SERVICE, > > + &tcp_bind_connect_p0, 0)); > > + > > + enforce_ruleset(_metadata, ruleset_fd); > > + EXPECT_EQ(0, close(ruleset_fd)); > > + } > > + > > + unix_stream_fd = socket(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0); > > + ASSERT_LE(0, unix_stream_fd); > > + > > + unix_dgram_fd = socket(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0); > > Minor mistyping SOCK_STREAM -> SOCK_DGRAM. Good catch! > > > + ASSERT_LE(0, unix_dgram_fd); > > + > > + /* Checks unix stream bind and connect for srv0. */ > > + EXPECT_EQ(-EINVAL, bind_variant(unix_stream_fd, &self->srv0)); > > + EXPECT_EQ(-EINVAL, connect_variant(unix_stream_fd, &self->srv0)); > > + > > + /* Checks unix stream bind and connect for srv1. */ > > + EXPECT_EQ(-EINVAL, bind_variant(unix_stream_fd, &self->srv1)) > > + { > > + TH_LOG("Wrong bind error: %s", strerror(errno)); > > + } > > + EXPECT_EQ(-EINVAL, connect_variant(unix_stream_fd, &self->srv1)); > > + > > + /* Checks unix datagram bind and connect for srv0. */ > > + EXPECT_EQ(-EINVAL, bind_variant(unix_dgram_fd, &self->srv0)); > > + EXPECT_EQ(-EINVAL, connect_variant(unix_dgram_fd, &self->srv0)); > > + > > + /* Checks unix datagram bind and connect for srv0. */ > > Should be "Checks... for srv1." indeed > > > + EXPECT_EQ(-EINVAL, bind_variant(unix_dgram_fd, &self->srv1)); > > + EXPECT_EQ(-EINVAL, connect_variant(unix_dgram_fd, &self->srv1)); > > +}
