| Message ID | 20240408094747.1761850-2-ivanov.mikhail1@huawei-partners.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Paul Moore |
| 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.
4/30/2024 4:36 PM, 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. Yeap, this is fix-patch. I have covered motivation and proposed solution in cover letter. Do you have any suggestions on how i can improve this? > > We also need to update the user documentation to explain that > LANDLOCK_ACCESS_NET_BIND_TCP also handles this case. ok, i'll update it. > >> >> 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") Yeah, thanks! > >> >> 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. will be fixed > >> { >> - __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. ok, i'll move it. > >> + >> +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. will be fixed > > The other comments in Landlock are written with the third person > (in theory everywhere): "Allows..." Indeed, i'll fix comments. Thanks! > >> + 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. Yeap, i'll fix comment. > >> + 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? ULP (Upper Layer Protocol) stands for protocols which are higher than transport protocol in OSI model. Linux has an infrastructure that allows TCP sockets to support logic of some ULP (e.g. TLS ULP). [1] There is a patch that prevents ULP sockets from listening if corresponding ULP implementation in linux doesn't have a clone method. [2] Landlock shouldn't return EACCES for ULP sockets that cannot listen due to some ULP restrictions. [1] https://lore.kernel.org/all/20170524162646.GA24128@davejwatson-mba.local/ [2] https://lore.kernel.org/all/4b80c3d1dbe3d0ab072f80450c202d9bc88b4b03.1672740602.git.pabeni@redhat.com/ > >> + 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? Yeap, i'll add a test for first check. I have not found a way to trigger the second check from userspace. Since socket wasn't binded to any port, this means that it cannot be part of a TCP connection in any state, so it has to be in TCPF_CLOSE state. Nevertheless i think that this check is required: * for consistency with inet stack (see __inet_listen_sk()) * i have not found any restrictions connected with sock locking for TCP-like protocols, so listen(2) can be called after sk->sk_prot->connect() method will change sock state in __inet_stream_connect() (e.g. to TCP_SYN_SENT). In that case this check may be required. What do you think? Btw this hook on socket_listen() should be fixed in order to not check socket that is already in TCP_LISTEN state. Calling listen(2) only changes backlog value, so landlock shouldn't do anything in this case. I'm not sure about ULP checking. I thought of adding test that creates espintcp ULP (net/xfrm/expintcp.c) socket and tries to listen on it. Since espintcp doesn't have clone method ULP check will be triggered. Problem is that kernel doesnt support this ULP module by default and it should be configured with CONFIG_XFRM_ESPINTCP option enabled. I think that selftests shouldn't depend on specific kernel configuration to be fully executed, so probably we should just skip this. What do you think? > >> + 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. will be fixed > >> + 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; indeed, will be fixed. > > > What would be the effect of listen() on an AF_UNSPEC socket? AF_UNSPEC is a family type that only addresses can use. Socket itself can only be AF_INET or AF_INET6 in TCP. > >> + /* 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. will be fixed > >> + >> + /* Socket is alredy binded to some port. */ > > This kind of spelling issue can be found by scripts/checkpatch.pl will be fixed > >> + 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 Mon, May 13, 2024 at 03:15:50PM +0300, Ivanov Mikhail wrote: > > > 4/30/2024 4:36 PM, 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. > > Yeap, this is fix-patch. I have covered motivation and proposed solution > in cover letter. Do you have any suggestions on how i can improve this? You can start this commit message with the same description as in the cover letter. [...] > > > > > + 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? > > ULP (Upper Layer Protocol) stands for protocols which are higher than > transport protocol in OSI model. Linux has an infrastructure that > allows TCP sockets to support logic of some ULP (e.g. TLS ULP). [1] OK, you can extend the comment with this information, but no need for the links. > > There is a patch that prevents ULP sockets from listening > if corresponding ULP implementation in linux doesn't have a clone > method. [2] > > Landlock shouldn't return EACCES for ULP sockets that cannot listen > due to some ULP restrictions. Looks good. > > [1] > https://lore.kernel.org/all/20170524162646.GA24128@davejwatson-mba.local/ > [2] https://lore.kernel.org/all/4b80c3d1dbe3d0ab072f80450c202d9bc88b4b03.1672740602.git.pabeni@redhat.com/ > > > > > > + 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? > > Yeap, i'll add a test for first check. > > I have not found a way to trigger the second check from userspace. > Since socket wasn't binded to any port, this means that it cannot > be part of a TCP connection in any state, so it has to be in TCPF_CLOSE If you're sure this cannot be triggered from user space, you can wrap the test with WARN_ON_ONCE(), but we need to be careful. I'd like to get the point of view of kernel network expert though. Eric, is this assumption correct? > state. Nevertheless i think that this check is required: > > * for consistency with inet stack (see __inet_listen_sk()) > > * i have not found any restrictions connected with sock locking > for TCP-like protocols, so listen(2) can be called after > sk->sk_prot->connect() method will change sock state in > __inet_stream_connect() (e.g. to TCP_SYN_SENT). In that case this > check may be required. > > What do you think? This looks good, but we need to explain this rationale in comments, with explicit mention of network stack functions. > Btw this hook on socket_listen() should be fixed in > order to not check socket that is already in TCP_LISTEN state. Calling > listen(2) only changes backlog value, so landlock shouldn't do anything > in this case. > > I'm not sure about ULP checking. I thought of adding test that creates > espintcp ULP (net/xfrm/expintcp.c) socket and tries to listen on it. > Since espintcp doesn't have clone method ULP check will be triggered. > Problem is that kernel doesnt support this ULP module by default and it > should be configured with CONFIG_XFRM_ESPINTCP option enabled. I think > that selftests shouldn't depend on specific kernel configuration to be > fully executed, so probably we should just skip this. What do you think? Testing with espintcp makes sense for this clone case. I hope it would not require too much boilerplate code though. We can and should add all the required kernel option in tools/testing/selftests/landlock/config, and we should not restrict tests to default kernel options, quite the contrary if it makes sense. [...] > > > > > + 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; > > indeed, will be fixed. > > > > > > > What would be the effect of listen() on an AF_UNSPEC socket? > > AF_UNSPEC is a family type that only addresses can use. > Socket itself can only be AF_INET or AF_INET6 in TCP. Indeed, it is worth mentioning in a comment.
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)