lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <076bfaa6-1e0b-c95b-5727-00001c79f2c0@huawei.com> Date: Fri, 15 Sep 2023 11:54:46 +0300 From: "Konstantin Meskhidze (A)" <konstantin.meskhidze@...wei.com> To: Mickaël Salaün <mic@...ikod.net>, Paul Moore <paul@...l-moore.com> CC: <artem.kuzin@...wei.com>, <gnoack3000@...il.com>, <willemdebruijn.kernel@...il.com>, <yusongping@...wei.com>, <linux-security-module@...r.kernel.org>, <netdev@...r.kernel.org>, <netfilter-devel@...r.kernel.org> Subject: Re: [PATCH v11.1] selftests/landlock: Add 11 new test suites dedicated to network 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@...wei.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). >> > . > .
Powered by blists - more mailing lists