Message-ID: <af72be74-50c7-d251-5df3-a2c63c73296a@huawei-partners.com>
Date: Mon, 2 Dec 2024 14:32:45 +0300
From: Mikhail Ivanov <ivanov.mikhail1@...wei-partners.com>
To: Mickaël Salaün <mic@...ikod.net>
CC: Günther Noack <gnoack@...gle.com>,
	<willemdebruijn.kernel@...il.com>, <gnoack3000@...il.com>,
	<linux-security-module@...r.kernel.org>, <netdev@...r.kernel.org>,
	<netfilter-devel@...r.kernel.org>, <yusongping@...wei.com>,
	<artem.kuzin@...wei.com>, <konstantin.meskhidze@...wei.com>
Subject: Re: [RFC PATCH v3 01/19] landlock: Support socket access-control

On 11/28/2024 11:52 PM, Mickaël Salaün wrote:
> On Thu, Nov 28, 2024 at 03:01:52PM +0300, Mikhail Ivanov wrote:
>> On 11/27/2024 9:43 PM, Mickaël Salaün wrote:
>>> On Mon, Nov 25, 2024 at 02:04:09PM +0300, Mikhail Ivanov wrote:
>>>> On 11/22/2024 8:45 PM, Günther Noack wrote:
>>>>> Hello Mikhail,
>>>>>
>>>>> sorry for the delayed response;
>>>>> I am very happy to see activity on this patch set! :)
>>>>
>>>> Hello Günther,
>>>> No problem, thanks a lot for your feedback!
>>>>
>>>>>
>>>>> On Mon, Nov 11, 2024 at 07:29:49PM +0300, Mikhail Ivanov wrote:
>>>>>> On 9/4/2024 1:48 PM, Mikhail Ivanov wrote:
>>>>>>> Landlock implements the `LANDLOCK_RULE_NET_PORT` rule type, which provides
>>>>>>> fine-grained control of actions for a specific protocol. Any action or
>>>>>>> protocol that is not supported by this rule can not be controlled. As a
>>>>>>> result, protocols for which fine-grained control is not supported can be
>>>>>>> used in a sandboxed system and lead to vulnerabilities or unexpected
>>>>>>> behavior.
>>>>>>>
>>>>>>> Controlling the protocols used will allow to use only those that are
>>>>>>> necessary for the system and/or which have fine-grained Landlock control
>>>>>>> through others types of rules (e.g. TCP bind/connect control with
>>>>>>> `LANDLOCK_RULE_NET_PORT`, UNIX bind control with
>>>>>>> `LANDLOCK_RULE_PATH_BENEATH`). Consider following examples:
>>>>>>>
>>>>>>> * Server may want to use only TCP sockets for which there is fine-grained
>>>>>>>       control of bind(2) and connect(2) actions [1].
>>>>>>> * System that does not need a network or that may want to disable network
>>>>>>>       for security reasons (e.g. [2]) can achieve this by restricting the use
>>>>>>>       of all possible protocols.
>>>>>>>
>>>>>>> This patch implements such control by restricting socket creation in a
>>>>>>> sandboxed process.
>>>>>>>
>>>>>>> Add `LANDLOCK_RULE_SOCKET` rule type that restricts actions on sockets.
>>>>>>> This rule uses values of address family and socket type (Cf. socket(2))
>>>>>>> to determine sockets that should be restricted. This is represented in a
>>>>>>> landlock_socket_attr struct:
>>>>>>>
>>>>>>>       struct landlock_socket_attr {
>>>>>>>         __u64 allowed_access;
>>>>>>>         int family; /* same as domain in socket(2) */
>>>>>>>         int type; /* see socket(2) */
>>>>>>>       };
>>>>>>
>>>>>> Hello! I'd like to consider another approach to define this structure
>>>>>> before sending the next version of this patchset.
>>>>>>
>>>>>> Currently, it has following possible issues:
>>>>>>
>>>>>> First of all, there is a lack of protocol granularity. It's impossible
>>>>>> to (for example) deny creation of ICMP and SCTP sockets and allow TCP
>>>>>> and UDP. Since the values of address family and socket type do not
>>>>>> completely define the protocol for the restriction, we may gain
>>>>>> incomplete control of the network actions. AFAICS, this is limited to
>>>>>> only a couple of IP protocol cases (e.g. it's impossible to deny SCTP
>>>>>> and SMC sockets to only allow TCP, deny ICMP and allow UDP).
>>>>>>
>>>>>> But one of the main advantages of socket access rights is the ability to
>>>>>> allow only those protocols for which there is a fine-grained control
>>>>>> over their actions (TCP bind/connect). It can be inconvenient
>>>>>> (and unsafe) for SCTP to be unrestricted, while sandboxed process only
>>>>>> needs TCP sockets.
>>>>>
>>>>> That is a good observation which I had missed.
>>>>>
>>>>> I agree with your analysis, I also see the main use case of socket()
>>>>> restrictions in:
>>>>>
>>>>>     (a) restricting socket creating altogether
>>>>>     (b) only permitting socket types for which there is fine grained control
>>>>>
>>>>> and I also agree that it would be very surprising when the same socket types
>>>>> that provide fine grained control would also open the door for unrestricted
>>>>> access to SMC, SCTP or other protocols.  We should instead strive for a
>>>>> socket() access control with which these additional protocols weren't
>>>>> accessible.
>>>>>
>>>>>
>>>>>> Adding protocol (Cf. socket(2)) field was considered a bit during the
>>>>>> initial discussion:
>>>>>> https://lore.kernel.org/all/CABi2SkVWU=Wxb2y3fP702twyHBD3kVoySPGSz2X22VckvcHeXw@mail.gmail.com/
>>>>>
>>>>> So adding "protocol" to the rule attributes would suffice to restrict the use of
>>>>> SMC and SCTP then?  (Sorry, I lost context on these protocols a bit in the
>>>>> meantime, I was so far under the impression that these were using different
>>>>> values for family and type than TCP and UDP do.)
>>>>
>>>> Yeap. Following rule will be enough to allow TCP sockets only:
>>>>
>>>> const struct landlock_socket_attr create_socket_attr = {
>>>> 	.allowed_access = LANDLOCK_ACCESS_SOCKET_CREATE,
>>>> 	.family = AF_INET{,6},
>>>> 	.type = SOCK_STREAM,
>>>> 	.protocol = 0
>>>> };
>>>
>>> We should indeed include the protocol type in the rule definition.
>>>
>>>>
>>>> Btw, creation of SMC sockets via IP stack was added quite recently.
>>>> So far, creation has been possible only with AF_SMC family.
>>>>
>>>> https://lore.kernel.org/all/1718301630-63692-1-git-send-email-alibuda@linux.alibaba.com/
>>>>
>>>>>
>>>>>
>>>>>> Secondly, I'm not really sure if socket type granularity is required
>>>>>> for most of the protocols. It may be more convenient for the end user
>>>>>> to be able to completely restrict the address family without specifying
>>>>>> whether restriction is dedicated to stream or dgram sockets (e.g. for
>>>>>> BLUETOOTH, VSOCK sockets). However, this is not a big issue for the
>>>>>> current design, since address family can be restricted by specifying
>>>>>> type = SOCK_TYPE_MASK.
>>>
>>> It looks like SOCK_TYPE_MASK is not part of UAPI, which means it could
>>> change with kernel versions (even while being in UAPI in fact).  This
>>> new socket creation control should allow to deny any socket creation
>>> known or unknow at the time of the user space program build, and
>>> whatever the available C headers.
>>
>> Agreed
>>
>>>
>>> This also means that Landlock should accept any domain, type, and
>>> protocols defined in rules.  Indeed, we don't want to reject rules for
>>> which some protocols are not allowed.
>>
>> Do you mean that Landlock should not make any assumptions about this
>> values during a build time? Currently, patchset provides boundary checks
>> for domain (< AF_MAX) and type (< SOCK_MAX) in landlock_add_rule().
> 
> The *running kernel* may not support some socket's domains or types,
> which may be confusing for users if the rule was tested on a kernel
> supporting such domains/types. >
> For the bitmask of domains or types, the issues to keep boundary checks
> would be when a subset of them is not supported.  Landlock would reject
> such rule and it would be difficult for users to identify the cause.

