Something is wrong with this patch. On Thu, Oct 17, 2024 at 07:04:49PM +0800, Mikhail Ivanov wrote: > Add two helpers for TCP bind/connect accesses, which will serve to perform > action-specific network stack level checks and safely extract the port from > the address. > > Return -EAFNOSUPPORT instead of -EINVAL in sin_family checks. > > Check socket state before validating address for TCP connect access. This > is necessary to follow the error order of network stack. > > Read sk_family value from socket structure with READ_ONCE to safely handle > IPV6_ADDRFORM case (see [1]). > > [1] https://lore.kernel.org/all/20240202095404.183274-1-edumazet@xxxxxxxxxx/ > > Fixes: fff69fb03dde ("landlock: Support network rules with TCP bind and connect") > Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@xxxxxxxxxxxxxxxxxxx> > --- > security/landlock/net.c | 543 +++++++++++++++++++++++----------------- > 1 file changed, 315 insertions(+), 228 deletions(-) > rewrite security/landlock/net.c (37%) > > diff --git a/security/landlock/net.c b/security/landlock/net.c > dissimilarity index 37% > index a3142f9b15ee..06791aba9196 100644 > --- a/security/landlock/net.c > +++ b/security/landlock/net.c > @@ -1,228 +1,315 @@ > -// SPDX-License-Identifier: GPL-2.0-only > -/* > - * Landlock LSM - Network management and hooks > - * > - * Copyright © 2022-2023 Huawei Tech. Co., Ltd. > - * Copyright © 2022-2023 Microsoft Corporation > - */ > - > -#include <linux/in.h> > -#include <linux/net.h> > -#include <linux/socket.h> > -#include <net/ipv6.h> > - > -#include "common.h" > -#include "cred.h" > -#include "limits.h" > -#include "net.h" > -#include "ruleset.h" > - > -int landlock_append_net_rule(struct landlock_ruleset *const ruleset, > - const u16 port, access_mask_t access_rights) > -{ > - int err; > - const struct landlock_id id = { > - .key.data = (__force uintptr_t)htons(port), > - .type = LANDLOCK_KEY_NET_PORT, > - }; > - > - BUILD_BUG_ON(sizeof(port) > sizeof(id.key.data)); > - > - /* Transforms relative access rights to absolute ones. */ > - access_rights |= LANDLOCK_MASK_ACCESS_NET & > - ~landlock_get_net_access_mask(ruleset, 0); > - > - mutex_lock(&ruleset->lock); > - err = landlock_insert_rule(ruleset, id, access_rights); > - mutex_unlock(&ruleset->lock); > - > - return err; > -} > - > -static const struct landlock_ruleset *get_current_net_domain(void) > -{ > - const union access_masks any_net = { > - .net = ~0, > - }; > - > - return landlock_match_ruleset(landlock_get_current_domain(), any_net); > -} > - > -static int check_access_port(const struct landlock_ruleset *const dom, > - __be16 port, access_mask_t access_request) > -{ > - 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; > -} > - > -static int hook_socket_bind(struct socket *const sock, > - struct sockaddr *const address, const int addrlen) > -{ > - __be16 port; > - struct sock *const sk = sock->sk; > - const struct landlock_ruleset *const dom = get_current_net_domain(); > - > - if (!dom) > - return 0; > - if (WARN_ON_ONCE(dom->num_layers < 1)) > - return -EACCES; > - > - if (sk_is_tcp(sk)) { > - /* Checks for minimal header length to safely read sa_family. */ > - if (addrlen < offsetofend(typeof(*address), sa_family)) > - return -EINVAL; > - > - switch (address->sa_family) { > - case AF_UNSPEC: > - case AF_INET: > - if (addrlen < sizeof(struct sockaddr_in)) > - return -EINVAL; > - port = ((struct sockaddr_in *)address)->sin_port; > - break; > - > -#if IS_ENABLED(CONFIG_IPV6) > - case AF_INET6: > - if (addrlen < SIN6_LEN_RFC2133) > - return -EINVAL; > - port = ((struct sockaddr_in6 *)address)->sin6_port; > - break; > -#endif /* IS_ENABLED(CONFIG_IPV6) */ > - > - default: > - return 0; > - } > - > - /* > - * 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. > - * > - * We could return 0 and let the network stack handle these > - * checks, but it is safer to return a proper error and test > - * consistency thanks to kselftest. > - */ > - if (address->sa_family == AF_UNSPEC) { > - /* addrlen has already been checked for AF_UNSPEC. */ > - const struct sockaddr_in *const sockaddr = > - (struct sockaddr_in *)address; > - > - if (sk->sk_family != AF_INET) > - return -EINVAL; > - > - if (sockaddr->sin_addr.s_addr != htonl(INADDR_ANY)) > - return -EAFNOSUPPORT; > - } else { > - /* > - * Checks sa_family consistency to not wrongfully return > - * -EACCES instead of -EINVAL. Valid sa_family changes are > - * only (from AF_INET or AF_INET6) to AF_UNSPEC. > - * > - * We could return 0 and let the network stack handle this > - * check, but it is safer to return a proper error and test > - * consistency thanks to kselftest. > - */ > - if (address->sa_family != sk->sk_family) > - return -EINVAL; > - } > - return check_access_port(dom, port, > - LANDLOCK_ACCESS_NET_BIND_TCP); > - } > - return 0; > -} > - > -static int hook_socket_connect(struct socket *const sock, > - struct sockaddr *const address, > - const int addrlen) > -{ > - __be16 port; > - struct sock *const sk = sock->sk; > - const struct landlock_ruleset *const dom = get_current_net_domain(); > - > - if (!dom) > - return 0; > - if (WARN_ON_ONCE(dom->num_layers < 1)) > - return -EACCES; > - > - if (sk_is_tcp(sk)) { > - /* Checks for minimal header length to safely read sa_family. */ > - if (addrlen < offsetofend(typeof(*address), sa_family)) > - return -EINVAL; > - > - switch (address->sa_family) { > - case AF_UNSPEC: > - case AF_INET: > - if (addrlen < sizeof(struct sockaddr_in)) > - return -EINVAL; > - port = ((struct sockaddr_in *)address)->sin_port; > - break; > - > -#if IS_ENABLED(CONFIG_IPV6) > - case AF_INET6: > - if (addrlen < SIN6_LEN_RFC2133) > - return -EINVAL; > - port = ((struct sockaddr_in6 *)address)->sin6_port; > - break; > -#endif /* IS_ENABLED(CONFIG_IPV6) */ > - > - default: > - return 0; > - } > - > - /* > - * Connecting to an address with AF_UNSPEC dissolves the TCP > - * association, which have the same effect as closing the > - * connection while retaining the socket object (i.e., the file > - * descriptor). As for dropping privileges, closing > - * connections is always allowed. > - * > - * For a TCP access control system, this request is legitimate. > - * Let the network stack handle potential inconsistencies and > - * return -EINVAL if needed. > - */ > - if (address->sa_family == AF_UNSPEC) > - return 0; > - /* > - * Checks sa_family consistency to not wrongfully return > - * -EACCES instead of -EINVAL. Valid sa_family changes are > - * only (from AF_INET or AF_INET6) to AF_UNSPEC. > - * > - * We could return 0 and let the network stack handle this > - * check, but it is safer to return a proper error and test > - * consistency thanks to kselftest. > - */ > - if (address->sa_family != sk->sk_family) > - return -EINVAL; > - > - return check_access_port(dom, port, > - LANDLOCK_ACCESS_NET_CONNECT_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), > -}; > - > -__init void landlock_add_net_hooks(void) > -{ > - security_add_hooks(landlock_hooks, ARRAY_SIZE(landlock_hooks), > - &landlock_lsmid); > -} > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Landlock LSM - Network management and hooks > + * > + * Copyright © 2022-2023 Huawei Tech. Co., Ltd. > + * Copyright © 2022-2023 Microsoft Corporation > + */ > + > +#include <linux/in.h> > +#include <linux/net.h> > +#include <linux/socket.h> > +#include <net/ipv6.h> > + > +#include "common.h" > +#include "cred.h" > +#include "limits.h" > +#include "net.h" > +#include "ruleset.h" > + > +int landlock_append_net_rule(struct landlock_ruleset *const ruleset, > + const u16 port, access_mask_t access_rights) > +{ > + int err; > + const struct landlock_id id = { > + .key.data = (__force uintptr_t)htons(port), > + .type = LANDLOCK_KEY_NET_PORT, > + }; > + > + BUILD_BUG_ON(sizeof(port) > sizeof(id.key.data)); > + > + /* Transforms relative access rights to absolute ones. */ > + access_rights |= LANDLOCK_MASK_ACCESS_NET & > + ~landlock_get_net_access_mask(ruleset, 0); > + > + mutex_lock(&ruleset->lock); > + err = landlock_insert_rule(ruleset, id, access_rights); > + mutex_unlock(&ruleset->lock); > + > + return err; > +} > + > +static const struct landlock_ruleset *get_current_net_domain(void) > +{ > + const union access_masks any_net = { > + .net = ~0, > + }; > + > + return landlock_match_ruleset(landlock_get_current_domain(), any_net); > +} > + > +static int check_access_port(const struct landlock_ruleset *const dom, > + __be16 port, access_mask_t access_request) > +{ > + 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; > +} > + > +/* > + * Checks that TCP @sock and @address attributes are correct for bind(2). > + * > + * On success, extracts port from @address in @port and returns 0. > + * > + * This validation is consistent with network stack and returns the error > + * in the order corresponding to the order of errors from the network stack. > + * It's required to not wrongfully return -EACCES instead of meaningful network > + * stack level errors. Consistency is tested with kselftest. > + * > + * This helper does not provide consistency of error codes for BPF filter > + * (if any). > + */ > +static int > +check_tcp_bind_consistency_and_get_port(struct socket *const sock, > + struct sockaddr *const address, > + const int addrlen, __be16 *port) > +{ > + /* IPV6_ADDRFORM can change sk->sk_family under us. */ > + switch (READ_ONCE(sock->sk->sk_family)) { > + case AF_INET: > + const struct sockaddr_in *const addr = > + (struct sockaddr_in *)address; > + > + /* Cf. inet_bind_sk(). */ > + if (addrlen < sizeof(struct sockaddr_in)) > + return -EINVAL; > + /* > + * For compatibility reason, accept AF_UNSPEC for bind > + * accesses (mapped to AF_INET) only if the address is > + * INADDR_ANY (cf. __inet_bind). > + */ > + if (addr->sin_family != AF_INET) { > + if (addr->sin_family != AF_UNSPEC || > + addr->sin_addr.s_addr != htonl(INADDR_ANY)) > + return -EAFNOSUPPORT; > + } > + *port = ((struct sockaddr_in *)address)->sin_port; > + break; > +#if IS_ENABLED(CONFIG_IPV6) > + case AF_INET6: > + /* Cf. inet6_bind_sk(). */ > + if (addrlen < SIN6_LEN_RFC2133) > + return -EINVAL; > + /* Cf. __inet6_bind(). */ > + if (address->sa_family != AF_INET6) > + return -EAFNOSUPPORT; > + *port = ((struct sockaddr_in6 *)address)->sin6_port; > + break; > +#endif /* IS_ENABLED(CONFIG_IPV6) */ > + default: > + WARN_ON_ONCE(0); > + return -EACCES; > + } > + return 0; > +} > + > +/* > + * Checks that TCP @sock and @address attributes are correct for connect(2). > + * > + * On success, extracts port from @address in @port and returns 0. > + * > + * This validation is consistent with network stack and returns the error > + * in the order corresponding to the order of errors from the network stack. > + * It's required to not wrongfully return -EACCES instead of meaningful network > + * stack level error. Consistency is partially tested with kselftest. > + * > + * This helper does not provide consistency of error codes for BPF filter > + * (if any). > + * > + * The function holds socket lock while checking the socket state. > + */ > +static int > +check_tcp_connect_consistency_and_get_port(struct socket *const sock, > + struct sockaddr *const address, > + const int addrlen, __be16 *port) > +{ > + int err = 0; > + struct sock *const sk = sock->sk; > + > + /* Cf. __inet_stream_connect(). */ > + lock_sock(sk); > + switch (sock->state) { > + default: > + err = -EINVAL; > + break; > + case SS_CONNECTED: > + err = -EISCONN; > + break; > + case SS_CONNECTING: > + /* > + * Calling connect(2) on nonblocking socket with SYN_SENT or SYN_RECV > + * state immediately returns -EISCONN and -EALREADY (Cf. __inet_stream_connect()). > + * > + * This check is not tested with kselftests. > + */ > + if ((sock->file->f_flags & O_NONBLOCK) && > + ((1 << sk->sk_state) & (TCPF_SYN_SENT | TCPF_SYN_RECV))) { > + if (inet_test_bit(DEFER_CONNECT, sk)) > + err = -EISCONN; > + else > + err = -EALREADY; > + break; > + } > + > + /* > + * Current state is possible in two cases: > + * 1. connect(2) is called upon nonblocking socket and previous > + * connection attempt was closed by RST packet (therefore socket is > + * in TCP_CLOSE state). In this case connect(2) calls > + * sk_prot->disconnect(), changes socket state and increases number > + * of disconnects. > + * 2. connect(2) is called twice upon socket with TCP_FASTOPEN_CONNECT > + * option set. If socket state is TCP_CLOSE connect(2) does the > + * same logic as in point 1 case. Otherwise connect(2) may freeze > + * after inet_wait_for_connect() call since SYN was never sent. > + * > + * For both this cases Landlock cannot provide error consistency since > + * 1. Both cases involve executing some network stack logic and changing > + * the socket state. > + * 2. It cannot omit access check and allow network stack handle error > + * consistency since socket can change its state to SS_UNCONNECTED > + * before it will be locked again in inet_stream_connect(). > + * > + * Therefore it is only possible to return 0 and check access right with > + * check_access_port() helper. > + */ > + release_sock(sk); > + return 0; > + case SS_UNCONNECTED: > + if (sk->sk_state != TCP_CLOSE) > + err = -EISCONN; > + break; > + } > + release_sock(sk); > + > + if (err) > + return err; > + > + /* IPV6_ADDRFORM can change sk->sk_family under us. */ > + switch (READ_ONCE(sk->sk_family)) { > + case AF_INET: > + /* Cf. tcp_v4_connect(). */ > + if (addrlen < sizeof(struct sockaddr_in)) > + return -EINVAL; > + if (address->sa_family != AF_INET) > + return -EAFNOSUPPORT; > + > + *port = ((struct sockaddr_in *)address)->sin_port; > + break; > +#if IS_ENABLED(CONFIG_IPV6) > + case AF_INET6: > + /* Cf. tcp_v6_connect(). */ > + if (addrlen < SIN6_LEN_RFC2133) > + return -EINVAL; > + if (address->sa_family != AF_INET6) > + return -EAFNOSUPPORT; > + > + *port = ((struct sockaddr_in6 *)address)->sin6_port; > + break; > +#endif /* IS_ENABLED(CONFIG_IPV6) */ > + default: > + WARN_ON_ONCE(0); > + return -EACCES; > + } > + > + return 0; > +} > + > +static int hook_socket_bind(struct socket *const sock, > + struct sockaddr *const address, const int addrlen) > +{ > + int err; > + __be16 port; > + const struct landlock_ruleset *const dom = get_current_net_domain(); > + > + if (!dom) > + return 0; > + if (WARN_ON_ONCE(dom->num_layers < 1)) > + return -EACCES; > + > + if (sk_is_tcp(sock->sk)) { > + err = check_tcp_bind_consistency_and_get_port(sock, address, > + addrlen, &port); > + if (err) > + return err; > + return check_access_port(dom, port, > + LANDLOCK_ACCESS_NET_BIND_TCP); > + } > + return 0; > +} > + > +static int hook_socket_connect(struct socket *const sock, > + struct sockaddr *const address, > + const int addrlen) > +{ > + int err; > + __be16 port; > + const struct landlock_ruleset *const dom = get_current_net_domain(); > + > + if (!dom) > + return 0; > + if (WARN_ON_ONCE(dom->num_layers < 1)) > + return -EACCES; > + > + if (sk_is_tcp(sock->sk)) { > + /* Checks for minimal header length to safely read sa_family. */ > + if (addrlen < sizeof(address->sa_family)) > + return -EINVAL; > + /* > + * Connecting to an address with AF_UNSPEC dissolves the TCP > + * association, which have the same effect as closing the > + * connection while retaining the socket object (i.e., the file > + * descriptor). As for dropping privileges, closing > + * connections is always allowed. > + * > + * For a TCP access control system, this request is legitimate. > + * Let the network stack handle potential inconsistencies and > + * return -EINVAL if needed. > + */ > + if (address->sa_family == AF_UNSPEC) > + return 0; > + > + err = check_tcp_connect_consistency_and_get_port( > + sock, address, addrlen, &port); > + if (err) > + return err; > + return check_access_port(dom, port, > + LANDLOCK_ACCESS_NET_CONNECT_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), > +}; > + > +__init void landlock_add_net_hooks(void) > +{ > + security_add_hooks(landlock_hooks, ARRAY_SIZE(landlock_hooks), > + &landlock_lsmid); > +} > -- > 2.34.1 > >
