[RFC PATCH v2 1/9] landlock: Refactor current_check_access_socket() access right check

[Mikhail Ivanov]

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 at 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).