TCP listen control

[l0kod]

LANDLOCK_ACCESS_NET_BIND_TCP is useful to limit the scope of "bindable" ports to forbid a malicious sandboxed process to impersonate a legitimate server process. However, bind(2) might be used by (TCP) clients to set the source port to a (legitimate) value. This use case is an issue because we cannot allow a whole range of ports (e.g., >= 1024).

A LANDLOCK_ACCESS_NET_LISTEN_TCP would make more sense to control incoming connections to a specific port.

Being able to restrict listen(2) may lead to a cover channel where the sandboxed process can infer some properties (e.g. port) from the socket file descriptior (e.g. if it was passed from another process). This doesn't seem to be a security issue though.

Controlling accept(2) might not be worth it because:

• performance impact (i.e. evaluated for each new client connection);
• it would make more sense to be able to control access according to the client address/port but
• sandboxing is to protect the outside of the sandbox from the inside.

See thread: https://lore.kernel.org/all/b4440d19-93b9-e234-007b-4fc4f987550b@digikod.net/

Related to #6

Cc @BoardzMaster

[sm1ling-knight]

The problem is that (TCP) client is unable to use ports that are restricted for server, right?

[l0kod]

The problem is that (TCP) client is unable to use ports that are restricted for server, right?

Yes, a client would not be allowed to bind to a specific port (for a newly initiated outgoing connection) if its domain handles LANDLOCK_ACCESS_NET_BIND_TCP but no rule explicitly allows binding on this port. This should not be an issue for most use cases, but some systems fine tune their TCP connections.

[sm1ling-knight]

The problem is that (TCP) client is unable to use ports that are restricted for server, right?

Yes, a client would not be allowed to bind to a specific port (for a newly initiated outgoing connection) if its domain handles LANDLOCK_ACCESS_NET_BIND_TCP but no rule explicitly allows binding on this port. This should not be an issue for most use cases, but some systems fine tune their TCP connections.

For example, a use case where some kind of server process forks the client and the client 's available ports are limited due to the parent ruleset, right?

[l0kod]

For example, a use case where some kind of server process forks the client and the client 's available ports are limited due to the parent ruleset, right?

If a new process is spawned per client connection, then the new process doesn't need any specific right to reply on the opened socket/file descriptor. Indeed, in this case, there is no call to connect(2) nor bind(2), only send/write and recv/read.

A use case where a client process does an explicit bind(2) and then a connect(2) on the same socket can happen on high-volume networks because the kernel may take too much time to find an available port. In this case a specific algorithm can be used by user space to efficiently deal with port allocation. This should probably not be used on a public network for security reasons though.

Another use case is to use a specific network interface or IP address.

[sm1ling-knight]

A use case where a client process does an explicit bind(2) and then a connect(2) on the same socket can happen on high-volume networks because the kernel may take too much time to find an available port. In this case a specific algorithm can be used by user space to efficiently deal with port allocation. This should probably not be used on a public network for security reasons though.

But why in this case client's domain might need to handle LANDLOCK_ACCESS_NET_BIND_TCP?

[l0kod]

A use case where a client process does an explicit bind(2) and then a connect(2) on the same socket can happen on high-volume networks because the kernel may take too much time to find an available port. In this case a specific algorithm can be used by user space to efficiently deal with port allocation. This should probably not be used on a public network for security reasons though.

But why in this case client's domain might need to handle LANDLOCK_ACCESS_NET_BIND_TCP?

They shouldn't unless we can specify a port range (#16). What would make more sense is to use LANDLOCK_ACCESS_NET_LISTEN_TCP instead.

[l0kod]

@BoardzMaster, do you still want to work on this or should we let this open for others?

[BoardzMaster]

Hi @l0kod. @sm1ling-knight he is my colleague and he works on this now. I will give him a hand.

[sm1ling-knight]

listen(2) can be called without any previous bind(2) call, so a malicious sandboxed process can accidentally impersonate some legitimate server process (if listen(2) assigns it a server port number). LANDLOCK_ACCESS_NET_LISTEN_TCP can also be useful to prevent such vulnerability.

[l0kod]

I guess you meant that when listen(2) is called without explicit bind(2) call, the kernel automatically binds the socket to an ephemeral port. So this could indeed be an issue. Thanks for bringing that up.

