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

["Konstantin Meskhidze (A)" <konstantin.meskhidze@huawei.com>]

9/14/2023 11:08 AM, Mickaël Salaün пишет:
> On Mon, Sep 11, 2023 at 01:13:24PM +0300, Konstantin Meskhidze (A) wrote:
>> 
>> 
>> 8/17/2023 6:08 PM, Mickaël Salaün пишет:
>> > On Sat, Aug 12, 2023 at 05:37:00PM +0300, Konstantin Meskhidze (A) wrote:
>> > > 
>> > > 
>> > > 7/12/2023 10:02 AM, Mickaël Salaün пишет:
>> > > > > On 06/07/2023 16:55, Mickaël Salaün wrote:
>> > > > > From: Konstantin Meskhidze <konstantin.meskhidze@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.
>> > > > > [...]
>> > 
>> > > > 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:
>> > > > > 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).

   yep, adding rule with port 0 (for bind) returns EINVAL then
   calling bind port 0 returns EACCES cause there is no rule with port 0.
> 
>> > 
>> > 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.

  I think it's possible to have LANDLOCK_ACCESS_NET_CONNECT_TCP with 
port 0 with LANDLOCK_ACCESS_NET_BIND_TCP at the same time, cause 
LANDLOCK_ACCESS_NET_BIND_TCP rule is allowed (by Landlock) with any 
other port but 0.

> 
>> > 
>> > Cc Paul
>> > 
>> > > 	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.
> 
>> 
>> > 
>> > > 	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.
> 
>> > 
>> > Yes, but don't need to bind to anything (same for the next ones).
>> > 
>> > > 	6. ip4, non-sandboxed, bind some available port, connect port 0 -> should
>> > > return ECONNREFUSED.
>> > 
>> > Yes, but without any binding.
>> > 
>> > > 	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.
> 
>> > 
>> > 
>> > > 
>> > > > > [...]
>> > > > > > +FIXTURE(inet)
>> > > > > +{
>> > > > > +	struct service_fixture srv0, srv1;
>> > > > > +};
>> > > > > The "inet" variants are useless and should be removed. The
>> > > "inet"
>> > > > 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).
>> > .
> .