| Message ID | 20240814030151.2380280-2-ivanov.mikhail1@huawei-partners.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Support TCP listen access-control | expand |
Hello! Thanks for sending round 2 of this patch set! On Wed, Aug 14, 2024 at 11:01:43AM +0800, Mikhail Ivanov wrote: > The current_check_access_socket() function contains a set of address > validation checks for bind(2) and connect(2) hooks. Separate them from > an actual port access right checking. It is required for the (future) > hooks that do not perform address validation. > > Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com> > --- > security/landlock/net.c | 41 ++++++++++++++++++++++++----------------- > 1 file changed, 24 insertions(+), 17 deletions(-) > > diff --git a/security/landlock/net.c b/security/landlock/net.c > index c8bcd29bde09..669ba260342f 100644 > --- a/security/landlock/net.c > +++ b/security/landlock/net.c > @@ -2,7 +2,7 @@ > /* > * Landlock LSM - Network management and hooks > * > - * Copyright © 2022-2023 Huawei Tech. Co., Ltd. > + * Copyright © 2022-2024 Huawei Tech. Co., Ltd. > * Copyright © 2022-2023 Microsoft Corporation > */ > > @@ -61,17 +61,34 @@ 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) It might be worth briefly spelling out in documentation that access_request in current_check_access_socket() may only have a single bit set. This is different to other places where access_mask_t is used, where combinations of these flags are possible. These functions do checks for special cases using "if (access_request == LANDLOCK_ACCESS_NET_CONNECT_TCP)" and the same for "bind". I think it's a reasonable way to simplify the implementation here, but we have to be careful to not accidentally use it differently. It is a preexisting issue, so I don't consider it a blocker, but it might be worth fixing while we are at it? > { > - __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, Re-reading the implementation of this function, it was surprised how specialized it is towards the "connect" and "bind" use cases, which it has specific code paths for. This does not look like it would extend naturally to additional operations. After your refactoring, current_check_access_socket() is now (a) checking that we are looking at a TCP address, and extracting the port, and then (b) doing connect- and bind-specific logic, and then (c) calling check_access_socket(). Would it maybe be possible to turn the code logic around by creating a "get_tcp_port()" helper function for step (a), and then doing all of (a), (b) and (c) directly from hook_socket_bind() and hook_socket_connect()? It would have the upside that in step (b) you don't need to distinguish between bind and connect because it would be clear from the context which of the two cases we are in. It would also remove the need for a function that only supports one bit in the access_mask_t, which is potentially surprising. Thanks, —Günther
8/20/2024 12:37 AM, Günther Noack wrote: > Hello! > > Thanks for sending round 2 of this patch set! > > On Wed, Aug 14, 2024 at 11:01:43AM +0800, Mikhail Ivanov wrote: >> The current_check_access_socket() function contains a set of address >> validation checks for bind(2) and connect(2) hooks. Separate them from >> an actual port access right checking. It is required for the (future) >> hooks that do not perform address validation. >> >> Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com> >> --- >> security/landlock/net.c | 41 ++++++++++++++++++++++++----------------- >> 1 file changed, 24 insertions(+), 17 deletions(-) >> >> diff --git a/security/landlock/net.c b/security/landlock/net.c >> index c8bcd29bde09..669ba260342f 100644 >> --- a/security/landlock/net.c >> +++ b/security/landlock/net.c >> @@ -2,7 +2,7 @@ >> /* >> * Landlock LSM - Network management and hooks >> * >> - * Copyright © 2022-2023 Huawei Tech. Co., Ltd. >> + * Copyright © 2022-2024 Huawei Tech. Co., Ltd. >> * Copyright © 2022-2023 Microsoft Corporation >> */ >> >> @@ -61,17 +61,34 @@ 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) > > It might be worth briefly spelling out in documentation that access_request in > current_check_access_socket() may only have a single bit set. This is different > to other places where access_mask_t is used, where combinations of these flags > are possible. > > These functions do checks for special cases using "if (access_request == > LANDLOCK_ACCESS_NET_CONNECT_TCP)" and the same for "bind". I think it's a > reasonable way to simplify the implementation here, but we have to be careful to > not accidentally use it differently. > > It is a preexisting issue, so I don't consider it a blocker, but it might be > worth fixing while we are at it? I think such comment is not required if we remove "current_check_access_socket()" as you suggest? In this function access_request can be a mask with multiple access rights included. > > >> { >> - __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, > > Re-reading the implementation of this function, it was surprised how specialized > it is towards the "connect" and "bind" use cases, which it has specific code > paths for. This does not look like it would extend naturally to additional > operations. > > After your refactoring, current_check_access_socket() is now (a) checking that > we are looking at a TCP address, and extracting the port, and then (b) doing > connect- and bind-specific logic, and then (c) calling check_access_socket(). > > Would it maybe be possible to turn the code logic around by creating a > "get_tcp_port()" helper function for step (a), and then doing all of (a), (b) > and (c) directly from hook_socket_bind() and hook_socket_connect()? It would > have the upside that in step (b) you don't need to distinguish between bind and > connect because it would be clear from the context which of the two cases we are > in. It would also remove the need for a function that only supports one bit in > the access_mask_t, which is potentially surprising. > > Thanks, > —Günther > Good idea! But I suggest using "get_port_from_addr_tcp()" naming to distinguish between extracting a port from an address structure and from a socket (as performed in hook_listen() in the next patch).
diff --git a/security/landlock/net.c b/security/landlock/net.c index c8bcd29bde09..669ba260342f 100644 --- a/security/landlock/net.c +++ b/security/landlock/net.c @@ -2,7 +2,7 @@ /* * Landlock LSM - Network management and hooks * - * Copyright © 2022-2023 Huawei Tech. Co., Ltd. + * Copyright © 2022-2024 Huawei Tech. Co., Ltd. * Copyright © 2022-2023 Microsoft Corporation */ @@ -61,17 +61,34 @@ 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 +176,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,
The current_check_access_socket() function contains a set of address validation checks for bind(2) and connect(2) hooks. Separate them from an actual port access right checking. It is required for the (future) hooks that do not perform address validation. Signed-off-by: Mikhail Ivanov <ivanov.mikhail1@huawei-partners.com> --- security/landlock/net.c | 41 ++++++++++++++++++++++++----------------- 1 file changed, 24 insertions(+), 17 deletions(-)