On Mon, Apr 08, 2024 at 05:47:46PM +0800, Ivanov Mikhail wrote:
> Make hook for socket_listen(). It will check that the socket protocol is
> TCP, and if the socket's local port number is 0 (which means,
> that listen(2) was called without any previous bind(2) call),
> then listen(2) call will be legitimate only if there is a rule for bind(2)
> allowing binding to port 0 (or if LANDLOCK_ACCESS_NET_BIND_TCP is not
> supported by the sandbox).
Thanks for this patch and sorry for the late full review. The code is
good overall.
We should either consider this patch as a fix or add a new flag/access
right to Landlock syscalls for compatibility reason. I think this
should be a fix. Calling listen(2) without a previous call to bind(2)
is a corner case that we should properly handle. The commit message
should make that explicit and highlight the goal of the patch: first
explain why, and then how.
We also need to update the user documentation to explain that
LANDLOCK_ACCESS_NET_BIND_TCP also handles this case.
>
> Create a new check_access_socket() function to prevent useless copy paste.
> It should be called by hook handlers after they perform special checks and
> calculate socket port value.
You can add this tag:
Fixes: fff69fb03dde ("landlock: Support network rules with TCP bind and connect")
>
> Signed-off-by: Ivanov Mikhail <ivanov.mikhail1@xxxxxxxxxxxxxxxxxxx>
> Reviewed-by: Konstantin Meskhidze <konstantin.meskhidze@xxxxxxxxxx>
> ---
> security/landlock/net.c | 104 +++++++++++++++++++++++++++++++++-------
> 1 file changed, 88 insertions(+), 16 deletions(-)
>
> diff --git a/security/landlock/net.c b/security/landlock/net.c
> index c8bcd29bde09..c6ae4092cfd6 100644
> --- a/security/landlock/net.c
> +++ b/security/landlock/net.c
> @@ -10,6 +10,7 @@
> #include <linux/net.h>
> #include <linux/socket.h>
> #include <net/ipv6.h>
> +#include <net/tcp.h>
>
> #include "common.h"
> #include "cred.h"
> @@ -61,17 +62,36 @@ static const struct landlock_ruleset *get_current_net_domain(void)
> return dom;
> }
>
> -static int current_check_access_socket(struct socket *const sock,
> - struct sockaddr *const address,
> - const int addrlen,
> - access_mask_t access_request)
> +static int check_access_socket(const struct landlock_ruleset *const dom,
> + __be16 port,
> + access_mask_t access_request)
Please format all patches with clang-format.
> {
> - __be16 port;
> layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_NET] = {};
> const struct landlock_rule *rule;
> struct landlock_id id = {
> .type = LANDLOCK_KEY_NET_PORT,
> };
> +
> + id.key.data = (__force uintptr_t)port;
> + BUILD_BUG_ON(sizeof(port) > sizeof(id.key.data));
> +
> + rule = landlock_find_rule(dom, id);
> + access_request = landlock_init_layer_masks(
> + dom, access_request, &layer_masks, LANDLOCK_KEY_NET_PORT);
> +
> + if (landlock_unmask_layers(rule, access_request, &layer_masks,
> + ARRAY_SIZE(layer_masks)))
> + return 0;
> +
> + return -EACCES;
> +}
This check_access_socket() refactoring should be in a dedicated patch.
> +
> +static int current_check_access_socket(struct socket *const sock,
> + struct sockaddr *const address,
> + const int addrlen,
> + access_mask_t access_request)
> +{
> + __be16 port;
> const struct landlock_ruleset *const dom = get_current_net_domain();
>
> if (!dom)
> @@ -159,17 +179,7 @@ static int current_check_access_socket(struct socket *const sock,
> return -EINVAL;
> }
>
> - id.key.data = (__force uintptr_t)port;
> - BUILD_BUG_ON(sizeof(port) > sizeof(id.key.data));
> -
> - rule = landlock_find_rule(dom, id);
> - access_request = landlock_init_layer_masks(
> - dom, access_request, &layer_masks, LANDLOCK_KEY_NET_PORT);
> - if (landlock_unmask_layers(rule, access_request, &layer_masks,
> - ARRAY_SIZE(layer_masks)))
> - return 0;
> -
> - return -EACCES;
> + return check_access_socket(dom, port, access_request);
> }
>
> static int hook_socket_bind(struct socket *const sock,
> @@ -187,9 +197,71 @@ static int hook_socket_connect(struct socket *const sock,
> LANDLOCK_ACCESS_NET_CONNECT_TCP);
> }
>
> +/*
> + * Check that socket state and attributes are correct for listen.
> + * It is required to not wrongfully return -EACCES instead of -EINVAL.
> + */
> +static int check_tcp_socket_can_listen(struct socket *const sock)
> +{
> + struct sock *sk = sock->sk;
> + unsigned char cur_sk_state = sk->sk_state;
> + const struct inet_connection_sock *icsk;
> +
> + /* Allow only unconnected TCP socket to listen(cf. inet_listen). */
nit: Missing space.
The other comments in Landlock are written with the third person
(in theory everywhere): "Allows..."
> + if (sock->state != SS_UNCONNECTED)
> + return -EINVAL;
> +
> + /* Check sock state consistency. */
Can you explain exactly what is going on here (in the comment)? Linking
to a kernel function would help.
> + if (!((1 << cur_sk_state) & (TCPF_CLOSE | TCPF_LISTEN)))
> + return -EINVAL;
> +
> + /* Sockets can listen only if ULP control hook has clone method. */
What is ULP?
> + icsk = inet_csk(sk);
> + if (icsk->icsk_ulp_ops && !icsk->icsk_ulp_ops->clone)
> + return -EINVAL;
Can you please add tests covering all these error cases?
> + return 0;
> +}
> +
> +static int hook_socket_listen(struct socket *const sock,
> + const int backlog)
> +{
> + int err;
> + int family;
> + const struct landlock_ruleset *const dom = get_current_net_domain();
> +
> + if (!dom)
> + return 0;
> + if (WARN_ON_ONCE(dom->num_layers < 1))
> + return -EACCES;
> +
> + /*
> + * listen() on a TCP socket without pre-binding is allowed only
> + * if binding to port 0 is allowed.
> + */
This comment should be just before the inet_sk(sock->sk)->inet_num
check.
> + family = sock->sk->__sk_common.skc_family;
> +
> + if (family == AF_INET || family == AF_INET6) {
This would make the code simpler:
if (family != AF_INET && family != AF_INET6)
return 0;
What would be the effect of listen() on an AF_UNSPEC socket?
> + /* Checks if it's a (potential) TCP socket. */
> + if (sock->type != SOCK_STREAM)
> + return 0;
As for current_check_access_socket() this kind of check should be at the
beginning of the function (before the family check) to exit early and
simplify code.
> +
> + /* Socket is alredy binded to some port. */
This kind of spelling issue can be found by scripts/checkpatch.pl
> + if (inet_sk(sock->sk)->inet_num != 0)
Why do we want to allow listen() on any socket that is binded?
> + return 0;
> +
> + err = check_tcp_socket_can_listen(sock);
> + if (unlikely(err))
> + return err;
> +
> + return check_access_socket(dom, 0, LANDLOCK_ACCESS_NET_BIND_TCP);
> + }
> + return 0;
> +}
> +
> static struct security_hook_list landlock_hooks[] __ro_after_init = {
> LSM_HOOK_INIT(socket_bind, hook_socket_bind),
> LSM_HOOK_INIT(socket_connect, hook_socket_connect),
> + LSM_HOOK_INIT(socket_listen, hook_socket_listen),
> };
>
> __init void landlock_add_net_hooks(void)
> --
> 2.34.1
>
>