I wrote a test and this indeed works, whereas it should not. Could you please send a patch to the LSM mailing list (with me in CC), to explain the issue and propose a fix for Landlock. I guess this should mainly add a socket_listen() hook implementation and check if binding on port 0 is allowed.

[sm1ling-knight]

I wrote a test and this indeed works, whereas it should not. Could you please send a patch to the LSM mailing list (with me in CC), to explain the issue and propose a fix for Landlock.

Ok, i'll do it.

I guess this should mainly add a socket_listen() hook implementation

I thought about adding another security plug function that would be called by the ip layer at the end of the listen() handler execution.

and check if binding on port 0 is allowed.

What do you mean by that? Binding on 0 port is also seems to be an issue, maybe we should add one more hook for LANDLOCK_ACCESS_NET_LISTEN_TCP to prevent automatic binding to a forbidden (by Landlock) port.

[l0kod]

I guess this should mainly add a socket_listen() hook implementation

I thought about adding another security plug function that would be called by the ip layer at the end of the listen() handler execution.

Well, it might be seen as an LSM issue, so yes, it would be nice to add security_socket_bind()-like calls where appropriate.

and check if binding on port 0 is allowed.

What do you mean by that? Binding on 0 port is also seems to be an issue, maybe we should add one more hook for LANDLOCK_ACCESS_NET_LISTEN_TCP to prevent automatic binding to a forbidden (by Landlock) port.

Binding on port 0 translates to binding to an ephemeral port, see the port_specific.bind_connect_zero test. That could help for the new hook call.

This should be a fix of LANDLOCK_ACCESS_NET_BIND_TCP, so no LANDLOCK_ACCESS_NET_LISTEN_TCP for now.

[sm1ling-knight]

So, we have to make following things for managing actions with ephemeral ports, right?

case	to-do
1. binding on 0 port	fix for the LANDLOCK_ACCESS_NET_BIND_TCP
2. call to listen(2) without explicit bind(2) call	detail of the LANDLOCK_ACCESS_NET_LISTEN_TCP implementation

[l0kod]

Explicit binding on port 0 is OK. It was discussed with @BoardzMaster, it reflects the bind(2) behavior, and it can be controlled with a LANDLOCK_ACCESS_NET_BIND_TCP rule on port 0.

However, binding on an ephemeral port with listen(2) (i.e. same as bind(2) on port 0) should only be allowed if there is a rule with LANDLOCK_ACCESS_NET_BIND_TCP on port 0 (or if LANDLOCK_ACCESS_NET_BIND_TCP is not handled).

The LANDLOCK_ACCESS_NET_LISTEN_TCP right should be complementary to LANDLOCK_ACCESS_NET_BIND_TCP. A sandboxed process should first be allowed to bind to a port (if it is restricted with LANDLOCK_ACCESS_NET_BIND_TCP), and then this same process should only be allowed to listen to the same port if LANDLOCK_ACCESS_NET_LISTEN_TCP is handled and if a LANDLOCK_ACCESS_NET_LISTEN_TCP rule exists for such port. To say it another way, if a sandbox handles LANDLOCK_ACCESS_NET_LISTEN_TCP but not LANDLOCK_ACCESS_NET_BIND_TCP, then a rule on port 0 for LANDLOCK_ACCESS_NET_LISTEN_TCP would be required to successfully call listen(2) without explicit binding.

[sm1ling-knight]

Thanks, got it)

[sm1ling-knight]

Hello, @l0kod ! Here you added this check in net.c:

        /*
         * For compatibility reason, accept AF_UNSPEC for bind
         * accesses (mapped to AF_INET) only if the address is
         * INADDR_ANY (cf. __inet_bind).  Checking the address is
         * required to not wrongfully return -EACCES instead of
         * -EAFNOSUPPORT.
         */
        if (access_request == LANDLOCK_ACCESS_NET_BIND_TCP) {
            const struct sockaddr_in *const sockaddr =
                (struct sockaddr_in *)address;

            if (sockaddr->sin_addr.s_addr != htonl(INADDR_ANY))
                return -EAFNOSUPPORT;
        }

I didn't understand, why Landlock design requires to make this network stack level check (why return of -EACCES is wrongful)?

[l0kod]

I didn't understand, why Landlock design requires to make this network stack level check (why return of -EACCES is wrongful)?

