[PATCH v11.1] selftests/landlock: Add 11 new test suites dedicated to network

[Mickaël Salaün]

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 at huawei.com>
> > 
> > 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@huawei.com/
> > 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 at huawei.com>
> > Co-developed-by: Mickaël Salaün <mic at digikod.net>
> > Signed-off-by: Mickaël Salaün <mic at digikod.net>

[...]

> > +// 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));
> > +}