On Sat, Aug 12, 2023 at 12:03:02AM +0300, Konstantin Meskhidze (A) wrote:

7/6/2023 5:55 PM, Mickaël Salaün пишет:

quoted

From: Konstantin Meskhidze <redacted>

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/ (local)
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 <redacted>
Co-developed-by: Mickaël Salaün <mic@digikod.net>
Signed-off-by: Mickaël Salaün <mic@digikod.net>

[...]

quoted

+// 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.

quoted

+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!

quoted

+	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

quoted

+	EXPECT_EQ(-EINVAL, bind_variant(unix_dgram_fd, &self->srv1));
+	EXPECT_EQ(-EINVAL, connect_variant(unix_dgram_fd, &self->srv1));
+}