| 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: <20230817.theivaoThia9@digikod.net>
Date: Thu, 17 Aug 2023 14:54:10 +0200
From: Mickaël Salaün <mic@...ikod.net>
To: "Konstantin Meskhidze (A)" <konstantin.meskhidze@...wei.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
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@...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.
> >
> > 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@...wei.com>
> > Co-developed-by: Mickaël Salaün <mic@...ikod.net>
> > Signed-off-by: Mickaël Salaün <mic@...ikod.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));
> > +}
Powered by blists - more mailing lists