Ok, I'll remove these checks.

> 
> I'm still wondering if the landlock_append_net_rule()'s -EAFNOSUPPORT
> return value for kernels without CONFIG_INET was a good idea.  We should
> probably return 0 in this case, which would be similar to not checking
> socket's domains nor types.

It seems that returning -EAFNOSUPPORT only complicates error checking
for landlock_append_net_rule() from the user's perspective. Probably the
only reason to check the correctness of restricted objects in Landlock
is to provide errors consistency in hooks.

> 
>>
>>>
>>> What about using bitmasks for the domain and type fields (renamed to
>>> "domains" and "types")?  The last protocol is currently 45/MCTP so a
>>> 64-bit field is enough, and 10/SOCK_PACKET also fits for the last socket
>>> type.
>>>
>>> We cannot do the same with the protocol because the higher one is
>>> 262/MPTCP though.  But it looks like a value of 0 (default protocol)
>>> should be enough for most use cases, and users could specify a protocol
>>> (but this time as a number, not a bitmask).
>>>
>>> To sum up, we could have something like this:
>>>
>>>     const struct landlock_socket_attr create_socket_attr = {
>>>     	.allowed_access = LANDLOCK_ACCESS_SOCKET_CREATE,
>>>     	.families = 1 << AF_INET | 1 << AF_INET6,
>>>     	.types = 1 << SOCK_STREAM,
>>>     	.protocol = IPPROTO_SCTP
>>>     };
>>
>> Looks good! I think it's a nice approach which will provide a sufficient
>> level of flexibility to define a single rule for a specific protocol (or
>> for related protocols).
>>
>> But, this adds possibility to define a single rule for the set of
>> unrelated protocols:
>>
>> /* Allows TCP, UDP and UNIX sockets. */
>> const struct landlock_socket_attr create_socket_attr = {
>> 	.allowed_access = LANDLOCK_ACCESS_SOCKET_CREATE,
>> 	.families = 1 << AF_INET | 1 << AF_INET6 | 1 << AF_UNIX,
>> 	.types = 1 << SOCK_STREAM | 1 << SOCK_DGRAM,
>> 	.protocol = 0
>> };
>>
>> Perhaps limiting the addition of one rule to only one address family
>> would be more clear in terms of rule semantics?:
>>
>> /* Allows TCP, UDP, UNIX STREAM, UNIX DGRAM sockets. */
>> const struct landlock_socket_attr create_socket_attrs[] = {
>> 	{
>> 		/* Allows IPv4 TCP and UDP sockets. */
>> 		.allowed_access = LANDLOCK_ACCESS_SOCKET_CREATE,
>> 		.family = AF_INET,
>> 		.types = 1 << SOCK_STREAM | 1 << SOCK_DGRAM,
>> 		.protocol = 0
>> 	},
>> 	{
>> 		/* Allows IPv6 TCP and UDP sockets. */
>> 		.allowed_access = LANDLOCK_ACCESS_SOCKET_CREATE,
>> 		.family = AF_INET6,
>> 		.types = 1 << SOCK_STREAM | 1 << SOCK_DGRAM,
>> 		.protocol = 0
>> 	},
>> 	{
>> 		/* Allows UNIX sockets. */
>> 		.allowed_access = LANDLOCK_ACCESS_SOCKET_CREATE,
>> 		.family = AF_UNIX,
>> 		.types = 1 << SOCK_STREAM | 1 << SOCK_DGRAM,
>> 		.protocol = 0
>> 	},
>> };
> 
> Because we are already mixing bitmasks and (protocol) value, I'm not
> sure it will help much.  I think in most cases the "families" bitmask
> would handle IPv4 and IPv6 the same (e.g. to only allow TCP with one
> rule).  I think this is also required to be able to have a 1:1 mapping
> with SELinux's socket_type_to_security_class().

