Message-ID: <36ac2fde-1344-9055-42e2-db849abf02e0@huawei-partners.com>
Date: Mon, 25 Nov 2024 14:04:09 +0300
From: Mikhail Ivanov <ivanov.mikhail1@...wei-partners.com>
To: Günther Noack <gnoack@...gle.com>
CC: <mic@...ikod.net>, <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/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
};

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

> 
> 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