On Mon, Sep 11, 2023 at 01:13:24PM +0300, Konstantin Meskhidze (A) wrote:

8/17/2023 6:08 PM, Mickaël Salaün пишет:

quoted

On Sat, Aug 12, 2023 at 05:37:00PM +0300, Konstantin Meskhidze (A) wrote:

quoted

7/12/2023 10:02 AM, Mickaël Salaün пишет:

quoted

quoted

On 06/07/2023 16:55, Mickaël Salaün wrote:
From: Konstantin Meskhidze <redacted>

quoted

quoted

This patch is a revamp of the v11 tests [1] with new tests

(see the

quoted

quoted

"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.
[...]

quoted

quoted

We should also add a test to make sure errno is the same with and
without sandboxing when using port 0 for connect and consistent with
bind (using an available port). The test fixture and variants should be
quite similar to the "ipv4" ones, but we can also add AF_INET6 variants,
which will result in 8 "ip" variants:

quoted

TEST_F(ip, port_zero)

{
	if (variant->sandbox == TCP_SANDBOX) {
		/* Denies any connect and bind. */
	}
	/* Checks errno for port 0. */
}

As I understand the would be the next test cases:

	1. ip4, sandboxed, bind port 0 -> should return EACCES (denied by
landlock).

Without any allowed port, yes. This test case is useful.

By tuning /proc/sys/net/ipv4/ip_local_port_range (see
inet_csk_find_open_port call) we should be able to pick a specific
allowed port and test it.  We can also test for the EADDRINUSE error to
make sure error ordering is correct (compared with -EACCES).

  Sorry, did not get this case. Could please explain it with more details?

According to bind(2), if no port are available, the syscall should
return EADDRINUSE. And this returned value should be the same whatever
the process is sandbox or not (and never EACCES). But as I explained
just below, we cannot know this random port from the LSM hook, so no
need to tweak /proc/sys/net/ipv4/ip_local_port_range, and your this is
correct:

1. ip4, sandboxed, bind port 0 -> should return EACCES (denied by
landlock).

quoted

However, I think the current LSM API don't enable to infer this random
port because the LSM hook is called before a port is picked.  If this is
correct, the best way to control port binding would be to always deny
binding on port zero/random (when restricting port binding, whatever
exception rules are in place). This explanation should be part of a
comment for this specific exception.

  Yep, if some LSM rule (for bind) has been applied a with specific port,
other attemps to bind with zero/random ports would be refused by LSM
security checks.

To say it another way, we should not allow to add a rule with port 0 for
LANDLOCK_ACCESS_NET_BIND_TCP, but return -EINVAL in this case. This
limitation should be explained, documented and tested.

With (only) LANDLOCK_ACCESS_NET_CONNECT_TCP it should be allowed though
(except if there is also LANDLOCK_ACCESS_NET_BIND_TCP) of course.
Another test should cover the case with a new rule with these two access
rights and port 0.

quoted

Cc Paul

quoted

	2. ip4, non-sandboxed, bind port 0 -> should return 0 (should be bounded to
random port).

I think so but we need to make sure the random port cannot be < 1024, I
guess with /proc/sys/net/ipv4/ip_local_port_range but I don't know for
IPv6.

  For ipv4 when connecting to a server a client binds to a random port
within /proc/sys/net/ipv4/ip_local_port_range, by default one my machine
this range is: cat /proc/sys/net/ipv4/ip_local_port_range
32768   60999.
But for ipv6 there is no such tuning range.

Ok, let's just assume that the test system doesn't have
ip_local_port_range < 1024, put this assumption in a comment, and don't
touch ip_local_port_range at all.

quoted

quoted

	3. ip6, sandboxed, bind port 0 -> should return EACCES (denied by
landlock).
	4. ip6, non-sandboxed, bind port 0 -> should return 0 (should be bounded to
random port).
	5. ip4, sandboxed, bind some available port, connect port 0 -> should
return -EACCES (denied by landlock).

If a rule allows connecting to port 0, then it should be ECONNREFUSED,
otherwise EACCES indeed. Both cases should be tested.

quoted

Yes, but don't need to bind to anything (same for the next ones).

quoted

	6. ip4, non-sandboxed, bind some available port, connect port 0 -> should
return ECONNREFUSED.

Yes, but without any binding.

quoted

	7. ip6, sandboxed, bind some available port, connect port 0 -> should
return -EACCES (denied by landlock)
	8. ip6, non-sandboxed, some bind available port, connect port 0 -> should
return ECONNREFUSED.

Correct?

Thinking more about this case, being able to add a rule with port zero
*for a connect action* looks legitimate.  A rule with both connect and
bind actions on port zero should then be denied.  We should fix
add_rule_net_service() and test that (with a first layer allowing port
zero, and a second without rule, for connect).

 So with first rule allowing port 0 connect action, the second rule with
some another port and connect action,

Yes, but the first rule being part of a first layer/restriction, and the
second rule part of a second layer.

as a result test should allow that.
Correct?

The first layer should return ECONNREFUSED when connecting on port 0
(allowed but nothing listening), and once the second layer is enforced,
EACCES should be returned on port 0.

quoted

quoted

quoted

quoted

[...]

quoted

+FIXTURE(inet)

+{
+	struct service_fixture srv0, srv1;
+};
The "inet" variants are useless and should be removed. The

"inet"

quoted

fixture can then be renamed to "ipv4_tcp".
  So inet should be changed to ipv4_tcp and ipv6_tcp with next

variants:

  - ipv4_tcp.no_sandbox_with_ipv4.port_endianness
  - ipv4_tcp.sandbox_with_ipv4.port_endianness
  - ipv6_tcp.no_sandbox_with_ipv6.port_endianness
  - ipv6_tcp.sandbox_with_ipv6.port_endianness
????

   in this case we need double copy of TEST_F(inet, port_endianness) :
	TEST_F(ipv4_tcp, port_endianness)
	TEST_F(ipv6_tcp, port_endianness)

There is no need for any variant for the port_endianness test. You can
rename "inet" to "ipv4_tcp" (and not "inet_tcp" like I said before).
.