Ok, agreed

> 
>>
>>>
>>>
>>>>>
>>>>> Whether the user is adding one rule to permit AF_INET+*, or whether the user is
>>>>> adding two rules to permit (1) AF_INET+SOCK_STREAM and (2) AF_INET+SOCK_DGRAM,
>>>>> that does not seem like a big deal to me as long as the list of such
>>>>> combinations is so low?
>>>>
>>>> Agreed
>>>
>>> I also agree, but this might change if users have to set a combination
>>> of families, types, and protocols.  This should be OK with the bitmask
>>> approach though.
>>>
>>>>
>>>>>
>>>>>
>>>>>> I suggest implementing something close to selinux socket classes for the
>>>>>> struct landlock_socket_attr (Cf. socket_type_to_security_class()). This
>>>>>> will provide protocol granularity and may be simpler and more convenient
>>>>>> in the terms of determining access rights. WDYT?
>>>>>
>>>>> I see that this is a longer switch statement that maps to this enum, it would be
>>>>> an additional data table that would have to be documented separately for users.
>>>>
>>>> This table is the general drawback, since it makes API a bit more
>>>> complex.
>>>>
>>>>>
>>>>> Do you have an example for how such a "security class enum" would map to the
>>>>> combinations of family, type and socket for the protocols discussed above?
>>>>
>>>> I think the socket_type_to_security_class() has a pretty good mapping
>>>> for UNIX and IP families.
>>>
>>> The mapping looks good indeed, and it has been tested for a long time
>>> with many applications.  However, this would make the kernel
>>> implementation more complex, and I think this mapping could easily be
>>> implemented in user space libraries with the bitmask approach, if really
>>> needed, which I'm not sure.
>>
>> I agree, implementing this in a library is a better approach. Thanks for
>> the catch!
>>
>>>
>>>>
>>>>>
>>>>> If this is just a matter of actually mapping (family, type, protocol)
>>>>> combinations in a more flexible way, could we get away by allowing a special
>>>>> "wildcard" value for the "protocol" field, when it is used within a ruleset?
>>>>> Then the LSM would have to look up whether there is a rule for (family, type,
>>>>> protocol) and the only change would be that it now needs to also check whether
>>>>> there is a rule for (family, type, *)?
>>>>
>>>> Something like this?
>>>>
>>>> const struct landlock_socket_attr create_socket_attr = {
>>>> 	.allowed_access = LANDLOCK_ACCESS_SOCKET_CREATE,
>>>> 	.family = AF_INET6,
>>>> 	.type = SOCK_DGRAM,
>>>> 	.protocol = LANDLOCK_SOCKET_PROTO_ALL
>>>> };
>>>>
>>>>>
>>>>> —Günther
>>>>
>>

Powered by blists - more mailing lists