Because network LSM hooks are called before any check on the network request (e.g. legitimate address or protocol), hooks implementations first need to check that the provided data is complete to avoid introducing bugs, and they should also return the appropriate error code (e.g. EAFNOSUPPORT) to let legitimate user space know that their request is wrong and that they should fix their code. As a side effect, this also makes Landlock (or any other LSM in theory) transparent for legitimate use case (e.g. a bad address). Returning EACCES would mask some useful information. An alternative would be to return 0 and let the network stack handle this check, but this is risky because if the network stack changes this check we would end up allowing this request. This is similar to bbf5a1d0e5d0fb3bdf90205aa872636122692a50 and related checks.

[sm1ling-knight]

I didn't understand, why Landlock design requires to make this network stack level check (why return of -EACCES is wrongful)?

Because network LSM hooks are called before any check on the network request (e.g. legitimate address or protocol), hooks implementations first need to check that the provided data is complete to avoid introducing bugs, and they should also return the appropriate error code (e.g. EAFNOSUPPORT) to let legitimate user space know that their request is wrong and that they should fix their code. As a side effect, this also makes Landlock (or any other LSM in theory) transparent for legitimate use case (e.g. a bad address). Returning EACCES would mask some useful information. An alternative would be to return 0 and let the network stack handle this check, but this is risky because if the network stack changes this check we would end up allowing this request. This is similar to bbf5a1d and related checks.

Am I correct in understanding that this only applies to not-socket argument checking? (e.g. addr, addrlen)

E.g. inet stack restricts calling to bind(2) with active socket (not TCP_CLOSE). But above test fails because bind_variant() returns -EACCES instead of -EINVAL - Landlock mask information about socket invalid state.

We don't want to provide appropriate check in hook, because it's not related to addr or addrlen (bind(2)) arguments, right?

TEST_F(port_specific, listen_bind)
{
    int bind_fd, ret;
    int port0, port1;

    port0 = 1024;
    port1 = 1025;

    /* Adds a rule layer with bind and connect actions. */
    if (variant->sandbox == TCP_SANDBOX) {
        const struct landlock_ruleset_attr ruleset_attr = {
            .handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP
        };
        const struct landlock_net_port_attr tcp_bind_port0 = {
            .allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP,
            .port = port0,
        };
        const struct landlock_net_port_attr tcp_bind_0 = {
            .allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP,
            .port = 0,
        };
        int ruleset_fd;

        ruleset_fd = landlock_create_ruleset(&ruleset_attr,
                             sizeof(ruleset_attr), 0);
        ASSERT_LE(0, ruleset_fd);

        /* Allow calling to listen(2) without explicit binding. */
        EXPECT_EQ(0,
              landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT,
                        &tcp_bind_0, 0));
        EXPECT_EQ(0,
              landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_PORT,
                        &tcp_bind_port0, 0));

        enforce_ruleset(_metadata, ruleset_fd);
        EXPECT_EQ(0, close(ruleset_fd));
    }

    bind_fd = socket_variant(&self->srv0);
    ASSERT_LE(0, bind_fd);

    /* Port for bind_fd is chosen randomly by kernel. */
    ret = listen(bind_fd, backlog);
    EXPECT_EQ(0, ret);

    set_port(&self->srv0, port1);
    ret = bind_variant(bind_fd, &self->srv0);
    EXPECT_EQ(-EINVAL, ret);

    EXPECT_EQ(0, close(bind_fd));
}

[l0kod]

Am I correct in understanding that this only applies to not-socket argument checking? (e.g. addr, addrlen)

E.g. inet stack restricts calling to bind(2) with active socket (not TCP_CLOSE). But above test fails because bind_variant() returns -EACCES instead of -EINVAL - Landlock mask information about socket invalid state.

We don't want to provide appropriate check in hook, because it's not related to addr or addrlen (bind(2)) arguments, right?

I think error codes should be consistent. In this case, we (and probably other LSMs) forgot to check socket's state and I think we should (if the additional check is not too complex, which should not be the case). Please, send a fix, we'll continue the discussion on the mailing list.

I have two patches that should improve the current state for all LSMs. I'll refresh them and send them. Feel free to pick them or reply with your own checks.

[l0kod]

I have two patches that should improve the current state for all LSMs. I'll refresh them and send them. Feel free to pick them or reply with your own checks.

https://lore.kernel.org/r/20240327120036.233641-1-mic@digikod.net