Message ID | 20240408094747.1761850-2-ivanov.mikhail1@huawei-partners.com |
---|---|
State | Handled Elsewhere |
Headers | show |
Series | Forbid illegitimate binding via listen(2) | expand |
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@huawei-partners.com> > Reviewed-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com> > --- > 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 > >
On Tue, Apr 30, 2024 at 03:36:30PM +0200, Mickaël Salaün wrote: > 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@huawei-partners.com> > > Reviewed-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com> > > --- > > security/landlock/net.c | 104 +++++++++++++++++++++++++++++++++------- > > 1 file changed, 88 insertions(+), 16 deletions(-) > > + if (inet_sk(sock->sk)->inet_num != 0) > > Why do we want to allow listen() on any socket that is binded? Please ignore this comment. I was initially thinking about a new access right, which would be good to have anyway, but with another series.
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) { - __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; +} + +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). */ + if (sock->state != SS_UNCONNECTED) + return -EINVAL; + + /* Check sock state consistency. */ + if (!((1 << cur_sk_state) & (TCPF_CLOSE | TCPF_LISTEN))) + return -EINVAL; + + /* Sockets can listen only if ULP control hook has clone method. */ + icsk = inet_csk(sk); + if (icsk->icsk_ulp_ops && !icsk->icsk_ulp_ops->clone) + return -EINVAL; + 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. + */ + family = sock->sk->__sk_common.skc_family; + + if (family == AF_INET || family == AF_INET6) { + /* Checks if it's a (potential) TCP socket. */ + if (sock->type != SOCK_STREAM) + return 0; + + /* Socket is alredy binded to some port. */ + if (inet_sk(sock->sk)->inet_num != 0) + 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)